Establish a new DefaultPluginManager to encapsulate best practices

What worries me a bit in this code is that the code in the constructor becomes relatively hard to override. AFAIK it's not possible to "skip" the constructor of this class but still call the one from CacheDecorator. This makes it hard for an actual manager to add an additional decorator. You would have to copy code from two classes and adjust it.

Which seems to make the whole decorator pattern kinda... pointless? Why having a very pluggable/mixable pattern if in most if not all cases, we end up with problems if we don't define it in the exact same way.

For example, the main reason for adding the ProcessDecorator was that it can happen within the Cache decorator, right? We are the cache decorator now, so everything happens within us anyway now. Why go through the trouble?

Given this base class, what use do decorators give us exactly?

+++ b/core/lib/Drupal/Core/Plugin/DrupalPluginManagerBase.phpundefined
@@ -0,0 +1,107 @@
+    parent::__construct($this->discovery, $cache_key . ':' . language(LANGUAGE_TYPE_INTERFACE)->langcode, 'cache', CacheBackendInterface::CACHE_PERMANENT, array());

As discussed before, a dynamic cache key needs special handling to be able to clear the caches. There are two options:

a) cache keys. Very easy to use, is what we've used so far but the downside is that at least for the default database cache backend, it means an additional query for each used cache tag.

b) Loop over all possible cache key variations and delete them explicitly. A bit more complicated but no runtime overhead. And in this case (with just a language suffix) easy to implement. We could override the cache clear method and do something like $this->cache->deleteMultiple($this->getCacheKeys()) if no cache tags are defined. That method would then loop over the language list.

After spending vastly more time with the plugin system, I have an even more radical stance:

+    $this->discovery = new ProcessDecorator($this->discovery, array($this, 'processDefinition'));
+    $this->discovery = new AlterDecorator($this->discovery, $alter_hook);
+    parent::__construct($this->discovery, $cache_key . ':' . language(LANGUAGE_TYPE_INTERFACE)->langcode, 'cache', CacheBackendInterface::CACHE_PERMANENT, array());

1. Everything including the ProcessDecorator + AlterDecorator + CacheDecorator SHOULD be the business of the PluginManager, as the single/atomic source of plugin definitions.

2. All discovery is different logic that does not pertain to the plugin manager. Discovery != Processing + Caching + Management.

   You can swap out the discovery at any time, and you can swap out the plugin manager any time, but these two major logic parts are mutually exclusive, and they each form their own vertical processing stacks. In fact, the discovery should be *injected* into plugin managers.

3. Caching is too special in order to happen at a decorator level:

   • The PluginManager needs to be the final, ultimate source of plugin definitions, be able inject new definitions "on demand".
   • The PluginManager has to be responsible for updating the cache accordingly

4. The AlterDecorator should not exist in the first place. It SHOULD be an inherent part of processing any plugin definitions.

   We don't need a decorator for something that simply has to be a standard for all plugin managers. Anything that is not alterable, does not account for Drupal's modular architecture and thus is simply wrong. Period. Therefore, it is the responsibility of every PluginManager to invoke an alter hook. That has absolutely *nothing* to do with discovery though. Hence, the current AlterDecorator on the discovery stack makes no sense.

5. The ProcessDecorator is what completely blows up any debug() and var_export(), as it recursively references the PluginManager class, which means that any kind of debug($entity) blows up with a fatal error, which is the utterly worst DX we ever had in Drupal's history.

   By moving the entire processing of definitions into the plugin manager, we inherently eliminate that worst-of-all-time DX disaster, since there is no recursion anymore.

We just had a long discussion in IRC about this.

2. Agreed about injecting. The main discussion point in IRC was whether processing/altering belongs into a custom manager or a custom discovery/processing class. Injecting is possible either way. While it doesn't make much sense to inject something different (e.g. yaml instead of annotation), it could be a valid use case to inject a faster implementation of the same thing. A native implementation for annotations for example :p

