## General developer forum

This is something that HQ have started proposing. It is a good idea, and has already been discussed before several times. E.g. https://tracker.moodle.org/browse/MDLSITE-2261?focusedCommentId=222150&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-222150

There wasn't a thread yet, so I am creating this one, rather than mising this up with my comments on the Events spec.

I like the proposal apart from the bit

look for file bbbdir/classes/ccc/ddd/eee/fff.php

That naming convention is a pain in the bum. (We are forced to use it for YUI modules). Please can we use

Average of ratings: -
I vote for classes/ccc/ddd/eee/fff.php because all class files in one long directory listing is a pain for me.

Average of ratings: -

Well, it won't be 'all classes' in one long directory listing, it will be 'all classes belonging to one plugin' in one directory listing.

Oh, I have just spotted some special cases:

• .../backup - this already contains some specific classes in a one-classe-per-file way. Moodle already deals with loading these when necessary. I guess we don't change anything here.
• .../tests - similarly, PHPunit loads these when necessary. We can leave this folder alone.

Excluding those, and sub-plugins, the quiz (my guess at the plugin with most classes) contains 25 classes

• 4 forms
• 1 exception type

For me, 25 class files in one folder is not a problem.

Typing a path like mod/quiz/classes/links/to/other/attempts.php is just horrible. It requires (in the best case) "l, Tab, Tab, Tab, Tab". The more sensible mod/quiz/classes/links_to_other_attempts.php requires just (l, Tab). It is worse if the name requires more than the first letter to find. It is also harder to get a list of all classes. With one folder, you just need ls. With deeply nested folders, you need ls -R, or find, or the equivalent in your IDE.

As I said before, I am not just guessing here. This is my experience with YUI modules, which are nice in every way except for the irritating paths they require you to use. I have also found it a pain any time I have had to try to find a particular PEAR or Zend class buried in the lib folder.

Book contains 3 classes. Can anyone find a plugin with more classes than quiz?

The problem we will need to solve is Moodle core. The list of classes there will be too big for one folder. My suggestion there would be:

1. Where possible, use real components to break the code into logical chunks; and
2. make a special case for lib/classes (or top-level classes folder if we think it is nicer to keep this outside lib, which is already too big). In that case, we could do one level of nesting, core_xxx_yyy_zzz -> classes/xxx/yyy_zzz.php).

While writing this post I had another possible idea. We could split the classes folder in other ways, for example we could have a separate .../forms folder, at the same level as .../classes, and put classes if there if their name ends _form? We could do something similar for _exception classes in a .../exceptions folder. On the whole, I think we should not do that. It adds complexity, and does not necessarily make it easier to find things.

Average of ratings: -

Oh, to answer my own question, core_question component. An initial search finds over 80 classes! (excluding classes belonging to other plugins like question types).

The way I would handle this is:

• Actually, I would like the code to be organised into two core components:
• core_question - code in question/bank/... - this is the code that deals with what happens when students attempt questions.
• core_questionbank - code in question/engine/... - the is the code that deals with what happens when teachers edit questions.
• There are a bunch of legacy classes relate to restoring old backup files, and I would not bother to move them to the classes folder.
• There are 18 classes in question/engine/states.php that are basically an implementation detail. question_state is that only class there that is part of the public API, so I would leave all these classes in the question/engine/classes/state.php file.

I think that gets the list down to two groups of ~25 classes, which is again manageable.

Average of ratings: -

I still disagree

• other loaders replace _ with /
• core will have many classes soon - plugin and core rules should be the same, we are talking hundreds here, not tens
• if we had classloading we would have a lot more classes everywhere, looking at current classes does not prove much
• proposed events would introduce many classes in all plugins
• Tab argumnet is valid for people using shell only, you are forgetting people with IDEs
• It is nice of you giving examples of other projects using subdirs, I take it as argument in my favour

