Friendica: Move global functions from include/ to class methods in src/

[annando]

We already are having some files in /include that are containing classes that we could move without a problem to /src.

This includes diaspora.php, dfrn.php and ostatus.php and possibly some more.

Tracking document: https://ethercalc.org/friendica_classes

Please update it with your planned changes.

Per @MrPetovan

So that everything is clear: in Model should only be static classes that interact with the DB with the same name as a database table.
In Object should be all the support classes that don't interact with any database table and shouldn't have the same name as a database table.

Remaining files:

• [ ] api.php (needs proper router first)
• [ ] conversation.php
• [x] dba.php (covered by #5410)
• [ ] enotify.php
• [x] items.php (#8880)
• [x] security.php (#5946)
• [x] text.php (#6042)

[MrPetovan]

@zeroadam expressed his interest in working on this issue.

[annando]

So now we should collect a list of files and their best class paths.

[MrPetovan]

Periodic reminder: https://ethercalc.org/friendica_classes

[annando]

Currently we cannot move every file on that list. the dba.php and Photo.php for example do contain class methods and normal functions. We had to split them before moving them.

Files where it should work are:

• dbm.php
• dfrn.php
• diaspora.php
• NotificationsManager.php

Attention:
If these are moved, then they need a use dba;and use dbm; as well, if these class methods are used in the files.

But if dbm.php is moved, then the use dbm; in all files under src has to be changed to the full path as well.

[MrPetovan]

PHP Fatal error: Uncaught Error: Class 'Friendica\Protocol\Crypto' not found in src/Protocol/DFRN.php:1299

I'm on it.

[annando]

Next files could be:

• cache.php
• DirSearch.php
• Emailer.php
• ForumManager.php
• Smilies.php
• xml.php

Relocating these should be less harder, since they aren't as much included as dbm.php.

[zeroadam]

So, will this issue stay open for all future include/ to src/ work to reference?

[MrPetovan]

Indeed, and already a big thank you for your work, it's much appreciated!

[zeroadam]

Absolutely welcome! Glad to finally be able to start contributing, and glad to continue to do so!!!

[annando]

You make @MrPetovan very happy with this work.

[MrPetovan]

So happy I stay up most of the night to track fatal errors with a third-party library! 😅

(I truly am happy)

[rabuzarus]

BTW: If someone has time and is very motivated; the method names in classes should be renamed to camelCase.
e.g. DBM::isResult( ) (instead of DBM::is_result( ))

[annando]

@rabuzarus I suggest that we are doing this after we moved all possible classes to /src. Then someone can have a look at the method under this folder and can clean this up.

[zeroadam]

I was also noticing this, but figured @annando wouldn't want all these changes in this particular work.

[rabuzarus]

Just thought, while breaking addon for friendica master, we should also rename the method names in this develop cycle. So we don't need to break it again in the next cycle.
But this was meant as suggestion.

[annando]

If @zeroadam keeps this speed, we will have relocated all possible files by Saturday. So in the following week he can do the rest of the work 😀

[zeroadam]

@rabuzarus we probably should've had a develop branch on addon now that you mention it.

@annando Loving the praise from you and @MrPetovan! I'll probably have xml.php done before the weekend. I'll be going out of town for a visit this weekend, but I'll have my laptop! 😀

[annando]

For your schedule: in two weeks we will have a hackathon. During that time there shouldn't be huge possibly blocking PRs.

[zeroadam]

Next up is Smilies. Friendica\Util ?

[MrPetovan]

At first glance I think so, it doesn't feel like "Core" to me. Although they are optional, so they could belong in an "Addon" or "Extension" namespace. What do you think @annando, @rabuzarus, @tobiasd ?

[annando]

I don't see them as add-on or extension. I guess we will have some more classes with content manipulation in the future, see for example the bbcode parser or several functions in text.php.

[MrPetovan]

Friendica\Render ?

[MrPetovan]

Friendica\Content ?

[annando]

Sounds better. "Render" could be used for the template parser, the bbcode parser, the smiley parser, ...

[annando]

When we are through with all existing classes, we will need to find good class names for the rest of the files - which will be interesting, since some of them are real piles of garbage :-)

[MrPetovan]

Friendica\Content\Smilies it is 😀

[zeroadam]

OK, sounds good!

On Fri, Nov 10, 2017, 15:10 Hypolite Petovan [e-mail]
wrote:

Friendica\Content\Smilies it is 😀

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/friendica/friendica/issues/3878#issuecomment-343573664,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFyWzHbU_qyQtLAae5OM4r0kcJA3jmMlks5s1K2bgaJpZM4QVF2d
.

[zeroadam]

Current suggestions (via spreadsheet) are:
Emailer -> Friendica\Util
ForumManager -> Friendica\Core
DirSearch -> no suggestions.

Where do you all think DirSearch should go, and do you agree with the other two?

[MrPetovan]

DirSearch is just one function search for a name in the global contact table, I don't believe its own class. It sounds like it would be better suited in something like Friendica\Model\GContact.

[MrPetovan]

include\Emailer.php looks like it should be handled by a third-party emailing library in vendor.

[MrPetovan]

And ForumManager is a mish-mash of model functions (basically SQL queries) and display functions, I don't think it is named right.

[zeroadam]

It seems that the next two mentioned are up in the air. Emailer @MrPetovan mentioned a third party library, and ForumManager needed thought. So what's next? :smile:

[MrPetovan]

All the classes in object, DBA and OStatus!

[annando]

DBA has to be split before moving. And since it will touch nearly every file, we possibly need another strategy to keep the overview.

[zeroadam]

Leaving DBA alone until further instruction. I have one PR and another right behind it ready. If the timing on your comment @annando is accurate, the Hackathon is still a week away? I'm guessing around 25 Nov?

[annando]

It will start Friday in a week. Sounds like you prepare for a sprint ;-)

[zeroadam]

Yes indeed! I found the dates when visiting the friendi.ca site in the blog. I'm surprised it's not on the event calendar

[tobiasd]

Yes indeed! I found the dates when visiting the friendi.ca site in
the blog. I'm surprised it's not on the event calendar

It is, in the events of the forum for Hackathons for Friendica ;-) https://snarl.de/display/0bf239191659b225bb91660066020807

[MrPetovan]

I'm in the process of moving include/Contact.php to src/, please don't disturb.

[annando]

What's next? I guess the following ones are easy:

• Emailer.php
• ForumManager.php
• friendica_smarty.php
• HTTPExceptions.php

[zeroadam]

OK sounds good. What about include/Photo.php?

Also, I thought there were outstanding questions regarding Emailer and ForumManager

[annando]

Photo.php does contain some "classic" functions and not only methods. So one has to see how to handle this. (Possibly by changing the functions to static methods) If you can make that until Wednesday, I'm fine.

Concerning the outstanding questions, I'm delegating this to the master of all classes @MrPetovan

[zeroadam]

Then I'll probably start with smarty and exceptions and mix in some standards for the rest of the src/ classes.

[MrPetovan]

Emailer should go Util until further notice about using a third-party library (which we should totally do). dba should also be replaced by a well-written third-party library (like https://github.com/paragonie/easydb which is almost a drop-in replacement).

[annando]

Just time it, so that at Friday we are having a stable code base.

[MrPetovan]

I kept this deadline in mind last week-end and I won't do any big changes until then, it's just a roadmap thing.

ForumManager is used for displaying the forum widget, so I guess it should be in Content or Content\Widget

[zeroadam]

OK will do (with regard to emailer). I'll break it up so the changesets aren't huge. And yes @annando I will be sure to stop/complete any work before Friday. It's Thanksgiving here in the US on Thursday so that'll help with the schedule for Berlin

[annando]

@MrPetovan I totally vote against replacing dba. This class - for example - automatically deletes child entries in other tables (which normally would be done with database relations).

Especially with the database stuff I want to have something that I can extend without any problem. And since the current class is working fine, I don't see any advantage in using some external library.

[MrPetovan]

The benefits of using a third-party library is using the services of someone else without them having to know anything about our specific project. Since libraries aren't project specific, they often focus on usability and especially security, in the case of @paragonie.

Yes, our DB-handling class may be more convenient because of the tweaks we made to it to fit our project, but nobody in our team knows what glaring security holes it currently has because none of us are expert in information security.

Think about what is our core "business" model here. It isn't how to send emails. It isn't how to interface with a database. It is implementing as many decentralized communication protocols as possible, and any time we spend on any other part of the project is sort of wasted because someone else, somewhere else, is making a better version of one the library we're hacking together.

Of course library choices should ideally be done at the start of a project, but it isn't possible with Friendica, so any new library choice will require some work to replace our existing ones. But it doesn't mean that choosing a better-crafted library to replace a support components isn't a good idea still.

[annando]

Concerning the database class I'm having several ideas how to further enhance it - which isn't possible with some thirdparty class that may fit with many but not all of our needs. That's why I sometimes like to reinvent the wheel.

Concerning most other libraries I'm totally with you.

[MrPetovan]

I understand, however the database handling component is critical from a security standpoint, and I'd hate for a big server to be hit with a SQL injection because we overlooked something in the library or we didn't replace all the q() calls around.

[annando]

Once we replaced all q(...) function calls with the dba:: methods we are only using prepared statements where parameters are automatically sanitised via the database which is more secure than sanitising each parameter (like we are doing with the old function).

[MrPetovan]

Yes, this definitely is a good direction. I'm specifically talking about blind spots we aren't considering right now because, well, we aren't aware of them.

[annando]

When moving the files, please skip include/items.php. I'm planning to do a rewrite of the item functions for the "model" namespace. This will also have an influence to files like include/threads.php and include/tags.php.

[annando]

What's next? At some point we will have to relocate dba.

[MrPetovan]

I remember you saying we should skip it for now because it touches almost every single file. Also is there a good reason to have dba and dbm separated?

[MrPetovan]

After dba, the next big project is to move include/items.php and include/conversation.php as well.

[annando]

Since we now have moved every class, this is the last one - and at some point in history we will have to move this as well.

The idea to separate dba and dbm was to have one class that was driver independent and another one that would be different for different drivers and database types.

Everything concerning the item handling is something that I would like to rewrite from scratch. The existing calls are ugly and incomplete.

[MrPetovan]

Then every library of global functions can become a class as well as we have started to do. Just pick your file, make it into a nice static class in the appropriate namespace, and voilà!
Examples:

• include/salmon.php in Friendica\Protocol\Salmon
• include/email.php in Friendica\Protocol\Email
• include/session.php in Friendica\Core\Session
• include/bbcode.php in Friendica\Content\BBCode
• etc...

[annando]

Concerning BBCode: That specific "thing" will need some love. Means: The parameters to the function "bbcode" are ... strange. They need to rearrange and have to be made easier. And especially that file contains several functions that should transform to private methods.

I see it as a task as well to have a look at all classes to check if only the needed methods are public.

[MrPetovan]

I agree with you, it is quite the undertaking as well.

[zeroadam]

Something for me to look into for the weekend! 😁

[zeroadam]

I have been trying to get Session moved. After several attempts/directions, I am unable to get it to work, there are no errors, but clearly session is not being stored. Any thoughts on the subject, I'm sure there's something I don't know about this!

[MrPetovan]

The trickiest change will be to update the session callbacks, did you check how to use a class method.

[annando]

See here: https://stackoverflow.com/questions/248478/can-callback-methods-in-phps-session-set-save-handler-be-private

[zeroadam]

Thank you both for the pointers/direction. After several more unfruitful attempts this morning, I believe it best to defer to one of you more experienced gentlemen. I think I will learn more from seeing your solution, than beating my head upon my keyboard!

[zeroadam]

Two files I have made several attempts and failed at relocating are: include/friendica_smarty.php and include/session.php. I would like one of you fine people to take these two, as I'm sure it'll be quick work for someone more experienced. Also, to see them solved and learn what I was missing, and so that they are checked off the list!

[MrPetovan]

The list of PRs above will show you unequivocally that experience doesn't mean much 😅

I'll take a stab at Session once the register issue is fixed. Right now it is the first priority.

[zeroadam]

Yes, I certainly agree that is priority!

[annando]

@MrPetovan since you brok... eh, converted the user functions, you are the best to fix it :-)

[zeroadam]

Another quick question. Our tracking document, as well as some beginning description for the issue, reference files that may have already had classes defined and easily moved. That document is close to complete, with the exception of dba, items, and conversation. Looking forward, are the rest of the files meant to stay in include, or is this another directory that wants to be empty?

[annando]

I would like to empty this folder completely. But there are some files that need to be split function by function for their new destinations.

[annando]

And we have to plan how to do the whole item handling. The item handling does have several linked tables like the table for the threads, the tags, the files and so on. They should be called only via the item handling. So the question is: Do they belong to the item class or do they have to be in their own class?

[zeroadam]

I would think they would go into the item class

Well they could go into a Model\Item class, being called by the Object\Item class.. @MrPetovan

[MrPetovan]

As a general rule:

• If the table has a meaningful unique identifier column with a primary key on it, it should have its own Model class
• If a table is only linked (a 1 - n relationship or an n - m relationship with only foreign keys), then it should be handled in the Model class(es) it is linked to.

[annando]

And now it is getting complicated: I would like to redo the whole item handling. To make the whole conversion process easier, I'm considering using completely new tables. The data from the old tables would then be converted in a longer process in the background that could last several days.

The first step is to encapsulate every call to the items in its own class. The next step is then to rewrite these methods for the new tables by keeping the method and class names the same.

But then we had an "Item" class where the main table could have the name "post" (or something else).

[MrPetovan]

It is complicated because we're working with an existing codebase with very little structure or internal coherence. Maybe we can identify precisely what change you want to make to items to avoid having two parallel versions for too long?

[annando]

My changes to the item table are that I want to replace that table completely. The new class will completely replace the "old" class. So there won't be any parallel time.

[MrPetovan]

Then we can keep the same name if you're replacing it entirely. Although I suggest you spin up a new node just to work on it so that your can keep using your current node.

[annando]

I will do it in a way that my current node is still running. I have experience in open heart surgery :-)

