A single View is made up of displays.
The ConfigEntity contains an array of plugin IDs, which used to be looped through and instantiated.
In #1817582: Lazy load display plugins, a class was added to help lazily instantiate these plugins.
This will be useful in more than just views.
In #1868772: Convert filters to plugins, Text formats (also a ConfigEntity) will be referencing multiple filters (also plugins), and image style/image effects will likely follow the same pattern.
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff.txt | 3.2 KB | dawehner |
#35 | drupal-1869566-35.patch | 19.16 KB | dawehner |
#34 | pluginbag-1869566-34.patch | 17.58 KB | tim.plunkett |
#34 | interdiff.txt | 599 bytes | tim.plunkett |
#32 | pluginbag-1869566-32.patch | 17.5 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettBorrowing the term "Bag" from Symfony.
Comment #2
tim.plunkettAfter some discussion with EclipseGC, he pointed out two things:
Sorry for the poor-mans-interdiff, but files were moved
Comment #3
EclipseGc CreditAttribution: EclipseGc commentedI think I'm pretty ++ to this.
This looks like a plugin specific DIC to me to some degree, and I like it, it's simple, straight forward, and already was introduced by views into core, so abstracting it out makes good sense to me. As to whether I'll ever use it, I dunno. This almost feels like a compliment to the existing manager stuff. $this->bag = new PluginBag(); and then a corresponding Factory decorator that shoves instances into the PluginBag? I dunno, maybe an interesting follow up to this.
Eclipse
Comment #4
dawehnerJust some comments/ideas.
It seems to be that we should provide a better comment here, which actually describes that this PluginBag can be used exactly on THIS particular use-case.
We could simplify this line with just isset($this->instanceIDs[$offset])
I'm wondering whether we could put the plugin manager in the constructor as dependency? Based on that we theoretically could also provide a default implementation?
Comment #5
EclipseGc CreditAttribution: EclipseGc commentedI'd prefer to make the manager wrap this (when it's used) per my suggestion of a factory decorator that hits this first to return instances.
Comment #6
tim.plunkettI don't know why we'd want to make the Manager even know about it.
So far this is just used by Views, and will shortly be used by #1868772: Convert filters to plugins.
I left the constructor out, but in the filters patch it'll be:
in core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.php
And in core/modules/filter/lib/Drupal/filter/Plugin/FilterBag.php
In that regard, I didn't want to add a constructor so that I can freely type hint.
The Views implementation doesn't get the manager injected yet, but that could be a follow-up. But it gets to type hint ViewExecutable.
Comment #7
Crell CreditAttribution: Crell commentedQuoth EclipseGc:
It sort of does, in that it does lazy loading by ID, but doesn't look like it provides any DI. Which therefore begs the question, why aren't we just using the DIC? And why aren't we doing any actual DI here?
Comment #8
EclipseGc CreditAttribution: EclipseGc commentedwell, there shouldn't really be any DI here, which is basically the answer and why I didn't press tim on exactly those questions. Because there's no DI, there's not really a good reason to use the full DIC. If there's this light weight plugin bag thingy, I'm actually ok with it.
Eclipse
Comment #9
tim.plunkettWe don't need any DI. We have the plugin ID and it's configuration in CMI, and we're using this to help instantiate it when appropriate.
Added a clear() method, which I need for filters, and adapt the views class for it as well.
Comment #10
damiankloip CreditAttribution: damiankloip commentedComment #11
damiankloip CreditAttribution: damiankloip commentedThis is looking good, but I'm already familiar with the code from the DisplayArray anyway, we'd reviewed/worked on that quite a bit :)
Not sure if this is weird or not? Maybe we should just call it PluginArray instead of PluginBag. Although I'm not opposed to how symfony use the 'Bag'.
I know this is how we are already doing it atm, seems like we should pass the manager into the constructor. Whether it's worth it or not, from a performance point of view, I don't know.
Comment #12
tim.plunkettBoth of those are things I wanted to fix, but didn't to keep the scope small.
1) I've grown fond of Bag already :) I think we should rename the View class, DisplayBag?
2) Yes, and that's what I do in the filter patch.
Should those be follow-ups, or should I just fix it?
Comment #13
damiankloip CreditAttribution: damiankloip commented1. Fair enough, I don't really mind :) I like bag too, I used it just the other day for a work project actually! I think consistency is the most important thing. Is bag suggestive enough of what it's used for? symfony 'bags' don't lazy load things, but I don't think that matters.
2. I think a follow up is good for this one, so I created it: #1870410: Inject display plugin manager into DisplayBag class. I've postponed it for now, as we might as well wait to see what happens here.
Comment #14
dawehnerWell, we redo a lot of this class already, so renaming seems fine in this issue.
Comment #15
damiankloip CreditAttribution: damiankloip commentedYep, I think that was the plan :) then we have the follow up for the injection.
Comment #16
tim.plunkettHere's it with the rename.
Comment #17
dawehnerDoes this description of the class helps?
Comment #18
dawehnerHere is a generic test
Comment #20
tim.plunkett#18: drupal-1869566-18.patch queued for re-testing.
Comment #21
damiankloip CreditAttribution: damiankloip commentedNitpicks:
Should this just be the same as the docblock tect for the testPluginBag method docblock?
exist
Test that ... ?
From IRC, maybe we should also test that the array is empty to start with.
Comment #22
dawehnerUpdate.
Comment #23
yched CreditAttribution: yched commentedOoh, interesting. I guess EntityDisplay could use it at some point - in a way, it can be seen as a collection of field formatters.
Comment #24
Crell CreditAttribution: Crell commentedSo we're saying nothing should use this class if it has any dependencies on external services? That means nothing that uses the database may use this? Nothing that itself has a cache may use this? Nothing that needs the language system (which should get turned into a service, per #1862202: Objectify the language system) may use this?
That does not sit well with me.
Comment #25
tim.plunkettUh, what? No one is saying that. The PluginBag constructor is left out, you can inject whatever you like.
Comment #26
yched CreditAttribution: yched commented@Crell: I don't understand #24 either. If I get things right, "not in the DIC" above means "the instanciated plugins are not put in the DIC", not "they can't read from the DIC".
The plugins created here are "local" to a certain object that holds their definitions (views displays in a view, filters in an input format, effects in an image style...). The pluginBag introduced here is remotely similar to a DIC in the sense that it holds references to lazy-created objects, but those objects have no business whatsoever being pushed out of their context into the "global" main request container.
(note that the "lazy creation" aspect is going to be useless in the case of text filters or image effects, where, unlike displays in a view, the parent object will need to instanciate all the listed plugins to get its job done anyway, but reusing the mechanism to store and persist a series of plugins makes sense and has no overhead)
Whether plugins are going to be able to read from the DIC (or receive injected services) is not related to this patch AFAICT, there's #1863816: Allow plugins to have services injected for that.
Comment #27
Crell CreditAttribution: Crell commentedThat was my read of the replies to my earlier comment. A common lazy-load structure is fine; my concern is it making #1863816: Allow plugins to have services injected more difficult. (And the fact that ArrayAccess is rather slow, but that's a separate matter.)
Comment #28
dawehnerSo what we should do is also provide get()/set() and slowly convert the usages if wanted.
In general it wouldn't make it harder to commit this and then figure out #1863816: Allow plugins to have services injected as there is already code in core which uses the new PluginBag pattern but no other plugins then ViewsDisplays uses it.
Comment #29
Crell CreditAttribution: Crell commentedHm. I'll take your word for it. Generalizing tools out of Views to be usable Drupal-wide is a good goal in and of itself.
Comment #30
catchArrayAccess might not be as fast as non-magic function calls, but it's infinitely faster than loading the fully instantiated plugin into an array. Adding get/set and slowly converting works for me though - there's not really a benefit to array access except for bc here.
Comment #31
tim.plunkettAdded the methods to be used directly, and documented that the ArrayAccess parts are going away.
Comment #32
tim.plunkettWhoops, I think this needed a reroll for #1850792: Make init() method consistent across all views plugins
Comment #33
dawehnerMissing docblock.
Just wondering why drupal doesn't use @deprecated
Comment #34
tim.plunkettThanks, added the @param
I'm not sure about @deprecated, I initially used that but realized we don't anywhere else, so I just went for the sentence form.
Comment #35
dawehnerExtended the test coverage to test the real methods as well.
As a follow up we might should add an interface for a plugin bag?
Comment #36
Crell CreditAttribution: Crell commentedThe only reason we don't use deprecated is squeamishness about telling people "this is going away" when we haven't actually removed it yet and can't guarantee that it is going away. At least that happened multiple times early in WSCCI. Frankly we should be using it a lot more for things that are "discouraged", where we know what the better approach is going to be.
IMO, let's use it here. It's a perfectly good use case.
Comment #37
tim.plunkettWell I don't think this issue is the right place to introduce a new documentation pattern, let's do that over here: #1876842: [policy, no patch] Use @deprecated to indicate a deprecated method or function
RTBC?
Comment #38
damiankloip CreditAttribution: damiankloip commentedI think this is looking pretty awesome, just one gripe:
This is pretty identical to the offsetExists method. I know that is needed to implement ArrayAccess but it might be better if we don't have the same logic in two places?
Comment #39
dawehnerThis was implemented in that way, to be sure to not add a temporary performance regression, as it would require yet another function call.
Comment #40
dawehnerOnce #1876942: Use direct methods instead of arrayAccess for display handlers is in, we can already remove the deprecated code.
Comment #41
damiankloip CreditAttribution: damiankloip commentedGreat, sounds good. The fact that this is temporary is cool with me :)
Comment #42
catchThis is great. Committed/pushed to 8.x. Are there places other than Views we could use this already?
This could probably use some kind of change notice, although not sure where the overall docs for the plugin system live or are going to live atm.
Comment #43
yched CreditAttribution: yched commentedEntityDisplay will probably leverage that too for field formatters. Probably as part of the refactor in #1875974: Abstract 'component type' specific code out of EntityDisplay though.
Comment #44
dawehnerhttp://drupal.org/node/1878416
Comment #45
aspilicious CreditAttribution: aspilicious commentedComment #46
BerdirMade some small corrections to the change notice. Looks good to me, although I'm not sure if it makes sense as a change notice. Maybe instead add it as a sub-page to http://drupal.org/node/1637614?
Comment #47
dawehnerYeah I agree that it's not strictly a change from d7 to d8.
Thank you for fixing the problems on there.
At what point in the D8 release cycle will we consider things like that a change, not a new thing?
A good point could be maybe the first beta?
Comment #48
xjmComment #49
dawehner@Berdir
To be honest, I'm not sure. Most people who will port a module might look at both locations, so maybe put a handbook page in there
and then link to it in the change notice?
Comment #50
tim.plunkettLooks good to me.