User talk:Dan Poltawski

Jump to: navigation, search

(truncated) Integrator discussion about core/subsystem confusion December 2015

Eloy Lafuente (stronk7) BEGIN of IGNORABLE RANT(s):

I've another disquisition in that tags issue, btw... and it's when it should be core or core_tag component. And, given that, when it should be under lib/classes or tag/classes.

We are mixing everything really badly. Some classes go to subsystem dir, events all go to core...

I agree that the main class is better in lib/classes so it's called core_tag (else it should be named core_tag_tag). But no matter of it, it's correct package is core_tag, not core.

But that's the only exception allowed to be @ /lib/classes. Any other class should go to tag/classes and be core_tag_xxxx

And about events... I still don't get why it was decided to move all them to lib/classes, but I hate that decision. Each component should handle its own ones. We do that for plugins but not for subsystems.

Also tests usually end under lib/tests... and not xxxx/lib/tests ....

Also, if we really don't want ANY db stuff in subsystems (install, upgrade, event observers, messaging, permisions, services, caches... we should forbid the existence of "db" for subsystems in order to "force" core (aka lib/db) to be used always explicitly.

I know we have other "core" classes there (core_user....) but the case is the same. They should be both @package core_xxxxx and @category xxxxxx (if the later API exists). Not @package core.

It's all related. Not critical at all but shows everything is not clearly ruled into our components/subsystems/apis world.

END of IGNORABLE RANT(s)

10:29:36 UTC
*Dan Poltawski ignores and asks if there is a way we can improve the situation without ranting? :) 10:35:30 UTC
Dan Poltawski pp 10:40:22 UTC
Dan Poltawski d 10:40:40 UTC
Eloy Lafuente (stronk7) Sorry, was afk. No, don't think we can ignite any movement about that right now. Surely need to have the X documentation system available and also a better phpdoc checker working in order to start moving the package/category/classes mess. 11:09:03 UTC
Eloy Lafuente (stronk7) so it was just a "rant to the air", sort of remembering there is some mess to organize out there at some point. 11:09:43 UTC
Eloy Lafuente (stronk7) note it would be perfectly possible for our autoloader, for example, to be able to load core_tag from tag/classes, instead of lib/classes. We just need to agree about it (or no). Once it's autoloaded, it's not big problem to proceed with the move later. 11:11:35 UTC
Eloy Lafuente (stronk7) or same applies to phpdoc checkers... it can be programmed to accept core_tag @package in lib/classes if the class name is the main one. 11:12:52 UTC
Eloy Lafuente (stronk7) I mean, it's just we have some "idiot" restrictions, both in the autoloader and in the phpdoc checker leading us to current class place and @package docs. 11:13:40 UTC
Eloy Lafuente (stronk7) About the core/subsystem rant... really I think we "banned" install & upgrade under sybsystem/db long ago. Not sure if we did the same for everything else, surely yes.

In any case the main problem with subsystems is that there is a clear overlapping between them and the official APIs we support. And that, mixed with autoload and with namespaces makes the thing really not deterministic.

Imagine you've the "search" subsystem but also want to allow "search" to be one of the official APIs (https://docs.moodle.org/dev/Core_APIs). We have a number of dualities like that.

And they lead to problems about where to put the classes. Coz you can have both:

lib/classes/core_search_xxxx search/classes/core_search_xxxx <= because it's a subsystem lib/classes/search/xxxxx <= because it's an api search/classes/search/xxxxx <= because it's a subsystem and an api


00:32:33 UTC
David Monllaó I don't have a clear separation in my mind between suybsystem and core API 00:35:22 UTC
Eloy Lafuente (stronk7) yeah, think that's pretty much the problem. 00:35:55 UTC
David Monllaó actually I mix that much both concepts that I thought they were the same 00:36:08 UTC
Eloy Lafuente (stronk7) and it was not much a problem, but when we did introduce namespaces, the problem grew 00:36:17 UTC
David Monllaó well, yes not all subsystems expose and API, but I thought all APIs are based on a single subsystem (although they depend on more) 00:36:56 UTC
David Monllaó for example badges, it is listed as a subsystem https://docs.moodle.org/dev/Frankenstyle#Core_subsystems, but not as a core API, shouldn't it be listed as a core API too? 00:38:00 UTC
Eloy Lafuente (stronk7) surely no coz it's not a transversal subsystem that others (core or plugins) can use. 00:38:39 UTC
Eloy Lafuente (stronk7) it's a functionality by itself 00:38:51 UTC
Eloy Lafuente (stronk7) for example, comments, ratings, tags... all them are subsystems... but with an API to be used by others. 00:39:20 UTC
Eloy Lafuente (stronk7) (surely search too is that sort of "transversal" subssystem) 00:39:45 UTC
Eloy Lafuente (stronk7) so, the code to support search in forums... is ok to go under forum/classes/search/xxxx 00:40:17 UTC
David Monllaó (backport requests voted) 00:40:34 UTC
David Monllaó ok, I get it 00:40:44 UTC
Eloy Lafuente (stronk7) but the search code itself is where the problem is. IMO it should ALL go to /search/classes 00:40:48 UTC
David Monllaó thanks for the explanation 00:40:49 UTC
David Monllaó it is both subsystem and API 00:40:57 UTC
Eloy Lafuente (stronk7) (but core_search, that right now cannot go there, unless we accomodate the autoloader to look there) 00:41:14 UTC
Eloy Lafuente (stronk7) and IMO, that's the more correct solution. 00:41:33 UTC
Eloy Lafuente (stronk7) avoiding core_subsystem to be @ lib/classes/subsystem and, instead, being selfcontained in its subsystem/classes/sybsystem 00:42:21 UTC
David Monllaó I don't expect much components using search API 00:42:31 UTC
Eloy Lafuente (stronk7) well, any plugin should, isn't it? 00:42:49 UTC
David Monllaó but if we allow it I udnerstand they should use lib/classes/search/core_search 00:42:57 UTC
Eloy Lafuente (stronk7) i mean, there will be a tokenizer, permssions... .some things to be defined by plugin component. 00:43:16 UTC
David Monllaó any plugin can implement search 00:43:17 UTC
David Monllaó but that would be using it as a subsystem isn't it? 00:43:32 UTC
Eloy Lafuente (stronk7) if you are "using it", then you are using its API. 00:44:35 UTC
Eloy Lafuente (stronk7) the public API of a subsystem 00:44:54 UTC
David Monllaó ok, comments example 00:44:57 UTC
David Monllaó I have comments/index.php this is the subsystem right? and a moodle plugin can use comments API to ...whatever 00:45:34 UTC
David Monllaó ok ok, I was saying the opposite, ok, makes sense 00:45:51 UTC
Eloy Lafuente (stronk7) heh, comments are too simple, haha. the api is basically 2 functions (add and get) 00:45:56 UTC
Eloy Lafuente (stronk7) imagine search... is really a good example. 00:46:12 UTC
David Monllaó so other plugins may implement search API to send data to the search engine 00:46:19 UTC
Eloy Lafuente (stronk7) what does a plugin need to implement search 00:46:30 UTC
David Monllaó and search/index.php, the subsystem, would list the results 00:46:38 UTC
David Monllaó 1 sec and I show you what I have in mind, I have a wip 00:46:59 UTC
David Monllaó https://github.com/dmonllao/moodle/compare/MDL-31989_master~3...MDL-31989_master#diff-ae87f97e046fb2d26ec7e4b6620fcedfR37 00:48:09 UTC
David Monllaó plugins need to implement search to specify what they are interested in sending to the search engine, forum posts, database entries, glossary entries... 00:48:42 UTC
Eloy Lafuente (stronk7) in other words... the code that forum needs in order to use the search subsystem (I imagine a process getting posts and sending them to the search indexer/engine). 00:48:53 UTC
David Monllaó this depends on the plugin type and the plugin 00:48:54 UTC
David Monllaó oh yes, ok, you was explaining it, I thought it was a question 00:49:29 UTC
Eloy Lafuente (stronk7) correct... and those individual implementations should go to plugin/classes/search/xxxxxx 00:49:32 UTC
David Monllaó yes 00:49:39 UTC
David Monllaó well, plugin/classes/search.php 00:49:46 UTC
David Monllaó I've added support for subsystems 00:50:03 UTC
David Monllaó anyway, ok, I get the idea 00:50:32 UTC
Eloy Lafuente (stronk7) aha, if not using namespaces, maybe. 00:50:38 UTC
David Monllaó do you talk about plugin/classes/search.php or "I've added support for subsystems"? 00:51:18 UTC
David Monllaó couldn't they go to tags/classes/search.php? 00:51:32 UTC
Eloy Lafuente (stronk7) if you're using namespaces (and I think that you are):

https://github.com/dmonllao/moodle/compare/MDL-31989_master~3...MDL-31989_master#diff-ae87f97e046fb2d26ec7e4b6620fcedfR25

I really think they should go to:

mod/forum/classes/search/indexer|searcher|whatever_the_class_does tags/classes/search/indexer|searcher|whatever_the_class_does

