Rename PluginBag to LazyPluginCollection

For what it's worth, PluginBag always terrified me, because suddenly I thought there was some deep Symfony concept I was missing to know how Views plugins worked. LazyPluginCollection is a much more self-explanatory name. :) So +1 for the rename, even though it's kinda a disruptive patch.

There's a few change records that would need updates:
https://drupal.org/list-changes/published/drupal?keywords_description=ba...

And plus we'd want one for this issue itself.

I read the whole patch.

Places that are not simple renames look totally fine. So (for what it is worth) all these changes are good to me.

1. nit.

   +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -296,13 +296,13 @@ public function calculateDependencies() {
   -    // @todo When \Drupal\Core\Config\Entity\EntityWithPluginBagInterface moves
   +    // @todo When \Drupal\Core\Config\Entity\EntityWithPluginCollectionInterface moves
        //   to a trait, switch to class_uses() instead.
   

   other comment blocks are re-wrapped, so wrapping this one also.

   and in 2 other places. see interdiff.

2. just checking. this is fine.

   +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultSingleLazyPluginCollectionTest.php
   @@ -0,0 +1,57 @@
   + * Contains \Drupal\Tests\Core\Plugin\DefaultSingleLazyPluginCollectionTest.
   
   +++ /dev/null
   @@ -1,57 +0,0 @@
   - * Contains \Drupal\Tests\Core\Plugin\DefaultSinglePluginBagTest.
   

   new test? or a rename that didn't match enough?
   ah, I see, a move. ok.

3. Question:

   core/modules/image/lib/Drupal/image/Entity/ImageStyle.php
   84: protected $effectsBag;
   350: if (!$this->effectsBag) {
   351: $this->effectsBag = new ImageEffectCollection(\Drupal::service('plugin.manager.image.effect'), $this->effects);
   352: $this->effectsBag->sort();
   354: return $this->effectsBag;

   These should have been renamed also?

Is this still on the table? We use Bag and Collection in various places, but I really like the "Lazy"-ness here, as it tells you more what this is actually about.

I'll try to do the reroll.

It's a "little bit" too much for me. :)

Rerolled by hand because so much has changed.

reviewing this.

RTBC from my POV, but this is a BC break, so justifying that:

- PluginBag is not only a strange name, but this is _not_ a bag, but a collection, such it is a violation of Symfony standards, which we try to apply to and can be very misleading if people try to use it as a bag.

Therefore the rename is well worth it. It is also easy to do the same in contrib/ modules that might be broken by that change as it is mainly a simple string / replace.

So while the rename is very broad, it is simple to port as the overall API remains the same.

Started looking at this. As I got more and more into it, I wondered if 'beta target' was accurate. This seems like it might not be allowed in at this point.

discussed with xjm. based on where we are in the beta, the benefit here may not outweigh the disruption, and we need some maintainer feedback to clarify that. let's wait for input here, and also maybe a post about beta changes criteria generally.

But did finish looking through it... posting the nits (I would normally have just fixed them). Let's hold off actually fixing this stuff until we get feedback on doing it at all.

I put a note in the issue summary.

1. +++ b/core/lib/Drupal/Core/Action/ActionCollection.php
   @@ -2,17 +2,17 @@
   -class ActionBag extends DefaultSinglePluginBag {
   +class ActionCollection extends DefaultSingleLazyPluginCollection {
   
   +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -13,7 +13,7 @@
   -use Drupal\Core\Condition\ConditionPluginBag;
   +use Drupal\Core\Condition\ConditionPluginCollection;
   

   why is the rename of ConditionPluginBag not using the ..Lazy.. pattern like other rename?

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -43,7 +43,7 @@
   -   * This is needed when the entity utilizes a PluginBag, to dictate where the
   +   * This is needed when the entity utilizes a LazyPluginCollection, to dictate where the
   

   this needs to be wrapped to 80 chars.

   but the previous patch in #4 did this...

   So I think there are some changes from #4 that might have been missed in this one... ?

3. +++ b/core/lib/Drupal/Core/Entity/EntityWithPluginCollectionInterface.php
   @@ -2,27 +2,27 @@
   -   * Returns the plugin bags used by this entity.
   +   * Returns the plugin collectionss used by this entity.
   

   typo in collectionss

4. +++ b/core/modules/editor/src/Entity/Editor.php
   @@ -102,7 +102,7 @@ public function calculateDependencies() {
   -    // @todo use EntityWithPluginBagsInterface so configuration between config
   +    // @todo use EntityWithPluginCollectionInterface so configuration between config
        //   entity and dependency on provider is managed automatically.
   

   this needs 80 char wrap also.

5. +++ b/core/modules/filter/src/Plugin/FilterBase.php
   @@ -44,8 +44,6 @@
       * The weight of this filter compared to others in a filter collection.
       *
   -   * @see FilterBase::$filterBag
   -   *
       * @var int
   
   @@ -60,9 +58,9 @@
   -  protected $bag;
   +  protected $collection;
   

   I wonder why this change in this issue? why not @see FilterBase::$filterCollection ... maybe because there is no filterBag property anyway... so instead of search and replace, just taking out the wrong @see. OK.

   or... it was referring to FilterBase::$bag which is now $collection.

   Noting for now, for later thinking.

6. +++ b/core/modules/system/core.api.php
   @@ -1155,19 +1155,19 @@
   - * A plugin bag is a class that extends \Drupal\Component\Plugin\PluginBag or
   + * A plugin collection is a class that extends \Drupal\Component\Plugin\LazyPluginCollection or
     * one of its subclasses; there are several examples in Drupal Core. If your
   

   needs 80 char wrap again.

7. +++ b/core/modules/views_ui/src/Tests/DisplayFeedTest.php
   @@ -49,7 +49,7 @@ protected function checkFeedViewUi($view_name) {
   -    // check whether a DisplayBag was returned in iterating over all displays.
   +    // check whether a DisplayCollection was returned in iterating over all displays.
   

   80 chars.

Also, xjm reminded me, that git diff --colorwords is helpful in situations like these.

I think the naming currently is a total Drupal WTF, so I think this renaming would be a big DX win

If we want core committer feedback, RTBC is the status to use.

@Core committers - Just some nits left.

Trying with alexpott for now.

Can we get approval / disproval of this issue before fixing them?

This is a tricky one for beta criteria, my personal opinion, not a 'decision' yet - let's see what others think.

1. The name is misleading rather than not ideal - i.e. there's tangible benefit in it expressing what it is, whereas it currently expresses what it's not.

On a hierarchy of renames: Accuracy > Precision > Aesthetics.

2. Not saying we should, but presumably the old class could be left in, unused, subclassing, with the old name for bc?

3. This was RTBC during DrupalCon Amsterdam, which #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? has a section that would allow a little more leeway to those specific patches during the next 3.5 weeks, but that's not been discussed with Dries or webchick yet (was only posted 36 hours ago). Even if we do that, we could still decide the change isn't worth it, but it does mean it's OK to specifically ask for committer feedback here. Leaving assigned to Alex.

I'm okay with going forward with this under the "Amsterdam exception" rules.

So now to fix up the nits that @Fabianx refers to in #14

Also lets have a change record for the issue to help people.

I started the draft change record: https://www.drupal.org/node/2352673

+1 for naming it lazy, because it tells you what happens. Yes, you could argue that this is an internal detail,
but on the other hand, people might realize what it is good for.

There are other classes renamed with suffix "Bag" that may be wrong in the current patch:

• ActionBag to ActionCollection, maybe it should be ActionLazyPluginCollection
• BlockPluginCollection should be BlockLazyPluginCollection?
• Etc ... there are more

Should those be renamed that way?

Fixed a bad wrap on a @todo.

Generated with `git diff -M10% --minimal 8.0.x`

I discovered this through the change record draft (which again appeared in the feed of *published* change records, and therefore is broken again…).

This name is so much better, together with reading the change record I now *finally* fully understand what this "PluginBag" thing is about!

@Wim Leers I went to make a comment on #2337375: Feed showing draft change records
but I could not see this draft record in
https://www.drupal.org/list-changes/drupal
or the rss feed, or the twitter feed for @drupal8changes

any thoughts on #21 ? Should we carry through the lazy wording on more class names?

taking off the needs change record tag since we have a draft and it looked good to @Wim Leers

#25: commented at #2337375-5: Feed showing draft change records.

Reroll.

Regarding #21, I think we should not include Lazy in the names of the classes that extend LazyPluginCollection, except for DefaultLazyPluginCollection DefaultSingleLazyPluginCollection.

ActionBag should probably be renamed to ActionPluginCollection and similarly for the rest.

@cilefen ok. since lazy can be argued to be an implementation detail, that seems ok to me. (and I can't get a strong response from others.)

needs work to do #28

Rerolled because of a comment change in #2235363: Document config dependencies.

For reference (and before I lose this), here are two class hierarchies in question:

LazyPluginCollection
-- DefaultLazyPluginCollection
---- ConditionLazyPluginCollection
---- FilterCollection
---- ImageEffectCollection
---- TipsCollection
---- DisplayCollection
-- DefaultSingleLazyPluginCollection
---- ActionCollection
---- BlockPluginCollection
---- SearchPluginCollection
-- TestLazyPluginCollection

LazyPluginCollectionTestBase
-- ConfigurablePluginCollectionTest
-- DefaultLazyPluginCollectionTest
-- DefaultSingleLazyPluginCollectionTest

I am going to remove Lazy from ConditionLazyPluginCollection, and change to FooPluginCollection for the other leaf classes.

OK, renamed classes via refactor tools in PHPStorm. The new hierarchy is shown in the image.

@mpdonadio: this patch will be smaller and easier to read if you make it with git diff -M10% --minimal

Minimal patch.

I missed two classes uses as strings for test mocks. Fixed those and they pass locally, along with a comment that was too long.

added a before and after hierarchy to the summary.

updated the change record with the rename details.

useful for reviewing:
on both head and with the patch
ag --ignore-case " bag" core/lib/* core/modules/*
ag --ignore-case "bag " core/lib/* core/modules/*
ag --ignore-case "pluginbag" core/lib/* core/modules/*

with the patch applied, useful for reviewing:
git diff --color-words -M10% --minimal 8.0.x

--
added to the summary
EntityWithPluginBagsInterface -> EntityWithPluginCollectionInterface
ConfigEntityBaseWithPluginBags -> ConfigEntityBaseWithPluginCollections
SearchPluginBagTest -> SearchPluginCollectionTest

and updated the change record again.

I read through the whole patch again. Looks ok to me.

Should we make the class names fully qualified in the Change Record?

Committed 22c8258 and pushed to 8.0.x. Thanks!

I think fully qualified classnames will make the CR harder to read.