As we are taking inspiration from the whole Symfony 'Bag' thing, I tihnk we should add an all() method that returns all instantiated plugin instances. This seems like a good idea and as we want a complete API for this class.

CommentFileSizeAuthor
#1 1886894-1.patch1.6 KBdamiankloip
d8.pluginBag-all.patch758 bytesdamiankloip

Comments

damiankloip’s picture

StatusFileSize
new1.6 KB

And a test?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I agree that there might be usecases for that and we shouldn't limit it.

We have proper testing as well, so that's RTBC from my site.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Hmm really? That mostly defeats the point of the class I think given it originally went in so there'd be a proper iterator. If you do all() aren't you going to just foreach() over them again anyway?

At the least this could use a comment to recommend iterating or whatever else on the class itself.

dawehner’s picture

@damian
What is a use-case for all? If you need all you might shouldn't start using it in the first place.

damiankloip’s picture

Status: Needs work » Needs review

I was just thinking that these 'bags' generally have an all() method, also, this is useful for debugging I think, instead of having to iterate over the pluginBag each time.

I don't feel overly strongly about this, so @catch, I leave this up to you. It just seems that this was originally based/inspired by the symfony parameterBags, so we should try to be consistent with those...

damiankloip’s picture

See #1868772: Convert filters to plugins for pretty much a similar usage in the FilterBag class. This does make sense for things like administration pages etc...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

So there is a usecase.

To be honest, I still not really sure whether plugin bags are useful for objects, which need all of the plugin instances all the time (beside the object is not used at all), but yeah if drupal does something then like crazy.

Patch does still apply.

damiankloip’s picture

Looks like #1938654: Export all properties of all views handlers and plugins will probably have a use case for this too, as we will need to iterate over all displays before saving.

webchick’s picture

Assigned: Unassigned » catch

Catch didn't seem to like this before, so moving it back to him for final sign-off.

catch’s picture

Status: Reviewed & tested by the community » Needs review

need to iterate over all displays before saving.

What's wrong with foreaching over the pluginbag for this?

damiankloip’s picture

Status: Needs review » Closed (won't fix)

Nothing, that will work just fine :)