[annando]

@zeroadam possibly you can update the list under https://ethercalc.org/friendica_classes with all missing files that still are under /include? Then @MrPetovan can have a look at it and decide where to place them. And of course we should see where we have to split the files into several classes.

[zeroadam]

Will do!

[zeroadam]

Done!

[MrPetovan]

I can't see any file name after line 75 in the document?

[zeroadam]

What was strange! I put them back in.. try again.

[MrPetovan]

It's good now.

[annando]

BTW: There are some files under /mod that are globally used as well - like mod/parse_url. Here we have to split the "module" part from the functionality as well.

Possibly these files could be added to the document as well?

[MrPetovan]

The mod folder is next on the chopping block after include indeed. Each mod will get its own class as well in Friendica\Module.

[annando]

Yeah, but there is functionality under mod that is used from include or src as well. This has to be separated from the modules and Friendica\Module. Everything else is dirty.

[MrPetovan]

Indeed, but that will be taken care of using the same method we have been using so far for include.

[annando]

I guess that the modules need to be converted in a "two step" method:

1. Separate the "global" functions from the "module" functions (stuff that is used globally vs. stuff that is only used from the modules) and make them to classes outside the "Module" namespace.
2. Converting the "module" functions to classes in the "Module" namespace.

And since the first step needs some preparation (defining under which namespace the methods have to b placed), we should do this prior to the second step.

