PHPLIB-81: Implement CachingIterator #424
Conversation
jmikola
force-pushed
the
phplib-81
branch
2 times, most recently
from
56bb401 to
ce7879a
Compare
| * effectively rewinds the Traversable and the wrapping Generator, which | ||
| * will execute up to its first yield statement. Additionally, this mimics | ||
| * behavior of the SPL iterators and allows users to omit an explicit call | ||
| * to rewind() before using the other methods. |
There was a problem hiding this comment.
@alcaeus: Strictly speaking, Iterators must call rewind() before iterating; however, most SPL iterators relax that restriction and rewind during construction. I kept that behavior, while adding $iteratorAdvanced to ensure that the initial call to rewind() does not exhaust all results. That may be why @bjori originally assumed you had implemented a BufferedIterator. Based on testPartialIterationDoesNotExhaust(), the original implementation was actually buffering up front!
| */ | ||
| class CachingIterator implements Countable, Iterator | ||
| { | ||
| private $items = []; |
There was a problem hiding this comment.
This helps if we count an empty Traversable, as the early return in storeCurrentItem() would have prevented this from ever being initialized from null to an array type.
| { | ||
| foreach ($traversable as $key => $value) { | ||
| yield $key => $value; | ||
| $this->iteratorAdvanced = true; |
There was a problem hiding this comment.
My understanding (and observation) is that the first call to wrapTraversable() from our constructor is going to immediately execute this function up to (and including?) the yield statement. The $iteratorAdvanced assignment will take place once we re-enter, immediately before foreach advances the Traversable (e.g. Iterator::next() if it happens to be an Iterator).
There was a problem hiding this comment.
Correct - this could be tested by adding an assertion on the iteratorAdvanced property on the testConstructorRewinds test:
$this->assertAttributeSame(false, 'iteratorAdvanced', $iterator);
There was a problem hiding this comment.
I didn't realize those assertions existed :)
I think I'll do without them here, since we can get by testing with just the public API. Will keep it in mind for future use, though!
| */ | ||
| private function wrapTraversable(Traversable $traversable) | ||
| { | ||
| foreach ($traversable as $key => $value) { |
There was a problem hiding this comment.
Just to recap, each foreach starts with invoking the internal iterator's rewind handler, followed by a validity check. If valid, keys and values are fetched. On subsequent iterations, a call to the next handler takes place instead of rewind.
| { | ||
| foreach ($items as $item) { | ||
| if ($item instanceof Exception) { | ||
| throw $item; |
There was a problem hiding this comment.
The stack trace of the exception only reports when it was constructed (it's not generated/updated when the exception is thrown). I was surprised to learn that, but it's not too important as we only ever assert that nothing is thrown (testing that initial rewind no longer exhausts the internal traversable).
There was a problem hiding this comment.
I believe I tried fixing that in Xdebug, but adding the previous exception to a special property.
| { | ||
| foreach ($traversable as $key => $value) { | ||
| yield $key => $value; | ||
| $this->iteratorAdvanced = true; |
There was a problem hiding this comment.
Correct - this could be tested by adding an assertion on the iteratorAdvanced property on the testConstructorRewinds test:
$this->assertAttributeSame(false, 'iteratorAdvanced', $iterator);
| use Countable; | ||
| use Generator; | ||
| use Iterator; | ||
| use Traversable; |
There was a problem hiding this comment.
Why do this if if you're not aliasing them, or plucking them out of another namespace? And if you want the root namespace, shouldn't it be use \Countable ?
There was a problem hiding this comment.
use statements use fully qualified names, so the leading backslash isn't necessary. Out of habit, I list all used classes at the top of the file, rather than making a special case of \Countable for classes in the global namespace.
| { | ||
| foreach ($items as $item) { | ||
| if ($item instanceof Exception) { | ||
| throw $item; |
There was a problem hiding this comment.
I believe I tried fixing that in Xdebug, but adding the previous exception to a special property.
Ensure that a call to CachingIterator::rewind() no longer exhausts results up front, which would impose a BufferedIterator design. Initializing the cache to an empty array also ensures that we can count an empty result set.
This will allow the returned CollectionInfoIterator and IndexInfoIterator classes to be rewound and iterated multiple times.
https://jira.mongodb.org/browse/PHPLIB-81
Supersedes #327