Make all core plugin managers be consistent as to whether $this or $this->discovery is passed to the factory

The PluginManager still implements the interface for discovery, and can provide some essential work around techniques in various circumstances. I would heavily advise against doing this, having the ability to pass the plugin manager here and have an additional level of discovery handling that doesn't absolutely require another decorator is really essential in my opinion. I'd mark this won't fix, but am willing to at least have the discussion.

TL:DR

If you do this all custom processing will have to be a new decorator and that's sub optimal IMO.

Eclipse

We already have the ProcessDecorator, I think that covers the use case of a PluginManager wanting to do any custom processing.

This seems to me how things should work, and we just didn't do it this way due to hacks.

Better reason, the commit to that issue was a hack I said was ok till we had a real solution. Processing was not the only reason to do this, just the most visible and easiest to run into. Going to to close this.

Sorry for the x-post tim. Just to be clear, the interface exists on the manager so any amount of inlining or control can be shifted as needed by the manager. Honestly, how core uses that could be up for discussion but this is the expected behavior. Since these calls are internal to the manager it doesn't really matter but it sets a possibly confusing precedent in our core usage in what are our de facto examples and is a bit of a premature optimization with likely minimal if any gain. That's why I see this as something we shouldn't be looking to fix.

While contrib should be able to pick which technique to use, it would be ideal for core to set a best practice. We currently have some plugin managers doing it one way and some the other, for no good reason.

I personally agree with neclimdul and prefer passing the manager itself, but I also understand the DX confusion of doing so, especially since in other parts of the plugin system, you pass $this->discovery. I don't know yet whether the solution to that is to make core pass $this->discovery, or to get to the bottom of why it's confusing to pass $this, and fix that point of confusion.

Since FetcherManager was the first plugin manager, I remember trying to pass $this->discovery in Views and having it blow up due to the processing. So yeah, we should be consistent.

Let's see if anything here fails.

I did notice that the test run for this patch was extremely long. If the TypedData performance hit is true, this might be a problem.

#7: discovery-1851706-7.patch queued for re-testing.

I did notice that the test run for this patch was extremely long.

Whoa! That first run was 81 mins. I asked for a retest to see if that was a fluke or really pointing to a serious slow down.

"Yeah, the factory expect a discovery object but well, you shouldn't really give it your discovery" ? I don't really see myself explaining that when training people on the Plugin API.

FWIW, here's an allegory. Feel free to use it in training if you like it.

Once upon a time, there was a Santa Claus. He had two very important responsibilities. He maintained a list of who's naughty and who's nice. And on Christmas Day, he delivered presents. The world wasn't as populated as it is now, so he managed to do all that by himself.

As the world population grew, he couldn't keep up. So now, each city has its own Santa Claus. Every child has a mobile phone application for asking Santa whether they've been naughty or nice. And every parent has a mobile phone application for letting Santa know when the kids are asleep and that it's time to deliver the presents to their house. The applications know the city each phone is in, and therefore, which Santa Claus to send the request to. Neither the children nor the parents know how their Santa Claus maintains the list or delivers the presents, whether he does it by himself or has a staff. Not knowing is part of the fun of Christmas.

In one small town in Nebraska, the local Santa does it all himself.

In Los Angeles, the local Santa decides to hire a private investigator, Dick, to keep tabs on who's naughty and who's nice. He also hires an actor/stunt-man, Bob, to put on the suit and deliver the presents. When Santa gets pinged by a child requesting to know which list he's on, Santa calls Dick and asks. When he gets pinged by a parent that the house is ready for present delivery, he passes that on to Bob. But, Bob needs to know which children in the house have been naughty and which have been nice, so he can pick out the right present for them. When Santa sent Bob out on Christmas Eve, he gave him his phone number, so that Bob could call him when he needed that info. Santa had considered just giving Dick's phone number to Bob, but he decided against that, because he didn't quite trust Dick, and wanted to oversee the operation himself.

In New York, the local Santa is a no nonsense guy. He doesn't have time or patience to deal with shady characters, so he hires the police chief to make the naughty/nice list and the fire captain to make the deliveries, and he gives the police chief's direct line to the fire captain.

Per the allegory in #12, I don't see there being a clear right or wrong answer here. Really, it's up to each plugin manager. But I do think consistency in core would be nice, and the argument in #8 to make core's default be the lowest overhead one, and for only plugin managers that need to intervene to do so, with a comment explaining why that intervention is needed, is pretty compelling.

I'm actually not sure that I agree on consistency per se here. Obviously we should be as consistent as possible, but if we have special situations that warrant a different approach, then we should consider what that special situation entails. This is actually one of the benefits of the Plugin Manager implementing all the relevant interfaces.

