Allow an empty EachPromise to be resolved by running the queue #178
Conversation
| @@ -80,9 +80,9 @@ public function promise(): PromiseInterface | |||
| } | |||
|
|
|||
| try { | |||
| $this->iterable->rewind(); | |||
There was a problem hiding this comment.
Flipped the ordering here so that createPromise() can check whether the iterable is empty.
| // In the case of empty, create a promise that will immediately become resolved via wait() or | ||
| // via Utils::queue()->run() (https://github.com/guzzle/promises/issues/176). We could simply | ||
| // create a fulfilled promise here, but that would be an observable behavioral change (see | ||
| // EachPromiseTest::testResolvesInCaseOfAnEmptyListAndInvokesFulfilled) |
There was a problem hiding this comment.
Curious if you agree with the approach taken here vs. changing testResolvesInCaseOfAnEmptyListAndInvokesFulfilled to call Utils::queue()->run() before checking that fulfilled is called.
Originally I had thought it would make sense for an empty EachPromise to resolve synchronously without even needing to pump the queue.
However, this solution works just as well for me in terms of making EachPromise "fiber-friendly" and preserves the existing behavior more exactly.
| @@ -430,4 +441,23 @@ public function testIsWaitableWhenLimited(): void | |||
| $this->assertSame(['a', 'c', 'b', 'd'], $called); | |||
| $this->assertTrue(P\Is::fulfilled($p)); | |||
| } | |||
|
|
|||
| public function testRewindsExhaustedIterator(): void | |||
There was a problem hiding this comment.
This just tests existing behavior that my first pass at the change broke; figured it was worth adding to document.
|
@GrahamCampbell thoughts? Do you think this change could be accepted? |
|
@GrahamCampbell any next steps for this? Looks like all checks are now passing |
GrahamCampbell
changed the title
|
Thanks for working on this. I'm going to target this at 2.1.x, rather than 2.0.x. |
|
Ooops, this introduces a bug caught by an upstream test suite. Fix: #179. |
|
Actually, I need to revert this. The promise resolving with null is a major issue. |
Closes #176.