Reduce duplication, tighten exception handling, and correct concurrency defaults#5
Conversation
Both ConcurrencyHandler and ReduceHandler contained an identical private getIterator() method. Moved it to src/Concerns/NormalizesIterator.php and applied the trait in both handlers, keeping the behavior identical. ConcurrencyHandler also gains a DEFAULT_CONCURRENCY constant (10) to replace the PHP_INT_MAX defaults in map(), mapSettled(), forEach(), and forEachSettled(). The docblocks for those methods already documented 10 as the safe limit, so this aligns the implementation with the stated contract.
SettledResult defined STATUS_FULFILLED, STATUS_REJECTED, and STATUS_CANCELLED as private string constants whose values were identical to the PromiseState enum. Replaced all usages with PromiseState::FULFILLED->value etc., keeping a single authoritative definition and eliminating the drift risk.
wait(): non-Throwable rejection reasons now produce PromiseRejectionException instead of a plain Exception, preserving the type already established in the exceptions namespace. cancel(): cache count(\) rather than evaluating it twice in the if/elseif chain. cancelChildren(): collect all exceptions thrown during child cancellation and surface them as AggregateErrorException when there are multiple, matching the pattern already used in cancel() itself. Previously only the first exception was surfaced and the rest were silently dropped. finally(): skip creating an intermediate Promise when the onFinally callback returns a non-Promise value, which is the common void case. The fulfill branch returns the value directly and the reject branch re-throws immediately, avoiding two object allocations per call.
PromiseWaitRejectionTest: wait() wraps non-Throwable reasons in PromiseRejectionException and passes Throwable instances through unchanged. PromiseFinallyAndCancelTest: finally() propagates the original resolved value and rejection reason when the callback returns void, chains a returned Promise before continuing, and rejects if that returned Promise rejects. cancel() with multiple throwing handlers surfaces an AggregateErrorException. ConcurrencyHandlerDefaultsTest: DEFAULT_CONCURRENCY constant is 10, map() respects it when no concurrency argument is given, null coalesces to it, and explicit overrides still work. SettledResultTest: status string values match PromiseState enum values for all three states and jsonSerialize() output is correct.
…havior map(), mapSettled(), filter(), forEach(), and forEachSettled() now default to 10 concurrent items rather than unlimited. Updated the section descriptions and the concurrency utility summary table to reflect this. Added a wait() example showing that non-Throwable rejection reasons are wrapped in PromiseRejectionException, so callers always receive a Throwable regardless of what the promise was rejected with.
There was a problem hiding this comment.
Thanks for the PR, the trait extraction and the SettledResult fix are clean. A couple of things worth discussing before merging:
Namespace: Concerns → Traits
The trait lives at Hibla\Promise\Concerns\NormalizesIterator. While Concerns is a recognised Laravel convention and other framework convention, this library follows a neutral namespace convention like Interfaces for interfaces, Traits for traits which is also consistent with how other Hibla packages organise their trait files. Renaming to Hibla\Promise\Traits\NormalizesIterator would keep things consistent across the Hibla ecosystem.
The concurrency default change is a behaviour-breaking decision, not a cleanup
This is the bigger concern. The PR changes PHP_INT_MAX to DEFAULT_CONCURRENCY = 10 for map(), mapSettled(), filter(), forEach(), and forEachSettled(). This is presented as aligning the implementation with the docblock, but it actually reverses the original design intent of these methods.
These functional combinators were deliberately unlimited by default to mirror PHP's built-in array_map(), array_filter(), and array_reduce() and all of which process every item without a concurrency cap. The PHP_INT_MAX was intentional, not an oversight. The docblock was wrong, not the code.
Changing the default from unlimited to 10 is a silent breaking change for anyone currently relying on these methods processing all items concurrently. Existing code that passes 25 items to Promise::map() expecting all 25 to run in parallel will now silently process them in batches of 10 with no warning.
The correct fix is to update the documentation to match the implementation and properly clarify the behavior, not the other way around. The README change in this PR actually does this correctly in some places ("Defaults to 10 concurrent items"), but the code change underneath it reverses a deliberate decision and please do not forget to satisfy max PHPstan check.
…ention - Rename trait namespace from Concerns to Traits per Hibla ecosystem convention - Revert finally() to the original intermediate Promise pattern; the owner clarified this is by design to handle both void and Promise returns uniformly - Restore PHP_INT_MAX as the concurrency default for filter(), confirming that all collection methods (map, mapSettled, filter, forEach, forEachSettled) are unlimited by default, mirroring array_map / array_filter / array_walk - Fix docblock for forEachSettled() and filter() to document unlimited default - Fix docblock for batch() and batchSettled() to document null concurrency defaulting to batchSize within each batch - Remove DEFAULT_CONCURRENCY constant from ConcurrencyHandler; it was the docblock that was wrong, not the original code - Update tests: remove DEFAULT_CONCURRENCY assertions, rewrite as functional correctness tests; rename finally describe block from optimization to behavior - Revert README concurrency descriptions and summary table back to Unlimited - Fix broken getReason() call in README wait() example to getMessage() - Add batch/batchSettled concurrency note to README
|
@rcalicdan all feedback from the review has been addressed in the latest commit (bfcdd49). Changes made in response to review comments:
Let me know if anything else needs attention. |
|
@jeffreybulanadi Hello, before merging this pull request make sure it pass all CI checks, thanks... |
|
Both missing method signatures in ConcurrencyHandler have been restored (map() and forEachSettled()). The CI runs for the two most recent commits are waiting on maintainer approval to execute. Could you approve the workflow run when you get a chance? Happy to address any other feedback as well. |
|
Done.. |
|
Pushed a fix for the PHPStan errors. Two changes: NormalizesIterator::getIterator() is now generic so PHPStan can track the element type through the iterator, and @var annotations were added on the batchTasks arrays in batch() and batchSettled() to resolve the callable type mismatch at the concurrent/concurrentSettled call sites. Could you approve the workflow run when you get a chance? |
|
@jeffreybulanadi Hello it might be better to properly run PHPStan locally on your machine so you can check for statatic analysis errors immidiately.. |
|
@rcalicdan Hi, ran it locally and it's clean. Pinned phpVersion to 8.4 in phpstan.neon as well so future local runs target the same version as CI. |
rcalicdan
left a comment
There was a problem hiding this comment.
Thank you, Everything is set for merging
Closes #6
This PR addresses the bugs and improvements described in issue #6.
Shared iterator normalization
ConcurrencyHandler and ReduceHandler each contained an identical private
getIterator() method. The method is now in a dedicated NormalizesIterator
trait under src/Traits/, following the Hibla ecosystem Traits convention.
Both handlers use the trait, removing the duplication entirely.
Concurrency defaults corrected in docblocks
map(), mapSettled(), filter(), forEach(), and forEachSettled() all accept a
nullable concurrency argument. The docblocks incorrectly described 10 as the
safe limit. The implementation has always used PHP_INT_MAX (unlimited), which
is correct and intentional, mirroring the semantics of array_map(),
array_filter(), and array_walk(). The docblocks now accurately describe
unlimited concurrency as the default. No code behavior was changed.
batch() and batchSettled() concurrency documented
When concurrency is null, both methods default to batchSize so every slot
within each batch runs simultaneously. This was undocumented; the docblocks
now state this clearly.
SettledResult coupled to its own constants
SettledResult defined STATUS_FULFILLED, STATUS_REJECTED, and STATUS_CANCELLED
as private string constants whose values were identical to the PromiseState
enum. Those constants are removed. All usages now reference
PromiseState::FULFILLED->value etc., keeping a single authoritative definition.
wait() used the wrong exception type
Both rejection paths in wait() threw new Exception(safeStringCast(...)) for
non-Throwable reasons. PromiseRejectionException already existed in the
exceptions namespace for exactly this case but was never called. wait() now
uses it, preserving type specificity for callers who want to catch by type.
cancelChildren() silently dropped exceptions
When multiple child promises each threw during their own cancel() call, only
[0] was re-thrown. Every subsequent exception was lost. The method now
collects all exceptions and follows the same pattern used in cancel() itself:
a single exception is re-thrown directly, multiple exceptions are wrapped in
AggregateErrorException.
Tests
New test files cover each of the behavioral changes:
Addressed review feedback