LazyPluginCollection should not implement \Iterator

Yes that fixes the problem but its an underlying problem of the architecture, is there any way to do it properly?

One example to solve that would be to disallow iterating but you can just request all IDs so you have to do the ->get() for your own.

Wow. Can we write tests for this first?

I think the only sane thing to do is to remove iterator support from the lazy plugin collection and instead expose a getAllIds method which allows you to safely travel over all entries.

I'm assuming this will be completely broken.

Right, that breaks the lazy-load. It'll be really sad to lose the Iterator-ability, but I don't know what else to do.

Brr.

I'd honestly get rid of iterators, if possible. Its magic which makes things hard to understand AND its problematic in terms of potential bugs.

Here is a patch which removes the iterator support entirely, let's see see how many failures are left.

Fixing a couple obvious places just to see some test results.

Fixing more failures.

Updating the issue summary.

Fixing filters

I'm worried about the implementation of getInstances(), but let's get this green first.

Ideally calling getInstances() twice in a row wouldn't cause all of those method calls. But I can't figure out a way to avoid that without causing more problems.

We still need tests that prove this is a problem in real code, without manually nesting two loops in the test.

Unless anyone else has ideas about how to make subsequent calls to getInstances() faster, I think this is done code-wise.

It needs a CR draft and the beta evaluation.

Oops, left in some code from a previous approach.

Here is an alternate version of the patch, with \IteratorAggregate instead. This is less explicit, and still has the issue of not iterating lazily, but it's not an API break.

I think #32 makes a lot of sense. I see no downside with it.

still has the issue of not iterating lazily

I think that's ok. How often do we have performance-critical code that starts iterating a plugin collection and exits part-way through? And if we do have that, then we can always have a follow-up to implement an alternate iterator that is lazy. So I think this non-API-changing approach is better than #30 where the lack of laziness becomes baked into the API for no reason and is then harder to change later.

A semantic point in favor of #32: the name *Collection is a strong implication that it can be iterated over.

Finally, my worry is that removing any iteration support will cause foreach ($collection as...) to fail silently. It will do nothing and emit no warning or error.

Finally, my worry is that removing any iteration support will cause foreach ($collection as...) to fail silently. It will do nothing and emit no warning or error.

WTFH

I still hate iterators, given their additional complexlity.

For now though #2431379-32: LazyPluginCollection should not implement \Iterator removes the complexity a bit and fixes the underlying bug.

Nice amount of simplification between #30 and #32. @tim.plunkett++

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9defbd4 and pushed to 8.0.x. Thanks!

This is a nit, but is the IS accurate for what got committed (and also the commit message)? Shouldn't the it be something like:

Proposed Resolution

Change LazyPluginCollection to implement \IteratorAggregate instead of \Iterator. This retain will foreach compatibility over collections, but lose the fine control over iteration (which is not used by core).

Or am I missing something?