Also, fwiw, the "fix" in #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator is a hack around a problem we weren't sure how to solve... it was not in and of itself a solution. I think I do have a solution that I'll be attempting to provide code for shortly. We'll see where things go from there.

Eclipse

The test suite takes 82 minutes with this patch. Two test runs in a row.

Out of curiosity, I'll post the opposite patch tomorrow.

Just to see what happens.

Yes and my point was that the typical case should be passing the plugin manager itself, not discovery specifically.

Eclipse

I'm sending that back for retest, but it finished in 48 minutes. Compared to 82 minutes.

Well, and to be clear, if we can say "wow that doubles the time for this one plugin type" then that seems a perfect example of when we'd special case it.

Eclipse

So what you're saying is "we *might* have some special case, so let's make sure everyone treats their case as special

The word "special" is implying a perspective.

Perspective 1: $this->discovery is a protected object. Passing it to the outside world breaks encapsulation. Do so only to objects that you trust (e.g., consider the factory object a "friend" in C++ lingo), and only in the "special" situations that need it (e.g., for plugin types that are instantiated frequently enough that the performance improvement is non-trivial).

Perspective 2: Most plugin managers should delegate all discovery responsibility to $this->discovery. They shouldn't need to further manipulate the definitions returned by $this->discovery. Given that, pass the narrower object to the factory. Only pass the full manager to the factory in the "special" situations that need it (e.g., when there's a good reason for the plugin manager to mediate interaction between the factory and the discovery).

Most plugin managers should delegate all discovery responsibility to $this->discovery

Emphasis mine. I'm coming from perspective 2.

Thanks effulgentsia, that was a great comment.

yched, I think eff nailed this, your "special" and mine are VERY different. I'm saying, that the plugin manager does certain handling that is universally beneficial. Any "special" case that needs to code around that (whatever the reason)... should code around that. We should not assume everyone understands all these aspects because we've already proven that everyone does not. The plugin manager simplifies much of this by making this fairly cookie cutter, we should use it as such. Passing $this->discovery will hurt us in the long run, and prevents any sort of plugin manager specific logic for definition lookups from ever firing, and instead forces the developer to rely 100% on new decorators. New decorators should provide the sort of universally applicable solutions we see in core, not plugin type specific code, and passing $this->discovery is going to force the issue on that.

We should not be passing $this->discovery except in "special" cases.

Eclipse

I have an idea that might break through the stalemate.

The original reason for wanting $this passed to the factory is so that PluginManagerBase::processDefinition() could be protected instead of public. We lost that in #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator, because running processDefinition after CacheDecorator was problematic. So now we have this code in HEAD (using FormatterPluginManager as an example):

$this->discovery = new AnnotatedClassDiscovery('field', 'formatter');
$this->discovery = new FormatterLegacyDiscoveryDecorator($this->discovery);
$this->discovery = new ProcessDecorator($this->discovery, array($this, 'processDefinition'));
$this->discovery = new AlterDecorator($this->discovery, 'field_formatter_info');
$this->discovery = new CacheDecorator($this->discovery, 'field_formatter_types', 'field');
$this->factory = new FormatterFactory($this);

And are debating that last line. But, what if instead, we do this:

$this->discovery = new AnnotatedClassDiscovery('field', 'formatter');
$this->discovery = new FormatterLegacyDiscoveryDecorator($this->discovery);

// Look where we insert $this in the chain.
$full_discovery = new AlterDecorator($this, 'field_formatter_info');

$full_discovery = new CacheDecorator($full_discovery, 'field_formatter_types', 'field');
$this->factory = new FormatterFactory($full_discovery);

And then this allows us to remove the ProcessDecorator and make processDefinition() protected again, and also gives the factory direct access to the object with the cached definitions.

Everyone wins?

Oh wait, that won't work as-is, because then the application code that calls $plugin_manager->getDefinitions() won't get the altered definitions. Dammit! But maybe there's some variation along these lines that could work?

LOL. Apparently #27 is essentially the same idea as EclipseGc posted in #1853382: Remove ProcessDecorator in favor of a solution that is in line with the original intent., and I posted what I thought was my idea *after* having skimmed the patch over there, without even realizing I had subconsciously stolen his idea.

But he ran into the same problem there as in #28: there is no way to do this without enlarging either DiscoveryInterface or PluginManagerInterface, and that defeats the whole purpose.

@effulgentsia

lol :-D

I think #28 is precisely the reason I dislike passing $this around. I want $plugin_manager->getInstance() to act on the same information that I get from $plugin_manager->getDefinition(). In practice, there's really no difference, but conceptually it's a WTF to me, if that's not the case.

The Factory get's a DiscoveryInterface in its constructor. So if you're calling anything other than getDefinition(s)() you're doing it wrong. And those functions usually are passed directly to the discovery anyway, so...