3. Making discovery + processing the same would also allow to keep the cache a decorator. @msonnabaum argued for this, making the main purpose of the manager the composition and delegation to discovery/process, factory, .. and not actually do stuff himself. Extending the manager from CacheDecorator is essentially what you are saying, just not explicit. I'm not yet 100% sure which is the correct approach but it should IMHO either be what you said (making it explicitly the responsibility of the manager) or moving processing out.

5. That is unfortunately also true for EntityNG anyway I think. I'm not yet sure if that can be fixed. I do think that debug(), dpm() and so on should have explicit support for entities/TypedData and if passed in a compatible class, read the properties of it and display the *data*, not the class structure. DebugSerializer? ;)

My stance on #3:

2. Processing/altering definitely does NOT belong into the discovery. Discovery SHOULD mean exactly what the name encompasses, nothing else.

   Processing/altering belongs into the manager, because it SHOULD be responsible for managing and processing definitions. If you want to swap that out, swap out the manager.

   Alas, the concrete swappable example of #3.2) wasn't actually related to the manager though — it circled back into discovery: A YAML-based discovery instead of an Annotation-based discovery still means discovery, not management.

3. I'm strongly against "making discovery + processing" the same. They SHOULD not be the same and they are not the same.

   Let me ask you:

   What flavors of ice-creams do you have? Will you look around?

   Now, I am being asked on behalf of you to serve someone some ice-cream:

   1. I'm grateful that you discovered the available ice-cream and will gladly take that info from you.
   2. I will process it (applying defaults like Milk meta data and stuff).
   3. My co-workers want to have a say on the topic of which ice-cream we deliver, so I hand the complete info them.
   4. In the end, I don't want to repeat this stupid work, as long as the available flavors you discovered stay the same, so I note down the final results, so I can remember.
   5. In the end, it is still ME who will serve the ice-cream.

   You are just the one who discovered that it is available. You have no business at all in serving ice-cream.

   At the same time, I CANNOT invent a new flavor that we're not able to serve. I'm bound to your discovery results.

   But I do NOT care at all HOW you discovered the flavors you discovered. That's your business, not mine.

   EDIT: I care for the "default info" I need in order to serve ice-cream, I care for my co-workers, and I obviously make sure that any changes to ice-cream flavors, be it the available flavors, or the served flavors, are appropriately taken into account. All of that is my business.

co-workers want to have a say on the topic of which ice-cream we deliver

Not necessarily that we *should* do this, but that sounds like a separate "Processor" object that gets injected just like the "Discovery" object. The two are still separate, but it keeps the re-use power for plugin managers. I.e. we could have a DefaultProcessor or whatever that applies defaults, runs *_alter(), etc. and the flexibility would still be left to the plugin manager.

At the same time, I CANNOT invent a new flavor that we're not able to serve. I'm bound to your discovery results.

But I do NOT care at all HOW you discovered the flavors you discovered. That's your business, not mine.

That's true - technically.

What I meant to say is that nothing is stopping you from altering the service definitions and inject a yaml discovery but then you won't be able to find any blocks as blocks are documented and implemented as an annotation-based discovery. My point was that there *is* a use case for doing injection, which is allowing to inject a different implementation that uses the same standard for discovery.

Logging/quoting myself:

• The CacheDecorator logic should be part of the manager IMHO. Also, the factory should be sucked up as well. Plus, discovery getting injected. As a result, a massive simplification of the current architecture.
• Most of my stance actually stems from the troublesome experience of introducing a PluginBag into the mix. Which currently exists for Views, and my own experience comes from #1868772: Convert filters to plugins — I'd be very grateful for an architectural review on the plugin system aspects from your side on that issue.
• But. I'd much rather have two pristine discovery services that are injected into managers: AnnotationDiscovery + DerivativeAnnotationDiscovery. Perhaps we don't even need two, but a single one should handle both in all cases. In any case, __construct(DiscoveryInterface $discovery) { $this->discovery = $discovery; } ...and when actually needed: $this->discovery->find($this->owner, $this->type); — Swap out AnnotationDiscovery with a better/faster one? No problem, swap out the service, like any other. Right now, the decorator pattern forces me to swap out a dozen of plugin managers, in order to swap out the annotation discovery. That's bad.
• Also, is there any reason for why we're force-omitting the possibility for derivatives from a top-down plugin manager perspective, instead of always "allowing" for it, bottom-up? Or alternatively, couldn't it be a simple additional parameter, as in: DiscoveryInterface->find($owner, $type, $derivatives = TRUE); ?

What is injecting getting us? Its a pretty big conceptual shift in responsibilities. Specifically, the manager is less "the keeper and sole authority on how plugins work" now because discovery is being dictated by something outside its control. Because of that, I want to be clear on why we're doing it not just "its cleary a big win for ."
What use cases are we looking to support?
What measurable improvements does it make to experiences for type developers or plugin developers?

Quick under-caffeinated thoughts on the last point:

Pros:

I have to admit it provides for a much better method of testing. Easily mocked discovery can better test code we are now requiring be in the manager.

Cons:

I can no longer just do new FooPluginManager() and start interacting with it and expect the behavior to be consistent. Any supporting code for figuring out how to inject the discovery also has to be available which means then really the only way to do that is bootstrap Drupal to some level and pull out of the container.

A possible conclusion is putting the discovery for the manager on the global container. This means that we not have 2 methods for accessing discovery on the container globally available for everyone and one of them is completely wrong to interact with the sole exception of you being the manager.

other thoughts?

thinking further, the issue of injecting is at best tangentially related to this issue isn't it? can we kick it to a new issue?

Sorry to add to the confusion with a shameless plug, but - any major rearchitecturing plans regarding Plugin API and injectability should probably also keep an eye towards #1863816: Allow plugins to have services injected.

Our main mechanism for object instantiation currently does not allow proper dependency injection and forces plugins classes to pull their dependencies using the procedural drupal_container(). With entities & config entities now being plugins, that's like 80% of our classes that are locked out of the acknowledged "D8 right way" & the associated unit testability.

In short, current plans over there, based on the current architecture, involve :
- Making each plugin expose their dependencies - putting that in the plugin definition probably won't cut it because we need inheritance (plugin B extends plugin A and probably needs plugin A's dependencies too). So a Plugin::getDependencyInfo() method maybe ?
- at DIC-compile time, collect the info and place it in the DIC so that it's alterable (e.g let me swap database.slave instead of database for this specific plugin_id on my site). Either in "abstract factory" service entries (one per plugin_id), but that probably wouldn't work well with the current design; or in container params (one per plugin_id) rather than services.

I'm going to try to reframe this issue, because it seems like you are talking past each other.

It seems like everyone agree that what is desired is to split the "discovery" responsibility into three individual steps of "discovery" (per se), "processing" and "caching".

@EclipseGc is suggesting to implement those steps as methods of a base abstract class. @berdir seems to be worried that doing so would reduce the co-reuse and overridability that CacheDecorator gives us.

It doesn't have to be that way. We can implement the three new steps with three different objects. That would enable a dependable ("trustworthing") flow while still allowing code reuse by composition. Also, it would nicely remove the needs for decorators, an anti-pattern that should be avoided at all costs :)

Actually, totally fine with a base class. I'm even fine with a default manager class that works out of the box.

Worked a bit on @EclipseGc's proposal and made into a DefaultPluginManager that works out of the box (almost) and just needs to be configured in the Bundle.

Notes:
- Tried to inject as much as possible
- Renamed to DefaultPluginManager and made it not abstract.
- Not extending from CacheDecorator currently, that somehow simply feels wrong. Also can't inject the cache backend into that currently
- Added a setCache() method that allows to set the used cache backend and the cache key prefix. Using a loop to clear the caches, which makes it possible to not having to use tags for a known, simple set of variations. language related stuff is the only thing that is currently not injected.
- Added a setAlterHook() that works with an injected module handler.
- Both methods can be directly defined in the Bundle, the advantage of using methods is the constructor doesn't have like 10 arguments and I can ensure that I can require everything for groups of arguments (e.g. cache backend + cache key or module handler + alter hook name)

This basically seems to work. I think I'm also starting to understand @EclipseGc now because right now we pass the discovery to the factory which means that the factory accesses the uncached, unaltered definitions.

Patch will probably explode, just trying to get some early feedback.

I think I'm also starting to understand @EclipseGc now because right now we pass the discovery to the factory which means that the factory accesses the uncached, unaltered definitions.

The moment alter, process & cache are taken out of the discovery and done in the manager (i.e discovery is only "the mechanism for initial collecting of definitions" rather than "the final list of definitions"), then sure, the factory totally needs to receive the manager rather than the discovery.

Also, +1 for not extending CacheDecorator, felt really confusing IMO.

DefaultPluginManager maintains a statc cache of definitions even if setCache() has not been used to configure a persistent cache. Probably makes sense, but given everything is called "cache" indistinctly, it's a little bit confusing (e.g you'll still need to call clearCachedDefinitions() when appropriate even though you haven't setCache())

+1 on removing the decorators once this is in :-)

This should fix the test failures or most of them.

The aggregator render one is a bit weird. That was exposed because the definition caching did not yet work and it looks like something in the test changes the feed.block to 0 so the query doesn't find it anymore which then results in those notices. Fixing caching made them go away but this sounds like something that should be made more fail-safe anyway, at least if we don't convert that to block instance configuration.

Not sure what do do about ->definitions. I can do that only if there is a cache bin, any real manager should be using one anyway. Not yet changed.

As there seems to be a basic consensus on the approach, I converted a number of additional plugin managers, 3 of them were removed completely (aggregator fetcher, tour tip and edit.module EditorManager (editor.module also has a EditorManager)) as all they did was setting the discovery and decorators. Mostly made that to see how this affects all those plugin managers ( we have a lot already!) Let's see if that introduces any new fails.

Forgot some use statements, this should fix the installer.

