User talk:Dan Poltawski

(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