Run PluginManagerBase::processDefinition() in a ProcessDecorator

I think what would be cool is a EventDecorator that dispatches an event via Symfony's EventDispatcher. Then the plugin manager could add listeners for both processDefinition() and drupal_alter(). I'll post a patch when I get a chance.

I think I disagree with the premise. First, aside from the posed question about processing and defaults, I think the code example is just the "right" way of doing things. The interface is there as the contract and show what methods you're expecting but the Manager is is the correct object to pass because in the situation we want to proxy and interact with the method calls.

Also as I've argued before, the Manager is the only object that knows anything about how those defaults should work and that's why it exists as a method on that object.

To address the points listed:

• Seems like a fairly minor thing but yeah, this does add some additional overhead. I'm not sure the php time this would add is generally going to be large but yeah.
• I'd argue that is just correct. Just an observation of the facts of the system. If you're not using the manager for its intended purpose, you incur the costs associated with that.
• Again, mostly observing how the system works. As I said, the Manager knows about the interactions of the other code components. That is its entire purpose. It constructs your factory and knows that its preprocessing or not and needs to just setup the factory correctly to use it.

I don't see either of the suggested approaches as being better then the current setup, I think they would actually spread out the responsibility rather then confining it to a particular object making things less clear.

Seems like the simple solution is fix them and educate? The process should be clearly documented in the source and examples provided with the original patch.

It provides no public functionality so it is the right way of handling it. I'd be private but that would obviously be non-functional.

Another possibility for fixing processDefinitions is have definitions be their own objects. The object could then contain the defaults and any more elaborate processing logic directly on the object. I don't think this would be too hard and has some nice type hinting possibilities.

The questions then boil down to
1) How does the default implementation work?
2) Related to 1, how do we make the DX simple so the process of creating a plugin type isn't too onerous? We could be adding yet another class definition to the process.
3) How do we make mocking definitions as simple as possible? new ViewsFilter(array('title' => 'Pretty Simple'));?
4) Performance?

We got stuck on this, we were passing $this->discovery, not $this to DefaultFactory.

I think it makes sense to move processDefinition() into the discovery object in some way. If that means that more people have to sub-class the discovery then so be it. The far superior approach IMO, would be to add a DefaultsDecorator. I don't think is is bad to have many decorators. Just stick CacheDecorator at the very end, and be done with it :-)

The DefaultsDecorator can have $defaults directly in its constructor, so FooPluginManager::__construct() would look like:

public function __construct() {
  $defaults = array(
    ...
  );
  $this->discovery = new CacheDecorator(new DefaultsDecorator(new AnnotatedClassDiscovery(...), $defaults), ...);
  $this->factory = new DefaultFactory($this->discovery);
}

So no subclassing would be needed other than the plugin manager.

This would solve my problem of AlterDecorator not running after the defaults have been processed...

Right, I forgot AlterDecorator in #11, but you get the idea...

So, the problem with this is that you couldn't customize default processing easily. Defaults processing is a function of the plugin manager right now, and since we always write a custom one of those, that's a pretty simple thing to just take care of if you need something custom. I agree that there is a problem that exists here, but I question if this is the right approach as I'm pretty sure it's going to be insufficient as well.

So, the problem with this is that you couldn't customize default processing easily.

What do you mean by that? I don't get what's not easy with something like #11.

In #1816916: Recursively merge in defaults I had to change InspectionTest::testInspection() because it was doing

$plugin = $this->testPluginManager->createInstance($id);
$definition = $plugin->getDefinition();

Whereas
$definition = $this->testPluginManager->getDefinition($id);
was needed for the definitions to be processed.

I'll take a stab at implementing a new Decorator.

Here's a first stab.
I'm getting some strange failures locally, so I'll let the bot have a go before investigating them. I fear that could be non-trivial.

Some remarks:
1. I went with the plural DefaultsDecorator instead of the more standard DefaultDecorator, to avoid confusion that this is the default decorator, instead of the decorator that applies defaults.

2. I put it into Component, even though it is not 100% stand-alone; the only dependency is NestedArray, which also lives in Component. I don't really care either way, but personally, this feels more Component-y than Core-y to me.

3. This makes PluginManagerBase purely a "pass-through" class with no own logic. (That is a good thing!)