Rerolling #17 just to see if it still speeds things up.

for the record, this is precisely what I don't want us to do. :-(

And for comparison, trying #7 again, but with static caching in the manager.

Thinking about this more, I now see 3 solid reasons favoring passing $this->discovery instead of $this:

1. PHP typehinting is not strict. Even if the factory typehints to DiscoveryInterface, if you pass it the full manager, it can invoke any of its methods. So, passing $this->discovery ensures a smaller surface area of contact.
2. Per #37, the need for $plugin to be serializable even if $plugin_manager isn't, so not wanting $plugin to reference its manager.
3. Since the common pattern is for $manager->discovery to include a cache decorator, so that's where static caching already is, and since PHP stack calls aren't free, it's more performant.

OTOH, the main reason for passing $this instead of $this->discovery is so that we guarantee that factories get the same definitions as application code in the case where the manager's getDefinition(s)() implementations are not mere proxies to the discovery object.

So, I think the real question here might be whether we foresee any plugin managers in core implementing getDefinition(s)() that are not mere proxies to the discovery object, and that's very related to #1853382: Remove ProcessDecorator in favor of a solution that is in line with the original intent. and what we end up doing with ProcessDecorator. However, we've been banging our heads on the ProcessDecorator problem for a while now, and haven't yet solved it.

Given all this, how about this as a way to proceed: we do #32 for now, given the benefits listed above, but we leave open the possibility of somehow solving the ProcessDecorator problem in a way that restores the ability of core plugin managers to do something non-trivial in their getDefinition(s) implementations, at which time, we can refine the decision as to what our default pattern is and when to deviate from it?

The discussion cropped up again with msonnabaum, just rerolling this.

I'll go poke at #1875182: Pass plugin definition to plugin instances instead of discovery object as well.

OK, I've not looked at #1875182: Pass plugin definition to plugin instances instead of discovery object but that sounds like an issue that needs fixing. Passing the definition instead of the manager->definition is a much better solution in my opinion. This only really matters for plugins extending PluginBase though. Still conceptually++ to that.

I'm still very much against $this->discovery being passed instead of $this. The flexibility of the system to support either is sort of the point here, and I don't think we should be legislating what all managers everywhere should always do. The reason Managers implement these interfaces is to allow flexibility in all these areas. We should strive to be as consistent as possible, and provide comments when we choose not to be. As such my counter proposal is that we standardize on $this, and not freak out when a plugin manager uses $this->discovery instead. It just means working with that particular plugin manager could occasionally involve some additional WTFs with regard to its discovery process. That will obviously be part of the learning involved with that Manager. I'd prefer we NOT enforce that same sort of WTF everywhere. I'm not really interested in what core is ACTIVELY doing in this regard. The ability of symfony bundles to allow for this stuff to be swapped out and extended means we should do the thing which will be most straight forward for developers USING these plugin managers, not the thing which could make their lives more difficult but will immediately make sense to plugin type writers. Learning to write plugin types is not a small thing, there are a lot of details, and this is just one of those issues developers will have to learn. If they don't do it the suggested way, there are ramifications. I'm very -- to standardizing on $this->discovery.

Eclipse

We should always default exposing the most limited public interface possible when passing objects around for delegation. Designing for maximum flexibility without solid use cases is a recipe for complexity, which we have a bit of a problem with.

If we're type hinting DiscoveryInterface, then lets pass an object that only implements that interface. If there's a legitimate reason for PluginManager to be passed, we should change the typehint.

There's some considerable conceptual overhead to the idea that PluginManager implements the interfaces of objects it delegates to, and it can possibly pass itself to them.

Considering the fact that any plugin manager can choose to pass $this if they want to, it seems totally obvious to change the default. It's beyond a special case, it's incestuous.

Failure was a testbot problem. Test has been requeued.

PHP typehinting is not strict. Even if the factory typehints to DiscoveryInterface, if you pass it the full manager, it can invoke any of its methods. So, passing $this->discovery ensures a smaller surface area of contact.

That's great but it's not reliable. You go on to point out:

OTOH, the main reason for passing $this instead of $this->discovery is so that we guarantee that factories get the same definitions as application code in the case where the manager's getDefinition(s)() implementations are not mere proxies to the discovery object.

Which of course has been our basic argument from the beginning. I'd be happy seeing this thread die and us not commit anything. Again there's not a good reason to standardize this as there are use cases for both. I'd prefer we guarantee the application code and the factory get the same things all the time.

Per #37, the need for $plugin to be serializable even if $plugin_manager isn't, so not wanting $plugin to reference its manager.

We apparently have an issue to solve this already, so I think we can remove it from the reasons not to do this? This is actually a problem of the DefaultFactory and PluginBase expectations, not a problem of passing the plugin manager to the factory.

