Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery

As discussed in IRC, we then need an alternative way for Plugins to be able to do a fast, memory-efficient file system scan for annotation parsing.

Thanks!

This is exactly what I stated in #1780396-32: Namespaces of disabled modules are registered during test runs (and following) already:

This patch rather indicates that the plugin system has to be corrected to not (seemingly randomly) assume that every registered namespace in the classloader is a possible plugin provider. The list of namespaces to check has to be injected. The job of a classloader is to load classes, not to be a registry for plugins.
[...]
The registered namespaces in the classloader are registered namespaces. Only. There can be multiple paths per namespace, there can be multiple registered namespaces per component. Where is the relation to a plugin registry?

I'm confident that fixing this issue will automatically fix #1780396: Namespaces of disabled modules are registered during test runs

So, the logical solution to do this is build a "namespace registry" that then registers those namespaces with the classloader. If that is the approach you guys want to take then that works. We just need a list of valid and active namespaces. I would suggest killing drupal_classloader() then though because we should never interact with the classloader directly.

Not exactly sure what's the difference of having the a registry on top of the classloader that manages namespaces over the classloader itself and how that can solve the problems I'm fighting with in the issue above but I'm looking forward to anything that helps there :)

It decouples the requesting of namespace lists from the auto-loader implementation since there is no interface for requesting those values from the auto-loader itself and it might not be implemented.

Since this depends on the list of enabled extensions and their bundles, I'm fairly sure that we need to wire this into the kernel.

One perhaps too simplistic way would be to actually bind it to registered kernel bundles; i.e., only bundles get to register plugin managers, and only bundles can provide plugins.

And, we're back to the fact that drupal is strange. In a pure PSR-0 environment, what plugins is doing with psr-0 components and scanning for plugins (which is actually the expected behavior) would be totally kosher because we don't provide code we don't intend to run. In drupal this would be kosher too as we don't register namespaces for disabled modules, EXCEPT that in tests we do.

I don't buy that this is "plugin's fault", while I do agree that this would be better as an injected list of namespaces, you still have to get a list of namespaces from somewhere, which ultimately means asking the autoloader. If we don't do that, then we're rebuilding the registry, and that seems an awful lot of trouble for what I view as being a hack within the testing framework. If we're proposing we build a registry of some sort again, why not build a registry of tests instead? The PSR-0 autoloader approach is working exactly as expected in all non-testing scenarios, and we're abusing the system for the sake of tests anyway, so it's not really surprising that we have this problem.

I suggest we figure out how to inject the annotated class discovery properly (a chore that needs doing to include it in Drupal\Components anyway) but this doesn't change the fact that someone, somewhere needs to be getting a list of available namespaces, and that in the case of Tests, we're going to get namespaces for modules that are not enabled, therefore, #1780396: Namespaces of disabled modules are registered during test runs seems to have the appropriate name and NOT be a duplicate of this to me. You still need a solution to the fact that tests need to be available for disabled modules. It looks to me like we should not have moved Tests to be PSR-0 classes.

Eclipse

you still have to get a list of namespaces from somewhere, which ultimately means asking the autoloader.

I don't see how this is the case.

Plugins can only be found in a subset of the registered namespaces, so asking the autoloader for all of them is a bug in itself.

I haven't profiled/debugged yet, but I'm pretty sure at the moment we're scanning /vendor for annotated plugins which is going to add a tonne of time to that process.

So for example if plugins can only be found in /core and the default namespaces registered on behalf of modules, we have that information available already without having to go into the classloader at all.

From reading the comments, it seems there's 2 issues here. One is whether to iterate all namespaces or only ones for enabled modules. For that, here's a rough patch. Any feedback on it?

The other issue is whether to use drupal_classloader() for knowing what the directory is for a given namespace. The problem being that if we do, then it's harder to swap out the classloader implementation, since there's no well agreed upon interface that classloaders need to implement other than the callable passed to spl_autoload_register(). I don't have a proposal for how to deal with that. Does anyone have a suggestion?

Is this what people have in mind for removing the drupal_classloader() call entirely?

Note: I don't think that either #9 or #10 are correct in and of themselves: just trying to elicit feedback on what people are wanting.

Here is how I see it:

General guideline:
1. Only classes of enabled modules should be available on class_exists().
2. Only plugins / annotated classes of enabled modules should be discovered.

However:
3. Exceptions to both exist.
4. Exceptions to both are independent of each other.
Just because a class or namespace is registered in the loader, or class_exists() returns true, does not mean it is available for plugin discovery. (although the other direction is probably true)

Conclusion:
We should aim to keep up with 1. and 2., but we should always be prepared that 3. and 4. will happen.
So,
- Clean up simpletest, to register only sub-namespaces: Yes, please! (until we hit a wall)
- Make annotated class discovery independent from class_exists(), or namespaces in the class loader: Yes, please!

@effulgentsia #9/#10 look along the right lines to me - only looking for plugins where we expect to find them.

OK, I've detailed my fix for this stuff a few times, I'm going to try again, on this issue.

My primary concern is that we're bastardizing the plugin system to support badly implemented details of the testing framework. I maintain, and will continue to do so, that this isn't a problem with plugins, but rather a problem with testing that plugins has made apparent. By the same token I'm willing to concede that DRUPAL should likely only be looking in the module's directories for plugins, the plugin system was built as a component ALWAYS with the intention of allowing this same sort of work to be done outside of drupal. That being said, we should be injecting the namespace list into the discovery, not directly calling it, and with that said, we should make drupal_classloader() standardized in a way that is sane and usable.

