fix!: correct silent-success defects and release v2.0.0 - #58
Conversation
Several paths let the engine do the wrong thing and still report success, which nothing in the suite or a user's logs would have caught. Fixed: - Compound conditions took the wrong branch. ConditionEvaluator had no && / || support but swallowed the operator into the right-hand side and string-compared, so 'order.total > 1000 && order.vip === true' was true for a 500 dollar order. Boolean operators, parentheses and negated groups are now supported; a malformed expression throws instead of collapsing to a boolean. - resume() on a failed workflow threw "Cannot transition from 'failed' to 'failed'" from inside the failure handler, destroying the original cause. FAILED -> RUNNING is now legal, resume retries the failed step, and the handler can no longer mask the real error. - Multi-root workflows executed one root and still reported COMPLETED. - Steps reachable only from each other are rejected at parse time. - Relational comparisons against a missing key are false (null coerces to 0, so 'missing.key < 1000' used to be true). - start() threw instead of overwriting an existing instance. - Workflows blocked on unmet prerequisites park in WAITING. - DelayAction honours the documented minutes/hours keys. - HttpAction reports missing ext-curl as a step failure. Breaking (pre-1.0, each with a migration note in CHANGELOG.md): - EmailAction -> FakeEmailAction; it never sent mail but reported 'status' => 'sent'. Payload is now 'sent' => false, 'mock' => true. - WorkflowBuilder::email() -> fakeEmail(). - ConditionAction drops the non-functional on_true/on_false and uses the shared condition grammar instead of its own '=' / 'is' parser. - FAILED is no longer terminal. Added: - Storage\InMemoryStorage now ships with the package; previously the only adapter lived in tests/ so the library could not run out of the box. - CI runs on pull requests, not just pushes. - SECURITY.md; corrected README action table and attribute examples. - Regression tests for every issue above (116 -> 161 tests). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Now installable there since #57 widened the dev tooling constraints. The library accepts php: ^8.3, which admits 8.5, so it should be tested there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
122c5f3 to
ed03cd0
Compare
|
Rebased onto
Note that Also picked up from the rebase: PHP 8.5 added to the test matrix. #57 widened Verified on PHP 8.3, 8.4 and 8.5 with Pest 4 / PHPUnit 12: 161 tests / 363 |
Fixes the five release blockers found in the pre-v0.1.0 review, plus the
should-fix items, and prepares the
v0.1.0release.The common thread: several paths let the engine do the wrong thing and still
report success — the failure mode a workflow engine can least afford, because
nothing in the test suite or a user's logs would catch it. Each was reproduced
against the real API before being fixed, and each now has a regression test.
Blockers
1. Compound conditions took the wrong branch, silently.
ConditionEvaluatorhad no
&&/||support, but rather than rejecting them it swallowed theoperator into the right-hand side and fell back to a string comparison:
Now supports
&&,||, parentheses and negated groups, with&&bindingtighter. A malformed expression throws instead of collapsing to a boolean.
2.
resume()on a failed workflow was broken, and masked its own error.The documented recovery path threw
Cannot transition workflow from 'failed' to 'failed'. Three defects chained: the executor never liftedFAILEDback toRUNNING; the finalFAILED -> COMPLETEDhop was rejected; and that exceptionwas raised inside the failure handler, destroying the original cause.
FAILED -> RUNNINGis now legal, resume retries the step that failed, and thehandler can never mask the real error.
3. Multi-root workflows dropped a branch and reported
completedat 66.67%progress — state and progress disagreeing, with no error. Every root now runs.
4.
EmailActionsent nothing but reported'status' => 'sent'. Renamed toFakeEmailAction; the payload is now explicitly'sent' => false, 'mock' => trueand it logs a warning.
5. CI never ran on pull requests (
push:only), whiledependabot-auto-mergeauto-merges minor/patch bumps with
contents: write. Both workflows now run onpull_request.Also fixed
false(nullcoerces to0,so
missing.key < 1000was true).start()throws instead of silently overwriting an existing instance.WAITINGinstead of sittingin
RUNNINGindistinguishable from one still executing.DelayActionhonours the documentedminutes/hourskeys — they were neverread, so
delay(hours: 2)paused for the one second default.HttpActionreports missingext-curlas a step failure, not a fatal error.Storage\InMemoryStorageships with the package; the only adapter used tolive in
tests/, so the library could not run a workflow out of the box.bodykey neither action read),attribute examples no longer claim retries/timeouts the engine does not perform,
and the condition grammar is documented.
Breaking changes
All pre-1.0 corrections; each has a migration note in
CHANGELOG.md.EmailAction→FakeEmailActionWorkflowActionwith a real mail transportWorkflowBuilder::email()→fakeEmail()ConditionActiondropson_true/on_falseconditionon the transitionConditionActionuses the shared grammar===/==/!=instead of=/isFAILEDis no longer terminalVerification
Full gate green locally on PHP 8.3 and 8.4 (both matrix versions):
composer validate --strictOKignoreErrorspattern that no longer matchedTwo pre-existing tests asserted the old broken behaviour (
FAILEDcannottransition to
RUNNING) and were updated to encode the new contract.🤖 Generated with Claude Code