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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Status: Needs review » Needs work

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.

I 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....

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

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]:

namespaces[Drupal\user] = ""                      ; or "/src" or "/app", etc.
namespaces[Drupal\user\SomeSubNamespace] = /app   ; or whatever
prefixes[SomePrefix] = /lib                       ; or whatever
namespaces[Symfony\Component\HttpFoundation] = /vendor

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

namespaces[Drupal\User] = "lib"

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.

EclipseGc’s picture

Status: Needs work » Needs review

Well 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

RobLoach’s picture

contrib to hijack and replace that abstract with another class of the same name

The 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.

EclipseGc’s picture

Inheriting 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.

RobLoach’s picture

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.

The 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.

EclipseGc’s picture

Which 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

sun’s picture

Let'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]:

  1. a list of enabled system extensions
  2. the filesystem path to each extension

The main questions to ask are:

  1. As this pertains to PSR-0-based autoloading, do we need to care for the bootstrap status of extensions at all?

    If yes, then we need two separate lists instead of one.

  2. At which exact point do we create the list?

    #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.

  3. Where do we store the list?

    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.

  4. When do we register extension namespaces?

    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.

Crell’s picture

I 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.

sun’s picture

We need to care for two scenarios:

  1. pre-/fail-system (install.php, update.php, fatal/non-db maintenance theme)

    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)

  2. regular operations

    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.

Crell’s picture

I 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.

catch’s picture

There'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:

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.

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.

neclimdul’s picture

@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.

EclipseGc’s picture

I'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.

pounard’s picture

Agree with EclipseGc here

RobLoach’s picture

FileSize
877 bytes

Along 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

  • Have clean namespace names loaded from the .info file: namespaces[] = Drupal\System
  • We could also support 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.

Status: Needs review » Needs work

The last submitted patch, module-lib-classes.patch, failed testing.

EclipseGc’s picture

I 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.

catch’s picture

@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.

neclimdul’s picture

catch 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...

Crell’s picture

It'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)

RobLoach’s picture

FileSize
772 bytes

Thanks to Seldaek's comment in the Composer thread, it's actually one less line of code than I had originally thought.

I really don't think we should register paths that don't exist.

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.

catch’s picture

I'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.

catch’s picture

Another 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).

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.68 KB

Same 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.

