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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
7.83 KB

Borrowing the term "Bag" from Symfony.

tim.plunkett’s picture

FileSize
7.9 KB
3.98 KB

After some discussion with EclipseGC, he pointed out two things:

  • What I had called "Plugin IDs" were really "Instance IDs", i.e., they refer to a unique instance in case two of the same plugin are used
  • This is decoupled enough to be a Component

Sorry for the poor-mans-interdiff, but files were moved

EclipseGc’s picture

I 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

dawehner’s picture

Just some comments/ideas.

+++ b/core/lib/Drupal/Component/Plugin/PluginBag.phpundefined
@@ -0,0 +1,111 @@
+ * Allows lazy initialization of plugins.

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.

+++ b/core/lib/Drupal/Component/Plugin/PluginBag.phpundefined
@@ -0,0 +1,111 @@
+    return isset($this->pluginInstances[$offset]) || isset($this->instanceIDs[$offset]);

We could simplify this line with just isset($this->instanceIDs[$offset])

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -42,124 +30,47 @@ class DisplayArray implements \ArrayAccess, \Iterator, \Countable {
+  protected function initializePlugin($display_id) {

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?

EclipseGc’s picture

I'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.

tim.plunkett’s picture

I 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

/**
 * Constructs a new Drupal\filter\Plugin\Core\Entity\FilterFormat object.
 */
public function __construct(array $values, $entity_type) {
  parent::__construct($values, $entity_type);

  $this->filterPlugins = new FilterBag($this, drupal_container()->get('plugin.manager.filter'));
}

And in core/modules/filter/lib/Drupal/filter/Plugin/FilterBag.php

class FilterBag extends PluginBag {
  public function __construct(FilterFormat $format, PluginManagerInterface $manager) {
    // @todo.
  }
}

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.

Crell’s picture

Quoth EclipseGc:

This looks like a plugin specific DIC to me to some degree

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?

EclipseGc’s picture

well, 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

tim.plunkett’s picture

FileSize
1.27 KB
8.13 KB

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

damiankloip’s picture

Issue tags: +Needs tests
damiankloip’s picture

This is looking good, but I'm already familiar with the code from the DisplayArray anyway, we'd reviewed/worked on that quite a bit :)

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -7,10 +7,12 @@
+class DisplayArray extends PluginBag {

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

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -42,124 +30,55 @@ class DisplayArray implements \ArrayAccess, \Iterator, \Countable {
+    $this->pluginInstances[$display_id] = drupal_container()->get("plugin.manager.views.display")->createInstance($display['display_plugin']);

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.

tim.plunkett’s picture

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

damiankloip’s picture

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

dawehner’s picture

Well, we redo a lot of this class already, so renaming seems fine in this issue.

damiankloip’s picture

Yep, I think that was the plan :) then we have the follow up for the injection.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
FileSize
11.4 KB

Here's it with the rename.

dawehner’s picture

FileSize
11.45 KB
520 bytes

Does this description of the class helps?

dawehner’s picture

FileSize
15.71 KB
4.41 KB

Here is a generic test

The last submitted patch, drupal-1869566-18.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

#18: drupal-1869566-18.patch queued for re-testing.

damiankloip’s picture

Nitpicks:

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginBagTest.phpundefined
@@ -0,0 +1,63 @@
+ * Tests the generic plugin bag.

Should this just be the same as the docblock tect for the testPluginBag method docblock?

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginBagTest.phpundefined
@@ -0,0 +1,63 @@
+    // A non existing instance_id shouldn't exists on the bag.

exist

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginBagTest.phpundefined
@@ -0,0 +1,63 @@
+    // Check that iterating over the plugins work.

Test that ... ?

From IRC, maybe we should also test that the array is empty to start with.

dawehner’s picture

Issue tags: -Needs tests
FileSize
15.72 KB
1.53 KB

Update.

yched’s picture

Ooh, interesting. I guess EntityDisplay could use it at some point - in a way, it can be seen as a collection of field formatters.

Crell’s picture

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

tim.plunkett’s picture

Uh, what? No one is saying that. The PluginBag constructor is left out, you can inject whatever you like.

yched’s picture

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

Crell’s picture

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

dawehner’s picture

(And the fact that ArrayAccess is rather slow, but that's a separate matter.)

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

Crell’s picture

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

catch’s picture

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

tim.plunkett’s picture

FileSize
2.97 KB
13.05 KB

Added the methods to be used directly, and documented that the ArrayAccess parts are going away.

tim.plunkett’s picture

FileSize
17.5 KB
dawehner’s picture

+++ b/core/lib/Drupal/Component/Plugin/PluginBag.phpundefined
@@ -0,0 +1,176 @@
+  public function remove($instance_id) {
+    unset($this->pluginInstances[$instance_id]);

Missing docblock.

+++ b/core/lib/Drupal/Component/Plugin/PluginBag.phpundefined
@@ -0,0 +1,176 @@
+   * This is deprecated, use \Drupal\Component\Plugin\PluginBag::has().

Just wondering why drupal doesn't use @deprecated

tim.plunkett’s picture

FileSize
599 bytes
17.58 KB

Thanks, 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.

dawehner’s picture

FileSize
19.16 KB
3.2 KB

Extended the test coverage to test the real methods as well.
As a follow up we might should add an interface for a plugin bag?

Crell’s picture

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

tim.plunkett’s picture

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

damiankloip’s picture

I think this is looking pretty awesome, just one gripe:

+++ b/core/lib/Drupal/Component/Plugin/PluginBag.phpundefined
@@ -0,0 +1,179 @@
+  public function has($instance_id) {
+    return isset($this->pluginInstances[$instance_id]) || isset($this->instanceIDs[$instance_id]);
+  }

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?

dawehner’s picture

This was implemented in that way, to be sure to not add a temporary performance regression, as it would require yet another function call.

dawehner’s picture

Once #1876942: Use direct methods instead of arrayAccess for display handlers is in, we can already remove the deprecated code.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Great, sounds good. The fact that this is temporary is cool with me :)

catch’s picture

Title: Allow any collection of plugins to be lazily instantiated » Change notice: Allow any collection of plugins to be lazily instantiated
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

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

yched’s picture

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

dawehner’s picture

aspilicious’s picture

Status: Active » Needs review
Berdir’s picture

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

dawehner’s picture

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

xjm’s picture

Issue tags: +Needs change record
dawehner’s picture

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

tim.plunkett’s picture

Title: Change notice: Allow any collection of plugins to be lazily instantiated » Allow any collection of plugins to be lazily instantiated
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Needs change record

Looks good to me.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.