4. This conflicts with (or in other words, incorporates) #1816916: Recursively merge in defaults. If that one wouldn't be a dependency for #1763974: Convert entity type info into plugins I would suggest closing that one as duplicate, but given that, we shouldn't hold that one up. It's trivial to re-roll this one in case that lands first. If this lands first, the other one is obsolete and I'll happily re-roll #1763974: Convert entity type info into plugins with the 2-line change that is needed in that case

5. This adds yet two more functions (getDefinition() and __call()) that would be obsolete with #1809200: Consolidate discovery code in a DiscoveryBase and DiscoveryDecoratorBase

Self-Dreditor-review:

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DefaultsDecorator.php
@@ -0,0 +1,72 @@
+   * @param DiscoveryInterface $discovery

Use the full namespace here.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DefaultsDecorator.php
@@ -0,0 +1,72 @@
+   *   An array of defaults that will be added to each plugin definition. For
+   *   nested arrays, each array level is merged using
+   *   \Drupal\Component\Utility\NestedArray::mergeDeep().

This weirdly makes it sound like mergeDeep() is called recursively.

Right, was pretty trivial after all...
*slapsforhead*

Oops, that was an inception patch. Sorry, it's late.

All right, that was a case of api.drupal.org having outdated/cached information, so I hadn't updated all Plugin managers. The interdiff shows just that.

This passes a bunch of tests, that previously failed, but I didn't run all of them.

Unassigning for now.

After wrestling with #1764232: CacheDecorator provides no way to clear cached definitions all night, I've come to terms with the DX regression for it, you know, actually working :)

However, this issue shouldn't be removing processDefinitions, so I've restored it.

Closing #1816916: Recursively merge in defaults and moving the tests here.

Forgot to update Views and the new test :)

Missed semicolons after the use statements, and lessened the scope of the Views changes.

I've tried to remain quiet on this one and let it work out but I feel pretty iffy about the idea of adding another decorator to solve it. We are sort banging decorators to death already. Decorators are a great tool for letting us quickly prototype and build systems but I think requiring half a dozen for the system to have core functionality is needlessly obfuscating and misses the point. If this is really a problem we should come up with a real solution, not try to bandage it with a decorator.

Note, my issues with CacheDecorator in #26 were due to my own misuse of the system.
As such I've reopened #1816916: Recursively merge in defaults

