Creating a new issue to actually get a patch together for module/theme based PSR-0 classes. As I observed in #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch]:
1.) I'm using drupal_parse_info_file() here and that's really bad because drupal currently doesn't call very often cause it's kind of expensive. In short we should probably have a cache of some sort here, but I just want to get this ball rolling.
2.) I've added this into system_list() because from previous conversations with catch I _THINK_ this is where he had said something like this should probably reside.
3.) It seems totally reasonable to me to implement both a namespaces and a fallbacks array into the info file and let modules define both of these things as I would imagine most classes provided by core (at least) would probably want to allow contrib to override them so they should probably be a fallback and not a namespace, still that seems an exercise that's sort of irrelevant at this stage, but I wanted to mention it.
Providing a patch without dsm()s in it.
Comment | File | Size | Author |
---|---|---|---|
#59 | module-PSR0-fix-1467126-59.patch | 2.16 KB | effulgentsia |
#58 | module-PSR0-fix-1467126-58.patch | 897 bytes | yched |
#57 | 1467126.patch | 897 bytes | catch |
#56 | 1467126.patch | 1.3 KB | catch |
#50 | platform.classloader.37.patch | 6.91 KB | effulgentsia |
Comments
Comment #1
RobLoachI had a patch somewhere to cache the information from the system table somewhere, not sure where it went, and Drupal.org's search is pretty slow currently....
Can you think of any use-case where we would need fallbacks?
effulgentsia posted the following in #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch]:
The idea is there, but Drupal Core doesn't need the PEAR prefixes, and the paths should be relative to the module's directory. Unix Philosophy, please.
user.info
This gives the developer strict access to define where the PSR-0 classes reside, and what namespaces they are.
This should probably wait on #1451524: Introduce new bootstrap phase for initializing class autoloading so we could pass some of it off cleanly through the bootstrap.
Comment #2
EclipseGc CreditAttribution: EclipseGc commentedWell the obvious use case for fallbacks is core IMO. If I am (for example) providing an abstract block class upon which all other blocks are built, then having the ability for contrib to hijack and replace that abstract with another class of the same name (in case I miss some use case or something) seems really appealing. All of core seems a candidate for this sort of behavior.
As far as the info files go, I really wish this all existed in a single file somewhere we could just load super early, but I think anything resembling a registry will get me lynched so, we'll see what other solutions emerge here.
Eclipse
Comment #3
RobLoachThe correct way around this is to inherit from the original class and override its functionality.
Symfony's Dependency Injection component touches on this subject. Fabien wrote a nice article series on Dependency Injection. It might be another good candicate for Drupal core.
Comment #4
EclipseGc CreditAttribution: EclipseGc commentedInheriting from the base and extending it won't do anything, that's what all the other classes do, and if you wanted to change the abstract class' functionality for all blocks that extend it, you've failed. Making it's namespace a namespace fallback however would give your module the ability to register the same namespace (not as a fallback) and you could provide a class of exactly the same name, and then all blocks that extended the original abstract would extend your abstract instead.
Comment #5
RobLoachThe problem with this is that you can end up with Dependency Inversion if you have a module re-implementing a Core class as the new class can have false assumptions.
Here's an example:
eclipsemodule/lib/Drupal/Core/Database/Connection.php re-implements the Connection class and adds a abstract function to the class. core/lib/Drupal/Core/Database/Driver/mysql/Connection.php is now broken because it does not implement that abstract function.
Open Close Principle states that "entities like classes, modules and functions should be open for extension but closed for modifications". This is why you do not want to replace Drupal Core's abstract classes and instead you want to inherit. It is rather out of scope for this issue, but I just wanted to get that out there. This issue is just to provide the namespace registration for /lib of modules.
In the Symfony module, it is done during hook_init(). It would be nice if Drupal Core did it a bit sooner than that though.
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedWhich is exactly why you'd use a namespace instead of a fallback in that case, though I'd also argue that if you are altering a base class like this and you don't know what you're doing or how to keep things all up to date, then you get what you deserve. It's the equivalent of adding form fields via an alter hook and wondering why they're not getting validated and saved.
Eclipse
Comment #7
sunLet's not turn this into a fruitless debate about programming patterns, please.
We have an autoloader. It supports different ways to register things. The meta information of an extension should be able to use all of them.
This patch should focus on these points mentioned in #1290658-140: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch]:
The main questions to ask are:
If yes, then we need two separate lists instead of one.
#1404198: Separate database cache clearing from static cache clearing and data structure rebuilding might be an important puzzle piece for this, as the list only needs updating when the code changes, and that patch introduces a global system-wide rebuild operation.
Extensions may provide class implementations for early bootstrap functionality. Therefore, the list should ideally be available at the earliest point in time. Lacking a facility for that, the earliest point in time seems to be the configuration/variable subsystem, so I'd suggest to store it there.
Idem, ASAP.
Baking this into system_list() does not only overload that function with different functionality, leads to performance problems, but is also way too late IMHO.
Comment #8
Crell CreditAttribution: Crell commentedI would mostly agree with sun here. Let's keep it simple, and as early as possible. We also should not try to address core-override logic right now. That's an edge case, and while it's cool that we'd be able to override core classes via class loader trickery in *most* cases there are, or should be, a cleaner way.
As I noted in IRC earlier, I'd suggest approaching it from a different angle. Starting from a CLI command (possibly based on Symfony's CLI component, possibly not), write the bare-bones script that starts up the core class loader, finds all module info files, generates a list of namespaces to register (and nothing else), and stick it somewhere crazy-early-accessible. Database is probably OK for now, since Drupal doesn't really work without the DB anyway. If we find a better alternative later, great. Just make sure that the rebuild process can run even with a stale, incorrect, or missing index.
Once that's done, put a button somewhere to run the same rebuild code. Problem solved.
Comment #9
sunWe need to care for two scenarios:
No list is available, because there's not even a storage the list could be retrieved from. For this case, we typically have special maintenance mode implementations that don't even try to consult the module system or database. Only in this case, it is acceptable to simply scan the filesystem for .info files and register everything for autoload. (this already happens somewhere in install.inc or so)
See #7. To amend @Crell, the list must only contain enabled extensions, so scanning the filesystem as in case 1. would be incorrect. As of now, the information about enabled extensions is only contained in {system}, so we need to query that.
Comment #10
Crell CreditAttribution: Crell commentedI think sun and I are saying mostly the same thing. :-) The rebuild process should not depend on any code not provided with core. That way, it can always run without worrying about a circular dependency on the index it is trying to build. (And yes, scan only for enabled modules.)
I disagree with continuing the "maintenance mode" hacks if we can avoid them. Rather, just make the rebuild system not depend on anything that it would be rebuilding, even indirectly, and then we don't need to worry about it. Depending on the DB is fine, since the DB is supported by core and only needs the hard coded core class paths to work.
Comment #11
catchThere's no need to use drupal_parse_info_file() here, the entire contents of the system table are already loaded into memory during system_list() so that information is available.
The fact we cache the entire contents of the system table is a bug, documented at #1061924: system_list() memory usage.
For now, since that bug is not fixed yet, we can just use the info file already available. However after that's fixed, I would think $lists['filepaths'] could hold the additional namespace roots registered by modules if there are any, so it's not showstopper.
@sun:
EclipseGc suggested this on irc and I spent a long time trying to persuade him this is a bad idea. We can not have any more cases of early bootstrap stuff depending on the module system, because that leads us back to #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap. If a module provides classes, then classes of the module should be available when the module is, and not before.
While some modules ship with classes like cache backends (such as memcache), those are never handled by the module system itself and this is not the issue to try to introduce that, it's just an accident of Drupal.org project module that this is the case and it's a very bad thing in general.
If we get to #1331486: Move module_invoke_*() and friends to an Extensions class then we are going to have to re-think bootstrap modules altogether, my preference would be to completely remove the concept, putting aggressive caching back in as a UI option and just load modules early so we can stop finding recursive dependency bugs all over core.
Comment #12
neclimdul@catch I agree, this probably isn't the issue to introduce it. I also find it frustrating that while asking not to discuss it in this issue, you're also pointing out problems. Makes rebutting any arguments you pose on unequal footing because of that.
@everyone We have some serious discussions about bootstrap availability for the "module system" that we're just bumping into tangentially here. It's always been sort of an issue which is why we have special module load levels. I'm going to suggest lets agree to table that discussion and get what we can in for now. We can get all frustrated with the problem again when we start use PSR-0 for classes that actually expose the problem. Its then that we'll really be able to identify the functional issues that are the root of the problem rather than speculating and architecting a system off that speculation.
Comment #13
EclipseGc CreditAttribution: EclipseGc commentedI'm just going to heartily agree with neclimdul here. We need a solution sooner rather than later, and if we make it so far as to looking at these early bootstrap systems in this cycle, and how psr-0 etc affect that, GREAT, but this issue is about getting module classes registered. Can we assume for the sake of argument that we're only worried about classes that would be available at the same level as the modules that provide them? That would speed things along greatly here. We all acknowledge there's a bigger conversation to have, but we're more likely to see the nuances involved in that conversation if we allow people to start bumping into the problem, which will happen sooner if we get a solution to this issue, rather than bikeshedding the details regarding it and pushing out real world scenarios however long that bikeshed takes.
Comment #14
pounardAgree with EclipseGc here
Comment #15
RobLoachAlong with #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo), the attached patch registers each module's
/lib/
directory as Drupal\$modulename . With this, we can have Drupal\system\MyClass registered at modules/system/lib/Drupal/system/MyClass.php.Todo
namespaces[] = Drupal\System
prefixes[] = Drupal\Core
if we want to support replacing classes.$loader->register(TRUE)
allows that. The "correct" way of accomplishing that is with dependency injection, but it is something to think about.Comment #17
EclipseGc CreditAttribution: EclipseGc commentedI really don't think we should register paths that don't exist. Assuming all modules provide a lib directory is probably a bad assumption long term.
Comment #18
catch@neclimdul, my comment doesn't say don't discuss it, just that we should not introduce it here. If people want to discuss it that's fine, but it's likely to take what could be quite a simple issue down a very deep rabbit hole.
Comment #19
neclimdulcatch ok, then I'll say lets not discuss it. I think we can all agree at least its a deep rabbit hole we only disagree on if there's good or bad on the other side. There's enough to be gained in this issue without that discussion(yet).
Any ideas why rob's patch failed testing? Seems like a trivial change...
Comment #20
Crell CreditAttribution: Crell commentedIt's probably failing because it relies on the Composer loader rather than Symfony loader, by the looks of it. (Symfony loader doesn't require you to recall ->register()). See #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo)
Comment #21
RobLoachThanks to Seldaek's comment in the Composer thread, it's actually one less line of code than I had originally thought.
It's actually faster than hitting the file system to check if the lib directory exists. It makes no sense to use Drupal\user\MyMadeUpClass if you know the class doesn't exist. Whether or not we have the file system check, using MyMadeUpClass will still result in a fatal error. That's why we just add the namespaces, and let the ClassLoader figure it out for us.
Comment #22
catchI've thought about this some more, and I think we should not support namespaces[] in the initial patch. We don't need that for core yet, and nothing stops a module registering namespaces with the autoloader themselves even if we never added it. The main reason for this is that it will avoid some complexity dealing with bootstrap vs. non-bootstrap modules which I would love to see resolved, but is unlikely to be before this issue goes in. Then there's two follow-ups, whether or not to register classes with the autoloader before the module itself is loaded or not, and how to support additional namespaces in modules.
Comment #23
catchAnother reason not to do it in the first patch, is because if namespaces[] is in .info files, then we're making the class loader dependent on hook_system_info_alter() (i.e. nothing stops me from adding or removing namespace declarations in that hook and expecting it to work, which is one of the problems we have with the registry now).
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedSame as #21, but for the Symfony autoloader (let's keep this issue decoupled from switching to Composer), with DRUPAL_ROOT prepended to the directory name, and with tests.
I agree with making anything .info file related into a follow-up, so that conversations about that can proceed without holding up work on plugins/blocks, which for the moment don't require anything but the default namespace.
Comment #25
neclimdulNot that it functionally matters but I thought we did explicit parenthesis like
new Foo()
.Other then that looks great. I'd feel better if we did this incrementally too and let and Composer just take advantage of the work that this puts in place.
Comment #26
donquixote CreditAttribution: donquixote commentedI suggest changing the issue title to sth like
"Configure PSR-0 autoloading in $module_dir/lib/ (temporary)"
The term "provider" has quite a vague meaning to me, and any talk about info files and fallbacks has been postponed / moved elsewhere (where, btw?). The $module_dir/lib/ for PSR-0 is all that is left. Also, in contrast to #1290658, this issue is about implementing a policy, not about deciding on one. The issue title should reflect this.
"temporary" to reflect the pending status of #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch] (can be removed once that is decided).
-------
I am not going to change the title myself, rather leave it to someone more involved in the work being done here.
And btw, I am ok with this as a logical step following #1290658 - no complaining this time :)
I would have some comments about bootstrap order, info files, and fallbacks vs namespaces, but I think this should go somewhere else. Any idea where?
Comment #27
EclipseGc CreditAttribution: EclipseGc commentedIt's more than just modules. My initial patch reflected this (though I'm certainly NOT pushing for that patch) but themes (for instance) should be able to provide classes as well, so this is not just about modules.
Again I will accept any solution that works for my block needs right now, however I want to make sure the point is made.
I've been using essentially Effulgentsia's patch in my development (minus all the fancy test updates cause he's just awesome like that and I'm not) so from a functional perspective, I won't fight that patch at all. Anyone else tried it? or do I need to be the person to RTBC it?
Eclipse
Comment #28
RobLoachWe're definitely not blocked by this... #1477218: Convert Tracker tests to PSR-0 / PSR-0 test class discovery makes Tracker register its own namespace. It's easy to move namespaces later on if we want to change it later. Otherwise, we're caught in an endless bikeshed like #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch].
Namespaces should be CamelCased, something like this would suffice:
Sun had a better solution in #drupal-wscii a couple days ago. He also benchmarked against a regex solution and it proved that using regex for it was slower.
Comment #29
sunBenchmark script in http://drupalbin.com/20980
Comment #30
Crell CreditAttribution: Crell commentedI disagree with camelcasing the namespace. Whether str_replace or regex is faster, both are slower than not bothering and letting it be literal. Remember, lots of modules already concatenate their names rather than use underscores. So "cacheexclude" would turn into "Cacheexclude', not what you'd expect it to be while reading it, "CacheExclude". (That happens to be one of my modules, but there are thousands of others.)
Normally I prefer properly camelcased namespaces, but for semi-magic naming I'd rather keep it as simple as possible. (Database drivers are lower-cased for the same reason.)
Comment #31
donquixote CreditAttribution: donquixote commented> Namespaces should be CamelCased
I would have thought this is off-topic here.
But, if you choose to bring it up:
Imo, the tradeoff here is superficial aesthetics vs technical clarity. And I totally support the latter.
Seeing the literal module name with underscores may look ugly, but it shows what we are dealing with: A Drupal module, not a whatever namespace fragment.
Comment #32
EclipseGc CreditAttribution: EclipseGc commentedI agree with Don, but again if we're going to argue it, I'm going to concede it just so we can keep moving. I'd suggest everyone else do the same. Time is WAY more important than camel casing (no matter what side of the argument) so let's NOT have one.
Comment #33
pounardI'm OK with default module namespace being lowercase, as long as modules can arbitrarily tell what their namespace is (info file or whatever other registration mecanism).
Comment #34
neclimdulI think its totally off topic. 1) its too magic to be predictable enough to be correct for all modules. 2) you can register your CamelCased namespace as Robert demonstrated 3) at some point we(maybe just me) want a way modules can register any namespace more cleanly.
This is probably performs better but wouldn't it make more sense to put this in drupal_load so at whatever point or method the module is loaded, the namespace is properly registered?
Comment #35
sunon #24: I stared at the call chains and different code path scenarios for the past 30 minutes, and I'm not sure whether module_load_all() is actually correct, or whether system_list() isn't actually the right location. Anyway, I think we need to figure that out in the long run, via follow-up issues.
re #34: drupal_load() can be invoked at any time, for any module - even disabled/not-installed ones. I wanted to reply that we probably don't want to do that, as that would register the namespaces of an otherwise not available module, but apparently, one reason for allowing that and doing so is that DrupalUnitTestCase tests currently need to invoke drupal_load() manually to actually load the .module code that they want to test. Thus, registering the module's classes at the same time would make sense.
(Obviously, tests are a long shot from this issue here, but well...)
In any case, the namespace should be registered before a module is loaded - because the .module might already contain use statements for its own classes.
Fixed that in attached patch, and moved the registration into drupal_load() for now.
Furthermore, moved the essential code into a separate drupal_classloader_register() function (and slightly tweaked drupal_classloader() to ensure it's lightning fast), so we can change our minds from where to call it more easily.
Lastly, pushed this code into a dedicated branch: http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/platfo...
Comment #36
sunComment #37
sunApparently, the module might even contain use statements for classes of another module. But modules may be loaded in semi-arbitrary order. In turn, it would be best if we'd register all module namespaces before we attempt to iterate over them and load them. In turn, that pretty much circles back to the system_list() approach.
Thus, attached patch moves the registration back into system_list(). (along with some other changes; see log)
Comment #38
Crell CreditAttribution: Crell commentedJust a note, a use statement does not trigger autoloading. Only executable code triggers autoloading. That means as long as you don't call any functions in a .module file that use a class that is not loaded, the code can be parsed without breaking anything. However, I know there are some modules that (wrongly IMO) put code in the .module outside of a function as a form of extra-magic pseudo-hook. That code would be affected. If we do not care about that code working because it's bad practice anyway, then the concern about registering namespaces before any .module file is not really warranted.
That said, there's nothing wrong with registering all namespaces first so we can go ahead and do so.
Comment #39
catchCould this be just a require then?
Apart from that I don't see anything to complain about. system_list() feels like the right place to do this (considering all the code around loading modules is in dire need of an overhaul, but within that mess it feels like the right place), and test addition is a nice bonus.
Comment #40
neclimdulI still feel like drupal_load is conceptually the right place but then I also learned yesterday that drupal_load() can only load modules despite its documentation and is basically just a wrapper with a static around the include_once to avoid calling it more then once. So if drupal_load actually loaded things it would be the perfect place but what ever works I guess.
edit: adjust wording to emphasis silliness of drupal_load()
Comment #41
amateescu CreditAttribution: amateescu commentedSwitched from require_once to require in http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/platfo...
Edit: I'm posting the url to my branches for all the platform patches just to let sun know that he has to merge before making any additional changes ;)
Comment #42
pounardWhy switching to require() instead of require_once()?
Comment #43
amateescu CreditAttribution: amateescu commentedBecause we know we'll only call it once. Micro optimization I guess :)
Comment #44
pounardSounds like a inefficient idea, require_once() is the safer version of require(), require() does not make any sense here since we're loading a class and some other piece of code could try to do it after or before. And using an opcode cache or not, require() vs require_once() fight is the least of our problems. A lot of benchmarks tends to contradict themselves regarding this fight. The only way to efficiently optimize a require call is not to do it, or use APC with stat disabled.
EDIT: As soon as a file contains symbols that can be defined only once, using a require() may generate WSOD, especially if arbitrary code (a module in our case) interact with a lower bootstrap level and pragmatically require(|_once)() the same file: in that particular case, require() is not an option anymore, require_once() *must* be used, it's both semantically more accurate, and also safer. Using require() in an autoloader makes sense, because the autoload is actually triggered only if the looked up symbol doesn't exists yet in memory, so it's safe, but in any other case, it isn't.
May I also add that the test on top of the require_once() statement in this patch is not done on the symbol existence (class_exists() usage) but on an arbitrary boolean that is not tied to it.
Comment #45
neclimdulIf we're going to have this discussion lets just take the optimization out of the patch since its not necessary. We can discuss the semantics of require_once and how to use it as an optimization anywhere. That or catch chooses one of the optimized versions and we bikeshed the optimization separately since non of these options break HEAD. Either way we've been holding this off long enough that we don't need to get caught up in this and I think everyone can agree on that.
Comment #46
amateescu CreditAttribution: amateescu commentedI don't have any strong preference on require()/require_once(), I just rolled that patch because catch asked for it.
So, we have two viable patches, #37 and #41. Let's get one in! :P
Comment #47
pounardYes, sorry for that bikeshed, let's just pick one and commit this!
Comment #48
catchI don't really care too much about the require vs. require_once, it's just a bit messy optimizing it away with a static cache but keeping require_once, we already do that in _drupal_bootstrap_full() as well so meh.
Agreed with the RTBC (for #37). It'd be good to get the other issue resolved before we convert too many classes, but I imagine it's going to end up matching the scheme here anyway.
Comment #49
sun@pounard makes a very good argument for the require_once in #37, and I think he's right.
#37 can be committed as patch, or pulled in from the platform-classloader branch (which doesn't contain the require change).
Would love to see this landing ASAP. I think it's trivial enough (and can and probably will be changed later anyway) to get in soon.
Comment #50
effulgentsia CreditAttribution: effulgentsia commentedReuploading #37 as clarity that that's the one that was RTBC'd. Thanks for all the great work everyone. This looks ready to me too.
Comment #51
catchOK I've gone ahead and committed/pushed this to 8.x. I'd like to see the other issue finally resolved before we do a mass conversion of existing core classes to match this - we should open the follow-up to track that though.
This will need a change notification too.
Comment #52
Dave ReidThis seems to have broken HEAD: http://qa.drupal.org/8.x-status
Comment #53
Dave ReidThis is missing the new files added:
core/modules/simpletest/tests/module_autoload_test/lib/Drupal/module_autoload_test/SomeClass.php
core/modules/simpletest/tests/module_autoload_test/module_autoload_test.info
Comment #54
catchSorry folks, pushed the new files...
Comment #55
yched CreditAttribution: yched commentedThe committed patch just doesn't work. The module list is build once and cached. So the code block where the namespaces are registered is not executed once the cache is primed :-)
No time for a patch tonight, though.
Comment #56
catchWhoops. Here's a patch.
Comment #57
catchAnd here's one without the not-entirely-unrelated change that stops the registry from scanning includes but shouldn't be in there.
Comment #58
yched CreditAttribution: yched commentedYup, but by that time, it's not $record anymore, it's $item ;-).
Comment #59
effulgentsia CreditAttribution: effulgentsia commented#58 looks good. This just modifies the test to fail with HEAD and pass with this patch. RTBC cause the test change is trivial, so catch's review of it will be sufficient.
Comment #60
jthorson CreditAttribution: jthorson commentedManually killed the testbot running #57 after 577k assertions. Suggest we don't re-test it. :)
Comment #61
catchOK I've committed the follow-up, back to change notification I think.
Comment #62
BerdirWhat exactly does need to be "change notified"? Just the newly added function or also how modules now have to structure/place their classes according to PSR-0 in a lib folder?
Comment #63
catchThe latter I think, also that files[] is no longer necessary for PSR-0 classes.
Comment #64
Crell CreditAttribution: Crell commentedShould this perhaps be a single change notice for "the registry is dead", "no more files[]" and "use $module/lib/ with PSR-0"? We could write that now or later after the registry is finally put out to pasture.
Comment #65
catchYeah that makes sense. Actually removing the registry is going to require refactoring simpletest's file discovery, so it could take a bit longer than converting all the runtime classes.
Comment #66
effulgentsia CreditAttribution: effulgentsia commentedIf we're gonna make the change notice say that modules should start namespacing their classes and putting them in $moduledir/lib, then I think we should first make at least one module in core follow that recommendation. Shall we start with SearchQuery? I think that has the fewest dependencies, so should be trivial to move and rename.
Comment #67
catchYep, there's a patch at #1513970: Convert SearchQuery to PSR-0 needing review.
Also see #1513210: Meta: Start converting module provided classes to PSR-0 for more follow-ups.
Comment #68
BerdirSearchExtender being trivial is what I thought as well ;) It's not exactly true, unfortunately... looks like we kinda forgot (or postponed?) converting Query Extenders to PSR-0...
Converting test classes (and test class discovery) is discussed in #1477218: Convert Tracker tests to PSR-0 / PSR-0 test class discovery. Should test classes be part of the same change notice or a different one? E.g. because it's being discussed in that issue if tests should live at $module/tests instead of $module/lib.
So I guess we could start with a change notice or we could completely postpone it until we have at least a single module converted.
Comment #69
effulgentsia CreditAttribution: effulgentsia commentedI RTBC'd #1513970: Convert SearchQuery to PSR-0. Nothing there jumped out at me as questionable or controversial. Should there have been? If so, please comment on that issue.
Comment #70
webchickI just added a note about $module/lib/ and files[] to the pre-existing APi change notice about PSR-0 autoloading: http://drupal.org/node/1320394
I think that's sufficient for this issue. If you disagree, feel free to re-open.
Comment #71
aspilicious CreditAttribution: aspilicious commentedSrry I have to reopen this. Namespace documentation is very important. At this moment the coding standards doesn't mention module namespacing at all. I spend to much time googling to find the issue with the proposed standard.
Webchick added: "Modules should place their classes in a $module/lib/ folder."
This doesn't mention how the folder structure in fact should look like. For example:
$module/lib/Drupal/Core/$module is allowed according to that rule. That's incorrect it should be $module/lib/Drupal/$module
As this is very important I suggest 2 things:
1) Add proper documentation to http://drupal.org/node/1353118
2) Create a change notice with "namespace(s)" and "modules" in the title so it can't be found easily.
Comment #72
xjm@aspilicious -- You could correct the example in the existing change notice? I assume we'll also add more detail and examples for the modules in #1513210: Meta: Start converting module provided classes to PSR-0?
Comment #73
neclimdulBecause this hasn't been about anything but modules for a long time and it keeps throwing me off.
Comment #74
EclipseGc CreditAttribution: EclipseGc commentedBecause we should really think about more than just modules.
Comment #75
sunFor anyone else following at home: The last two follow-ups merely debated the issue title.
/me had no idea what these guys are ranting about, since issue title diffs are not sent in e-mail notifications...
Comment #76
RobLoachUnfollowing this issue. I have absolutely no interest in PSR-0 being added to themes or profiles. Themes should be strictly templates, and profiles should just point to a bunch of modules and maybe some initial glue configuration. I'm also assuming that the other PSR-0 issues will end up providing enough examples and documentation for people to "just get it".
Drupal core:
core/lib/Drupal
Drupal modules:
modules/{modulename}/lib/Drupal/{modulename}
Comment #77
aspilicious CreditAttribution: aspilicious commentedOk I'm going to add documentation myself it was not the intention to start a new discussion I just needed an issue to point to missing docs and documented standards.
Feel free to change the docs if they are incorrect because I'm just guessing by looking at examples.
Comment #78
aspilicious CreditAttribution: aspilicious commentedComment #79
aspilicious CreditAttribution: aspilicious commentedSrry, changing priority
Comment #80
sunPSR-0 is also required for profiles and themes, because both should at least be able to contain tests. Tests are not supported for themes currently, but I consider that as a bug. Tests for profiles are absolutely essential.
But anyway, I'd be fine if the change notice focuses on modules. We can add final note to state that the identical pattern applies to profiles and themes as well. This needs to be implemented first though. We can do that in a separate issue.
Comment #81
neclimdulWith sun here. This issue was decidedly finished we where just putting up docs and stuff. Can we go back to that state and follow up with other things? I'm very sorry my title change stirred up so much controversy about this.
Comment #82
effulgentsia CreditAttribution: effulgentsia commentedI opened a new issue for themes: #1538478: Register lib/ directories of themes as PSR-0 roots for Drupal\$theme namespace.
Profiles are modules according to the {system} table, so no new issue for that needed, I think.
Comment #83
effulgentsia CreditAttribution: effulgentsia commented@aspilicious: FYI: as per #69, search.module in HEAD has an example of a PSR-0 class inside "lib".
Comment #84
donquixote CreditAttribution: donquixote commentedI think the term "extension" would cover all three, and it is a lot less confusing than "provider".
If not for the issue title, then at least for documentation.
I would personally find it weird if we do not do the same for themes and profiles that we do for modules. If that needs to be a separate issue, fine.
Comment #85
sunI've clarified the change notice on http://drupal.org/node/1320394