question/ is not a good example of code using frankenstyle for core subsystem, it is unique. The proposal for class loading is intended primarily for plugins and lib/classes/*, I am afraid it is too late to enforce strict frankenstyle naming in existing core subsystems.

In theory we could add special classloaders for existing core subsystems that do not use frankenstyle, performance should not be critical factor here.

Average of ratings:Useful (1)

I agree with a lot of it, but not all of this.

I think that the most fundamental problem is that it mixes up two completely difference concepts: giving classes good descriptive names, and file paths that group files on file-system in a logical place. For example, mod_quiz_links_to_other_attempts may not be the best name for a class ever, but it does clearly describe what objects of that class are. mod/quiz/classes/links/to/other/attempts.php is a horrible path.

If, right from the start, you know that class-names will be used to group code in folders on disc. Then you can define your class names appropriately. For example Zend_Rest_Server is quite a good name. On the other hand, sometimes you cannot avoid bad names. E.g. HTML_QuickForm_Renderer_Array sounds like an Array of renderers, but it is not.

However, in many parts of Moodle we already have class names that are designed to be good names, with not thought to how that would map to file paths. If we rename the classes now, we break backwards compatibility in many places, just so we can use the same horrible convention as other projects. (I know. It is good to follow the same conventions as everyone else, but sometimes, if a whole lot of lemmings are running off a cliff, you should not join in.)

You mention IDEs. IDEs make the problem worse than command-line. Navigating to deeply nested folders is a real pain there.

Another example is Java. Java groups classes into files according to package names, not class names. This works well, since it lets you have good class names. However, deeply nested package names are still a pain when navigating directory trees. As a result, IDEs have an option to list packages names flattened out, so it is only one click to see the classes in the om.tnavigator.reports.std package with one click, not 4. I don't know of any IDE that does a similar thing for navigating around class files for PHP.

"proposed events would introduce many classes in all plugins" - I have already tried to explain in the other thread why that is a bad idea.

Average of ratings: -
I use IDEs, there is no problem with clicking - we have automatic completion and jump to source...

The spec is not intended for existing core classes and I definitely do not propose renaming of anything.

The class loading spec depends on already established rules for naming of everything in Moodle - Frankenstlye. Martin defined OOP naming rules long time ago, I never liked them, you need to live with them too (all lowercase, same underscores for different things, no namespaces).

Sorry but you did not explain why it is bad idea to put events in classes, I tried to explain real problems in your proposed design and I made a proof of concept that demonstrates the design with code bits, resolves many problems and works consistently

Average of ratings: -

I hope you are right about IDEs. Even at the moment, I sometimes load the file I want to starting to type a class name into a random bit of code, auto-complete, then CTRL + Click. Crazy, but quick!

I am surprised you say that auto-loading should not be for existing code. I have worked hard, in the code I write, to follow best practice. E.g. name classes started mod_quiz_; actually uses classes to organise code; etc. Now you are saying "Because you tried to do the right thing, Tim, you are stewed. You will never be able to benefit from auto-loading." I'm not very happy about that, especially when there are alternative ways to implement auto-loading that work better with existing classes. Oh well.

Average of ratings: -
You are misinterpreting me, the classloading proposal uses consistent existing rules for class names. The classes/ in tinymce is already fully compatible. If anything uses frankentyle rules for plugin class names you could just move the class code to new file in classes/ without renaming or any BC breakage.

I suppose me and you are not going to come to any agreement on classes/ structure. I am going to respect the voting of other core developers.

The options are:

mod_xxx_aaa_bbb_ccc
1/ mod/xxx/classes/aaa/bbb/ccc.php - fully nested on _
2/ mod/xxx/classes/aaa/bbb_ccc.php - 1 level of dirs
3 mod/xxx/classes/aaa_bbb_ccc.php - flat

core_aaa_bbb_ccc
A/ lib/classes/aaa/bbb/ccc.php
B/ lib/classes/aaa/bbb_ccc.php
C/ lib/classes/aaa_bbb_ccc.php - in my opinion unacceptable

The core subsystems would have to be probably whitelisted one by one, the rules there should be similar. Most of core subsystems do not use frankenstyle with core_ prefix, introducing it now would break BC badly.

1A is current proposal, 2B seems reasonable too. Mixture of 23BC is another possibility I cosidered and even implemented before (it was a bit slower).

Average of ratings: -

That is a nice summary of the options. I agree with you that C is impossible.

My vote would be 3B. Like you, I would respect the voting of other core developers (more opinions please!). The fundamental point is that auto-loading will be a big win, however we choose to finally implement it. Thank you very much for working on it.

I do think that it is worth having the conversation now to try to find the best way to implement it before we integrate one solution.

How do you feel about classes, rather than lib/classes? lib is already huge, and is a nasty mixture of third-party code and core code. Ah! of course, component core maps to path lib, so the classloader will look in lib/classes, and we don't really want to add an exception for this case.

Average of ratings: -
Oh, we do agree after all about everything except the subdirs - easily resolved by voting, we can change it at any time before release if necessary.

I believe we can make it all reasonably fast no matter what rules or BC non-frankenstyle hacks we use for core subsystems.

Thanks a lot for your contribution!!!

Average of ratings: -
Maybe we could support fully nested and flat classes dirs at the same time both in core and plugins. That way we could move stuff around as necessary depending on actual number of files there. It would not be that bad for performance.

That is 13AC

Average of ratings: -

To me, the main concern is probably "which one is the fastest?". But if the result is similar on any of them, I'd probably go with 2B, assuming that we properly name our classes.

What I like about 2B is that

• You have a semblance of structure, but not too nested so you can easily find yourself in there.
• It is not too complicated to learn [component]_[directory]_[anything_else_here].php > [component]/classes/[directory]/anything_else_here.php
• It could help developers structuring their code and naming their classes in a better way

I am not sure I'd be seduced by the idea of allowing 2 formats. I think it's better to have consistency over flexibility.

Average of ratings: -

If C is unacceptable then I don't see how we can go with 3.

I vote for 2B, but i've never used automatic class loading in any other context and I do think that going with the defacto php standard for this would be good.

(One thing about 2 and B I don't like is that that directory is kinda arbitary, but i'm sure it'd be easy to get used to it.)

I am not sure I'd be seduced by the idea of allowing 2 formats. I think it's better to have consistency over flexibility.

Agreed.

Average of ratings: -

C is unacceptable because there are just too many different classes in Moodle 'core' that have not been broken down into separate, logical, core subsystems. But that is really a problem with Moodle core, not with the class-loading proposal for modules. Sadly, it is a problem for Moodle core that is essentially unfixable, but I think we should view any exception made for Moodle core to be an inevitable work-around for that. We should not let the problems of Moodle core make things unnecessarily nasty for all other components.

I am afraid I now want to confuse matters by proposing an option 4 / D:

mod_xxx_aaa_bbb_ccc
4/ mod/xxx/classes/cccs/aaa_bbb_ccc.php - 1 level of dirs based on last word.

The reason this is not completely crazy is that we have a lot of classes called something like ..._form, or ..._renderer or ..._exception.

mod/quiz/classes/forms/preflight_check_form.php would be an specific example.

34CD might then be a good choice. I am not sure, and I am not sure how much a choice hurts performance.

I am also not sure that D really solved the problem for Moodle core. But, this might be worth considering.

Average of ratings: -
I disagree that there should be a bazillion of core subsystems in order to make lib/ nearly empty. I would prefer if we moved majority of classes into lib/classes with nested structure.

I do not like 4 much and I think D is not acceptable for our lib/classes/ directory. Combining 4D with anything else seems too ambiguous.

Thanks for the new options, I agree we need to consider them all now.

Average of ratings: -

Bit late on this but here's my opinions:

I would be vaguely in favour of option 2 provided that we are allowed to give classes run-together names in order to ensure they come in the same directory. If we aren't allowed to use run-together names for that reason, then I'd go with option 3.

To put this another way, I have a plugin called format_sciencelab. It has a handful of classes. I want those classes to be in one folder called format/sciencelab/classes; I don't want to have to make a separate level of folders for no reason just for those 5 files.

Here are two example class names:

format_sciencelab_filter
format_sciencelab_searchfilter

As I understand it, either proposal 2 or 3 (& possibly 1?) would result in me being able to put both those classes directly in the classes folder, which is where I have them now. So, yay! And 2 would give the added flexibility of letting me make classes called, I dunno,

format_sciencelab_creature_frog
format_sciencelab_creature_zombie

if I really wanted a 'creature' directory.

So basically, assuming I understood it right I'm down for option 2 if format_sciencelab_searchfilter is allowed as a class name (i.e. if you're allowed to optionally leave out the underline between words in the last bit). If it isn't allowed then I want 3.

One other thing - I am not too bothered about core code with one exception, I think it should go in /classes (/whatever/else) instead of /lib/classes (/whatever/else). The reason is that it's really annoying have Moodle core code dumped in the same place as third-party libraries (just from the point of view of e.g. wanting to exclude third-party stuff from searches, or something like that). This said... this wouldn't really solve the problem because of all existing stuff not being moved... maybe it would be more sensible to move the third-party stuff to a 'thirdparty' root directory? Hmm... OK, better leave this out...

So anyhow, I'm not that bothered about core.

--sam

Average of ratings:Useful (1)

Having read most of what has gone back & forth on this thread, I'm inclined to agree with Tim that I don't like excessive nesting of subdirectories (and I am an IDE user myself - PHPStorm).

Option 2 - mod/xxx/classes/aaa/bbb_ccc.php seems to me to be the best version for plugins - enough nesting to prevent excessive numbers of files in one directory, without the opposite problem of having to navigate through large numbers of subdirectories to find what you are looking for.

I'm not quite sure how that maps onto core libraries (and whether the same solution is needed for both plugins and core code).

I notice that the discussion has also moved on to namespaces, but that is something I can't comment on at all, as I have no experience of working with namespaces ...

Average of ratings: -

I would agree with Petr here but for a different reason because /cc/dd/ee/ff.php is standard in several standard frameworks out there like Symphony, which will make it easier for developers to transition between the two. Making it easier for PHP developers to start developing in Moodle.

Average of ratings:Useful (1)

Regarding underscores in plugin names (https://moodle.org/local/chatlogs/index.php?conversationid=12944#c439512 ). I suggest a hard-line approach. We will only make auto-loading work for plugins where the frankenstyle name plugintype_pluginname does not contain any underscores in the pluginname.

Many plugin types enforce that. Others only allow it for backwards compatbility. If you want to benefit from auto-loading, fix your plugin. Is that too harsh?

Average of ratings: -
I was complaining about underscores in plugin names long time ago, everybody ignored me - we will have to live with this forever. We can not ignore these plugins, sorry.

Average of ratings: -

+1.

Average of ratings: -

This looks great and currently, I'm in favor of the mod_xxx_aaa_bbb_ccc === mod/xxx/classes/aaa/bbb/ccc.php route out of the available options.

I was then thinking about the problem about plugins that have underscores in their name and we could do something like: mod_xxx_aaa_bbb_ccc actually goes to mod/xxx/aaa/bbb/ccc.php.  This breaks frankenstyle things, EG: blocks would be blocks_blockname_aaa_bbb_ccc and also plugins that are buried very deeply like grade/grading/form/rubric would have to have ridiculously long class names.  So, that idea sucks.

That naturally led me to namespaces.  We could do things like component is the namespace and then class names under that component use underscores.  So, mod_forum\aaa_bbb_ccc would map to mod/forum/aaa/bbb/ccc.php.  Even better, just go full namespaces, mod_forum\aaa\bbb\ccc would map to the same  mod/forum/aaa/bbb/ccc.php (Whatever PSR-0 does plus our special sauce to handle frankenstyle).  And yes, I saw above that namespaces was a no no, but I'm not really seeing the problem here if we are refactoring class names already to take advantage of autoloading.  We should use the best tool for the job and I think namespaces is the tool for this job.

Also, the above allows us to do some potentially faster autoloading because we can identify the component accurately and quickly.  From what I have seen, the quickest autoloaders are basically class maps.  EG: an array where the key is the class name and the value is the absolute class file path.  Doing one array map for all of Moodle would probably be silly and take up too much memory, but one map per component/subsystem that is cached somewhere doesn't seem so silly.

Lastly, you may have noticed that I dropped the classes directory from my examples.  The reason is so the autoloader could load files in component/backup, component/db, etc.  Unless the intention was to collapse all of those into the classes directory?

Cheers, Mark

Average of ratings:Useful (3)

I agree that this might be the moment to re-consider namespaces in Moodle. Can someone remind me why we rejected them last time?

Average of ratings: -

Hehe, I have just independently arrived to the same conclusion about namespaces (when discussing it with Fred), then Tim pointed me to this post, great!

I have already tested it and it seems like the best option to me, see second commit in https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading

The end result for developers would be:

1/ classes without namespaces

• new core_xxx_yyy_zzz() ---> lib/classes/core/xxx_yyy_zzz.php
• new mod_forum_some_thing() ---> mod/forum/classes/some_thing.php

2/ classes with namespaces

• new \core\xxx_yyy() ---> in lib/classes/xxx_yyy.php
• new \core\xxx\yyy.php() ---> lib/classes/xxx/yyy.php
• new \core\event\base.php() ---> lib/classes/event/base.php
• new \core\event\something_happened() ---> lib/classes/event/something_happened.php
• new \mod_forum\event\post_rated() ---> mod/forum/classes/event/post_rated.php

Average of ratings: -

Ah, great news!  Looking forward to trying this out!

After doing some benchmarking, if you find that things are a little sluggish, I recommend investigating the class map idea.  Composer somehow generates one automatically, so you might be able to borrow some code there.  I suppose though if you just scan a plugin's classes directory, it wouldn't be very challenging to build one.

Cheers, Mark

Average of ratings: -
Good idea, I have measured that the file_exists() is the slowest part.

The class map could be stored in the same cache file as the list of plugins. The only potential problem might be memory use, but I guess even few hundreds of kilobytes of data would be acceptable.

Average of ratings: -

So, Petr implemented this, Yay! (MDL-39854)

The problem now is what to do when you want to upgrade your code to take advantage (e.g. MDL-33071) and that requires you to rename your classes to have the proper frankenstyle prefix. For example, suppose you need to rename a class from quiz_old_name extends mod_quiz_new_name. This causes two sorts of code to break:

1. References to the old class name in executable code, e.g.

quiz_old_name::SOME_CONSTANT;

or

$x = new quiz_old_name(); 2. References to the old class name in function declarations. function do_something(quiz_old_name$x) {
}

This is most serious when ther is a method in a base class that you are overriding in your subclass.

You can fix many of the backwards compatibility problems by doing

/** * @deprecated since Moodle 2.6. Use mod_quiz_new_name instead. */class quiz_old_name extends mod_quiz_new_name { }

