Skip to content

Instantly share code, notes, and snippets.

@enygma
Created October 22, 2016 15:19
Show Gist options
  • Save enygma/c3fff1ceb3f6b6643ae5c7b182a81f53 to your computer and use it in GitHub Desktop.
Save enygma/c3fff1ceb3f6b6643ae5c7b182a81f53 to your computer and use it in GitHub Desktop.
Is this a bug with the iterator handling and foreach?
<?php
/**
* In the example below, I make an iterator and assign three values to it: "foo", "bar" and "baz".
* I then manually remove the first one (index 0) and pass the rest into foreach. I expected it
* to just start with index 1 and go on and loop through the rest but there's no output at all.
*
* Is this a bug?
*/
class Items implements \Iterator
{
private $items = [];
private $index = 0;
public function current()
{
return $this->items[$this->index];
}
public function next()
{
$this->index++;
}
public function valid()
{
return isset($this->items[$this->index]);
}
public function key()
{
return $this->index;
}
public function rewind()
{
$this->index = 0;
}
public function add($item)
{
$this->items[] = $item;
}
public function remove($index)
{
unset($this->items[$index]);
}
}
//-------------------
$items = new Items();
$items->add('foo');
$items->add('bar');
$items->add('baz');
$items->remove(0);
// Expected that the loop would start with the first index of "1"
// but it doesn't output anything
foreach($items as $item) {
echo $item."\n";
}
@CNAAOGE
Copy link

CNAAOGE commented Oct 22, 2016

Not at all!

@enygma
Copy link
Author

enygma commented Oct 22, 2016

I ran it through 3v4l.org too: https://3v4l.org/UXF3A

@dzuelke
Copy link

dzuelke commented Oct 22, 2016

valid() will return false so even the first iteration never happens!

@enygma
Copy link
Author

enygma commented Oct 22, 2016

@dzuelke I guess I'll just have to get the values directly then....bleh.

@ciaranmcnulty
Copy link

ciaranmcnulty commented Oct 22, 2016

The bug is in the implementation, which contains two contrary assumptions:

  1. The implementations of rewind() and next() (and the way you initialise $index) assume the underlying array will have sequential numeric keys starting at 0.
  2. The implementation of remove() removes array entries but will lead to non-sequential keys in the underlying array

The combination of these two contradictory models breaks because as @dzuelke points out, valid() will fail at the first unset key. This is not a PHP issue - your object is controlling at all times what $index is set to, and should take responsibility for how it's moved forwards or reset (iterators can of course have whatever keys they want, numeric or otherwise).

You can resolve this by either making rewind() and next() more aware of what keys are actually in the array, or you can make remove() renumber the array when removing entries.

@andytson
Copy link

@enygma how about changing the unset($this->items[$index]); to array_splice($this->items, $index, 1);? This will reset the indices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment