Solve Trust issues with Plugin Discovery

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?

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.

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

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.

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: Change notice: 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); ?

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?

Ok, I've commented on #1863816: [Followup] Allow plugins to have services injected at this point, and I don't think it has anything to do with this issue.

I've had a number of conversations with people about this topic and other related to it at this point, so I'm going to attempt to dump all of that here now.

The plugin manager separates code into sub classes it can delegate to for the sake of reuse, not for the sake of concerns/responsibility/separation. All responsibilities concerned with managing the plugins of a given type reside with the plugin manager at all times. Separation of code is 100% for the purposes of reuse. Any plugin manager could contain all the code necessary to perform any discovery, instantiation or mapping task. That is the nature of the plugin system. Injection of discovery or factory or mapper is unnecessary and doesn't actually buy us anything. Any benefits we might see to mocking discovery or factory or mapper are within unit test situations that prove the concept, not within implementations (which is what we're discussing here). It is important to note that despite the fact that sun makes a very very good argument for removing the vast majority of decorators (basically everything except DerivativeDecorator) the removal of base Discovery classes has not been argued for, and as such, I don't think we should entertain that at all. We still have a need for annotation, hook and static discovery, and the option to build completely new base discovery components is ++ in my opinion. In short, a manager instantiates the classes it needs because it has to be its own final authority. Injecting a different sort of discovery/factory/mapper into the manager is likely to be little more than a novelty. Replacement of ALL use cases of a particular discovery is the only argument that has merit here in my opinion, and we lose so much control and increase the barrier to creating new plugin types substantially that I have to discourage it. This will lead to arguments that we should inject the factory (which is also not true) which will lead to more confusion once people understand there's a dependency there that appears circular.

With that being said, I still feel a DrupalPluginManagerBase is the appropriate way to solve ALL of these issues. This class will remove the need for the Cache/Process/Alter decorators, and this will simplify all discovery declaration in the constructor to simply 1 discovery class and a decorator (if derivatives are desired). https://gist.github.com/4674513 is my abstract class for this approach. I will finish that soon and provide some tests for it, but the most common pattern has been discovery->defaults->alter->cache, and this class will formalize that in code.

Once this class goes in, the vast majority of PluginManagers should be able to just extend this class and declare their discovery and factory. We should also formalize that all factory classes that have been created be removed in favor of code in the manager itself unless they are reusable. This should end a lot of the confusion that has persisted about the nature of the relationship between manager, discovery and factory.

I hope this seems sane and straight forward and makes sense to all parties here. I've stressed a lot over this topic because I want to eliminate the apparent confusion over what the intention within the manager is and hopefully put an end to this debate before it can spread to contrib-land.

Eclipse

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

The last submitted patch, default-plugin-manager-1903346-17.patch, failed testing.

The last submitted patch, default-plugin-manager-1903346-20.patch, failed testing.

The last submitted patch, default-plugin-manager-1903346-22.patch, failed testing.

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.

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

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.

The last submitted patch, default-plugin-manager-1903346-30.patch, failed testing.

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

This could all use {@inheritdoc}

Needs parameter docs.

I guess this line could also be removed?

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.

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

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