Fix support for IteratorAggregate and EmptyIterator objects in loops#4141
Conversation
| } while ($seq instanceof \IteratorAggregate); | ||
| $this->seq = $seq; | ||
| } elseif (is_iterable($seq)) { | ||
| $this->seq = $seq; |
There was a problem hiding this comment.
Here $seq is more than probably Iterator or IteratorAggregate but that's almost "internal"...
Internal (built-in) classes that implement this interface can be used in a foreach construct and do not need to implement IteratorAggregate or Iterator.
Not sure if this could happen IRL .. but maybe a elseif ($seq instanceof \Iterator) would be more precise here ?
| do { | ||
| $seq = $seq->getIterator(); | ||
| } while ($seq instanceof \IteratorAggregate); | ||
| $this->seq = $seq; |
There was a problem hiding this comment.
PHPStorm was not happy so i looked a bit and...
IteratorAggregate::getIterator() does not return an Iterator but only a Traversable ...
so after the loop here $this->seq could totally be a simple array
Would something like this work 'out of the box' ?
if (is_array($seq))
// ArrayIterator($seq)
elseif ($seq instanceof Traversable)
// IteratorIterator ($seq)
else
// EmptyIterator
There was a problem hiding this comment.
The best would be to create a PR with examples of something that fails and the fix for those tests.
There was a problem hiding this comment.
Well i spend time and clearly IRL that won't ever happen.
The last possibilities would be those classes PHP say can be used in "foreach" while not implementing "Iterator" directly, but i cannot find an example.
Sorry for the waste of time here.
| $this->seq = is_iterable($seq) ? (is_array($seq) ? new \ArrayIterator($seq) : $seq) : new \ArrayIterator([]); | ||
| if (is_array($seq)) { | ||
| $this->seq = new \ArrayIterator($seq); | ||
| } elseif ($seq instanceof \IteratorAggregate) { |
There was a problem hiding this comment.
There is a much simpler way: using IteratorIterator
| $this->previous = ['valid' => false, 'key' => null, 'value' => null]; | ||
| $this->current = ['valid' => $this->seq->valid(), 'key' => $this->seq->key(), 'value' => $this->seq->current()]; | ||
| if ($this->seq->valid()) { | ||
| $this->previous = ['valid' => false, 'key' => null, 'value' => null]; |
There was a problem hiding this comment.
shouldn't we always assign previous ?
There was a problem hiding this comment.
That was something i wonder, but there is only one situation getPrevious() can be called without beeing set, it's if someone call "loop.previous" in the else part of its for ... else .... endfor.
Because the node inside the for block can only be called if there is something in the loop (so when this->seq->isValid() is true)
There was a problem hiding this comment.
and in such case, we still want it to be able to return null as value, not a PHP error
No description provided.