PluginManagerBase::processDefinition() provides a very cool mechanism to massage discovered plugin definitions before they are used in the open. Typical use case :
- providing default values for missing entries - which ensures a plugin type can evolve by adding new properties without littering code with isset()s
- during a "X as plugins" conversion patch, providing a LegacyClass to handle implementations that have not been ported to plugins yet (that's what we do in #1742734: [META] Widgets as Plugins currently, works just great)
Problem is, processDefinition() currently runs within PluginManager::getDefinition(), that is, outside the discovery object.
Thus :
- processDefinition() stays out of CacheDecorator, and re-runs on every PluginManager::getDefinition() call in the request (that might be a lot)
- $this->discovery->getDefinition() will get you an unmassaged definition, and it thus actually improper for actual use.
Only PluginManager::getDefinition() is trustable.
- As a consequence, the introspection methods provided by PluginBase only return trustable definitions if PluginBase::__construct() receives a PluginManagerBase, not just a DiscoveryInterface (which is what the code and signature would imply), so that the getDefinition() that PluginBase uses is the massaged one and not the raw one.
In order to do that, this means that the factory needs to be injected a PluginManagerBase rather than a simple DiscoveryInterface.
That is, your custom PluginManagerFoo class needs to do :
public function __construct() {
$this->discovery = new WhateverDiscoveryMechanism();
$this->factory = new WhateverFactory($this);
// And not :
// $this->factory = new WhateverFactory($this->discovery);
// which you'd think would work based on the DefaultFactory::__construct() signature
}
The last point is "just" fairly confusing (just look at my convoluted explanations above), and easily missed.
The first point looks like a performance issue.
Comment | File | Size | Author |
---|---|---|---|
#86 | betterprocessing.patch | 35.3 KB | EclipseGc |
#79 | plugin-1764278-79.patch | 14.82 KB | tim.plunkett |
#79 | interdiff.txt | 750 bytes | tim.plunkett |
#76 | drupal-1764278-76.patch | 14.74 KB | tim.plunkett |
#76 | interdiff.txt | 3.14 KB | tim.plunkett |
Comments
Comment #1
yched CreditAttribution: yched commentedSo to summarize:
- the processDefinition() method should be called within the Discovery, so that Discoveries return massaged definitions
- yet the method itself is definitely much better off living in the PluginManager as currently, since this is the class where your 'plugin type' specific logic lives and that you'll need to implement no matter what, while you'll be most likely reusing existing Discoveries.
Possible approaches I see :
- the Discovery object needs to be injected the PluginManager, so that it can call its processDefinition() method
- the processDefinition job is another discovery decorator. Do we really want to add more, though ?
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #3
neclimdulI 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:
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.
Comment #4
yched CreditAttribution: yched commentedFunny, the 3 subclasses of PluginManagerBase we have in core right now all do
which would then be the 'wrong' way.
So OK, they happen to not provide $defaults so their processDefinition() does nothing.
Then a couple subsystems will figure out that they need to pass $this instead of $this->factory if they want processDefinition(), some others won't, and people looking in core examples to learn how the plugin system works will just be confused.
That's too subtle for good DX, IMO.
Comment #5
neclimdulSeems 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.
Comment #6
yched CreditAttribution: yched commentedas @effulgentsia pointed in IRC, PluginManagerBase::processDefinition() is currently protected, so the discovery wouldn't be able to call it.
What's the benefit if having the method "protected" ?
Comment #7
neclimdulIt provides no public functionality so it is the right way of handling it. I'd be private but that would obviously be non-functional.
Comment #8
neclimdulAnother 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?
Comment #9
tim.plunkettWe got stuck on this, we were passing $this->discovery, not $this to DefaultFactory.
Comment #10
yched CreditAttribution: yched commentedNot closely related, but shameless plug : #1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result"
Comment #11
tstoecklerI 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:
So no subclassing would be needed other than the plugin manager.
Comment #12
tim.plunkettThis would solve my problem of AlterDecorator not running after the defaults have been processed...
Comment #13
tstoecklerRight, I forgot AlterDecorator in #11, but you get the idea...
Comment #14
EclipseGc CreditAttribution: EclipseGc commentedSo, 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.
Comment #15
tstoecklerWhat do you mean by that? I don't get what's not easy with something like #11.
Comment #16
tim.plunkettIn #1816916: Recursively merge in defaults I had to change InspectionTest::testInspection() because it was doing
Whereas
$definition = $this->testPluginManager->getDefinition($id);
was needed for the definitions to be processed.
Comment #17
tstoecklerI'll take a stab at implementing a new Decorator.
Comment #18
tstoecklerHere'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
Comment #19
tstoecklerSelf-Dreditor-review:
Use the full namespace here.
This weirdly makes it sound like mergeDeep() is called recursively.
Comment #21
tstoecklerRight, was pretty trivial after all...
*slapsforhead*
Comment #22
tstoecklerOops, that was an inception patch. Sorry, it's late.
Comment #24
tstoecklerAll 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.
Comment #25
tstoecklerUnassigning for now.
Comment #26
tim.plunkettAfter 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.
Comment #28
tim.plunkettForgot to update Views and the new test :)
Comment #30
tim.plunkettMissed semicolons after the use statements, and lessened the scope of the Views changes.
Comment #31
neclimdulI'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.
Comment #32
tim.plunkettNote, 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
Comment #33
tstoecklerLooks good. No idea why you marked it needs work. I can't RTBC myself, but marking "needs review" at least.
Comment #34
tstoecklerAlso, any reason you reverted that? Seems there's no reason for that.
Comment #35
tim.plunkett#30: drupal-1764278-30.patch queued for re-testing.
Comment #37
tim.plunkettThat'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.
Comment #38
tstoecklerRerolled. 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).
Comment #40
tstoecklerThis should be better.
Comment #41
yched CreditAttribution: yched commentedWould it be doable to have a decorator that receives processDefinition as a closure and runs it ?
Would be more flexible than just handling defaults, and still keep the code of the callback within the Manager class ?
Comment #42
tstoecklerSure 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
than (untested)
or alternatively
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.
Comment #43
EclipseGc CreditAttribution: EclipseGc commented@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
Comment #44
neclimdulThis 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.
Comment #45
yched CreditAttribution: yched commentedWouldn't this "just work" (tm) ? :
re @neclimdul:
I don't think I have a problem with stacking decorators - although rather than a single line of nested decoraters and params :
they're definitely more legible as a stacked series of assignations:
This simply describes the algorithm for building the list of definitions of this plugin type. made of well-identified algorithmic blocks with well-identified behaviors. I don't see the problem with that, and that's much better than each plugin type writing its own custom code, and you having to figure it out separately for each plugin type you encounter.
If you get performance issues, then it means the discovery process you need is costly and should use CacheDecorator. I don't think that's very different from D7 : your info hook is costly ? wrap it in a cache (which is what most info hooks end up doing in D7)
Comment #46
tim.plunkettAre 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.
Comment #47
yched CreditAttribution: yched commentedDoh. That's what you get for writing pseudo-code in the morning.
I edited #45 and fixed that.
Comment #48
tstoecklerWell, 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.
Comment #49
yched CreditAttribution: yched commentedAttached patch does as proposed in #45.
- Moves calling processDefinition() to a ProcessDecorator discovery
- Removes special logic in PluginManagerBase::getDefinition[s](), those simply delegate to the discovery now
- Updates all existing plugin types that currently make use of ProcessDefinition to use the new ProcessDecorator (and clarifies their discovery stack by unfolding the wrapping - see end of #45)
Comment #50
yched CreditAttribution: yched commentedAs a followup, this means we could get rid of the static and persistent cache handled within entity_get_info(), and make EntityManager use CacheDecorator instead. I didn't do that for now.
Comment #52
yched CreditAttribution: yched commentedOops, duplicated 'use' statement.
Comment #53
tim.plunkettI really like this approach. I'd RTBC, but I'd like sign-off from one of the other Plugin system gurus.
Comment #54
tim.plunkettEclipseGC pointed out that this could be Drupal\Component
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
Comment #55
yched CreditAttribution: yched commentedFixed #54.
Right. I only took care of the ones added in the new ProcessDecorator.php, not the existing ones that happen to be close to the lines I touch in the affected Manager class files :-)
Comment #56
yched CreditAttribution: yched commentedBump ?
Comment #57
tstoecklerI 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.
Comment #58
yched CreditAttribution: yched commentedYes, you need this so that the "pass by reference" works with call_user_func() (hence the use of call_user_func_array(), btw)
Comment #59
sunI 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.
Comment #60
tstoecklerRe #58: Thanks @yched. I somehow missed the array... You are, of course, correct.
Comment #61
yched CreditAttribution: yched commentedWell, I do disagree with #59, and actually my anseers are in #45 :-). In short, decorators are well-identified algorithmic blocks, much better than inconsistent custom code in each plugin type. Cachedecorator is there if your discovery is costly. The current code *is* problematic performace wise because because processDefinition runs each time you access a definition (and with entityNG, that's a *lot*)
(sorry, on the road again, typing from my phone, staying concise)
Also, I'm open to alternate fixes, but we haven't come to any in 3 months now. This is now hindering progress on entity type info cleanup (entity_get_info() uses its own cache instead of cachedecorator because of this issue.
Comment #62
sunOk, 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.
Comment #63
EclipseGc CreditAttribution: EclipseGc commentedI 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
Comment #64
catchHas 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:
No it doesn't.
Comment #65
yched CreditAttribution: yched commentedRerolled after the recent followup commit in #1763974: Convert entity type info into plugins (added one discovery decorator in EntityManager), and fixes the phpdoc bug pointed in #64.
Can't assemble an interdiff because of the merge conflicts.
re @catch #64:
Not exactly. The processing this patch deals with is currently done already. But it's done outside of the discovery process, and thus outside of caching, and thus gets executed *every single time* you access a plugin definition.
The patch puts this processing into the discovery stack,
- which is where it needs to be for performance reasons (because it then can be below the cache point),
- which is also where it belongs. As pointed in the OP, the discovery not being able to provide "processed" definitions means the stuff that take an injected Discovery (e.g PluginBase) cannot in fact receive a Discovery, but need to be passed the PluginManager, because right now it's the only thing that can hand out processed definitions - which is a wtf.
Those are general worries about discovery performance and are more about the cost of annotations discovery, but really cannot be tacked onto this patch IMO.
The only thing that this patch adds compared to the current situation is one more function call (the decorating ProcessDecorator::getDefinitions()) during cache rebuild. The processing itself is currently already done - only performed much more often. Are we really balancing one additional function call on cache rebuild vs much less processing at runtime ?
So back to NR for neclimdul, then, but bumping to major.
Comment #66
catch@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.
Comment #67
neclimdulI 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.
Comment #68
yched CreditAttribution: yched commentedWell, given that we have no other approach for now, I'll take this as a back to RTBC then.
Comment #69
webchickI tried to understand what a ProcessDecorator was by reading the calling code.
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."
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.
Comment #70
EclipseGc CreditAttribution: EclipseGc commentedok, typically we're discouraging this sort of notation.
in favor of:
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
Comment #71
yched CreditAttribution: yched commentedEr, why would we be discouraging the "not inlined" notation ?
This is an illegible mess which from which you can't tell which arguments apply where, and inevitably triggers "oh no lets not add a new decorator" reactions :
While this provides a clear view on the discovery logic used by this plugin type:
re: process before or after alter : heh, that's a question every system with a "collect info + alter" pattern had to answer separately in D7, and I'm not sure we were consistent. _field_info_collate_types() D7 in fact did "fill-in defaults then alter", but that was not possible with current HEAD where process happens outside the discovery stack. But if process gets put back in the discovery stack, I'm fine with moving it before alter.
Comment #72
effulgentsia CreditAttribution: effulgentsia commentedWould it help with docs and partially alleviate #59 if we rename ProcessDecorator to ManagerDecorator, pass it
$this
instead ofarray($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.
Comment #73
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #74
yched CreditAttribution: yched commentedRegarding performance impact :
From #1811816-30: Benchmark node_page_default vs. node view :
(thats on a page with 10 views blocks)
650 calls to Drupal\views\Plugin\Type\PluginManager::processDefinition(), 5% of the page request
Comment #75
catch@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.
Comment #76
tim.plunkettBecause 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.
Comment #77
tim.plunkettOkay, that went in!
Comment #78
yched CreditAttribution: yched commentedThanks for the reroll, @tim.plukett !
Regarding "process before alter" :
I was a bit hesitant to change the order on existing plugin types (since process currently runs outside the discovery, thus after alter), so I gave the bot a test run in a separate issue earlier today.
Turns out, this works fine for the existent plugin types we have, but breaks hell with EntityManager:
- ProcessDecorator then AlterDecorator on all PluginManagers : 2 fail(s), and 7,248 exception(s)
- ProcessDecorator then AlterDecorator on all except EntityManager: green
Comment #79
tim.plunkettI opened #1848964: EntityManager should process its definitions before altering them for #78.
Rerolled with an explicit @todo.
Comment #80
yched CreditAttribution: yched commentedThis adresses @webchick's concerns in #69, so back to RTBC ?
Comment #81
sunNote 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.)
Comment #82
sunSorry, x-post.
Comment #83
sdboyer CreditAttribution: sdboyer commented+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.
Comment #84
catchCommitted/pushed to 8.x, thanks!
Comment #85
yched CreditAttribution: yched commentedCool !
So, cleanup patch for the
$this->factory = new WhateverFactory($this) / new WhateverFactory($this->discovery);
WTF mentioned in the OP : #1851706: Make all core plugin managers be consistent as to whether $this or $this->discovery is passed to the factoryComment #86
EclipseGc CreditAttribution: EclipseGc commentedI'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:
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
Comment #87
EclipseGc CreditAttribution: EclipseGc commentedoops
Comment #88
EclipseGc CreditAttribution: EclipseGc commentedOK, going to do this in a new issue.
Comment #89
EclipseGc CreditAttribution: EclipseGc commentednew issue: #1853382: Remove ProcessDecorator in favor of a solution that is in line with the original intent.
Comment #90.0
(not verified) CreditAttribution: commentedreword a bit