neclimdul’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/tests/module_test.moduleundefined
@@ -104,6 +109,23 @@ function module_test_hook_dynamic_loading_invoke_all() {
+  try {
+    $obj = new Drupal\module_autoload_test\SomeClass;

Not 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.

donquixote’s picture

I 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?

EclipseGc’s picture

It'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

RobLoach’s picture

We'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:

$module = 'views_convert';
$namespace = str_replace(' ', '', ucwords(str_replace('_', ' ', $module)));

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.

sun’s picture

$name = implode(array_map('ucfirst', explode('_', $module)));

Benchmark script in http://drupalbin.com/20980

Crell’s picture

I 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.)

donquixote’s picture

> 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.

EclipseGc’s picture

I 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.

pounard’s picture

I'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).

neclimdul’s picture

I 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.

+++ b/core/includes/module.incundefined
@@ -20,8 +20,12 @@ function module_load_all($bootstrap = FALSE) {
   if (isset($bootstrap)) {
+    $loader = drupal_classloader();
     foreach (module_list(TRUE, $bootstrap) as $module) {
       drupal_load('module', $module);
+
+      // Register the module's namespace to the module's lib path.
+      $loader->registerNamespace("Drupal\\$module", DRUPAL_ROOT . '/' . dirname(drupal_get_filename('module', $module)) . '/lib');

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?

sun’s picture

on #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...

sun’s picture

Status: Needs work » Needs review
sun’s picture

the .module might already contain use statements for its own classes.

Apparently, 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)

Crell’s picture

Just 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.

catch’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -2865,15 +2865,15 @@ function drupal_get_complete_schema($rebuild = FALSE) {
   if (!isset($loader)) {
+    // Include the Symfony ClassLoader for loading PSR-0-compatible classes.
+    require_once DRUPAL_ROOT . '/core/vendor/Symfony/Component/ClassLoader/UniversalClassLoader.php';

Could 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.

neclimdul’s picture

I 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()

amateescu’s picture

Switched 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 ;)

pounard’s picture

Why switching to require() instead of require_once()?

amateescu’s picture

Because we know we'll only call it once. Micro optimization I guess :)

pounard’s picture

Sounds 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.

neclimdul’s picture

If 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.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

I 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

pounard’s picture

Yes, sorry for that bikeshed, let's just pick one and commit this!

catch’s picture

I 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.

sun’s picture

@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.

effulgentsia’s picture

Reuploading #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.

catch’s picture

Title: Provider based PSR-0 Classes » Change notification for: Provider based PSR-0 Classes
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

OK 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.

Dave Reid’s picture

Title: Change notification for: Provider based PSR-0 Classes » [HEAD BROKEN] Change notification for: Provider based PSR-0 Classes

This seems to have broken HEAD: http://qa.drupal.org/8.x-status

Trying to get property of non-object	Notice	module.inc	430	module_enable()	
Autoloader loads classes from an enabled module.	Other	module.test	264	ModuleClassLoaderTestCase->testClassLoading()
Dave Reid’s picture

This 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

catch’s picture

Title: [HEAD BROKEN] Change notification for: Provider based PSR-0 Classes » Change notification for: Provider based PSR-0 Classes

Sorry folks, pushed the new files...

yched’s picture

The 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.

catch’s picture

Title: Change notification for: Provider based PSR-0 Classes » Provider based PSR-0 Classes that load more than once
Category: task » bug
Priority: Critical » Major
Status: Active » Needs review
FileSize
1.3 KB

Whoops. Here's a patch.

catch’s picture

FileSize
897 bytes

And here's one without the not-entirely-unrelated change that stops the registry from scanning includes but shouldn't be in there.

yched’s picture

Yup, but by that time, it's not $record anymore, it's $item ;-).

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.16 KB

#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.

jthorson’s picture

Manually killed the testbot running #57 after 577k assertions. Suggest we don't re-test it. :)

catch’s picture

Title: Provider based PSR-0 Classes that load more than once » Change notification for: Provider based PSR-0 Classes
Category: bug » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

OK I've committed the follow-up, back to change notification I think.

Berdir’s picture

What 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?

catch’s picture

The latter I think, also that files[] is no longer necessary for PSR-0 classes.

Crell’s picture

Should 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.

catch’s picture

Yeah 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.

effulgentsia’s picture

If 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.

catch’s picture

Yep, 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.

Berdir’s picture

SearchExtender 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.

effulgentsia’s picture

It's not exactly [trivial], unfortunately... looks like we kinda forgot (or postponed?) converting Query Extenders to PSR-0...

I 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.

webchick’s picture

Title: Change notification for: Provider based PSR-0 Classes » Provider based PSR-0 Classes
Priority: Critical » Normal
Status: Active » Fixed

I 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.

aspilicious’s picture

Priority: Normal » Critical
Status: Fixed » Active

Srry 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.

xjm’s picture

@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?

neclimdul’s picture

Title: Provider based PSR-0 Classes » Module PSR-0 namespace auto registration

Because this hasn't been about anything but modules for a long time and it keeps throwing me off.

EclipseGc’s picture

Title: Module PSR-0 namespace auto registration » Provider based PSR-0 namespace auto registration

Because we should really think about more than just modules.

sun’s picture

For 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...

RobLoach’s picture

Unfollowing 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}

aspilicious’s picture

Ok 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.

aspilicious’s picture

Status: Active » Fixed
aspilicious’s picture

Priority: Critical » Normal

Srry, changing priority

sun’s picture

Priority: Normal » Critical
Status: Fixed » Active

PSR-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.

neclimdul’s picture

With 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.

effulgentsia’s picture

Title: Provider based PSR-0 namespace auto registration » PSR-0 namespace auto registration for modules
Priority: Critical » Normal
Status: Active » Fixed

I 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.

effulgentsia’s picture

@aspilicious: FYI: as per #69, search.module in HEAD has an example of a PSR-0 class inside "lib".

donquixote’s picture

I 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.

sun’s picture

I've clarified the change notice on http://drupal.org/node/1320394

Automatically closed -- issue fixed for 2 weeks with no activity.