but it does not fix all the problems caused by 2. I have been playing with the following test script, without really reaching any conclusions:

<?phprequire_once(__DIR__ . '/config.php');interface testi {    public function imethod($x);}class mod_test { public function method1(mod_test$x) {        print_object('In method'); // DONOTCOMMIT    }    public function method2(test $x) { print_object('In method'); // DONOTCOMMIT }}/** * @deprecated since Moodle 2.6 */class test extends mod_test// If you uncomment the next line, you get strict standards warnings about// method1 and method2. Otherwise you don't!// implements testi{ public function method1(test$x) {        parent::method1($x); } public function method2(mod_test$x) {        parent::method2($x); } public function imethod($x) {    }}$x = new test();$y = new mod_test();// This works.$y->method1($x);// This fails with Argument 1 passed to test::method1() must be an instance of test. instance of mod_test given$x->method1($y);

Basically, I am wondering, what is the best way to upgrade my code to work nicely in Moodle 2.6, without breaking backwards-compatibility too much.

Note that it is very easy to search and replace quiz_old_name -> mod_quiz_new_name in any custom code, so perhaps we just need to move forwards, and annoy everyone my making them fix their custom code when they upgrade to 2.6. Having been on the other end of that sort of thing, fixing OU-specific code that was broken by Moodle HQ changes in core, I don't really want to do that.

Average of ratings: -