Looks good. No idea why you marked it needs work. I can't RTBC myself, but marking "needs review" at least.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DefaultsDecorator.php
@@ -56,8 +56,8 @@ public function getDefinition($plugin_id) {
-    foreach ($definitions as $plugin_id => &$definition) {
-      $definition = NestedArray::mergeDeep($this->defaults, $definition);
+    foreach ($definitions as $plugin_id => $definition) {
+      $definitions[$plugin_id] = NestedArray::mergeDeep($this->defaults, $definition);

Also, any reason you reverted that? Seems there's no reason for that.

#30: drupal-1764278-30.patch queued for re-testing.

That's why I marked it needs work.

And as far as #34 goes, that's just a coding style quirk, I think we avoid using the by reference style when we need the key.
($definitions as &$definition) or ($definitions as $plugin_id => $definition)
but not ($definitions as $plugin_id => &$definition)

Not important though.

Rerolled. The only non-reroll changes are removing a stale comment in PluginManagerBase::processDefinition about applying defaults, and re-reverting the pass-by-reference in the foreach. As you correctly point out, the key is actually not needed here. Not providing an interdiff, as that would be obscured, because most of the changes were actually removing whole hunks from the patch (which would turn up as adding stuff in the interdiff).

This should be better.

Sure that would be totally possible. If that is what is agreed on, I would roll such a patch.
I personally don't think that is very good architecture, though. Basically all Decorators except DerivativeDecorator are (more or less) one-line wrappers around the parent implementation. I.e. we could just as well convert AlterDecorator to use such a closure-based Decorator. I think it is much clearer though, to write

$defaults = array(...);
$this->discovery = new AlterDecorator(new DefaultsDecorator(..., $defaults), 'some_info');

than (untested)

$defaults = array(...);
$add_defaults = function($definition) use ($defaults) { 
  return NestedArray::mergeDeep($defaults, $definition);
}
$alter = function($definition) {
  drupal_alter('some_info', $definition);
  return $definition;
}
$this->discovery = new ClosureDecorator(new ClosureDecorator(..., $add_defaults), $alter);

or alternatively

$defaults = array(...);
$add_defaults_and_alter = function($definition) use ($defaults) { 
  $definition = NestedArray::mergeDeep($defaults, $definition);
  drupal_alter('some_info', $definition);
  return $definition;
}

$this->discovery = new ClosureDecorator(..., $add_defaults_and_alter);

I think the former is much clearer. And I don't think we will get to the point where we literally have 20 decorators, at which point creating little one-off decorators for every thing would get unwieldy and combining things into a single ClosureDecorator might make sense.

Also apart from being IMO clearer, creating real decorators for real use-cases increases re-use and avoids duplicated code. Sticking a drupal_alter() somewhere versus instantiating an AlterDecorator doesn't really make a difference, but I would imagine if everyone writes their own "adding defaults" decorator themself, half of them forget to use NestedArray properly and run into problems down the road.

@yched re #41

I had passingly suggested that we should have a callable parameter for such a decorator. That would support anonymous functions, functions and static methods, so I think it's likely to be the best solution if we honestly want to move this issue forward.

Eclipse

This issue has bothered me since it move to trying to use a decorator pattern. I've had trouble coming up with a way to articulate that because I don't want to stop discussion on the problem. I think I finally figured it out.

The decorators provided with the plugin system where not intended to be a model for adding core functionality. They where there to provide a simple method for quickly developing simple features. I don't see defaults as fitting that design and the deep nesting of decorators we're setting up is destined to cause of performance and logic problems. I'd much rather see the problem addressed as a deeper change to the system if we're going to do it.

Are you confusing $this->factory with $this->discovery?
I have no strong opposition to this, as long as entity info goes in as is and continues to work.

Well, I guess
yched + EclipseGC + neclimdul + tim.plunkett > tstoeckler
:-)

I won't lose a second of sleep over this, but in terms of working on this patch, consider me unsubscribed. Whichever way we go, we should really get this in. It will be super confusing for anyone working with Plugins.

I really like this approach. I'd RTBC, but I'd like sign-off from one of the other Plugin system gurus.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/ProcessDecorator.phpundefined
@@ -0,0 +1,73 @@
+ * Definition of Drupal\Core\Plugin\Discovery\ProcessDecorator.

EclipseGC pointed out that this could be Drupal\Component

+++ b/core/lib/Drupal/Core/Plugin/Discovery/ProcessDecorator.phpundefined
@@ -0,0 +1,73 @@
+   * @param string $cache_key
+   *   The cache identifier used for storage of the definition list.
+   * @param string $cache_bin
+   *   The cache bin used for storage and retrieval of the definition list.

These are copy/pasted :)

Also, "Definition of" should be "Contains", and references to class names in docs should start with a leading \, like \Drupal\Core

+++ b/core/lib/Drupal/Component/Plugin/Discovery/ProcessDecorator.php
@@ -0,0 +1,71 @@
+    foreach ($definitions as $plugin_id => &$definition) {
+      call_user_func_array($this->processFunction, array(&$definition, $plugin_id));
+    }

I don't think you need to explicitly pass $definition by reference. It is passed by reference automatically inside the foreach if I'm not mistaken.

Otherwise looks good.

I think I agree with @neclimdul in #44 — each decorator should serve a concrete and unique purpose that cannot or should not be handled by the core plugin system itself. A decorator to process definitions in a better or optimized way moves the decorator pattern to a level that is detrimental for the overall system, its architecture, performance, as well as DX.

I think the patch makes clear that something has to be done about the problem, but I do think that the architectural concerns should be taken more seriously. We should consider to address this with a base discovery class or an enhanced base plugin manager class instead.

Re #58: Thanks @yched. I somehow missed the array... You are, of course, correct.

Ok, let me clarify my stance:

I want to defer to @neclimdul to sign off this patch. The patch itself looks good to me, but not necessarily from an architectural perspective. Thus, if @neclimdul is fine with this (for now), let's make sure to (quickly) move forward with the current patch.

That said, let me mark this RTBC, since, patch-wise, there is nothing wrong with the current patch. It passes tests, is functionally correct, and appears to unblock other issues. Thus, if @neclimdul doesn't strongly object with architectural concerns in a day or two, we should go ahead.

I am basically on the exact same page as sun here. neclimdul has reservations, and I understand why. That being said, I think sun has concisely summed up probably neclimdul's misgivings... still yched's point about there not having been another solution provided in the last 3 months seems pretty relevant to me, so... I am deferring to him.

Eclipse

Has anyone profiled plugin discovery recently? I'm assuming not since afaik we scan all of vendor for plugins at the moment which ought to be horrible. If no-one takes it on I'll probably do it myself soon since I need to get more familiar with the plugin system in general anyway.

I'm concerned about dealing with the plugin discovery performance purely with caching, that's only acceptable if discovering plugins is a non-blocking operation.

This patch doesn't look like it'll affect things too badly on its own, but it's part of a pattern of adding more and more processing during discovery while we're also using plugins for more and more things in the critical path at the same time, which could end up very messy if it's not performance tested as it goes along.

If I empty my caches, then it takes 20 seconds to get plugin definitions and I can't serve any pages with views on them (or load any entities, or anything else) until they're collected then that's not going to be pretty on a busy site.

I didn't really review for architecture yet, and I'd like to see neclimdul chime in here again before committing anything so not doing that now.

I did spot one comment issue while looking through:

+++ b/core/lib/Drupal/Component/Plugin/Discovery/ProcessDecorator.phpundefined
@@ -0,0 +1,71 @@
+  /**
+   * Constructs a \Drupal\Core\Plugin\Discovery\CacheDecorator object.

No it doesn't.

@yched. Yeah I realise this patch is primarily fixing an existing (runtime) performance issue, I guess I'll open a new issue to look at plugin system performance more generally.

I stand by this being the wrong approach. Adding decorators to solve problems is not the right way to address an issue, it just adds developer complexity. That said, I don't have time ATM to take a different approach to this issue so if its actually blocking something we can put this in as a stopgap.

Also, this issue should not be about performance, its about a possibly needed feature. The performance aspect is at best secondary because there are probably faster ways to do it.

I tried to understand what a ProcessDecorator was by reading the calling code.

+    $this->discovery = new AnnotatedClassDiscovery('views', 'wizard');
+    $this->discovery = new AlterDecorator($this->discovery, 'views_plugins_wizard');
+    $this->discovery = new ProcessDecorator($this->discovery, array($this, 'processDefinition'));
+    $this->discovery = new CacheDecorator($this->discovery, 'views:wizard', 'views_info');

1) Why is Process running after Alter? Shouldn't alter be the last thing in the pipeline?

2) "What's a ProcessDecorator?" *looks up the class definition* "Not helpful."

+/**
+ * Allows custom processing of the discovered definition..
+ */

What's "custom processing"? How do I know if I need this decorator on my plugin? We should add some of the examples from the OP in the docs so people understand this.

ok, typically we're discouraging this sort of notation.

$this->discovery = new AnnotatedClassDiscovery('views', 'wizard');
$this->discovery = new AlterDecorator($this->discovery, 'views_plugins_wizard');
$this->discovery = new ProcessDecorator($this->discovery, array($this, 'processDefinition'));
$this->discovery = new CacheDecorator($this->discovery, 'views:wizard', 'views_info');

in favor of:

$this->discovery = new CacheDecorator(new AlterDecorator(new ProcessDecorator(new AnnotatedClassDiscovery('views', 'wizard'), array($this, 'processDefinition')), 'views_plugins_wizard'), 'views:wizard', 'views_info');

Also, @webchick, I've rearranged the above code in the order I would expect to see it most often. discovery then process, then alter, then cache.

Process is used (most regularly) for injecting defaults that should be available across all plugins of a given type. I know you want a code example, I'll see if I can find you one shortly.

Eclipse

Would it help with docs and partially alleviate #59 if we rename ProcessDecorator to ManagerDecorator, pass it $this instead of array($this, 'processDefinition'), and add processDefinition() to PluginManagerInterface? That might help firm up the concept/flow of:
- Ask the plugins themselves for their definitions (AnnotatedClassDiscovery)
- Have the manager add defaults or otherwise adjust (ManagerDecorator)
- Broadcast for anyone to alter (AlterDecorator)
- Cache

Downside though would be that plugin managers aren't required to use a ManagerDecorator, so adding processDefinition() to their interface is maybe unwarranted; we can put that into a new interface instead, but then we'd need to name that new interface (ProcessDefinitionInterface is, well, icky).

Everything in this comment can also be a follow up, but just adding it here in case it helps get docs to where webchick is willing to commit.

Er, why would we be discouraging the "not inlined" notation ?

I agree. I like inlining if we're adding 1 decorator, maybe 2. But multiple lines is much easier on the eyes once we get to 3.

@yched thanks for that, let's definitely get this in as a performance improvement and if something else comes along later to replace it we can look at it separately.

Agreed that the mulit-line syntax for lots of decorators is a lot more legible.

Because this and #1764232: CacheDecorator provides no way to clear cached definitions both change the exact same lines, and that was RTBC, I've rerolled this on top of that patch. It will fail for now if sent to the bot (but could be requeued later).

I've added the docs that @webchick requested in #69, and I've swapped the order of Process and Alter as requested, and as described in #70. It's how it used to work, and was a limitation of the code we're fixing.

Also, with #1838368: Do not blindly use the ternary operator; it always returns copies of non-object values in mind, I've changed the unnecessary usage of the ternary in added code only. No reason to add more work for later.

The last thing I did was rename $processFunction to $processCallback, because it is a callable, and all of our usages are methods anyway.

Okay, that went in!

I opened #1848964: EntityManager should process its definitions before altering them for #78.

Rerolled with an explicit @todo.

Note that the process vs. alter order question essentially circles back into the info hook vs. alter hook problem space:

1) If you only have an alter hook, then all that is expected from alter hook implementations is to manipulate existing data. No new data is added, so it is perfectly fine to process before alter.