Making drupal_classloader sane will require us to build an Adapter Interface that wraps potential classloaders in favor of a standardized methodology for handling namespace/fallback retrieval etc etc. This will relocate any sort of code updates for the foreseeable future into this adapter when changing classloaders, and if used in conjunction with the DIC, will also mean that classloaders are swappable in core so long as an appropriate adapter class is provided for the classloader. Plugins then provides 2 annotated class discovery types, one in components that is 100% injected, and a second in core that can call to the adapter for a list of namespaces when none is injected into it. This gives developers the same DX as currently (since they won't have to figure out how to inject the list) it also means that developers can specify a list, or alter the list if they are willing to figure out how to retrieve it themselves (this feature has been asked for multiple times now). None of this fixes the actual problem with testing, but it does provide us a platform from which to ensure that the only PSR-0 namespaces that the plugin system ever gets are the ones of enabled modules, so it can walk around the problem that testing is making for us, but NONE of this actually fixes the problem that testing has introduced here, this is JUST a hack. Granted, it's a REALLY clean hack, and one we should have likely performed whether we had this problem or not, but the fact remains that the problem is the testing system, not plugins.

I hope this solution seems sane to everyone involved. It should clean up our architecture and fix both this and #1780396: Namespaces of disabled modules are registered during test runs at the same time. Would love some feedback on this proposal.

Eclipse

As a side note, this will only fix #1780396: Namespaces of disabled modules are registered during test runs for plugins, if any other system is scanning psr-0 libs for classes, this same problem will rear its head again and again and again. We SHOULD be having the problem with [Module]Bundle.php now, but as I understand it, this is hardcoded to only work for enabled modules, not psr-0 libs.

As I mentioned many times already, PHP needs a PSR-x for plugins.

Not only PSR-0 has no agreed-upon interface for the class loader, it also doesn't give you the coverse of itself: PSR-0 says that if you are looking for a class named X you could find it in a file named f(X), it doesn't tell you anything about how to introspect for classes from the filesystem.

Here is what I suggested back in the original annotation issue:

One approach would be to implement a proper NamespaceIterator (an iterator that allows to navigate through the namespace hierarchy and returns Reflection* objects) and register it in the same way we register the class loader.

If we go that route, we would implement an extension of PSR-0 in which the converse of PSR-0 is true. And in that way, we would become independent of the class loader.

I'm willing to concede that DRUPAL should likely only be looking in the module's directories for plugins, the plugin system was built as a component ALWAYS with the intention of allowing this same sort of work to be done outside of drupal.

if any other system is scanning psr-0 libs for classes, this same problem will rear its head again and again and again

I agree we should allow annotation discovery (for plugins and other things) outside of Drupal modules, but it should be Drupal modules that tell us where to look. We should always ask those modules directly, and not via the class loader.

We should accept that as a specialty (or say, a "feature") of Drupal, that everything depends on which modules are enabled.

And btw, even without that specialty, I think the class loader is the wrong tool to look for available namespaces. Even if other software might do it like that, I would consider it abuse. Think of lazy/late registration of namespaces, or any kind of conditional registration - those are imo very legitimate techniques that should be allowed in a class loader.

Actually, @chx started doing that in Doctrine with ClassFinderInterface, we just need to extend that a little bit to support iteration.

@EclipseGc - if you're going to ask the classloader for the namespaces, then how is it going to know which ones to return, since we definitely don't want all of them. Injecting the namespaces into the plugin discovery is fine, but they should still not come from the classloader.

That's why we have an adapter with a method that lets me ask for something like

$namespaces = drupal_classloader()->getEnabledModuleNamespaces();

Or something like that. And that method, instead of returning all namespaces, runs them through a list of enabled modules first and removes all non-module/disabled-module namespaces. In this way No plugin implementation actually has to change anywhere, we have a centralized system we trust to return valid PSR-0 dirs, and we can also step around that system, should it ever be in our best interest to do so.

In this way we build an interface specific to drupal's needs to wrap all classloaders, and utilize individual classes as adapters for classloaders. Anyone could write an adapter for drupal for any classloader at that point, and all the complexity resides therein, and swapping out classloaders doesn't become problematic any longer. Regardless of whether the Plugin system exposed this problem or not, an Adapter pattern around the classloader system is probably a good idea.

Eclipse

#21
then why start with drupal_classloader() in the first place?

$namespaces = drupal_get_enabled_module_namespaces();

and done.
Bothering the classloader for this is wrong.
And it is not even necessary for any 3rd party integration stuff, because 3rd party code definitely does not know about a function named drupal_classloader().

Or something like that. And that method, instead of returning all namespaces, runs them through a list of enabled modules first and removes all non-module/disabled-module namespaces.

We might well want to allow plugins to live in Drupal|Core though, so it'd remove those too then?

We have the list of enabled modules, and we can get the namespaces for those very easily per effulgentsia's patch, so why get all the namespaces from the class loader then filter some out? When we can just provide a list of what we want?

@donquixote

Sorry, I'm probably not being completely clear. drupal_classloader() would become the actual adapter class itself, so anytime you need anything from whatever classloader was being used, drupal_classloader() would actually be the adapter class around that with a consistent interface, so we can always call methods we expect to exist.

@catch

Plugins is available as soon as the classloader is, so we can parse PSR-0 dirs super super early if necessary and not rely on some other mechanism for it. drupal_get_path() doesn't have that benefit last I checked, and while I won't claim to understand the classloader system completely, the namespaces are already registered at this point regardless, so I'm not sure I see what sort (if any) extra effort it is to ask the classloader over the drupal equivalent itself.

That being said, the adapter can make exceptions for ANY directory we want, so this doesn't mean we remove Drupal\Core and Drupal\Component, just that we have direct control over what we want to return for this sort of circumstance. The simple truth is that most plugins will happen post-bootstrap, but as core stands right now, that's actually not a requirement, and I'd like that to continue to be true. Similarly, Plugins was developed as a component with greater concerns than just Drupal, and as I said, we can solve this problem without changing code in it significantly. I'd just prefer to maintain the spirit of what we set out to provide, and my proposal here does that while at the same time providing mechanisms for swapping out classloader architecture from now till kingdom come.

To speak directly to effulgentsia's patch, I think we're much better off injecting the namespaces with a default, sane fallback if NULL is given. effulgentsia's patch does something close to what that sane default would be, but as outlined above, my solution actually doesn't require a bootstrapped drupal in order to function (granted in that case perhaps NO module dirs are returned, but for plugins that need to operate at that level, and different set of namespaces could be injected instead of relying on the fallback). I hope I'm making sense.

Eclipse

#15: Why are you so keen on hacking this into the class loader?

And this is not only for testing, but this issue was created because the normal ClassLoader (compared the the universal one) was slowed down by the annotations system, so this is related to performance as well.

What is wrong with effulgentsia's approach? Why do we need the class loader?

Plugins is available as soon as the classloader is, so we can parse PSR-0 dirs super super early if necessary and not rely on some other mechanism for it.

If you parse PSR-0 directories as soon as the classloader is available, then you'll miss all the modules, because those are registered by system_list(). If the classloader adapter needs to filter out enabled modules, then it needs to have knowledge of what an enabled module is, which introduces a dependency on the modules system.

Like I said injecting the module list into the Plugin discovery seems fine, but that's not at all the same as stuffing all of this information into the class loader (or an adaptor), which is only supposed to register classes, not know which ones we care about at any one particular time.

I understand the original intention, but reality is, the plugin system is application-specific and bound to Drupal right now — that is, as long as plugins in the plugin system are not properly namespaced. Fact is, as soon as you'd try to introduce plugins in non-Drupal components, you will end up with name clashes and false-positive matches. (sufficiently explained over there)

That's an additional reason for not scanning arbitrary vendor namespaces.

And as #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory already clarifies, the namespaces registered in the classloader are not sufficient in the first place, since the only two namespaces we know about Drupal components are Drupal\Component and Drupal\Core — but those two do not point to actual components, but only common containers for components that share certain semantics. The actual components are Cache, Entity, FileTransfer, KeyValueStore, Language, Lock, Mail, Queue, Routing, StreamWrapper, TypedData, Updater, and of course, Plugin itself. But the classloader does not provide a list of components. The classloader only knows what to load from where. It does not even guarantee that something can be loaded from anywhere. Anything else are random assumptions that don't hold true, by design.

Therefore, the classloader is the wrong resource to ask for components/extensions that may want to participate in the plugin system.

And lastly, #2 is constantly being ignored here — PSR-0 classloaders have shared semantics. They support multiple file paths for a single namespace. They support PEAR-style prefixes, which cannot be translated into plugin system subdirectories in the first place.

The bottom line is and remains to be: Relying on the classloader for any kind of discovery mechanism is just simply wrong.

@Damien's idea of a reverse locator sounds interesting, although it sounds relatively complex, and I'm not sure whether it doesn't share the second of the above problems, too. (i.e., how do you know that Drupal\Core\Entity exists without a filesystem scan?)

What is wrong with effulgentsia's approach?

The main thing that's wrong with it is that it bakes the logic into the AnnotatedClassDiscovery class. I agree with EclipseGc that it would be better to move the bulk of AnnotatedClassDiscovery from Drupal\Core to Drupal\Component, and have it neither depend on module_list() nor on drupal_classloader(), but rather get the namespaces and their directories injected. That still leaves the question of what should do the injection, and perhaps a way to move forward on that is start posting some patches and see if we can come to consensus on the interface and implementation of the injector. #19, #21, and #22 are all possible starting points, and we're probably only talking about a few lines of code that we need to come to consensus on.

I spoke with EclipseGc on IRC and we came to the consensus for now that an adapter as from #21 is nice to have and solves the problems that class loaders have no defined interface, but is a different issue.

What is important for EclipseGc is that:

* he has a function to ask for appropriate libs
* he can inject that function into the AnnotatedClassDiscovery

So I agree with #28 on the approach.

@sun

Your argument is the first one that has technical merit thus far for me, so thank you for that. That being said, injecting the list of namespaces (or really, directories) walks around that problem pretty significantly, and leaves it up to the caller to solve the problem for themselves. In this case, separate from this issue, we need a function or method that returns the list WE consider sane. The AnnotatedClassDiscovery will parse any list with the appropriate structure.

That being said, I will work on a fix for the AnnotatedClassDiscovery so that once we have that sane list, it can be properly injected. However the patch I provide will not attempt to solve that problem. That needs to be solved in a followup.

Eclipse

Let's see how this patch fares.

Eclipse

+++ b/core/lib/Drupal/Core/Plugin/Discovery/DrupalAnnotatedClassDiscovery.php
@@ -0,0 +1,30 @@
+ * Definition of Drupal\Core\Plugin\Discovery\DrupalAnnotatedClassDiscovery.
...
+use Drupal\Component\Plugin\Discovery\AnnotatedClassDiscovery;

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php
@@ -9,7 +9,7 @@
-use Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery;
+use Drupal\Core\Plugin\Discovery\DrupalAnnotatedClassDiscovery;

There's no reason to rename the AnnotatedClassDiscovery in Drupal\Core — just alias the the one from Component by using "as PluginAnnotatedClassDiscovery".

Seemed odd to have 2 discovery classes with the same name. I'm totally ok with your suggestion if no one else is going to scream about it though. Once I figure out the test failures, if no one has said otherwise, I'll do what you said as well.

Eclipse

Uh. Why exactly would you alias a class if you're trying to move it? I lost you on that...

Crell, we'll have an AnnotatedClassDiscovery in Drupal\Component\Plugin\Discovery and Drupal\Core\Plugin\Discovery. One extends the other, so we'd have to alias it. This is why I named them differently, but I'm also not opposed to naming them the same.

Eclipse

Just asking:
Isn't that just a temporary thing? In the end we should kill the Drupal\Core\ one, and just constructor-parameterize the generic one?
So if you want to go piecemeal, you should probably do whatever has the smallest commitdiff. (= not renaming)

Oh. Yeah, aliasing is how we're handling extending Symfony code when we do that. That's better than forcing separate base names.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -0,0 +1,108 @@
+  protected $annotation_namespace;
+  protected $annotation_dir;
+  protected $reflection_class;

camelCase, please. And variable documentation.

@donquixote

No this would not be temporary. If you look at the patch, the Core one just satisfies the constructor parameters of the Component one for simpler DX.

Eclipse

Ok, trying with another patch.

Eclipse

status for testing

Enough of this, namespaces belong to the kernel, it already holds the module list, come on.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -0,0 +1,108 @@
+        $prefix = implode(DIRECTORY_SEPARATOR, array(
+          $ns,
+          'Plugin',
+          $this->owner,
+          $this->type
+        ));
+        $dir .= DIRECTORY_SEPARATOR . $prefix;

This may be a separate issue, but being able to override this in a subclass would really be useful, see #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity

I really think at this point that this is critical (or #1780396: Namespaces of disabled modules are registered during test runs, I'm still not exactly sure how they exactly they are related and if anything of that one is left if this is resolved, we'll see), it's causing strange failures in various issues, see for example #1292470-221: Convert user pictures to Image Field, #1836204-10: Test failure in Drupal\system\Tests\Menu\BreadcrumbTest and so on.

The bootstrap kernel patch has been commited now, according to @chx it should now be possible to get the list of namespaces from there?

Edit: Fixed wrong issue reference.

Now that we have #1848200: Random warnings in tests in field_ui.admin.inc which causes the test problems #45 doesn't seem to apply anymore.

After talking with katbailey, chx and berdir we came to the conclusion
that we should register the namespaces on the container so the annotation discovery could use
that information additional.

Additional with the path from eclipseGC this seems to be the way to go and make progress on that issue.

It feels ugly that you have to register both the namespaces on the classLoader and the container, maybe we should abstract that?

+++ b/core/includes/bootstrap.incundefined
@@ -3149,6 +3149,9 @@ function drupal_classloader() {
+    drupal_container()->setParameter('container.namespaces', $namespaces);
+

I don't think this will work as the call to drupal_classloader() right in index.php happens before the kernel creates the main container - you'd just be registering the parameter to the soon-to-be-defunkt "bootstrap container".

Going to take a crack at this as it affects the work I'm doing in #1331486: Move module_invoke_*() and friends to an Extensions class as well - basically the two parameters we need are container.modules, which just has module names as keys and module paths as values, and then container.namespaces, which has namespaces as keys and namespace paths as values. Right now we have container.modules which has module names as keys and namespace paths as values :-/

How 'bout this?

Something is seriously wrong with testbot today - the test apparently failed ('Installing: failed to complete installation by setting admin username/password/etc.') but that hasn't been reflected here yet. Moreover, I've run the installer locally without problems and I've run tests using run-test.sh and everything seems to be fine. No idea what's going on :-/

This is the same patch as before...

#51: 1836008.container.namespaces.51.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -355,6 +366,7 @@ protected function buildContainer() {
+    $container->setParameter('container.namespaces', $this->moduleNamespaces);

This would explicit not allow to register plugins in \Drupal\Core or \Drupal\Component, so if we want plugins there, it should be in entity module / system module?

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -179,27 +191,28 @@ public function registerBundles() {
+        $path = $this->moduleData($module)->uri;

Maybe we could optimize it and just use $this->moduleData instead of calling the method again so twice per module.

Extended the tests.

OK, well at least we have some reproducible test failures to work with now - I'll try and get to the bottom of that. At a glance it seems to be just the tests that specify the 'standard' profile.

This would explicit not allow to register plugins in \Drupal\Core or \Drupal\Component, so if we want plugins there, it should be in entity module / system module?

Yes, I discussed this with chx and he said it didn't make sense for there to be plugins registered in either the \Drupal\Core or \Drupal\Component namespace and that sounds pretty reasonable to me but if anyone knows of a reason why we do in fact need these namespaces added to container.namespaces please let me know.

Maybe we could optimize it and just use $this->moduleData instead of calling the method again so twice per module.

Certainly, I will make that change when I roll a new patch, once I've figured out these pesky test failures :-/

That essentially circles back into the discussion that happened here, as well as in #1821846: Consider better naming conventions for plugin types owned by includes, #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory, and related issues.

To my knowledge, there are a couple of patches in the queue already, which need to add actual plugin implementations to Drupal\Core\Plugin\* currently — because the current plugin discovery code registers the Drupal\Core namespace only, that's the only place to place plugins for components right now.

However, as discussed here and in aforementioned issues, the plugin system should actually register each (or individual) components as possible plugin namespaces, so e.g., the Entity component can register and ship with plugins, and other components can, too.

Thus, the Drupal\Component and Drupal\Core namespaces indeed don't need to be in container.namespaces. Instead, we need the actual/individual components in there.

Everything sun points out in #58 is exactly what my patches are attempting by allowing directory/namespace injection. We stop actually caring about the "namespace" itself in favor of whatever we were told to do... so if the caller says "Hey looking in Drupal/Core/Entity" for plugins." then we would do exactly that, namespaces are irrelevant then, and we are simply left with the question of "how do we format our list of directories that we want to search by default" as a general question for D8. We seem to have run off looking at completely different problems best as I can tell.

Eclipse

Instead, we need the actual/individual components in there.

OK, I am coming late to this discussion and also have an ulterior motive for wanting to make this change (as mentioned in #48) so I'm not sure I know how to address this. In the meantime I've found what was causing the test failures, and it looks to me like a bug in the existing code. So if this is green I think this change should be made regardless of the additional requirements for the plugins system.

#60 was a cross-post with #59 - I'm going to fix that remaining test failure and post this in a separate issue, because it seems like I have basically hijacked this one - sorry!

Created #1849700: DrupalKernel's container.modules param should contain module filenames, not full namespace paths

Everything sun points out in #58 is exactly what my patches are attempting

Not exactly. I like #40, but it still has this:

    if (empty($namespaces)) {
      $this->namespaces = drupal_classloader()->getNamespaces();
    }

And it is removing that that has become the focus of this issue since #43.

We seem to have run off looking at completely different problems best as I can tell.

Indeed. How should we deal with that? Merge #40 and #55 and tackle both aspects in this one issue? Or split issues?

I would prefer to split issues, but that's sort of a "he who does, wins" scenario, and I'm not doing this tonight for sure.

With regard to the $this->namespaces, the whole point is that once we get my patch working, it's a 1 line change, and I'm happy to change that 1 line so long as (in a separate issue) someone codes up a proper replacement. These are really completely different concerns, and until a replacement exists, that line should stand.

That's just my 2 cents. I won't fight changing that line at all once a proper replacement exists, but I really don't want this code to have all sorts of logic to figure that out.

Eclipse

The more pressing part of this issue is settling on how Drupal core should determine where to scan for plugins, so leaving this issue as that part. I moved EclipseGc's partitioning of AnnotatedClassDiscovery into #1849752: Abstract non-Drupal-specific parts of AnnotatedClassDiscovery into a Drupal\Component base class.

OK, great, can we then focus this issue on building a function that returns just the directories that we want core to scan for plugins?

#1849700: DrupalKernel's container.modules param should contain module filenames, not full namespace paths landed. Anyone here up for reviewing #1849752: Abstract non-Drupal-specific parts of AnnotatedClassDiscovery into a Drupal\Component base class? I think it's ready, and should hopefully be a quick review and non-controversial commit, that could then make this issue just about changing the implementation of Drupal\Core\AnnotatedClassDiscovery::getPluginNamespaces from this:

  protected function getPluginNamespaces() {
    $plugin_namespaces = array();
    foreach (drupal_classloader()->getNamespaces() as $namespace => $dirs) {
      $plugin_namespaces["$namespace\\Plugin\\{$this->owner}\\{$this->type}"] = $dirs;
    }
    return $plugin_namespaces;
  }

to something else.

#1849752: Abstract non-Drupal-specific parts of AnnotatedClassDiscovery into a Drupal\Component base class just went in.

This is almost certainly wrong, but posting it for feedback. This ensures that we only search in namespaces of enabled modules (#1780396: Namespaces of disabled modules are registered during test runs), but in addition to being ugly, it means we don't search in non-module namespaces. In HEAD, we don't currently have plugins in non-module namespaces, because per #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory, it's not yet settled where we want them and how that relates to what's registered in the class loader.

In HEAD, we don't currently have plugins in non-module namespaces

We do now as of #1845546: Implement validation for the TypedData API. Unfortunately, they're in Drupal\Core\Plugin\Validation rather than in Drupal\Core\Validation, but that's an issue for #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory. Though the issues are related: basically, if we don't use drupal_classloader() to determine which namespaces to search for plugins, then what should we use? Thanks to #1331486: Move module_invoke_*() and friends to an Extensions class, we now have a Drupal\Core\Extension subsystem. Should we add a class there that could inform us of what module and non-module namespaces/locations are valid roots for plugins?

This will certainly conflict with the effort of #1903346: Establish a new DefaultPluginManager to encapsulate best practices so maybe that issue should be fixed first, but here is a first simple approach.

Installation of drupal works for me with that but there is for sure some work left, so I ask for principal review.

How about this instead:

1. Add a new Kernel parameter %plugin.namespaces%.
2. For Drupal core components that have/need plugins, maintain a static list in DrupalKernel.
3. For all modules/extensions, which are processed by DrupalKernel anyway already, add their namespaces to %plugin.namespaces%.

Or the more straightforward version, which presumes that we finally have a bundle per component:

1. Have a *Bundle class per component/extension. (i.e., Drupal\Core\Cache\CacheBundle, Drupal\Core\Validation\ValidationBundle, etc)
2. Make each bundle add itself to the %plugin.namespaces% container parameter, when needed.
3. Inject the parameter into the plugin annotation class discovery service, which is registered by Drupal\Core\Plugin\PluginBundle.

In any case, the result would be the same:

var_dump($container->getParameter('plugin.namespaces'));
=> array(
     'Entity' => 'Drupal\Core\Entity',
     'Validation' => 'Drupal\Core\Validation',
   ...
     'comment' => 'Drupal\comment',
     'field' => 'Drupal\field',
     'node' => 'Drupal\node',
     'views' => 'Drupal\views',
   ...

I totally agree that the kernel has a lot of overlapping functionality, see DrupalKernel::registerModuleNamespaces()
which basically does something really similar. Conceptually though I'm wondering why to separate module namespaces from plugin namespaces. If the container
would have a module.namespaces or a namespaces parameter in general, maybe other parts could reuse it and the kernel wouldn't be "coupled" with plugins.

One problem at the moment is that the ParameterBag class does not straightforward allow you to add something to existing parameters, what about providing another interface, which includes a new method like addToParameter.

Started to work on idea number 1) of sun and it is working good so far. Do you think this is the right direction,
then I will convert all the plugin managers, and maybe remove some of the custom stuff of core's AnnotationClassDiscovery.

Sending it to bot.

* Renamed plugin.namespaces to container.namespaces to avoid some confusion.
* Ported all plugin managers/bundles to pass on the value.

Added the argument to the constraint manager, that should fix most of the fails...

Both the aggregator fetcher manager and the block manager aren't loaded from the container all the time.

Certainly especially the part in the PluginUIBase is a pure workaround, though I'm not sure how to get that injected properly.

This time with the aggregatorBundle.

Merged in @dawehner's changed and also fixed a few additional tests like editor.

The remaining failures might be tougher, let's see how this goes.

Huh, that seems to have been easier to fix than expected.

The namespace definitions for Core/Component were wrong and the hardcoded namespace in the annotated discovery test was using one array() too much.

This should be green.

@RobLoach: #1863816: Allow plugins to have services injected is the issue for injecting things into plugins. I'd like to keep the changes here minimal but if others have a better suggestion on how to do it, I'm all ears.

Re-roll that adds array type hints (Note: The annotated discovery class is missing quit some @params and more importantly, defintion of used properties like $this->owner and $this->type but I don't think we should fix that here.

Also, this should now fix most if not all of #1780396: Namespaces of disabled modules are registered during test runs, so removing disabled modules checks including now unecessary process decorators to prove that.

Both this two recent interdiffs are looking good.

Agreed. The patch is looking very good. I particularly love

- *   manager = "Drupal\block\Plugin\Type\BlockManager",
+ *   manager = "plugin.manager.block",

this part. I filed #1927766: Resolve the new @todo in PluginUIBase however.

+++ b/core/modules/block/lib/Drupal/block/Plugin/system/plugin_ui/BlockPluginUI.phpundefined
@@ -46,7 +46,8 @@ public function form($form, &$form_state, $facet = NULL) {
+    // @todo Find out how to let the manager be injected into the class.

If we move to having a route definition doing this work we'll get this sort of injection very very easily.

I am very comfortable with this, so +10000000 from me. We can work on the PluginUI weirdness in a followup, but moving to the routing system should make the stuff you were commenting easier. Definitely RTBC imo.

Eclipse

Looks also great to me.

AFAICS, this implements approach A) from #75, which is definitely a step in the right direction. We should still investigate B) in a follow-up issue though.

B) sounds like an interesting idea to look into in a follow-up, yes. That would solve the issues with the current weird namespace: Drupal\Core\Plugin is now used both by the Plugin component and Drupal\Core plugin implementations.

Is #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory the right place to do that?

I'm pretty sure the last set of annotation changes fixed that cause I'm actively using it.

Eclipse

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -16,11 +16,15 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
+  function __construct($owner, $type, $root_namespaces = NULL, array $plugin_namespaces = array()) {

$root_namespaces either needs to be defaulted to an empty array or not optional. The current default NULL will cause a PHP warning with the foreach in getPluginNamespaces(). Also it should have a type hint of array.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -31,15 +35,18 @@ function __construct($owner, $type, $root_namespaces = NULL) {
+    if (!empty($this->plugin_namespaces)) {

Should be $this->pluginNamespaces... the fact this didn't cause any failures implies we're missing test coverage.

Not sure, whether this additional test function is a hack, feel free to comment.

Hm..

+   * @todo Figure out how to let this comment still be TRUE.
    * This is overridden rather than set in the constructor, because Drupal
    * modules can be enabled (and therefore, namespaces registered) during the
    * lifetime of a plugin manager.

Apart from few unit tests where the managers are manually managed (and noone of the reviewers seemed to care about that there), the fact that this patch is green proves, I think, that we might not have to make this comment TRUE again. So if that is OK, then we can remove the method and prepare the directories in advance in the constructor.

You'll notice that the only thing left in this class is now the constructor, where we simply shuffle around the arguments. Which also means that this could easily be done outside of the class and then we can completely remove it. However, I would suggest that we postpone that on a sane default manager (#1903346: Establish a new DefaultPluginManager to encapsulate best practices) to avoid duplicating those code lines.

Will have to merge in the tests from @dawehner but we might not actually need them anymore, the only thing they would be testing is that we correctly pass the $plugin_namespaces through. And even that would go away when moved to the manager.

Thoughts?

#99: annotated-discovery-classloader-1836008-99.patch queued for re-testing.

Discussed with @EclipseGC, we discussed the option of passing the module handler into that class so that the list of namespaces can be updated. Created some untested example code for that: https://gist.github.com/Berdir/5042373

Not sure if we shouldn't go with @dawehner's patch for now and look into that in a follow-up issue as we might want to move module namespace handling from the kernel to the module handler and then work with namespaces instead of module names?

Also, @EclipseGc noticed that this probably does not yet support themes and we might need to add some special handling for that.

yeah, we probably need to double check if this looses themes or not, and do something about it.

Wow, yeah that seems to be a more complicated problem.

In order to get the themes listed here as well, we have to make the DrupalKernel aware of the themes,
which shouldn't be impossible but probably out of scope of that issue.

The problem is that it would be a regression compared to now, where themes can provide plugins.

We could defer it to the manager and say if a manager wants to support plugins from themes, he needs to extend the list himself? Doesn't make sense for entity types, for example :)

Discussed in IRC with @alexpott and @EclipseGc and agreed on this:

- Doing something similar to http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D... shouldn't be too hard
- Managers that want to allow themes to provide plugins should, for now, add the themes namespaces to the last manually and it shouldn't happen by default.
- Themes remain special until they get an installation status, which will happen in #1067408: Themes do not have an installation status. We can re-visit this afterwards.

No test coverage yet, we don't have a theme yet that provides one I think and we can't test a protected method (don't want to make it public just for that). Can we postpone that on when we actually add a layout?

Otherwise we could also postpone support for that feature for when we actually need it.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -16,8 +16,17 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
+   * @param array $root_namespaces
+   *
+   * @param array $plugin_namespaces
+   *   An array of paths keyed by it's corresponding namespaces.

Real minor nit - missing description of $root_namespaces

Can we postpone that on when we actually add a layout?

Yes.

Also I think we should

postpone support for that feature for when we actually need it.

Putting untested and unused code in is probably unnecessary as code tends to get better when it has implementations! :)

Added the @param description for that from my previous patch. interdiff is against #98.

#111: annotated-discovery-classloader-1836008-111.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -16,8 +16,19 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
+   * @param array $root_namespaces
+   *   Array of root paths keyed by the corresponding namespace to look for
+   *   plugin implementations, \Plugin\$owner\$type will be appended to each
+   *   namespace.
+   * @param array $plugin_namespaces
+   *   An array of paths keyed by it's corresponding namespaces.
    */
-  function __construct($owner, $type, $root_namespaces = NULL) {
+  function __construct($owner, $type, array $root_namespaces = array(), array $plugin_namespaces = array()) {

Discussed the two arguments with @alexpott, who mentioned that it is not clearly explained which is doing what, so as someone implementing it, you won't know what do use.

Then we noticed that..
a) The only purpose of that is to work around the plugin naming/namespace standards
b) #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory is currently not using it and we were not able to come up with use case for supporting that, because..
c) You can always use the default Drupal\component or your own child class of that if you want to derivate from the standard.
d) In fact, you can not mix the two namespaces because the current implementation only uses plugin namespaces if there any. Which means it is completely useless and there is zero reason for not using the component implementation directly.

So especially d) makes it obvious that $plugin_namespaces should be removed and we should document the other one better to explain what it's exactly doing. The whole class should probably also get some better documentation (annotated discovery implementation that enforces the plugin naming standards and then some explanation of that or so). Not sure how far we want to go with documentation improvements here because the fact that this is poorly documented is not introduced here and there are more things missing, like proteted definitions with docblocks for owner/type and so on.

Will assign to me when I have time to work on it but that won't happen before tonight.

So how about this...

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -25,37 +25,19 @@
-   * @todo Figure out how to let this comment still be TRUE.
-   * This is overridden rather than set in the constructor, because Drupal
-   * modules can be enabled (and therefore, namespaces registered) during the
-   * lifetime of a plugin manager.

That means we're almost back to my patch from #99 or so.

The problem is that while it currently seems to be correct that it can be simplified like this, we're also getting rid of this @todo and I'm not sure if we can do that.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -25,37 +25,19 @@
     $this->owner = $owner;
     $this->type = $type;

If we'd do that, then this assignment wouldn't be necessary anymore.

How about this?

Improving my english...

#118: annotated-discovery-classloader-1836008-118.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -16,32 +16,33 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
+      $plugin_namespaces["$namespace\\Plugin\\{$owner}\\{$type}"] = array($dir);

I'm wondering whether we should get rid of supporting multiple dirs per namespaces? This is something which is just the case for the UniversalClassLoader and shouldn't be part of PSR0?

#118: annotated-discovery-classloader-1836008-118.patch queued for re-testing.

Did the following locally because test bots vary way to much.

$ php core/scripts/run-tests.sh --url http://d8/ User

Did run it twice with 8.x and this patch...

8.x: 4m5s / 3m49s
this: 2m59s / 2m53s

The same with --concurrency 8

8.x 57s / 57s
this: 40s / 40s

That is... more than I expected. Doesn't translate 1:1 to testbots but it does affect almost every single test, concurrency or not. Like. I don't think there's anything else that we can do so easily that has such an impact.

Hrm. I'm actually not able to see why this patch could have any substantial impact on test performance.

Are we able to explain that or is this magical? In case of the latter, we need to get to the bottom of it.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -186,12 +189,6 @@ public function __construct() {
-    // @todo Remove this check once http://drupal.org/node/1780396 is resolved.
-    if (!module_exists($definition['module'])) {
-      $definition = NULL;
-      return;

Yes, we are, this. Not that magical.

Right now, the annotation parser parses *every single entity type* annotation that we have in core. And then we throw it out again in processDefinition(). And we do that every time we throw away the entity info cache, which happens at least three times on every web test method, possibly even more, not quite sure how module_enable/that config entity lookup interact there...

That's because simpletest registers the namespace for all disabled modules so that tests could be loaded. Which results in all of them being parsed because we look in all namespaces that are registered.

At the moment, that means ~51 entity types. Fun :)

Thanks for clarifying! That makes perfect sense.

The only other minor remark/nitpick I'd be able to share about this patch is the improper formatting of the subsequent lines of the added @todo comment in AnnotatedClassDiscovery.

Aside from that, I believe we should move forward here. Some recent comments talked about some lack of test coverage, but TBH, the implicit test coverage is the most important and sufficient here. If we want to file a dedicated issue for explicit test coverage for a particular detail, we can always do so. I won't bump this back to RTBC this time, but I'll generally support a RTBC from @Berdir, who seems to be much more involved here.

I'll RTBC this. It's a clear win for performance, but it also finally rids us of that awful processDecorator() hack. (I'm allowed to call it awful because I wrote it).

catch looks like he's been all over this issue, so assigning to him for a final check-over.

Needed a reroll due to #1925576: "Tip" plugins managed by TourManager ??

Also fixed @todo formatting.

Should still be rtbc if green...

I'm having a crazy week but if someone pings me once this comes back green I'll try to commit it asap.

Re-roll looks good.

Response about the tests note in #128:

Tests have been requested for two things, that themes can provide plugins and the additional $plugin_namespaces argument. We decided to not cover theme-provided plugins here so we can't and don't need to add test coverage for that. And $plugin_namespaces actually had test coverage but we figured out (see #114) that the argument is useless and remove it including it's test coverage. So I think we're good in regards to test coverage.

Also opened a follow-up for the @todo about updating the namespace list: #1930020: Inject a class into the Core annotated discovery implementation so that namespaces can be updated

Committed/pushed to 8.x, thanks!

This needs a change notice I think, or more likely updating one somewhere.

Adding tag

There was no change notice for the initial annotations patch see http://drupal.org/node/1683644#comment-6909502

However there is documentation page which I've updated http://drupal.org/node/1882526

Removing tags / changing title... if anyone feels this isn't right please set them back.

Restoring category and priority

Very nice performance wins :) Faster tests = better tests!

However, I wonder why changes like these were made:

-    $this->enableModules(array('edit_test'), FALSE);
+    $this->enableModules(array('edit_test'));
+    $this->editorManager = new EditorManager($this->container->getParameter('container.namespaces'));
+    $this->editorSelector = new EditorSelector($this->editorManager);
+    $this->metadataGenerator = new MetadataGenerator($this->accessChecker, $this->editorSelector, $this->editorManager);

These changes don't break anything, but they instantiate objects that were already being instantiated in a testSomething() method. (It seems this particular change was made at #86.)

@WimLeers

- The objects need to be re-instantiated so that they use the updated namespaces arguments. This usually happens automatically due to the kernel rebuild but not if you create the managers manually. See also #1930020: Inject a class into the Core annotated discovery implementation so that namespaces can be updated, which would eliminate the need for this.
- The additional argument to enableModules() has been removed, it is always FALSE now. So I removed that while I was changing these lines anyway.

#139: Oh, that makes sense. Thank you! :)

Here's a patch to remove the remaining processDefinition in edit.module's plugin manager. It also removes the 'module' annotation keys from (edit|editor|ckeditor).module's plugins: #2006278: Remove leftover 'module' annotation keys and plugin manager processDefinition() to deal with disabled modules during tests.