Since the common pattern is for $manager->discovery to include a cache decorator, so that's where static caching already is, and since PHP stack calls aren't free, it's more performant.

I actually pointed this out in reference to entities, and how the manager should probably be the implementor of cache, and not rely on a decorator, which I think this gets to the heart of the problem here. We have made some great decorators, so great in fact that core might not need anything else. Yay for us. But truly, none of that changes the fact that the plugin manager could and perhaps should be the one implementing some of this. Furthermore, I intend on getting back to the patch that rips the process decorator out of core as well, and that has implications here. I feel like we're putting on blinders and ignoring the very real fact that things look so weird right now because the plugin maintainers made some concessions to keep certain use cases moving, and that we always intended those to be temporary, and we're not past feature/api freeze yet. With that said, I propose we, at the VERY LEAST, postpone this. If all the relevant dates come and go and we've not changed the code in such a way that this doesn't make sense anymore, then I'll stop being a blocking pain in the butt, but we're not there yet, and I, for one, would like to opportunity to factor out the concessions we made early on.

I'd like to know what the performance implications of this are before it gets postponed on any other issues, yched's description makes it sound like things are pretty bad as it is currently.

The factory needs the definitions. If we don't pass it some discovery object, then it can't get the associated class for a given plugin id. It's given a discovery object to prevent the need for getting all definitions on plugin instantiation.

As I've said a couple times already, I'm not opposed to various plugin managers making the decision to pass $this->discovery instead of $this, but this issue appears to be one about policy, and I'm just asking we postpone a policy decision, not that we prevent any plugin manager from making a good decision for itself. i.e. I'm not going to attempt to block a specific manager's move to $this->discovery just because I am ideologically opposed to it on the whole. Nor will I try to push through patches that move all plugin managers to passing just $this. I will however attempt to block a policy decision at this point when the plugin subsystem has made a number of concessions I'd prefer to see factored out. I hope that's a reasonable request, this seems the sort of change we could make at almost any time before code freeze.

There are good use cases for both.

Eclipse

The worry here is about which class has the canonical discovery. Only when the manager overrides getDefinitions() does the manager take responsibility away from the discovery. This patch respects that.

No longer worth the effort.

RTBC assuming tests pass.

For the record this makes unexpected behaviors more likely, requires us to always make new decorator classes for our discovery processes (no in manager overrides) and prevents the possibility of more informative exceptions in the factory. Also, when unexpected behaviors happen, it'll require a ridiculous amount of knowledge about the plugin system in order to trouble shoot it.

Eclipse

That's only if people override methods on their manager. As a contrib developer, I always copy/paste from core when learning something, and this will lead to writing decorators. Then there won't be any problems.

The only issue will be if they choose to deviate from core's behavior, but don't go far enough.

#50: discovery-1851706-50.patch queued for re-testing.

Look, it's not like the plugin development process didn't include a significant discussion on this topic. We did it this way to begin with. We moved away from this approach for good reason. If core doesn't want to learn from that, fine, it is what it is. I'm unwilling to discuss it further. Core's doing it wrong, we lose all sorts of benefits by doing it this way, and apparently no amount of reasoning is going to change that. You don't have to justify it further, I concede. Go and do it wrong. I have more important things to spend my energy on.

Eclipse

No that is untrue. No number or type of Decorators have the slightest effect on this topic. This is only relevant if you do not allow the plugin manager to handle the responsibilities that belong to it. By passing $this->discovery instead of $this, you are doing exactly that, usurping the manager's responsibilities.

Eclipse

#1892462: EntityManager should use the CacheDecorator took care of the EntityManager hunks. This is the same patch as above, minus that.

#59: discovery-1851706-59.patch queued for re-testing.

Random fail on #1901546: Random failures in Drupal\system\Tests\Entity\ConfigEntityQueryTest

RTBC++

This is tagged "needs profiling" but I see not any profiling. And in #47 catch asks for it again.

The direction this took is towards the faster option (one less PHP call from the manager to the discovery object for every use by the factory), so profiling isn't needed. Had other reasons swayed it to the other option, then profiling would have been needed to know which managers that was acceptable on, and which ones are too much in the critical path.

had a discussion with webchick about this tonight, and I think I have some good examples about exactly why this bothers me. So I'm going to set this to NR pending one last follow up. If this is still unconvincing, then fine we'll just re-rtbc the existing patch, but I'd like to make one last pass on this before we just make a blanket decision here.

Eclipse

errr... it's late, sorry.

OK, ran a new solution past fago and timplunkett in IRC, follow up can be found here:

#1903346: Establish a new DefaultPluginManager to encapsulate best practices

Let's rtbc this and fix it in a follow up.

Eclipse

Glad you all were able to come to an agreement on this.

Committed to 8.x. WIll push in a few mins.