This should fix the test failures.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.phpundefined
@@ -0,0 +1,274 @@
+  public function __construct($owner, $type, array $namespaces) {
+    $this->owner = $owner;
+    $this->type = $type;

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -149,7 +149,11 @@ public function build(ContainerBuilder $container) {
     $container->register('plugin.manager.entity', 'Drupal\Core\Entity\EntityManager')
+      ->addArgument('Core')
+      ->addArgument('Entity')

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorTest.phpundefined
@@ -64,7 +64,7 @@ function setUp() {
-    $manager = new EditorManager($this->container->getParameter('container.namespaces'));
+    $manager = new EditorManager('editor', 'editor', $this->container->getParameter('container.namespaces'));

Seems a bit weird / needlessly verbose to inject the $owner and $type properties - like, do we want to allow using EntityManager with different $owner and $type values ?
Unlike the list of namespaces, that depend on external conditions and need to be injected, it looks like those would be hardcoded in the manager ?

I just did that because that's the constructor arguments that the default plugin manager now expects. Probably makes sense to override the constructor and specify it there in case we still have a manager.

Ok, that was too much there, did run into serialization issues with the database connection and so on.

Re-roll without changing all the plugin managers yet. Want to get this green again first and then attack them one be one or at least in smaller groups.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.phpundefined
@@ -31,12 +26,12 @@ class BlockManager extends PluginManagerBase {
-    $this->factory = new DefaultFactory($this->discovery);
+  public function createInstance($plugin_id, array $configuration = array(), Block $entity = NULL) {
+    $plugin_class = DefaultFactory::getPluginClass($plugin_id, $this);
+    return new $plugin_class($configuration, $plugin_id, $this, $entity);

This looks like a bad merge, block plugins no longer need the entity.

This certainly makes all of the managers smaller, but now that each one will need its own annotation (see #1966246: [meta] Introduce specific annotations for each plugin type), should we come up with a Drupalism to match the setAlterHook, setCache, etc.?

Ok, that was too much there, did run into serialization issues with the database connection and so on.

This is now possible as #1953800: Make the database connection serializable just got in!

Yes it was :) Let's try this.

A method for that might make sense, will integrate that when I convert the first one that already uses such a class.

We might want to get this in without converting all managers as I started above to avoid making the patch too big I guess. But probably a few example conversions to see that it works.

@amateescu: I know, that's why I picked this up again :)

\Traversable it is.

Great stuff so far, here are a couple of tiny points.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.phpundefined
@@ -0,0 +1,266 @@
+   * Implements Drupal\Component\Plugin\PluginManagerInterface::createInstance().
...
+   * Implements Drupal\Component\Plugin\PluginManagerInterface::getInstance().
...
+   * Implements Drupal\Component\Plugin\Discovery\DicoveryInterface::getDefinition().
...
+   * Implements \Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface::getDefinitions().
...
+   * Implements \Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface::clearCachedDefinitions().

This could all use {@inheritdoc}

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.phpundefined
@@ -0,0 +1,266 @@
+  public function processDefinition(&$definition, $plugin_id) {

Needs parameter docs.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/DefaultsTestPluginManager.phpundefined
@@ -23,7 +22,6 @@ public function __construct() {
     $this->factory = new DefaultFactory($this->discovery);

I guess this line could also be removed?

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.phpundefined
@@ -0,0 +1,266 @@
+   * @var \Drupal\Core\Extension\ModuleHandler
...
+   * @param \Drupal\Core\Extension\ModuleHandler $module_handler
...
+  public function setAlterHook(ModuleHandler $module_handler, $alter_hook = NULL) {

Just a nitpick: We have an interface for module handlers.

- Fixed the inheritdoc stuff and cleaned up the class a bit, there were a few methods that are already in the base implementation and aren't needed.
- The DefaultFactory can't be removed in the test manager because we don't invoke parent::__construct() as we're using a completely different discovery. Maybe move that to a separate method so that we can override it separately from the constructor?
- Changed to ModuleHandlerInterface.
- Simplified getDefinition()
- Not sure I follow the remark about cache problem with block manager?
- CachedDiscoveryInterface: I'm wondering if we shouldn't merge that in the default interface? It doesn't say anything about persistent cache and I can't see the reason to not at least to a static caching and when you do that, being able to reset that could be useful in a test or so. And even if not, in the worst case it's an empty function that you have to implement. The reason I'm wondering about this is that you don't get autocomplete for that method in e.g. Drupal::pluginManager() (see #1950670: Decide what methods should be added for plugin managers to \Drupal) and e.g. for block manager, it's used quite frequently. For now, we need the separate interface for the cache decorator but we can simply do PluginManagerInterface extends CachedDiscoveryInterface. Thoughts?
- alterHook/cache id is set through the methods defined in services.yml.
- Converted the managers in core.services.yml and also added support for the annotation namespace/class name arguments. Can't be a separate method as we need it in the constructor and it can't really be defined in services.yml anyway. So managers with a custom class might need to keep their constructor override. For those where it's just a trivial parent::__construct('Block', $annotations), we could completely move it to services.yml as my earlier patches did. Not sure how I feel about that yet. And how many of those will actually stay like that.

Let's see how this goes...

Hm, silly mistake.

Excluding the entity manager for now although I guess those test fails were due to the missing cache tag.

I really like that there is no a common base, so for example most plugins will have derivative support.

PluginManagerInterface extends CachedDiscoveryInterface.

It's a good point that you will get autocompletion support though conception it just feels wrong. Maybe we have to accept it.

In general i'm wondering whether there should be tests for this new plugin manager. For sure there are enough users of this code, so we are save, but it always feels wrong to rely on such functionality.

Work in progress.

- As discussed with a few people here, we're making the default plugin manager abstract for the following two reasons:
a) Being able to have a single spot where the plugin discovery logic is specified: which subdirectory, alter hook etc.
b) Being able to identify a certain plugin manager based on the class, e.g. $class being a ConditionPluginManager instead of a DefaultPluginManager.
- Also injected the language manager, there's an issue that will make language_list() a method but that's not yet in.
- Started to add mocked PHPUnit tests. Having some troubles there with t().

Ok, converted the already converted plugin managers.

Also using the static discovery now in the test which avoids the t() problem, so was able to remove that again.

The phpunit test looks quite good, with the exception of calling parent::setup. That should never be necessary when using UnitTestCase.

Forgot the use in BlockManager, removed the parent call and the unecessary $namespaces assignment.

+++ b/core/tests/bootstrap.phpundefined
@@ -1,5 +1,7 @@
+use Drupal\Component\Utility\String;

Just noting that this change is not needed

+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.phpundefined
@@ -0,0 +1,177 @@
+  function setUp() {

docs + visibility

+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.phpundefined
@@ -0,0 +1,177 @@
+  function testDefaultPluginManager() {
...
+  function testDefaultPluginManagerWithAlter() {
...
+  function testDefaultPluginManagerWithEmptyCache() {
...
+  function testDefaultPluginManagerWithFilledCache() {

visibility

+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.phpundefined
@@ -0,0 +1,177 @@
+    $module_handler = $this->getMock('Drupal\Core\Extension\ModuleHandler');
+
+    // Configure the stub.
+    $alter_hook_name = $this->randomName();
+    $module_handler->expects($this->once())
+      ->method('alter')
+      ->with($this->equalTo($alter_hook_name), $this->equalTo($this->expectedDefinitions));
+
+    $plugin_manager = new TestDefaultPluginManager($this->namespaces);
+    $plugin_manager->setAlterHook($module_handler, $alter_hook_name);
+
+    $this->assertEquals($this->expectedDefinitions, $plugin_manager->getDefinitions());

We should test that the alter hook is not invoked when alter is not set.

...
Some notes to the test in general:

Missing testcoverage for: clearCachedDefinitions, getDefinition, __construct (not sure whether it is easy to test that).

Thanks. __construct() can't be tested with phpunit due to t() in the annotations, so we can't use annotation discovery. getDefinition() should be easy, clearCachedDefinitions() is also not possible, we first need language_list() to be a method on the language manager. There's an issue for that.

Note that those two functions have a lot of test coverage, e.g. through block manager web tests.

This should fix the test failures, unnecessary use, missing public.

I don't think it's necessary to add more specific for not invoking the alter hook. Because if it would get called, the initial test would die with a fatal error as $this->moduleHandler is NULL, so...

Re-roll, no more ugly constant defining in the test :)

me--

This should fix the last fails.

Oh c'mon :)

That happens if you are too lazy to run a test locally and too stupid to correctly copy & paste ;)

+++ b/core/lib/Drupal/Core/Condition/ConditionManager.phpundefined
@@ -7,35 +7,36 @@
+   *   An object that implements \Traversable which contains the root paths keyed by the corresponding namespace to look
+   *   for plugin implementations,

Ups, what happened here

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.phpundefined
@@ -0,0 +1,211 @@
+   * Sets the cache backend that should be used.

I'm wondering whether we can describe it better what is doing beside setting the cache backend.

+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.phpundefined
@@ -0,0 +1,154 @@
+  public function setUp() {

How can that work: setUp is protected in UnitTestCase

If you are here: Use {@inheritdoc}

+++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitGdTest.phpundefined
@@ -215,7 +215,7 @@ function testManipulations() {
+    $manager = new ImageToolkitManager($this->container->get('container.namespaces'), $this->container->get('cache.cache'), $this->container->get('language_manager'), $this->container->get('module_handler'));

The image toolkit code doesn't support a module handler. Why do we pass it in?

Improved the documents and removed the module handler argument (see second part of #59 for reason why that was there).

setUp() visibility/doc is a mess right now in core, see "grep -R -C2 'function setUp' core/tests/". The parent implement is protected, but you can always make methods more open, just not less. So we have one case of protected, a few public and mostly not specified at all right now in core/tests. Same for documentation. AFAIK, our coding standards that say no docblock for getInfo() and setUp() also applies to PHPUnit, so leaving that out for now. We also have quite a few parent::setUp() calls there, looks like something to check for the conversions and clean-up existing ones in a separate issue...

Thanks for your constant effort on this issue!!

setUp() visibility/doc is a mess right now in core, see "grep -R -C2 'function setUp' core/tests/". The parent implement is protected, but you can always make methods more open, just not less. So we have one case of protected, a few public and mostly not specified at all right now in core/tests. Same for documentation. AFAIK, our coding standards that say no docblock for getInfo() and setUp() also applies to PHPUnit, so leaving that out for now. We also have quite a few parent::setUp() calls there, looks like something to check for the conversions and clean-up existing ones in a separate issue...

I prefer sane arguments vs. existing standards and at least some people started to use it.

    /**
     * Sets up the fixture, for example, open a network connection.
     * This method is called before a test is executed.
     *
     */

This seems to be a valuable information we can just use.

Yes, I had something like that in the previous patches and it helps to group these two arguments together, I did set them directly from the service definition, which @yched didn't like because import information about how the plugin works (alter hook name, subdir) were then in different places, which is a valid argument.

I also still have that method in the test class to be able to set it from the test.

As a compromise, maybe re-add the method and set it from the constructor? protected so it has to be set from the constructor? maybe we're trying to babysit too much and should just lead by example?)

Like this?

Did that.

Last patch passed, back to needs review.

The alter hook point got adressed.

Yes! Let's get this in!

DefaultPluginManager doesn't seem very default if its abstract. Why isn't this a base class as is the defacto standard for this sort of inheritance?

I tried to give a thorough review of the rest of the code looks pretty solid other than what seems to be some code smell with requiring a language manager to manage caches.

Seems like that interaction between languages and cache here could be cleaner. Especially the sort of weirdness with a manager that already uses a language manager but is also having one set by the caching stuff. Also, if there was a system not dependent on languages it would be forced to use it still. That last point is a bit of a stretch though. I'd suggest we tackle that in a follow up because the requirement is largely tied to how the cache system works so we could get off in the weeds really quick.

Crossposting. I don't see FilterPluginManager in the patch in #71. Need to make sure all plugin managers are changed?

We already had the language dependency before, I'm not adding that. the language_list() call is ugly but that will hopefully be soon be a method on the language manager, so we can unit-test that as well.

Plugin managers are not strictly required to rely on the language, they could not use that base class or override/not use the relevant methods. But if you use caching and at least one @Translation thing, not using the language in the cache key is simply a bug and will hurt you sooner or later. So I think having that behavior by default makes sense. We could easily make the language manager optional and if not given, don't do per-language suffixes but see above. Thoughts?

Valid point about the name, it wasn't abstract when that name was chosen. The thing is, we already have a PluginManagerBase so unless we convert all managers, we can't use that name. But we should probably merge it back into that one once we're done with the conversions. It's too late here, so I can only think of silly names... PluginManagerNGBase, TrustedPluginManagerBase.. better ideas? ;)

clarifying points 2 and 3

Add task to create follow-up issue to fix the rest of the plugin managers and list those files per berdir.

#2010412: [Meta] Change plugin managers to use DefaultPluginManager

I created a follow-up issue for the rest of the plugin manager classe per Berdir's comments in IRC.

The old title was the reason this was opened, but does not describe what this accomplishes.
Is this title any better?

+++ b/core/lib/Drupal/Core/Validation/ConstraintManager.phpundefined
@@ -39,18 +37,14 @@ class ConstraintManager extends DefaultPluginManager {
-    $this->alterInfo($module_handler, 'validation_constraint');
-    $this->setCache($cache, $language_manager, 'validation_constraint');
+    $this->alterInfo($this->container->get('module_handler'), 'validation_constraint');
+    $this->setCache($this->container->get('cache.cache'), 'validation_constraint');

This kinda stinks, IMO. Instead of having typehinting you have to pull magical stings out of the container. I'm -1 on this change.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.phpundefined
@@ -11,13 +11,14 @@
-use Drupal\Component\Plugin\Factory\DefaultFactory;
+use Drupal\Core\Plugin\Factory\ContainerFactory;

@@ -81,25 +89,30 @@
-    $this->factory = new DefaultFactory($this);
+    $this->factory = new ContainerFactory($this);

This, however, I'm very +1

I think the body of the constructor is more important than the signature when it comes to simplicity

How does it solve the setCache() issue? It seems to just hide the setCache() issue, if that's what you want you can also just remove the CacheBackendInterface from the constructor and get it from the container but that doesn't change that setCache() needs the cache backend and the language manager to work.

It also makes (unit) testing harder, which you haven't changed and will fail now.

Well, it's not a detail of just that implementation, it needs to match the implementation in clearCachedDefinitions(), which relies on the language list and will also use the language manager once that has a method to get this information. So I'd rather make it an optional argument and only append the language suffix conditionally. Because then we can check the existence of the language manager and only do the loop in clearCachedDefinitions if we have one.

So I see that we will likely need the container anyway for the ContainerFactory, that shouldn't have gotten in without the injected container, so I'm fine with injecting that into the constructor and getting things from there. Should still be possible to mock it, although it's a bit ugly.

This is a reroll of #71, which had consensus and was RTBC before this whole issue got derailed.

I addressed @neclimdul's concern about it not being abstract, and I also fixes some typos/docs errors.

#95: plugin-1903346-95.patch queued for re-testing.

I really like the status of this patch

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.phpundefined
@@ -22,7 +22,7 @@
-abstract class DefaultPluginManager extends PluginManagerBase implements PluginManagerInterface, CachedDiscoveryInterface {
+class DefaultPluginManager extends PluginManagerBase implements PluginManagerInterface, CachedDiscoveryInterface {

Well, that doesn't change the fact that you can't use it out of the box, which was initially the idea but has the downside of having important information in the services.yml file.

Another possibility would be to call it DrupalPluginManager, as EclipseGc initially did. And I think we could merge it back into PluginManagerBase once everything is converted. I don't think there is a reason to not use this and if there is, you can still build a completely new class that respects the interface?

In short, I don't care about the name/abstract, whatever gets in.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -0,0 +1,227 @@
+   * Cache backend instance.
+   *
+   * @var \Drupal\Core\Cache\CacheBackendInterface $cache
+   */
+  protected $cache;

Should we rename this to $cacheBackend? The module handler is $moduleHandler and the language manager is $languageManager.

+++ b/core/tests/Drupal/Tests/Core/Plugin/TestPluginManager.php
@@ -0,0 +1,49 @@
+class TestPluginManager extends DefaultPluginManager {
+
+  /**
+   * Constructs aa ConditionManager object.

Stale comment.

Also swapped the DefaultFactory for ContainerFactory as was done by @EclipseGc in #84, missed that during my reroll of the older patch.

In addition to swapping $cache for $cacheBackend, swapped setCache() for setCacheBackend().

Fixed the stale comment.

Berdir pointed out a mismatch in naming of $cache/$cacheBackend, and when I let PHPStorm fix that, it fixed the rest of the docblock as well. Not out of scope since it was added in this patch.

#102: plugin-1903346-102.patch queued for re-testing.

#102: plugin-1903346-102.patch queued for re-testing.

its certainly a step forward

Committed 1d60d2e and pushed to 8.x. Thanks!

Need to check the existing plugins change notices...

Can we please get a change notice for this one? There should be a change notice showing how to do things properly, or at least it should briefly describe why this is better and refer to some examples in core.

Added https://drupal.org/node/2034535 for a change notice (we currently have 0 other change notices that even mention the word "PluginManager" and updated the documentation at https://drupal.org/node/1637730 (which certainly needs more work after all the changes we made).

Lovely, thank you! Now we can point core contributors to that change notice to get started on the latest best practice :)

Hi!
I am rethinking some of this over at
#2033611-11: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information. ff

I really need some feedback over there.