2) However, if you only have an alter hook and no info hook, and when there is a chance that alter hooks may need to add data (instead of only manipulating). Anything that is being added by the alter hook is not processed, and hook implementors essentially need to manually populate all keys/properties that are added by the process decorator by default, or they need a helper function to do so.

So far, plugins are not solving the horizontal extensibility aspect of former info hooks, which essentially means that we cannot be 100% sure yet that it is guaranteed that no contributed or custom module will have a legit use-case and need to add information to collected plugin definitions.

In turn, instead of or in addition to #1848964: EntityManager should process its definitions before altering them, it is perfectly possible that we're much more on the safe side by ultimately merging the info hook + alter hook decorators into the process decorator - guaranteeing a standardized processing for all plugin types, and ensuring extensibility in all dimensions. (Also bear in mind that - unlike events - hooks are very cheap in terms of performance, so there's no harm in invoking an additional one.)

Sorry, x-post.

it is perfectly possible that we're much more on the safe side by ultimately merging the info hook + alter hook decorators into the process decorator - guaranteeing a standardized processing for all plugin types, and ensuring extensibility in all dimensions.

+1 to merging them, and opening a later issue to do that (though i think it'd be OK to have a merged one for most cases, and leave the individual decorators as well). i'm in the camp that's getting antsy with the way decorators are being used to provide essential functionality, and merging some of it together would be helpful.

Committed/pushed to 8.x, thanks!

I'm just going to re-open this issue. This is my first stab at a non-decorator based fix.

I introduced a DiscoveryBase class that slims a bunch of various things in all discovery classes down by not having to repeat code, I introduced useDefaults() and processDefaults() methods to specify where in the discovery defaults should be processed, this should even allow base discovery classes to have the defaults processed at their level. This should also make caching of defaults totally viable.

a discovery methodology might look thus:

$this->discovery = new AnnotatedClassDiscovery('views', $type);
$this->discovery = new AlterDecorator($this->discovery, 'views_plugins_' . $type);
$this->discovery = new CacheDecorator($this->discovery->useDefaults($this), 'views:' . $type, 'views_info');

The useDefaults() call on the discovery class is actually happening at the alter level, this means the defaults aren't present for the alter. If you wanted them for the alter, the $this->discovery->useDefaults($this) would actually be passed to the Alter (which means we're actually processing defaults at the AnnotatedClassDiscovery level).

The system continues to expect $this->defaults and PluginManagerInterface::processDefinition() to exist, and routes this logic through them appropriately, so you still have complete control over this logic at the plugin manager level.

This is just a first pass at a solution I think can work. Hopefully we can figure out how to move forward here. Some weirdness within the EntityManager will throw some exceptions on this patch. I'll be looking at #1848964: EntityManager should process its definitions before altering them next since I think a number of the issues can be solved there.

Eclipse

oops

OK, going to do this in a new issue.

new issue: #1853382: Remove ProcessDecorator in favor of a solution that is in line with the original intent.