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.

Files: 
CommentFileSizeAuthor
#35 interdiff.txt3.2 KBdawehner
#35 drupal-1869566-35.patch19.16 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 50,081 pass(es).
[ View ]
#34 pluginbag-1869566-34.patch17.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,061 pass(es).
[ View ]
#34 interdiff.txt599 bytestim.plunkett
#32 pluginbag-1869566-32.patch17.5 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,003 pass(es).
[ View ]
#31 pluginbag-1869566-31.patch13.05 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,620 pass(es), 587 fail(s), and 2,438 exception(s).
[ View ]
#31 interdiff.txt2.97 KBtim.plunkett
#22 interdiff.txt1.53 KBdawehner
#22 drupal-1869566-21.patch15.72 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,338 pass(es).
[ View ]
#18 interdiff.txt4.41 KBdawehner
#18 drupal-1869566-18.patch15.71 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,316 pass(es).
[ View ]
#17 interdiff.txt520 bytesdawehner
#17 drupal-1869566-17.patch11.45 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,218 pass(es).
[ View ]
#16 plugins-1869566-16.patch11.4 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,365 pass(es).
[ View ]
#9 plugins-1869566-8.patch8.13 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,381 pass(es).
[ View ]
#9 interdiff.txt1.27 KBtim.plunkett
#2 interdiff.txt3.98 KBtim.plunkett
#2 plugins-1869566-2.patch7.9 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,388 pass(es).
[ View ]
#1 plugins-1869566-1.patch7.83 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,381 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new7.83 KB
PASSED: [[SimpleTest]]: [MySQL] 49,381 pass(es).
[ View ]

Borrowing the term "Bag" from Symfony.

StatusFileSize
new7.9 KB
PASSED: [[SimpleTest]]: [MySQL] 49,388 pass(es).
[ View ]
new3.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

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

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?

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.

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.

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?

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

StatusFileSize
new1.27 KB
new8.13 KB
PASSED: [[SimpleTest]]: [MySQL] 49,381 pass(es).
[ View ]

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.

Issue tags:+Needs tests

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.

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?

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.

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

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

Assigned:tim.plunkett» Unassigned
StatusFileSize
new11.4 KB
PASSED: [[SimpleTest]]: [MySQL] 49,365 pass(es).
[ View ]

Here's it with the rename.

StatusFileSize
new11.45 KB
PASSED: [[SimpleTest]]: [MySQL] 49,218 pass(es).
[ View ]
new520 bytes

Does this description of the class helps?

StatusFileSize
new15.71 KB
PASSED: [[SimpleTest]]: [MySQL] 49,316 pass(es).
[ View ]
new4.41 KB

Here is a generic test

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

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

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

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.

Issue tags:-Needs tests
StatusFileSize
new15.72 KB
PASSED: [[SimpleTest]]: [MySQL] 49,338 pass(es).
[ View ]
new1.53 KB

Update.

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

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.

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

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

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

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

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.

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.

StatusFileSize
new2.97 KB
new13.05 KB
FAILED: [[SimpleTest]]: [MySQL] 48,620 pass(es), 587 fail(s), and 2,438 exception(s).
[ View ]

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

StatusFileSize
new17.5 KB
PASSED: [[SimpleTest]]: [MySQL] 50,003 pass(es).
[ View ]

+++ 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

StatusFileSize
new599 bytes
new17.58 KB
PASSED: [[SimpleTest]]: [MySQL] 50,061 pass(es).
[ View ]

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.

StatusFileSize
new19.16 KB
PASSED: [[SimpleTest]]: [MySQL] 50,081 pass(es).
[ View ]
new3.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?

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.

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?

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?

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

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

Status:Needs review» Reviewed & tested by the community

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

Title:Allow any collection of plugins to be lazily instantiatedChange 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.

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.

Status:Active» Needs review

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?

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?

Issue tags:+Needs change record

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

Title:Change notice: Allow any collection of plugins to be lazily instantiatedAllow 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.