[9.x] Fix takeUntilTimeout method of LazyCollection
#41354
Conversation
| return new static(function () use ($timeout) { | ||
| $iterator = $this->getIterator(); | ||
|
|
||
| if (! $iterator->valid() || $this->now() > $timeout) { |
There was a problem hiding this comment.
This is actually still pulling that extra element. The iterator's valid method always pulls the next item 😞
There was a problem hiding this comment.
Ah sorry, I did not know that 😞
| yield $iterator->key() => $iterator->current(); | ||
|
|
||
| while ($iterator->valid() && $this->now() < $timeout) { | ||
| $iterator->next(); | ||
|
|
||
| yield $iterator->key() => $iterator->current(); | ||
| } |
There was a problem hiding this comment.
This can be simplified into a single foreach block.
| public function testTakeUntilTimeoutWithPastTimeout() | ||
| { | ||
| $timeout = Carbon::now()->subMinute(); | ||
|
|
||
| $mock = m::mock(LazyCollection::class.'[now]'); | ||
|
|
||
| $results = $mock | ||
| ->times(10) | ||
| ->tap(function ($collection) use ($mock, $timeout) { | ||
| tap($collection) | ||
| ->mockery_init($mock->mockery_getContainer()) | ||
| ->shouldAllowMockingProtectedMethods() | ||
| ->shouldReceive('now') | ||
| ->times(1) | ||
| ->andReturn( | ||
| (clone $timeout)->add(1, 'minute')->getTimestamp(), | ||
| ); | ||
| }) | ||
| ->takeUntilTimeout($timeout) | ||
| ->all(); | ||
|
|
||
| $this->assertSame([], $results); | ||
|
|
||
| m::close(); | ||
| } |
There was a problem hiding this comment.
This test doesn't actually test that it's lazy. It simply tests the returned result. The actual values pulled out of the original generator are not inspected here.
Look at the SupportLazyCollectionIsLazyTest class for how to ensure it is actually lazy.
In fact, creating a laziness test for the code in this PR would fail, since it still always pulls an extra unnecessary item from the original generator.
|
|
||
| yield $iterator->key() => $iterator->current(); | ||
|
|
||
| while ($iterator->valid() && $this->now() < $timeout) { |
There was a problem hiding this comment.
Same here. It is always pulling an extra item.
3 participants
Bug was discovered in #41275 by @JosephSilber.
The current implementation of
takeUntilTimeoutmethod inLazyCollectionalways fetches the next item before checking if the timeout has been exceeded. This new implementation only fetches the next item when we are sure that the timeout has not been exceeded.