00:53:48 UTC
Eloy Lafuente (stronk7) I mean, they are an API use (level 2 namespace) 00:54:18 UTC
Eloy Lafuente (stronk7) to name a class with the name of an api is not ideal. 00:54:49 UTC
David Monllaó oh! ok ok 00:55:41 UTC
David Monllaó everything makes sense now 00:55:47 UTC
David Monllaó Yesterday I thought that things like core_search were allowed in lib/classes but not documented 00:56:45 UTC
David Monllaó (looking at what I wrote Yesterday) 00:56:53 UTC
Eloy Lafuente (stronk7) core_search is allowed.... more exactly... is the only place where it works and that's the reason we are putting all them there in lib/classes. 00:57:41 UTC
Eloy Lafuente (stronk7) my rant is that we should allow (instructing the autoloader to do that) them to be under search/classes/core_search (the main class). 00:58:36 UTC
David Monllaó if no other component than search is going to use core_search it could go to search/classes/manager 00:58:36 UTC
David Monllaó is it only a naming thing then or we have a strict rule that all subsystems expose stuff as core_subsystem? 00:59:13 UTC
Eloy Lafuente (stronk7) (note I've not looked what your main core_search class does). Just saying that it would be great to have subsystems señf-contained. Instead of current status where we MUST put the main class into lib/classes and others in subsystem/classes. Ideally the whole subsystem should be under /subsystem. 01:00:30 UTC
David Monllaó ok 01:01:17 UTC
David Monllaó makes sense 01:01:19 UTC
Eloy Lafuente (stronk7) Right now I see the core_tags, core_search, core_user classes (all them under lib/classes) like a wrong location caused by the limitations of the autoloader, not able to find them under their subsystems. 01:01:30 UTC
Andrew Nicols wait a sec... 01:01:33 UTC
Andrew Nicols So you want core_search to be loaded from search/classes/core_search.php instead of lib/classes/search.php ? 01:01:55 UTC
Eloy Lafuente (stronk7) correct. 01:02:09 UTC
Eloy Lafuente (stronk7) towards subsystem auto-contained. 01:02:25 UTC
Eloy Lafuente (stronk7) (keepng apart api uses that can be spread over all plugins as commented above) 01:02:42 UTC
Eloy Lafuente (stronk7) it could be search/classes/search.php too in fact, autoloader decides. where to find and what to find. NP here. 01:03:41 UTC
David Monllaó ok 01:03:57 UTC
David Monllaó actually what plugins implementing search extend is core_search\base.php 01:04:24 UTC
David Monllaó the current lib/classes/core_search acts as a core_search\manager 01:04:42 UTC
David Monllaó I should rename it 01:04:55 UTC
Eloy Lafuente (stronk7) aha 01:05:00 UTC
David Monllaó this was a very useful conversation Eloy 01:05:11 UTC
David Monllaó thanks 01:05:14 UTC
Andrew Nicols core_search\search 01:05:17 UTC
Eloy Lafuente (stronk7) again, note i don't know if names are correct or no. I just (headbang) about having subsystems stuff under lib/classes. 01:05:52 UTC
David Monllaó I would like to avoid core_search\search as all plugins implementing search name the class search, mod_forum\search, core_comments\search... and it might be condusing 01:05:58 UTC
Andrew Nicols I agree with renaming - I don't think that we should put exceptions into the autoloader. We should write a policy about naming and stick to it. 01:06:01 UTC
Andrew Nicols Agree with that. I hit the same thing earlier this week when I was tinkering with disguises 01:06:13 UTC
David Monllaó at least I would prevent having a new core_xxx into lib/classes 01:06:23 UTC
Andrew Nicols definitely 01:06:33 UTC
Eloy Lafuente (stronk7) only if they are subsystems 01:06:40 UTC
Andrew Nicols And raise an issue to move all of the existing ones perhaps. 01:06:41 UTC
Andrew Nicols Yup - for subsystems 01:06:52 UTC
Eloy Lafuente (stronk7) for example core_component or core_eloy are perfect. 01:07:00 UTC
Eloy Lafuente (stronk7) but core_user, core_tags, core_search are not 01:07:14 UTC
David Monllaó makes sense 01:07:30 UTC
Andrew Nicols I dislike the way that we do all of this stuff. We should have really namespaced everything with moodle in the first place, but that's done now and hard to move back on. 01:07:36 UTC
David Monllaó (offtopic: I'm tempted to just trigger the run phpunit mssql in nightly, the queue seems complex) 01:08:10 UTC
Eloy Lafuente (stronk7) yep, agree with both sides of your paragraph. 01:08:14 UTC
Andrew Nicols moodle\user and moodle\user\foo, rather than core_user and user_foo 01:08:16 UTC
Andrew Nicols Anyway, +1 to rename the class + write some docs; -1 to add exceptions to our autoloader 01:09:04 UTC
Eloy Lafuente (stronk7) well, not sure if it's exception. or missing thing. in any case, really class names should be more descriptive than the subsystem name, agree (manager, indexer, searcher....) 01:10:35 UTC
Eloy Lafuente (stronk7) I can imagine a core_user class being legit name, for example. 01:11:14 UTC
Eloy Lafuente (stronk7) but hate having it @ lib/classes :-) 01:11:38 UTC
Eloy Lafuente (stronk7) anyway 01:11:41 UTC
Eloy Lafuente (stronk7) at least hope it's now a bit clearer

1) under subsystem, all the stuff to make it work. 2) under plugin/classes/subsystem/the uses of the subsystem api.

01:13:04 UTC
Eloy Lafuente (stronk7) (for subsystems with an api exposed to other components) 01:13:25 UTC
David Monllaó  :yes: 01:13:55 UTC
Eloy Lafuente (stronk7) api/inheritance 01:13:56 UTC