User talk:Dan Poltawski
From MoodleDocs
(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):
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 |