[MrPetovan]

I can provide with an abstract Module class all the modules will have to extend so that we can separate library functions from the module functions.

[zeroadam]

I have updated our EtherCalc sheet to the current state of include/ as of my latest PR for contact_selector.

@annando One of the next moves I was looking into, touches both include/items.php and mod/item.php. I know you had said something about ignoring these, so does that mean that this file should be skipped? I don't think moving the file and not updating the calls in the item files will result in success.

[annando]

"include/items.php" is a monster. I would like to split the functions for insert and delete of item entries from there first. For the rest we (you?) can then see how to handle it. I really would like to split this file into several files - especially I think that some functions fit better in existing protocol classes.

[MrPetovan]

If you rename function calls in include/items.php it’s fine. You can touch, but you can’t move it!

[annando]

I will move functions out of items.php into the new Item.php class file. So I guess this will create problems. Possibly some other files would be better at the moment.

[zeroadam]

Ahh ok that clears it up, I did see that @MrPetovan recent PR updated these files, and ultimately that's what I was wondering.

[MrPetovan]

Like I said, we can easily deal with a few calls renaming here and there, but not with two different move implementations.

If your next class relocation changes a few calls in items, it’s fine, but please don’t relocate anything in items yet.

[zeroadam]

Looking ahead to pgettext, the functions t() and tt(). Is there some better name that these can be changed to? As you can imagine, searching for occurrences of t( produces quite the list!

[tobiasd]

Looking ahead to pgettext, the functions t() and tt(). Is there some
better name that these can be changed to?

Maybe StringMarkedForTranslation and StringMarkedForTranslationPluralForm? TBH I think is this case, less is more and t() and tt() are about perfect.

[zeroadam]

OK, I can accept that. I thought I'd better ask either way 😊

On Sat, Jan 20, 2018, 11:40 Tobias Diekershoff [e-mail]
wrote:

Looking ahead to pgettext, the functions t() and tt(). Is there some
better name that these can be changed to?

Maybe StringMarkedForTranslation and
StringMarkedForTranslationPluralForm? TBH I think is this case, less is
more and t() and tt() are about perfect.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/friendica/friendica/issues/3878#issuecomment-359184289,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFyWzASjR1dgiIArnhri1NqOUNszHT9Vks5tMhbngaJpZM4QVF2d
.

[MrPetovan]

Agreed, t() and tt() should stay, even if they are wrappers for namespaced methods.

[annando]

Thanks for keeping short function names in this special case!

[zeroadam]

After a great many commits, and error messages, and more commits. I have most of this work (for pgettext) done. One issue I have discovered, is that I don't know how to make this work in the one template that I've come across so far. The filebrowser.tpl is the tab when you are making a new post and want to upload or select a picture to attach to the post. The tab has a button at the bottom labeled 'Upload'. In the template it looks like this {{"Upload"|t}}. My understanding is that this then goes through replace_macros and passes the string to the t() function, which now doesn't exist. Simply changing the t in that template to Class::t does not work, and neither does changing the whole thing to Class::t('Upload'). Does anyone have an idea how to accomplish this? Maybe @rabuzarus ?

[MrPetovan]

Ow, this is going to hurt. when I said

Agreed, t() and tt() should stay, even if they are wrappers for namespaced methods.

I meant I wanted to keep the global functions.

Now that you changed it, maybe look at this: https://stackoverflow.com/a/24992622 ?

Second solution, the one we actually use for all other internationalized labels, add a new template variable $upload and set it in replace_macros() to Class::t('Upload')

[zeroadam]

I meant I wanted to keep the global functions.

You mean leave t() and tt() in include/pgettext.php and not move those functions to a class?

[MrPetovan]

Yep, too late now, if it's a 4-letter class it's okay.

[zeroadam]

I thought we just didn't want to rename the functions, I didn't realize it was meant not to move.

But yes, it is indeed your suggested class name.

[rabuzarus]

Does anyone have an idea how to accomplish this? Maybe @rabuzarus ?

I'm sorry but I can't help you with this :-/ The only thing I remember is that in the filebrowser upload was solved through some javascript.

[zeroadam]

I was able to get this working with the method @MrPetovan Petovan linked. Basically
I pass in a $upload variable from the phone side and use it in the button
template

On Tue, Jan 23, 2018, 19:28 rabuzarus [e-mail] wrote:

Does anyone have an idea how to accomplish this? Maybe @rabuzarus
https://github.com/rabuzarus ?

I'm sorry but I can't help you with this :-/ The only thing I remember is
that in the filebrowser upload was solved through some javascript.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/friendica/friendica/issues/3878#issuecomment-359979155,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFyWzIcSShNJLWEbVJUP-j5l3xSmwUxxks5tNnk8gaJpZM4QVF2d
.

[zeroadam]

Php side...

On Tue, Jan 23, 2018, 19:31 Adam Magness [e-mail] wrote:

I was able to get this working with the method @MePetovan linked.
Basically I pass in a $upload variable from the phone side and use it in
the button template

On Tue, Jan 23, 2018, 19:28 rabuzarus [e-mail] wrote:

Does anyone have an idea how to accomplish this? Maybe @rabuzarus
https://github.com/rabuzarus ?

I'm sorry but I can't help you with this :-/ The only thing I remember is
that in the filebrowser upload was solved through some javascript.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/friendica/friendica/issues/3878#issuecomment-359979155,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFyWzIcSShNJLWEbVJUP-j5l3xSmwUxxks5tNnk8gaJpZM4QVF2d
.

[MrPetovan]

Suggestion for @JonnyTischbein

[zeroadam]

This one is a bit of a bear. I tried to break it up in smaller chunks, but as a result there's a bunch of commits. It took me all weekend, it was quite a large file (60 methods). Anyway, don't go cross-eyed reviewing this! Matching PR in addons

[zeroadam]

I know it came up earlier when I started splitting text.php, however, I'm now come to the point again, where the code makes me want to create a Strings utility class. Are there any strong objections to this?

[MrPetovan]

If it isn't specifically for XML, HTML or BBCode strings, sure.