From 9a732b2b093478a3023cb10261bb8e431525b60b Mon Sep 17 00:00:00 2001 From: LamLam1 Date: Sat, 12 Sep 2026 13:34:14 +0800 Subject: [PATCH 1/2] fix: correct silent-success defects and prepare the v0.1.0 release 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 --- .github/workflows/phpstan.yml | 6 + .github/workflows/run-tests.yml | 11 +- CHANGELOG.md | 99 +++++ CLAUDE.md | 35 +- README.md | 101 +++++- SECURITY.md | 42 +++ composer.json | 7 +- PLAN.md => docs/PLAN-2025-refactor.md | 4 + phpstan.neon.dist | 1 - src/Actions/ConditionAction.php | 89 ++--- src/Actions/DelayAction.php | 41 ++- src/Actions/EmailAction.php | 45 --- src/Actions/FakeEmailAction.php | 107 ++++++ src/Actions/HttpAction.php | 8 + src/Core/DefinitionParser.php | 94 ++++- src/Core/Executor.php | 51 ++- src/Core/StateManager.php | 8 +- src/Core/WorkflowBuilder.php | 24 +- src/Core/WorkflowDefinition.php | 39 +- src/Core/WorkflowEngine.php | 20 +- src/Core/WorkflowInstance.php | 12 + src/Core/WorkflowState.php | 7 +- .../InvalidWorkflowStateException.php | 16 + src/Storage/InMemoryStorage.php | 146 ++++++++ src/Support/Arr.php | 26 ++ src/Support/ConditionEvaluator.php | 343 ++++++++++++++++-- tests/Integration/EventDispatchTest.php | 2 +- tests/Support/InMemoryStorage.php | 67 ---- tests/TestCase.php | 2 +- tests/Unit/BuilderExecutionTest.php | 2 +- tests/Unit/BuiltInActionsTest.php | 162 +++++++++ tests/Unit/ExecutorIterationTest.php | 2 +- tests/Unit/ExecutorRetryTest.php | 2 +- tests/Unit/PHP83FeaturesTest.php | 12 +- .../Support/ConditionEvaluatorBooleanTest.php | 110 ++++++ tests/Unit/WorkflowBuilderAutoIdTest.php | 4 +- tests/Unit/WorkflowEngineTest.php | 2 +- tests/Unit/WorkflowReachabilityTest.php | 230 ++++++++++++ tests/Unit/WorkflowRecoveryTest.php | 174 +++++++++ tests/Unit/WorkflowStateTransitionTest.php | 4 +- 40 files changed, 1871 insertions(+), 286 deletions(-) create mode 100644 SECURITY.md rename PLAN.md => docs/PLAN-2025-refactor.md (98%) delete mode 100644 src/Actions/EmailAction.php create mode 100644 src/Actions/FakeEmailAction.php create mode 100644 src/Storage/InMemoryStorage.php delete mode 100644 tests/Support/InMemoryStorage.php create mode 100644 tests/Unit/BuiltInActionsTest.php create mode 100644 tests/Unit/Support/ConditionEvaluatorBooleanTest.php create mode 100644 tests/Unit/WorkflowReachabilityTest.php create mode 100644 tests/Unit/WorkflowRecoveryTest.php diff --git a/.github/workflows/phpstan.yml b/.github/workflows/phpstan.yml index 7395a1a..0a9f16b 100644 --- a/.github/workflows/phpstan.yml +++ b/.github/workflows/phpstan.yml @@ -7,6 +7,12 @@ on: - 'phpstan.neon.dist' - '.github/workflows/phpstan.yml' + pull_request: + paths: + - '**.php' + - 'phpstan.neon.dist' + - '.github/workflows/phpstan.yml' + jobs: phpstan: name: phpstan diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 31f0e30..c7d4782 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -9,7 +9,16 @@ on: - 'composer.json' - 'composer.lock' - + + pull_request: + paths: + - '**.php' + - '.github/workflows/run-tests.yml' + - 'phpunit.xml.dist' + - 'composer.json' + - 'composer.lock' + + jobs: test: runs-on: ${{ matrix.os }} diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b64113..bfd1112 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,105 @@ All notable changes to `workflow-engine-core` will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## v2.0.0 - 2026-09-12 + +Fixes several cases where the engine silently did the wrong thing and still +reported success — the failure mode a workflow engine can least afford. + +**Why 2.0.0 and not 1.1.0:** `v1.0.0` was tagged as a stable release, so the +corrections below — which change public class names and runtime behaviour — are +breaking changes under semver and require a major bump. Every one is listed with +its migration. If you are on `v0.0.4-alpha` or `v1.0.0`, read the *Changed* +section before upgrading. + +### Fixed + +- **Compound conditions no longer evaluate to the wrong branch.** `ConditionEvaluator` + had no `&&`/`||` support, but instead of rejecting them it swallowed the operator + into the right-hand side and fell back to a string comparison — so + `order.total > 1000 && order.vip === true` returned **true** for a 500 dollar + order, with no exception and nothing in the logs. Boolean operators, parentheses + and negated groups are now supported, and a malformed expression throws + `InvalidWorkflowDefinitionException` instead of collapsing to a boolean. +- **`resume()` works on a failed workflow.** Recovering a failed instance — the + documented recovery path — threw `Cannot transition workflow from 'failed' to + 'failed'`. Three defects chained: the executor never lifted `FAILED` back to + `RUNNING`; the final `FAILED -> COMPLETED` hop was rejected; and the resulting + exception was raised *inside* the failure handler, destroying the original cause. + `FAILED -> RUNNING` is now a legal transition, resume retries the step that + failed, and the failure handler can no longer mask the real error. +- **Multi-root workflows no longer drop a branch.** A definition with two + independent entry points executed only one of them and still reported + `COMPLETED` (with progress stuck below 100%). Every root now runs. +- **Unreachable steps are rejected at parse time.** A group of steps reachable + only from each other could never execute, yet the workflow reported success. +- **Relational comparisons against a missing key are `false`.** `null` coerces to + `0` in PHP, so `missing.key < 1000` was *true* and steps gated on data that was + never set would run. +- **`start()` no longer overwrites an existing instance.** Reusing a workflow ID + silently discarded the earlier run's state and history; it now throws + `InvalidWorkflowStateException`. +- **Blocked workflows park in `WAITING`.** A workflow whose steps were all blocked + on unmet prerequisites stayed in `RUNNING` forever, indistinguishable from one + still executing. +- **`DelayAction` honours `minutes` and `hours`.** Both were documented but never + read, so `delay(hours: 2)` silently paused for the one second default. +- **`HttpAction` reports a missing cURL extension** as a step failure instead of a + fatal "undefined function" error. + +### Changed + +- **BREAKING: `EmailAction` is now `FakeEmailAction`.** It never sent email, but + returned `'status' => 'sent'` — a workflow could show a delivered confirmation + that did not exist. The payload is now explicitly `'sent' => false, 'mock' => true`, + and it logs a warning. *Migration: implement `WorkflowAction` with your own mail + transport for real delivery.* +- **BREAKING: `WorkflowBuilder::email()` is now `fakeEmail()`**, for the same + reason. *Migration: rename the call, or switch to your own action.* +- **BREAKING: `ConditionAction` no longer accepts `on_true` / `on_false`.** They + were read but never consumed by the engine, so the documented branching did not + exist. *Migration: branch with a `condition` on the transition instead.* +- **BREAKING: `ConditionAction` uses the shared condition grammar.** It previously + carried its own parser accepting `=`, `is` and `is not`, which no other part of + the engine understood. *Migration: use `===`, `==`, `!=` etc.* +- **BREAKING: `FAILED` is no longer a terminal state** — it can transition to + `RUNNING` (resume) or `CANCELLED`. Code asserting that failed workflows are + immutable needs updating. +- `WorkflowState::isFinished()` still reports `true` for `FAILED`; use + `canTransitionTo()` to test whether an instance can still move. + +### Added + +- `Storage\InMemoryStorage` now **ships with the package**. Previously the only + implementation lived in `tests/` under `autoload-dev`, so the library could not + run a workflow out of the box without first writing an adapter. +- `WorkflowDefinition::getFirstSteps()` returns every entry point. +- `Support\Arr::has()` distinguishes an absent key from one holding `null`. +- Boolean operators (`&&`, `||`), parentheses and negated groups in conditions. +- `SECURITY.md` with a disclosure process and scope notes. +- Regression tests for every issue above (116 -> 161 tests). + +### CI + +- `run-tests.yml` and `phpstan.yml` now run on **pull requests**, not just pushes. + Combined with `dependabot-auto-merge.yml` auto-merging minor and patch bumps, + dependency updates could previously reach `main` without the suite ever running + against the merge result. +- Removed a stale PHPStan `ignoreErrors` pattern that no longer matched anything. + +### Docs + +- Documented the condition grammar, including the two rules that stop a mistyped + condition from silently routing a workflow the wrong way. +- Corrected the built-in actions table: it documented a `body` key for + `EmailAction` and `HttpAction` that neither read, `minutes`/`hours` for + `DelayAction` that were ignored, and branching for `ConditionAction` that did + not exist. +- Fixed the PHP attribute examples, whose inline comments claimed retries and + timeouts that the attributes do not actually perform — the engine still does not + read them (this is noted in the README). +- Archived the completed `PLAN.md` to `docs/PLAN-2025-refactor.md`. + ## v1.0.0 - 2026-09-12 First stable release. diff --git a/CLAUDE.md b/CLAUDE.md index 2941c2d..d14054c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,7 +6,7 @@ Project context for Claude Code and AI-assisted development. **workflow-engine-core** is a framework-agnostic PHP workflow engine. Zero production dependencies. PHP 8.3+. MIT licensed. -Status: **v0.0.2-alpha** — active development, not production-ready. +Status: **v2.0.0** — stable. Breaking changes follow semver and are documented in CHANGELOG.md. Related package: `solution-forest/workflow-engine-laravel` (Laravel integration layer). @@ -50,23 +50,24 @@ WorkflowBuilder → WorkflowDefinition → WorkflowEngine → Executor → Actio | Namespace | Purpose | |-----------|---------| | `Core\` | WorkflowEngine, WorkflowBuilder, Executor, StateManager, WorkflowInstance, WorkflowDefinition, WorkflowContext, ActionResult, Step, DefinitionParser, ActionResolver | -| `Actions\` | BaseAction, LogAction, EmailAction, HttpAction, DelayAction, ConditionAction | +| `Actions\` | BaseAction, LogAction, FakeEmailAction, HttpAction, DelayAction, ConditionAction | | `Contracts\` | WorkflowAction, StorageAdapter, EventDispatcher, Logger | | `Attributes\` | WorkflowStep, Retry, Timeout, Condition | | `Events\` | WorkflowStartedEvent, WorkflowCompletedEvent, WorkflowFailedEvent, WorkflowCancelledEvent, StepCompletedEvent, StepFailedEvent, StepRetriedEvent | | `Exceptions\` | WorkflowException (base), InvalidWorkflowDefinitionException, InvalidWorkflowStateException, ActionNotFoundException, StepExecutionException, WorkflowInstanceNotFoundException | | `Support\` | NullLogger, NullEventDispatcher, SimpleWorkflow, Uuid, Timeout, ConditionEvaluator, Arr | +| `Storage\` | InMemoryStorage (ships with the package; non-durable) | ### State Machine ``` -PENDING → RUNNING → COMPLETED - ↓ ↓ ↑ - FAILED WAITING - ↑ ↓ ↑ - FAILED ← PAUSED - ↑ -CANCELLED ← (any non-terminal state) +PENDING ──→ RUNNING ──→ COMPLETED (terminal) + ↓ ↑ + WAITING / PAUSED + ↓ ↑ + FAILED ──→ RUNNING (resume retries the failed step) + ↓ + CANCELLED (terminal) ``` **Valid transitions (enforced at runtime):** @@ -74,7 +75,11 @@ CANCELLED ← (any non-terminal state) - `RUNNING` → `WAITING`, `PAUSED`, `COMPLETED`, `FAILED`, `CANCELLED` - `WAITING` → `RUNNING`, `FAILED`, `CANCELLED` - `PAUSED` → `RUNNING`, `FAILED`, `CANCELLED` -- Terminal states (`COMPLETED`, `FAILED`, `CANCELLED`) → no transitions allowed +- `FAILED` → `RUNNING` (recovery via `resume()`), `CANCELLED` +- Terminal states (`COMPLETED`, `CANCELLED`) → no transitions allowed + +`FAILED` is recoverable: `resume()` returns the instance to `RUNNING` and retries +the step that failed. A workflow blocked on unmet prerequisites parks in `WAITING`. Invalid transitions throw `InvalidWorkflowStateException`. @@ -154,15 +159,15 @@ $engine->cancel($instanceId, 'reason'); ## CI/CD GitHub Actions workflows: -- `run-tests.yml` — Matrix: PHP 8.3/8.4 × prefer-lowest/prefer-stable -- `phpstan.yml` — Static analysis on .php changes +- `run-tests.yml` — Matrix: PHP 8.3/8.4 × prefer-lowest/prefer-stable (runs on push **and** pull_request) +- `phpstan.yml` — Static analysis on .php changes (runs on push **and** pull_request) - `fix-php-code-style-issues.yml` — Auto-format with Pint on push - `update-changelog.yml` — Auto-update CHANGELOG on release - `dependabot-auto-merge.yml` — Auto-merge minor/patch dependency updates ## File Counts -- 46 source files in `src/` -- 25 test files in `tests/` -- 93 tests, 224+ assertions +- 47 source files in `src/` +- 27 test files in `tests/` +- 161 tests, 363+ assertions - PHPStan level 6 compliance diff --git a/README.md b/README.md index 3ebd6eb..dd16e6f 100644 --- a/README.md +++ b/README.md @@ -91,8 +91,10 @@ $definition = WorkflowBuilder::create('order-processing') ->addStep('fulfillment', FulfillOrderAction::class) ->build(); -// Create engine with storage adapter and event dispatcher -$engine = new WorkflowEngine($storageAdapter, $eventDispatcher); +// Create engine with a storage adapter (InMemoryStorage ships with the package) +use SolutionForest\WorkflowEngine\Storage\InMemoryStorage; + +$engine = new WorkflowEngine(new InMemoryStorage(), $eventDispatcher); // Start and run the workflow $instanceId = $engine->start( @@ -118,7 +120,9 @@ $workflow = WorkflowBuilder::create('order-flow') $builder->addStep('fraud_check', FraudCheckAction::class); }) ->addStep('payment', ProcessPaymentAction::class, timeout: 300, retryAttempts: 3) - ->email('order-confirmation', 'customer@example.com', 'Order Confirmed') + // fakeEmail() records an email but does NOT send one - this package ships + // no mail transport. Use your own action for real delivery. + ->fakeEmail('order-confirmation', 'customer@example.com', 'Order Confirmed') ->build(); // Quick templates for common patterns @@ -142,7 +146,8 @@ class ReliableApiAction extends BaseAction { public function execute(WorkflowContext $context): ActionResult { - // Retries up to 3 times with exponential backoff starting at 1s + // Metadata only - the engine does not read this attribute yet. + // For real retries use: ->addStep('id', Action::class, retryAttempts: 3) return ActionResult::success(); } } @@ -157,7 +162,8 @@ class TimedAction extends BaseAction { public function execute(WorkflowContext $context): ActionResult { - // Will timeout after 30 seconds + // Metadata only - the engine does not read this attribute yet. + // For a real timeout use: ->addStep('id', Action::class, timeout: 30) return ActionResult::success(); } } @@ -172,7 +178,8 @@ class PremiumProcessingAction extends BaseAction { public function execute(WorkflowContext $context): ActionResult { - // Only executes when order.amount > 100 + // Metadata only - the engine does not read this attribute yet. + // For real gating use: ->when('order.amount > 100', fn ($b) => ...) return ActionResult::success(); } } @@ -213,6 +220,41 @@ $workflow = WorkflowBuilder::create('conditional-flow') ->build(); ``` +### Condition Syntax + +Conditions are parsed, never `eval()`'d. The same grammar is used by step +conditions, transition conditions, `when()` and `ConditionAction`. + +```php +// Comparisons: === !== == != > < >= <= +'order.total > 1000' +'user.plan === "premium"' +'status != cancelled' // bare words are treated as strings + +// Truthy checks and negation +'user.active' +'!user.suspended' + +// Boolean operators, with && binding tighter than || +'order.total > 1000 && user.vip === true' +'user.vip || order.total > 5000' + +// Parentheses to override precedence +'(user.vip || order.total > 5000) && !order.refunded' +``` + +Dot notation reads nested data (`order.customer.email`). Values may be numbers, +quoted strings, bare words, `true`, `false` or `null`. + +Two rules keep a mistyped condition from quietly routing a workflow the wrong way: + +- **A malformed expression throws** `InvalidWorkflowDefinitionException` rather + than collapsing to a boolean. Chained comparisons (`a > 1 > 2`), dangling + operators and unbalanced parentheses are all rejected. +- **A relational comparison against a missing key is `false`.** Because `null` + coerces to `0` in PHP, `missing.key < 1000` would otherwise be *true* and a + step gated on data that was never set would run. + ### Workflow Lifecycle Management ```php @@ -222,6 +264,18 @@ $instance = $engine->getInstance($instanceId); $engine->resume($instanceId); $engine->cancel($instanceId, 'No longer needed'); +// Instance IDs are caller-supplied and must be unique: start() throws +// InvalidWorkflowStateException rather than overwriting an existing instance. + +// Recovering a failed workflow: fix the cause, then resume to retry the +// step that failed. resume() is rejected for COMPLETED and CANCELLED. +try { + $engine->resume($instanceId); +} catch (StepExecutionException $e) { + // Still failing - the exception carries the real cause, not a + // state-machine error from the failure handler. +} + // Track progress $progress = $instance->getProgress(); // 0.0 to 100.0 $summary = $instance->getStatusSummary(); @@ -295,13 +349,13 @@ WorkflowBuilder → WorkflowDefinition → WorkflowEngine → Executor → Actio ### State Machine ``` -PENDING → RUNNING → COMPLETED - ↓ ↓ ↑ - FAILED WAITING - ↑ ↓ ↑ - FAILED ← PAUSED - ↑ -CANCELLED ← (any non-terminal state) +PENDING ──→ RUNNING ──→ COMPLETED (terminal) + ↓ ↑ + WAITING / PAUSED + ↓ ↑ + FAILED ──→ RUNNING (resume retries the failed step) + ↓ + CANCELLED (terminal) ``` **Valid transitions:** @@ -309,7 +363,16 @@ CANCELLED ← (any non-terminal state) - `RUNNING` → `WAITING`, `PAUSED`, `COMPLETED`, `FAILED`, `CANCELLED` - `WAITING` → `RUNNING`, `FAILED`, `CANCELLED` - `PAUSED` → `RUNNING`, `FAILED`, `CANCELLED` -- Terminal states (`COMPLETED`, `FAILED`, `CANCELLED`) → no further transitions +- `FAILED` → `RUNNING` (via `resume()`), `CANCELLED` +- Terminal states (`COMPLETED`, `CANCELLED`) → no further transitions + +`FAILED` is **recoverable, not terminal**: calling `resume()` puts the instance +back into `RUNNING` and retries the step that failed, which is how you recover a +workflow once the underlying cause is fixed. + +A workflow whose next steps are all blocked on unmet prerequisites parks in +`WAITING` rather than sitting in `RUNNING`, so a stuck instance is +distinguishable from one still executing. State transitions are validated at runtime — invalid transitions throw `InvalidWorkflowStateException`. @@ -318,7 +381,7 @@ State transitions are validated at runtime — invalid transitions throw `Invali | Namespace | Contents | |-----------|----------| | `Core\` | WorkflowEngine, WorkflowBuilder, Executor, StateManager, WorkflowInstance, WorkflowDefinition, WorkflowContext, ActionResult, Step, DefinitionParser, ActionResolver | -| `Actions\` | BaseAction, LogAction, EmailAction, HttpAction, DelayAction, ConditionAction | +| `Actions\` | BaseAction, LogAction, FakeEmailAction, HttpAction, DelayAction, ConditionAction | | `Contracts\` | WorkflowAction, StorageAdapter, EventDispatcher, Logger | | `Attributes\` | WorkflowStep, Retry, Timeout, Condition | | `Events\` | WorkflowStartedEvent, WorkflowCompletedEvent, WorkflowFailedEvent, WorkflowCancelledEvent, StepCompletedEvent, StepFailedEvent, StepRetriedEvent | @@ -332,10 +395,10 @@ Six ready-to-use actions are included: | Action | Purpose | Config Keys | |--------|---------|-------------| | **LogAction** | Log messages with placeholder replacement (`{user.name}`) | `message`, `level` (debug/info/warning/error) | -| **EmailAction** | Mock email sending with template support | `to`, `subject`, `body`, `template` | -| **HttpAction** | HTTP requests with `{{ variable }}` template variables | `url`, `method`, `headers`, `body` | -| **DelayAction** | Pause execution for a specified duration | `seconds`, `minutes`, `hours` | -| **ConditionAction** | Evaluate boolean expressions and branch (`on_true`/`on_false`) | `condition`, `on_true`, `on_false` | +| **FakeEmailAction** | ⚠️ Records an email; **does not send one** (no mail transport ships with this package) | `to`, `subject`, `template`, `data` | +| **HttpAction** | HTTP requests with `{{ variable }}` template variables (requires `ext-curl`) | `url`, `method`, `data`, `headers`, `timeout`, `connect_timeout`, `verify_tls`, `max_redirects` | +| **DelayAction** | Pause execution for a specified duration (blocking) | `hours`, `minutes`, `seconds`, `microseconds` | +| **ConditionAction** | Evaluate a condition and record the result in workflow data | `condition` | | **BaseAction** | Abstract base class for custom actions | — | ### WorkflowState Helpers diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..9ed76c0 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,42 @@ +# Security Policy + +## Supported Versions + +| Version | Supported | +|---------|-----------| +| 2.x | ✅ | +| 1.0.x | ⚠️ Superseded by 2.0.0; upgrade recommended | +| < 1.0 | ❌ (alpha releases, no longer maintained) | + +## Reporting a Vulnerability + +Please **do not** open a public issue for security problems. + +Report vulnerabilities privately through +[GitHub Security Advisories](https://github.com/solutionforest/workflow-engine-core/security/advisories/new), +or by email to **info@solutionforest.com**. + +Include as much of the following as you can: + +- A description of the issue and its impact +- Steps to reproduce, or a proof-of-concept workflow definition +- Affected version(s) + +We aim to acknowledge reports within 5 working days and to ship a fix or +mitigation for confirmed issues in the next patch release. + +## Scope Notes + +A few behaviours are intentional and are **not** vulnerabilities: + +- **Actions execute arbitrary PHP.** A workflow definition names a class that the + engine instantiates and runs. Treat workflow definitions as trusted code, not as + user input — never build a definition's `action` value from an untrusted source. +- **Conditions are parsed, not evaluated.** `ConditionEvaluator` uses a small + hand-written parser and never calls `eval()`. A malformed condition throws + rather than executing anything. +- **`HttpAction` follows redirects** (max 3 by default, HTTP/HTTPS only, TLS + verification on). It does not filter private or link-local addresses, so do not + point it at a URL supplied by an untrusted party without your own SSRF controls. +- **`DelayAction` blocks the current process.** A long delay in a web request will + hold that worker; run long-delay workflows from a queue or CLI worker. diff --git a/composer.json b/composer.json index 1e08775..442fd46 100644 --- a/composer.json +++ b/composer.json @@ -17,7 +17,12 @@ } ], "require": { - "php": "^8.3" + "php": "^8.3", + "ext-json": "*" + }, + "suggest": { + "ext-curl": "Required by HttpAction to make HTTP requests", + "ext-pcntl": "Enables step timeout enforcement (CLI/worker contexts only)" }, "require-dev": { "laravel/pint": "^1.22", diff --git a/PLAN.md b/docs/PLAN-2025-refactor.md similarity index 98% rename from PLAN.md rename to docs/PLAN-2025-refactor.md index 5cb31a9..7c015c5 100644 --- a/PLAN.md +++ b/docs/PLAN-2025-refactor.md @@ -1,3 +1,7 @@ +> **Archived.** Phases 1–8 of this plan were completed and shipped; it is kept +> for historical context only and is not a current roadmap. See `CHANGELOG.md` +> for what actually landed. + # Implementation Plan: workflow-engine-core Improvements ## Philosophy diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 7be727a..9f6ff17 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -5,6 +5,5 @@ parameters: tmpDir: build/phpstan ignoreErrors: - '#has no value type specified in iterable type array#' - - '#Match arm comparison between .+ and .+ is always true#' - '#PHPDoc tag @param for parameter .+ with type .+ is not subtype of native type#' - '#Parameter .+ has invalid type#' diff --git a/src/Actions/ConditionAction.php b/src/Actions/ConditionAction.php index 23f3062..bf3ab9f 100644 --- a/src/Actions/ConditionAction.php +++ b/src/Actions/ConditionAction.php @@ -5,10 +5,26 @@ use SolutionForest\WorkflowEngine\Attributes\WorkflowStep; use SolutionForest\WorkflowEngine\Core\ActionResult; use SolutionForest\WorkflowEngine\Core\WorkflowContext; -use SolutionForest\WorkflowEngine\Support\Arr; +use SolutionForest\WorkflowEngine\Support\ConditionEvaluator; /** - * Condition evaluation action with advanced expression parsing + * Records the result of a condition expression in the workflow data. + * + * This action **evaluates** a condition; it does not branch. Branching is a + * property of the graph, not of a step, so route the workflow with a condition + * on the transition (or on the target step) instead: + * + * ```php + * 'transitions' => [ + * ['from' => 'check', 'to' => 'premium', 'condition' => 'order.total > 1000'], + * ['from' => 'check', 'to' => 'standard', 'condition' => 'order.total <= 1000'], + * ] + * ``` + * + * The result is merged into the workflow data as `result`, so later steps and + * transitions can read it. + * + * Conditions use the grammar documented on {@see ConditionEvaluator}. */ #[WorkflowStep( id: 'condition_check', @@ -30,8 +46,6 @@ public function getDescription(): string protected function doExecute(WorkflowContext $context): ActionResult { $condition = $this->getConfig('condition'); - $onTrue = $this->getConfig('on_true', null); - $onFalse = $this->getConfig('on_false', null); if (! $condition) { return ActionResult::failure('Condition is required'); @@ -43,10 +57,9 @@ protected function doExecute(WorkflowContext $context): ActionResult return ActionResult::success([ 'condition' => $condition, 'result' => $result, - 'next_action' => $result ? $onTrue : $onFalse, ]); - } catch (\Exception $e) { + } catch (\Throwable $e) { return ActionResult::failure( "Condition evaluation failed: {$e->getMessage()}", ['condition' => $condition] @@ -59,64 +72,18 @@ protected function doExecute(WorkflowContext $context): ActionResult * * @param array $data */ - private function evaluateCondition(string $condition, array $data): bool - { - // Simple expression parser for common patterns - if (preg_match('/^(.+?)\s*(=|!=|>|<|>=|<=|is|is not)\s*(.+)$/', $condition, $matches)) { - $left = trim($matches[1]); - $operator = trim($matches[2]); - $right = trim($matches[3]); - - $leftValue = $this->getValue($left, $data); - $rightValue = $this->getValue($right, $data); - - return match ($operator) { - '=' => $leftValue == $rightValue, - '!=' => $leftValue != $rightValue, - '>' => $leftValue > $rightValue, - '<' => $leftValue < $rightValue, - '>=' => $leftValue >= $rightValue, - '<=' => $leftValue <= $rightValue, - 'is' => $leftValue === $rightValue, - 'is not' => $leftValue !== $rightValue, - default => throw new \InvalidArgumentException("Unsupported operator: {$operator}") - }; - } - - // Check for boolean values - if (in_array(strtolower($condition), ['true', '1', 'yes'])) { - return true; - } - - if (in_array(strtolower($condition), ['false', '0', 'no'])) { - return false; - } - - // Direct data access - return (bool) $this->getValue($condition, $data); - } - /** + * Evaluate a condition expression against workflow data. + * + * Delegates to {@see ConditionEvaluator} so that every condition in the + * library — step conditions, transition conditions and this action — speaks + * exactly one grammar. This class previously carried its own parser that + * accepted "=" and "is", which no other part of the engine understood. + * * @param array $data */ - private function getValue(string $expression, array $data): mixed + private function evaluateCondition(string $condition, array $data): bool { - // Remove quotes for string literals - if (preg_match('/^["\'](.+)["\']$/', $expression, $matches)) { - return $matches[1]; - } - - // Check for numeric values - if (is_numeric($expression)) { - return str_contains($expression, '.') ? (float) $expression : (int) $expression; - } - - // Check for boolean literals - return match (strtolower($expression)) { - 'true', 'yes' => true, - 'false', 'no' => false, - 'null', 'empty' => null, - default => Arr::get($data, $expression) - }; + return ConditionEvaluator::evaluate($condition, $data); } } diff --git a/src/Actions/DelayAction.php b/src/Actions/DelayAction.php index b157c1b..c9a9fe4 100644 --- a/src/Actions/DelayAction.php +++ b/src/Actions/DelayAction.php @@ -22,27 +22,46 @@ public function getDescription(): string protected function doExecute(WorkflowContext $context): ActionResult { - $seconds = $this->getConfig('seconds', 1); - $microseconds = $this->getConfig('microseconds', 0); + // `minutes` and `hours` are accepted alongside `seconds` because the + // builder's delay() sugar and the documentation both offer them; they + // used to be silently ignored, turning delay(hours: 2) into one second. + $units = [ + 'hours' => 3_600_000_000, + 'minutes' => 60_000_000, + 'seconds' => 1_000_000, + 'microseconds' => 1, + ]; - if (! is_numeric($seconds) || $seconds < 0) { - return ActionResult::failure('Invalid delay seconds specified'); - } + $provided = array_filter( + array_keys($units), + fn (string $unit): bool => $this->getConfig($unit) !== null + ); + + $totalMicroseconds = 0; + + foreach ($provided as $unit) { + $value = $this->getConfig($unit); - if (! is_numeric($microseconds) || $microseconds < 0) { - return ActionResult::failure('Invalid delay microseconds specified'); + if (! is_numeric($value) || $value < 0) { + return ActionResult::failure("Invalid delay {$unit} specified"); + } + + $totalMicroseconds += $value * $units[$unit]; } - // Convert to total microseconds - $totalMicroseconds = ($seconds * 1000000) + $microseconds; + // Preserve the historical default of a one second pause when no unit + // is configured at all. + if ($provided === []) { + $totalMicroseconds = $units['seconds']; + } if ($totalMicroseconds > 0) { usleep((int) $totalMicroseconds); } return ActionResult::success([ - 'delayed_seconds' => $seconds, - 'delayed_microseconds' => $microseconds, + 'delayed_seconds' => $totalMicroseconds / 1_000_000, + 'delayed_microseconds' => (int) $totalMicroseconds, 'delayed_at' => (new \DateTime('now', new \DateTimeZone('UTC')))->format('c'), ]); } diff --git a/src/Actions/EmailAction.php b/src/Actions/EmailAction.php deleted file mode 100644 index f893111..0000000 --- a/src/Actions/EmailAction.php +++ /dev/null @@ -1,45 +0,0 @@ -getConfig('template', 'default'); - $to = $context->getConfig('to', ''); - $subject = $context->getConfig('subject', ''); - $data = $context->getConfig('data', []); - - // Mock email sending - in real implementation this would send actual emails - $emailData = [ - 'template' => $template, - 'to' => $to, - 'subject' => $subject, - 'data' => $data, - 'sent_at' => date('Y-m-d H:i:s'), - 'status' => 'sent', - ]; - - return ActionResult::success(['email_sent' => $emailData]); - } - - public function canExecute(WorkflowContext $context): bool - { - return ! empty($context->getConfig('to')); - } - - public function getName(): string - { - return 'Send Email'; - } - - public function getDescription(): string - { - return 'Sends an email using the specified template and configuration'; - } -} diff --git a/src/Actions/FakeEmailAction.php b/src/Actions/FakeEmailAction.php new file mode 100644 index 0000000..7deafbd --- /dev/null +++ b/src/Actions/FakeEmailAction.php @@ -0,0 +1,107 @@ +mailer->send(...); + * + * return ActionResult::success(['sent' => true]); + * } + * } + * ``` + */ +class FakeEmailAction implements WorkflowAction +{ + private readonly Logger $logger; + + /** + * @param array $config Step configuration + * @param Logger|null $logger Logger used to warn that no mail is sent + */ + public function __construct( + private readonly array $config = [], + ?Logger $logger = null + ) { + $this->logger = $logger ?? new NullLogger; + } + + public function execute(WorkflowContext $context): ActionResult + { + $template = $context->getConfig('template', 'default'); + $to = $context->getConfig('to', ''); + $subject = $context->getConfig('subject', ''); + $data = $context->getConfig('data', []); + + $this->logger->warning('FakeEmailAction did not send an email', [ + 'workflow_id' => $context->getWorkflowId(), + 'step_id' => $context->getStepId(), + 'to' => $to, + 'hint' => 'Implement WorkflowAction with a real mail transport to send email.', + ]); + + return ActionResult::success([ + 'email' => [ + 'template' => $template, + 'to' => $to, + 'subject' => $subject, + 'data' => $data, + 'recorded_at' => (new \DateTime('now', new \DateTimeZone('UTC')))->format('c'), + // Never report a delivery that did not happen. + 'sent' => false, + 'mock' => true, + ], + ]); + } + + public function canExecute(WorkflowContext $context): bool + { + return ! empty($context->getConfig('to')); + } + + public function getName(): string + { + return 'Fake Send Email'; + } + + public function getDescription(): string + { + return 'Records the email that would be sent; does not deliver anything'; + } + + /** + * Get the step configuration this action was constructed with. + * + * @return array + */ + public function getConfig(): array + { + return $this->config; + } +} diff --git a/src/Actions/HttpAction.php b/src/Actions/HttpAction.php index 3471ea8..4b6e8e1 100644 --- a/src/Actions/HttpAction.php +++ b/src/Actions/HttpAction.php @@ -46,6 +46,14 @@ protected function doExecute(WorkflowContext $context): ActionResult return ActionResult::failure('URL is required for HTTP action'); } + // ext-curl is a suggested, not required, dependency: report its absence + // as a normal step failure rather than a fatal "undefined function". + if (! function_exists('curl_init')) { + return ActionResult::failure( + 'HttpAction requires the cURL extension (ext-curl), which is not loaded.' + ); + } + // Process template variables in URL and data $url = $this->processTemplate($url, $context->getData()); $data = $this->processArrayTemplates($data, $context->getData()); diff --git a/src/Core/DefinitionParser.php b/src/Core/DefinitionParser.php index 2ff5966..e7d1b2c 100644 --- a/src/Core/DefinitionParser.php +++ b/src/Core/DefinitionParser.php @@ -193,6 +193,8 @@ private function validateDefinition(array $definition): void foreach ($definition['transitions'] as $transitionIndex => $transition) { $this->validateTransition($transition, $steps, $transitionIndex); } + + $this->validateReachability($steps, $definition['transitions'], $definition); } // Validate optional version field @@ -214,6 +216,92 @@ private function validateDefinition(array $definition): void } } + /** + * Ensure every declared step is reachable from one of the workflow's roots. + * + * A step that no transition leads to, and that isn't itself an entry point, + * can never execute. Before this check the engine would run what it could + * reach and then report the whole workflow COMPLETED, silently dropping the + * orphaned branch — so an unreachable step is rejected at parse time. + * + * @param array> $steps Normalized steps keyed by ID + * @param array $transitions Raw transition list + * @param array $definition The full definition, for error context + * + * @throws InvalidWorkflowDefinitionException If any step is unreachable + */ + private function validateReachability(array $steps, array $transitions, array $definition): void + { + $stepIds = array_keys($steps); + + // Build an adjacency list and collect every step that is a transition + // target, so the roots are whatever is left over. + $outgoing = []; + $hasIncoming = []; + + foreach ($transitions as $transition) { + if (! is_array($transition) || ! isset($transition['from'], $transition['to'])) { + continue; // Already reported by validateTransition(). + } + + $from = (string) $transition['from']; + $to = (string) $transition['to']; + + $outgoing[$from][] = $to; + $hasIncoming[$to] = true; + } + + $roots = array_values(array_filter( + $stepIds, + static fn (string $id): bool => ! isset($hasIncoming[$id]) + )); + + // A fully cyclic graph has no root; execution falls back to the first + // declared step, so treat that as the entry point here too. + if ($roots === []) { + $roots = $stepIds === [] ? [] : [$stepIds[0]]; + } + + $reachable = []; + $queue = $roots; + + while ($queue !== []) { + $current = array_shift($queue); + + if (isset($reachable[$current])) { + continue; + } + + $reachable[$current] = true; + + foreach ($outgoing[$current] ?? [] as $next) { + if (! isset($reachable[$next])) { + $queue[] = $next; + } + } + } + + $unreachable = array_values(array_filter( + $stepIds, + static fn (string $id): bool => ! isset($reachable[$id]) + )); + + if ($unreachable !== []) { + throw new InvalidWorkflowDefinitionException( + sprintf( + 'Workflow contains unreachable step(s): %s. Every step must be reachable '. + 'from a starting step via transitions, otherwise it can never execute.', + implode(', ', array_map(static fn (string $id): string => "'{$id}'", $unreachable)) + ), + $definition, + array_map( + static fn (string $id): string => "Step '{$id}' is not reachable from any starting step.", + $unreachable + ) + ); + } + } + /** * Normalize step definitions to consistent associative array format. * @@ -231,13 +319,13 @@ private function validateDefinition(array $definition): void * // Sequential array with ID properties (will be normalized) * $steps = [ * ['id' => 'step1', 'action' => 'LogAction', 'timeout' => '30s'], - * ['id' => 'step2', 'action' => 'EmailAction'] + * ['id' => 'step2', 'action' => 'FakeEmailAction'] * ]; * * // Already associative (returned as-is) * $steps = [ * 'step1' => ['action' => 'LogAction', 'timeout' => '30s'], - * 'step2' => ['action' => 'EmailAction'] + * 'step2' => ['action' => 'FakeEmailAction'] * ]; * ``` * @@ -309,7 +397,7 @@ private function normalizeSteps(array $steps): array * * // Step with timeout and retry * $step = [ - * 'action' => 'EmailAction', + * 'action' => 'FakeEmailAction', * 'timeout' => '30s', * 'retry_attempts' => 3, * 'parameters' => ['to' => 'user@example.com'] diff --git a/src/Core/Executor.php b/src/Core/Executor.php index 743b45a..c10d28e 100644 --- a/src/Core/Executor.php +++ b/src/Core/Executor.php @@ -130,7 +130,22 @@ public function execute(WorkflowInstance $instance): void 'trace' => $e->getTraceAsString(), ]); - $this->stateManager->setError($instance, $e->getMessage()); + // Recording the failure must never replace the failure itself. If + // the instance is in a state that cannot legally move to FAILED + // (or storage rejects the write), swallow that secondary error and + // log it — the caller needs the original cause, not the bookkeeping + // exception that happened while reacting to it. + try { + $this->stateManager->setError($instance, $e->getMessage()); + } catch (\Throwable $bookkeepingError) { + $this->logger->error('Failed to record workflow failure state', [ + 'workflow_id' => $instance->getId(), + 'state' => $instance->getState()->value, + 'original_error' => $e->getMessage(), + 'bookkeeping_error' => $bookkeepingError->getMessage(), + ]); + } + $this->eventDispatcher->dispatch(new WorkflowFailedEvent($instance, $e)); // Re-throw the original throwable to maintain the error context @@ -153,9 +168,20 @@ public function execute(WorkflowInstance $instance): void */ private function processWorkflow(WorkflowInstance $instance): void { - // If workflow is not running, transition it to running - if (in_array($instance->getState(), [WorkflowState::PENDING, WorkflowState::PAUSED, WorkflowState::WAITING])) { + // If workflow is not running, transition it to running. FAILED is + // included so that resuming a failed workflow actually retries it + // instead of executing steps while still marked failed (which would + // then trip the state machine on the final FAILED -> COMPLETED hop). + if (in_array($instance->getState(), [ + WorkflowState::PENDING, + WorkflowState::PAUSED, + WorkflowState::WAITING, + WorkflowState::FAILED, + ])) { $instance->setState(WorkflowState::RUNNING); + // Clear the previous failure so a recovered run doesn't keep + // reporting a stale error message. + $instance->setErrorMessage(null); $this->stateManager->save($instance); } @@ -207,10 +233,23 @@ private function processWorkflow(WorkflowInstance $instance): void $progressed = true; } - // If no steps made progress this iteration, the workflow is stuck - // (e.g. all next steps were blocked on unmet prerequisites). Exit - // the loop and let the next resume() reattempt. + // No step made progress: every candidate was blocked on an unmet + // prerequisite. Park the workflow in WAITING rather than leaving it + // in RUNNING, where a permanently stuck instance is indistinguishable + // from one that is still executing. if (! $progressed) { + $blocked = array_map(static fn (Step $s): string => $s->getId(), $nextSteps); + + $this->logger->warning('Workflow is waiting on unmet prerequisites', [ + 'workflow_id' => $instance->getId(), + 'blocked_steps' => $blocked, + ]); + + if ($instance->getState()->canTransitionTo(WorkflowState::WAITING)) { + $instance->setState(WorkflowState::WAITING); + $this->stateManager->save($instance); + } + return; } } diff --git a/src/Core/StateManager.php b/src/Core/StateManager.php index 2ebdb4b..47cf2ff 100644 --- a/src/Core/StateManager.php +++ b/src/Core/StateManager.php @@ -252,7 +252,13 @@ public function markStepFailed(WorkflowInstance $instance, string $stepId, strin public function setError(WorkflowInstance $instance, string $error): void { $instance->setErrorMessage($error); - $instance->setState(WorkflowState::FAILED); + + // Only move the state when the instance isn't already failed; a repeat + // failure would otherwise throw on the illegal FAILED -> FAILED hop. + if ($instance->getState() !== WorkflowState::FAILED) { + $instance->setState(WorkflowState::FAILED); + } + $this->save($instance); } diff --git a/src/Core/WorkflowBuilder.php b/src/Core/WorkflowBuilder.php index 00c4eb6..d7a8229 100644 --- a/src/Core/WorkflowBuilder.php +++ b/src/Core/WorkflowBuilder.php @@ -2,6 +2,7 @@ namespace SolutionForest\WorkflowEngine\Core; +use SolutionForest\WorkflowEngine\Actions\FakeEmailAction; use SolutionForest\WorkflowEngine\Contracts\WorkflowAction; use SolutionForest\WorkflowEngine\Exceptions\InvalidWorkflowDefinitionException; @@ -37,7 +38,7 @@ * * ```php * $workflow = WorkflowBuilder::create('newsletter') - * ->email('newsletter-template', '{{ user.email }}', 'Weekly Newsletter') + * ->fakeEmail('newsletter-template', '{{ user.email }}', 'Weekly Newsletter') * ->delay(hours: 1) * ->http('https://api.example.com/track', 'POST', ['user_id' => '{{ user.id }}']) * ->build(); @@ -310,7 +311,11 @@ public function when(string $condition, callable $callback): self } /** - * Add an email step using pre-configured email action (common pattern). + * Add a placeholder email step that records, but does not send, an email. + * + * ⚠️ This adds a {@see FakeEmailAction}, + * which **does not deliver mail** — this library ships no mail transport. Use + * it to prototype a flow, then swap in your own action backed by a real mailer. * * @param string $template Email template identifier * @param string $to Recipient email address (supports placeholders like "{{ user.email }}") @@ -320,15 +325,18 @@ public function when(string $condition, callable $callback): self * * @example * ```php - * $builder->email( + * $builder->fakeEmail( * 'welcome-email', * '{{ user.email }}', * 'Welcome to {{ app.name }}!', * ['user_name' => '{{ user.name }}'] * ); + * + * // Real delivery: use your own action instead. + * $builder->addStep('welcome', SendWelcomeEmailAction::class); * ``` */ - public function email( + public function fakeEmail( string $template, string $to, string $subject, @@ -336,7 +344,7 @@ public function email( ): self { return $this->addStep( $this->generateStepId('email'), - 'SolutionForest\\WorkflowEngine\\Actions\\EmailAction', + FakeEmailAction::class, [ 'template' => $template, 'to' => $to, @@ -603,7 +611,7 @@ public function userOnboarding(string $name = 'user-onboarding'): WorkflowBuilde { return WorkflowBuilder::create($name) ->description('Standard user onboarding process') - ->email( + ->fakeEmail( template: 'welcome', to: '{{ user.email }}', subject: 'Welcome to {{ app.name }}!' @@ -636,7 +644,7 @@ public function orderProcessing(string $name = 'order-processing'): WorkflowBuil ->addStep('validate_order', 'App\\Actions\\ValidateOrderAction') ->addStep('charge_payment', 'App\\Actions\\ChargePaymentAction') ->addStep('update_inventory', 'App\\Actions\\UpdateInventoryAction') - ->email( + ->fakeEmail( template: 'order-confirmation', to: '{{ order.customer.email }}', subject: 'Order Confirmation #{{ order.id }}' @@ -663,7 +671,7 @@ public function documentApproval(string $name = 'document-approval'): WorkflowBu ->description('Document approval process') ->addStep('submit_document', 'App\\Actions\\SubmitDocumentAction') ->addStep('assign_reviewer', 'App\\Actions\\AssignReviewerAction') - ->email( + ->fakeEmail( template: 'review-request', to: '{{ reviewer.email }}', subject: 'Document Review Request' diff --git a/src/Core/WorkflowDefinition.php b/src/Core/WorkflowDefinition.php index 27313bf..5d37f3e 100644 --- a/src/Core/WorkflowDefinition.php +++ b/src/Core/WorkflowDefinition.php @@ -165,22 +165,47 @@ public function getMetadata(): array */ public function getFirstStep(): ?Step { - // Find step with no incoming transitions + $firstSteps = $this->getFirstSteps(); + + return $firstSteps[0] ?? null; + } + + /** + * Find every entry point of the workflow. + * + * A workflow may legitimately begin with more than one root — several + * independent branches that later converge. Returning only one of them + * would silently drop the others while the workflow still reported success, + * so every step with no incoming transition is treated as a starting point. + * + * @return array All steps with no incoming transitions; falls + * back to the first declared step when the graph + * is entirely cyclic. + */ + public function getFirstSteps(): array + { $stepsWithIncoming = []; foreach ($this->transitions as $transition) { $stepsWithIncoming[] = $transition['to']; } + $roots = []; foreach ($this->steps as $step) { - if (! in_array($step->getId(), $stepsWithIncoming)) { - return $step; + if (! in_array($step->getId(), $stepsWithIncoming, true)) { + $roots[] = $step; } } - // If no step found without incoming transitions, return first step + if ($roots !== []) { + return $roots; + } + + // Every step has an incoming transition (a fully cyclic graph): fall + // back to the first declared step so execution can still begin. $stepsArray = $this->steps; + $first = reset($stepsArray); - return reset($stepsArray) ?: null; + return $first === false ? [] : [$first]; } /** @@ -206,9 +231,7 @@ public function getFirstStep(): ?Step public function getNextSteps(?string $currentStepId, array $data = []): array { if ($currentStepId === null) { - $firstStep = $this->getFirstStep(); - - return $firstStep ? [$firstStep] : []; + return $this->getFirstSteps(); } $nextSteps = []; diff --git a/src/Core/WorkflowEngine.php b/src/Core/WorkflowEngine.php index 4eceb94..2362d76 100644 --- a/src/Core/WorkflowEngine.php +++ b/src/Core/WorkflowEngine.php @@ -115,6 +115,13 @@ public function __construct( */ public function start(string $workflowId, array $definition, array $context = []): string { + // Refuse to clobber an existing instance. IDs are caller-supplied, so a + // collision is a realistic mistake, and silently overwriting would + // discard the earlier run's state and history with no way to notice. + if ($this->storage->exists($workflowId)) { + throw InvalidWorkflowStateException::alreadyExists($workflowId); + } + // Parse definition $workflowDef = $this->parser->parse($definition); @@ -174,6 +181,15 @@ public function resume(string $instanceId): WorkflowInstance throw InvalidWorkflowStateException::cannotResumeCompleted($instanceId); } + if ($instance->getState() === WorkflowState::CANCELLED) { + throw new InvalidWorkflowStateException( + "Cannot resume workflow '{$instanceId}' because it was cancelled", + WorkflowState::CANCELLED, + WorkflowState::RUNNING, + $instanceId + ); + } + $this->executor->execute($instance); return $instance; @@ -251,7 +267,9 @@ public function cancel(string $instanceId, string $reason = ''): WorkflowInstanc { $instance = $this->stateManager->load($instanceId); - if ($instance->getState()->isFinished()) { + // A failed workflow can still be abandoned, so ask the state machine + // rather than assuming every "finished" state is uncancellable. + if (! $instance->getState()->canTransitionTo(WorkflowState::CANCELLED)) { throw new InvalidWorkflowStateException( "Cannot cancel workflow '{$instanceId}' because it is in '{$instance->getState()->value}' state", $instance->getState(), diff --git a/src/Core/WorkflowInstance.php b/src/Core/WorkflowInstance.php index f4f8cb7..6ee007d 100644 --- a/src/Core/WorkflowInstance.php +++ b/src/Core/WorkflowInstance.php @@ -351,6 +351,18 @@ public function isStepCompleted(string $stepId): bool */ public function getNextSteps(): array { + // If the current step never completed — it failed, or execution was + // interrupted — it is still the next thing to do. Following the + // outgoing transitions here would silently skip past the step that + // needs retrying, which is exactly what resuming is meant to fix. + if ($this->currentStepId !== null && ! $this->isStepCompleted($this->currentStepId)) { + $currentStep = $this->definition->getStep($this->currentStepId); + + if ($currentStep !== null) { + return [$currentStep]; + } + } + return $this->definition->getNextSteps($this->currentStepId, $this->data); } diff --git a/src/Core/WorkflowState.php b/src/Core/WorkflowState.php index 3e46678..f7cb434 100644 --- a/src/Core/WorkflowState.php +++ b/src/Core/WorkflowState.php @@ -260,7 +260,12 @@ public function canTransitionTo(self $state): bool // From PAUSED: can resume running, fail, or be cancelled self::PAUSED => in_array($state, [self::RUNNING, self::FAILED, self::CANCELLED]), - // Terminal states cannot transition to other states + // From FAILED: a failed workflow is recoverable. Resuming it puts the + // instance back into RUNNING so the failed step can be retried once the + // underlying cause is fixed; it may also be abandoned outright. + self::FAILED => in_array($state, [self::RUNNING, self::CANCELLED]), + + // COMPLETED and CANCELLED are terminal. default => false, }; } diff --git a/src/Exceptions/InvalidWorkflowStateException.php b/src/Exceptions/InvalidWorkflowStateException.php index 6a4dd44..628d157 100644 --- a/src/Exceptions/InvalidWorkflowStateException.php +++ b/src/Exceptions/InvalidWorkflowStateException.php @@ -175,6 +175,22 @@ public static function cannotCancelFailed(string $instanceId): static ); } + /** + * Create an exception for starting a workflow whose ID is already taken. + * + * @param string $instanceId The workflow instance ID + */ + public static function alreadyExists(string $instanceId): static + { + return new self( + "Cannot start workflow '{$instanceId}' because an instance with that ID already exists. ". + 'Use a unique instance ID, or delete the existing instance first.', + WorkflowState::PENDING, + WorkflowState::PENDING, + $instanceId + ); + } + /** * Create an exception for attempting to start an already running workflow. * diff --git a/src/Storage/InMemoryStorage.php b/src/Storage/InMemoryStorage.php new file mode 100644 index 0000000..5c50508 --- /dev/null +++ b/src/Storage/InMemoryStorage.php @@ -0,0 +1,146 @@ +start('demo', $definition->toArray(), ['user_id' => 1]); + * ``` + */ +class InMemoryStorage implements StorageAdapter +{ + /** @var array Instances keyed by workflow ID */ + private array $instances = []; + + public function save(WorkflowInstance $instance): void + { + $this->instances[$instance->getId()] = $instance; + } + + /** + * @throws WorkflowInstanceNotFoundException If no instance has that ID + */ + public function load(string $id): WorkflowInstance + { + if (! isset($this->instances[$id])) { + throw WorkflowInstanceNotFoundException::notFound($id, self::class); + } + + return $this->instances[$id]; + } + + /** + * Find instances matching simple criteria. + * + * Supported keys: `state` (string or WorkflowState value), `definition_name`, + * `limit` and `offset`. Unknown keys are ignored. + * + * @param array $criteria + * @return array + */ + public function findInstances(array $criteria = []): array + { + $results = array_values($this->instances); + + if (isset($criteria['state'])) { + $state = $criteria['state']; + $wanted = is_string($state) ? $state : ($state->value ?? null); + + $results = array_values(array_filter( + $results, + static fn (WorkflowInstance $i): bool => $i->getState()->value === $wanted + )); + } + + if (isset($criteria['definition_name'])) { + $name = $criteria['definition_name']; + + $results = array_values(array_filter( + $results, + static fn (WorkflowInstance $i): bool => $i->getDefinition()->getName() === $name + )); + } + + $offset = isset($criteria['offset']) ? max(0, (int) $criteria['offset']) : 0; + $limit = isset($criteria['limit']) ? max(0, (int) $criteria['limit']) : null; + + if ($offset > 0 || $limit !== null) { + $results = array_slice($results, $offset, $limit); + } + + return $results; + } + + public function delete(string $id): void + { + unset($this->instances[$id]); + } + + public function exists(string $id): bool + { + return isset($this->instances[$id]); + } + + /** + * Apply partial updates to a stored instance. + * + * Instances are mutable objects held by reference, so the engine's own + * writes are already visible here; this method exists to satisfy the + * contract and to support callers that patch state directly. + * + * @param array $updates + * + * @throws WorkflowInstanceNotFoundException If no instance has that ID + */ + public function updateState(string $id, array $updates): void + { + $instance = $this->load($id); + + if (isset($updates['data']) && is_array($updates['data'])) { + $instance->mergeData($updates['data']); + } + + if (array_key_exists('error_message', $updates)) { + $errorMessage = $updates['error_message']; + $instance->setErrorMessage(is_string($errorMessage) ? $errorMessage : null); + } + + if (isset($updates['current_step_id'])) { + $instance->setCurrentStepId((string) $updates['current_step_id']); + } + + $this->save($instance); + } + + /** + * Remove every stored instance. + */ + public function flush(): void + { + $this->instances = []; + } + + /** + * Count the stored instances. + */ + public function count(): int + { + return count($this->instances); + } +} diff --git a/src/Support/Arr.php b/src/Support/Arr.php index 3efd3a3..ba30368 100644 --- a/src/Support/Arr.php +++ b/src/Support/Arr.php @@ -25,6 +25,32 @@ public static function get(array $array, string $key, mixed $default = null): mi return $array; } + /** + * Determine whether a nested key exists, using dot notation. + * + * Distinguishes "the key is absent" from "the key holds null", which + * `get()` alone cannot express. + * + * @param array $array + */ + public static function has(array $array, string $key): bool + { + if (array_key_exists($key, $array)) { + return true; + } + + $current = $array; + + foreach (explode('.', $key) as $segment) { + if (! is_array($current) || ! array_key_exists($segment, $current)) { + return false; + } + $current = $current[$segment]; + } + + return true; + } + /** * Get the class "basename" of a class string (without namespace). */ diff --git a/src/Support/ConditionEvaluator.php b/src/Support/ConditionEvaluator.php index 18ee24b..fca7bc7 100644 --- a/src/Support/ConditionEvaluator.php +++ b/src/Support/ConditionEvaluator.php @@ -4,21 +4,53 @@ use SolutionForest\WorkflowEngine\Exceptions\InvalidWorkflowDefinitionException; +/** + * Evaluates workflow condition expressions against workflow data. + * + * The grammar is deliberately small and is *not* PHP code — expressions are + * parsed, never `eval()`d: + * + * ``` + * expression := or_expression + * or_expression := and_expression ( "||" and_expression )* + * and_expression := term ( "&&" term )* + * term := "(" expression ")" | comparison | truthy + * comparison := key operator literal + * truthy := [ "!" ] key + * key := word ( "." word )* + * operator := "===" | "!==" | ">=" | "<=" | "==" | "!=" | ">" | "<" + * literal := "true" | "false" | "null" | quoted-string | number | bare-word + * ``` + * + * Anything outside this grammar throws InvalidWorkflowDefinitionException. + * The evaluator never guesses: a malformed expression is a definition bug and + * is surfaced loudly rather than silently collapsing to a boolean. + * + * @example + * ```php + * ConditionEvaluator::evaluate('order.total > 1000', ['order' => ['total' => 1500]]); // true + * ConditionEvaluator::evaluate('user.active && user.plan === "premium"', $data); + * ConditionEvaluator::evaluate('a.b > 1 || (c === "x" && !d)', $data); + * ``` + */ final class ConditionEvaluator { /** - * Evaluate a condition expression against workflow data. + * Comparison operators, longest-first so that "===" wins over "==" and + * ">=" over ">" when scanning. * - * Supports two forms: - * - Comparison: "key operator value" where operator is one of ===, !==, >=, <=, ==, !=, >, < - * and value is a boolean, null, integer, float, or quoted string literal. - * - Truthy key: "key" or "!key" to check whether the dotted key is truthy/falsy. + * @var array + */ + private const OPERATORS = ['===', '!==', '>=', '<=', '==', '!=', '>', '<']; + + /** + * Evaluate a condition expression against the supplied workflow data. * - * @param string $condition Condition expression (e.g., "user.plan === 'premium'") + * @param string $condition The condition expression * @param array $data Workflow data to evaluate against - * @return bool True if condition evaluates to true + * @return bool The result of the expression * - * @throws InvalidWorkflowDefinitionException If condition format is invalid + * @throws InvalidWorkflowDefinitionException If the expression is malformed */ public static function evaluate(string $condition, array $data): bool { @@ -31,15 +63,183 @@ public static function evaluate(string $condition, array $data): bool ); } - // Comparison form: "key operator value". - if (preg_match('/^(\w+(?:\.\w+)*)\s*(===|!==|>=|<=|==|!=|>|<)\s*(.+)$/', $trimmed, $matches)) { + return self::evaluateExpression($trimmed, $data, $condition); + } + + /** + * Evaluate a full expression, honouring `||` (lowest precedence) then `&&`. + * + * @param array $data + * + * @throws InvalidWorkflowDefinitionException + */ + private static function evaluateExpression(string $expression, array $data, string $original): bool + { + $expression = trim($expression); + + if ($expression === '') { + throw InvalidWorkflowDefinitionException::invalidCondition( + $original, + 'Encountered an empty sub-expression; check for a dangling "&&" or "||".' + ); + } + + // `||` binds loosest, so split on it first: the operands are then + // whole `&&` chains. + $orParts = self::splitOnOperator($expression, '||'); + if (count($orParts) > 1) { + foreach ($orParts as $part) { + if (self::evaluateExpression($part, $data, $original)) { + return true; // Short-circuit. + } + } + + return false; + } + + $andParts = self::splitOnOperator($expression, '&&'); + if (count($andParts) > 1) { + foreach ($andParts as $part) { + if (! self::evaluateExpression($part, $data, $original)) { + return false; // Short-circuit. + } + } + + return true; + } + + return self::evaluateTerm($expression, $data, $original); + } + + /** + * Split an expression on a boolean operator at paren depth zero, ignoring + * occurrences inside quoted strings. + * + * @return array The operands; a single-element array means the + * operator was not present at the top level. + * + * @throws InvalidWorkflowDefinitionException If parentheses are unbalanced + */ + private static function splitOnOperator(string $expression, string $operator): array + { + $parts = []; + $current = ''; + $depth = 0; + $quote = null; + $length = strlen($expression); + $operatorLength = strlen($operator); + + for ($i = 0; $i < $length; $i++) { + $char = $expression[$i]; + + if ($quote !== null) { + $current .= $char; + if ($char === $quote) { + $quote = null; + } + + continue; + } + + if ($char === '"' || $char === "'") { + $quote = $char; + $current .= $char; + + continue; + } + + if ($char === '(') { + $depth++; + $current .= $char; + + continue; + } + + if ($char === ')') { + $depth--; + if ($depth < 0) { + throw InvalidWorkflowDefinitionException::invalidCondition( + $expression, + 'Unbalanced parentheses: unexpected ")".' + ); + } + $current .= $char; + + continue; + } + + if ($depth === 0 && substr($expression, $i, $operatorLength) === $operator) { + $parts[] = $current; + $current = ''; + $i += $operatorLength - 1; + + continue; + } + + $current .= $char; + } + + if ($quote !== null) { + throw InvalidWorkflowDefinitionException::invalidCondition( + $expression, + 'Unterminated quoted string.' + ); + } + + if ($depth !== 0) { + throw InvalidWorkflowDefinitionException::invalidCondition( + $expression, + 'Unbalanced parentheses: missing ")".' + ); + } + + $parts[] = $current; + + return $parts; + } + + /** + * Evaluate a single term: a parenthesised expression, a comparison, or a + * truthy key check. + * + * @param array $data + * + * @throws InvalidWorkflowDefinitionException + */ + private static function evaluateTerm(string $term, array $data, string $original): bool + { + $term = trim($term); + + // Parenthesised sub-expression — only when the leading "(" actually + // closes at the very end, so "(a) && (b)" isn't mistaken for a group. + if (str_starts_with($term, '(') && self::closesAtEnd($term)) { + return self::evaluateExpression(substr($term, 1, -1), $data, $original); + } + + // Negated group: !(...) + if (str_starts_with($term, '!(')) { + $inner = substr($term, 1); + if (self::closesAtEnd($inner)) { + return ! self::evaluateExpression(substr($inner, 1, -1), $data, $original); + } + } + + // Comparison form: "key operator literal". + if (preg_match('/^(\w+(?:\.\w+)*)\s*(===|!==|>=|<=|==|!=|>|<)\s*(.+)$/s', $term, $matches)) { $key = $matches[1]; $operator = $matches[2]; $rawValue = trim($matches[3]); - $value = self::parseLiteral($rawValue, $condition); + $value = self::parseLiteral($rawValue, $original); $dataValue = Arr::get($data, $key); + // Relational operators against a missing key are meaningless: PHP + // would coerce null to 0/"" and quietly report that `missing < 10` + // is true. A condition on data that isn't there has not been met. + if (self::isRelational($operator) && ! Arr::has($data, $key)) { + return false; + } + return match ($operator) { '===' => $dataValue === $value, '!==' => $dataValue !== $value, @@ -49,12 +249,11 @@ public static function evaluate(string $condition, array $data): bool '!=' => $dataValue != $value, '>' => $dataValue > $value, '<' => $dataValue < $value, - default => false, }; } // Truthy key form: "key" or "!key". - if (preg_match('/^(!?)(\w+(?:\.\w+)*)$/', $trimmed, $matches)) { + if (preg_match('/^(!?)(\w+(?:\.\w+)*)$/', $term, $matches)) { $negate = $matches[1] === '!'; $dataValue = Arr::get($data, $matches[2]); @@ -62,16 +261,80 @@ public static function evaluate(string $condition, array $data): bool } throw InvalidWorkflowDefinitionException::invalidCondition( - $condition, - 'Condition must be a truthy key (e.g. "user.active") or "key operator value" (e.g. "user.plan === \'premium\'").' + $original, + sprintf( + 'Could not parse "%s". A term must be a truthy key (e.g. "user.active"), '. + 'a comparison (e.g. "user.plan === \'premium\'"), or a parenthesised group. '. + 'Combine terms with "&&" and "||".', + $term + ) ); } /** - * Parse a literal value from its string form into a typed PHP value. + * Determine whether a leading "(" is closed by the final character, i.e. + * the whole term is one parenthesised group. + */ + private static function closesAtEnd(string $term): bool + { + if (! str_starts_with($term, '(') || ! str_ends_with($term, ')')) { + return false; + } + + $depth = 0; + $quote = null; + $length = strlen($term); + + for ($i = 0; $i < $length; $i++) { + $char = $term[$i]; + + if ($quote !== null) { + if ($char === $quote) { + $quote = null; + } + + continue; + } + + if ($char === '"' || $char === "'") { + $quote = $char; + + continue; + } + + if ($char === '(') { + $depth++; + } elseif ($char === ')') { + $depth--; + + // Closed before the end: this is "(a) && (b)", not a group. + if ($depth === 0 && $i !== $length - 1) { + return false; + } + } + } + + return $depth === 0; + } + + /** + * Whether an operator compares magnitude (as opposed to equality). + */ + private static function isRelational(string $operator): bool + { + return in_array($operator, ['>', '<', '>=', '<='], true); + } + + /** + * Parse the right-hand side of a comparison into a PHP value. + * + * Bare words are accepted as strings for convenience ("status == active"), + * but anything that is neither a recognised literal nor a single bare word + * is rejected. That rejection is what stops an unsupported expression such + * as "total > 1000 && vip" from being silently treated as a string + * comparison against the literal "1000 && vip". * - * Supports: true/false, null, integers, floats, and quoted strings. - * Unquoted identifiers are returned as strings for backwards compatibility. + * @throws InvalidWorkflowDefinitionException */ private static function parseLiteral(string $raw, string $condition): mixed { @@ -83,23 +346,32 @@ private static function parseLiteral(string $raw, string $condition): mixed } $lower = strtolower($raw); + if ($lower === 'true') { return true; } + if ($lower === 'false') { return false; } + if ($lower === 'null') { return null; } - // Quoted strings. + // Quoted strings — the quote must close at the very end, otherwise + // "'a' && b" would be read as a single string value. $len = strlen($raw); if ($len >= 2) { $first = $raw[0]; $last = $raw[$len - 1]; + if (($first === '"' && $last === '"') || ($first === "'" && $last === "'")) { - return substr($raw, 1, -1); + $inner = substr($raw, 1, -1); + + if (! str_contains($inner, $first)) { + return $inner; + } } } @@ -107,11 +379,38 @@ private static function parseLiteral(string $raw, string $condition): mixed if (preg_match('/^-?\d+$/', $raw)) { return (int) $raw; } + if (preg_match('/^-?\d+\.\d+$/', $raw)) { return (float) $raw; } - // Fallback: treat as an unquoted string (backwards compatible). - return $raw; + // Bare word (an unquoted string such as "active" or "in_progress"). + if (preg_match('/^[A-Za-z_][A-Za-z0-9_.-]*$/', $raw)) { + return $raw; + } + + foreach (self::OPERATORS as $operator) { + if (str_contains($raw, $operator)) { + throw InvalidWorkflowDefinitionException::invalidCondition( + $condition, + sprintf( + 'Right-hand side "%s" contains the operator "%s". '. + 'Chained comparisons are not supported — combine separate '. + 'comparisons with "&&" or "||" instead.', + $raw, + $operator + ) + ); + } + } + + throw InvalidWorkflowDefinitionException::invalidCondition( + $condition, + sprintf( + 'Could not parse "%s" as a value. Expected true, false, null, a number, '. + 'a quoted string, or a bare word.', + $raw + ) + ); } } diff --git a/tests/Integration/EventDispatchTest.php b/tests/Integration/EventDispatchTest.php index 02390f5..0b19ad1 100644 --- a/tests/Integration/EventDispatchTest.php +++ b/tests/Integration/EventDispatchTest.php @@ -4,7 +4,7 @@ use SolutionForest\WorkflowEngine\Core\WorkflowEngine; use SolutionForest\WorkflowEngine\Core\WorkflowInstance; use SolutionForest\WorkflowEngine\Core\WorkflowState; -use SolutionForest\WorkflowEngine\Tests\Support\InMemoryStorage; +use SolutionForest\WorkflowEngine\Storage\InMemoryStorage; use SolutionForest\WorkflowEngine\Tests\Support\SpyEventDispatcher; describe('Event Dispatching', function () { diff --git a/tests/Support/InMemoryStorage.php b/tests/Support/InMemoryStorage.php deleted file mode 100644 index df2fb68..0000000 --- a/tests/Support/InMemoryStorage.php +++ /dev/null @@ -1,67 +0,0 @@ -instances[$instance->getId()] = $instance; - } - - public function load(string $id): WorkflowInstance - { - if (! isset($this->instances[$id])) { - throw new \InvalidArgumentException("Workflow instance not found: {$id}"); - } - - return $this->instances[$id]; - } - - public function findInstances(array $criteria = []): array - { - if (empty($criteria)) { - return array_values($this->instances); - } - - $filtered = array_filter($this->instances, function ($instance) use ($criteria) { - foreach ($criteria as $key => $value) { - // Simple implementation for basic filtering - if ($key === 'state' && $instance->getState()->value !== $value) { - return false; - } - } - - return true; - }); - - return array_values($filtered); - } - - public function delete(string $id): void - { - unset($this->instances[$id]); - } - - public function exists(string $id): bool - { - return isset($this->instances[$id]); - } - - public function updateState(string $id, array $updates): void - { - if (! isset($this->instances[$id])) { - throw new \InvalidArgumentException("Workflow instance not found: {$id}"); - } - - // Simple update implementation - // In a real implementation, this would update specific fields - $instance = $this->instances[$id]; - $this->instances[$id] = $instance; - } -} diff --git a/tests/TestCase.php b/tests/TestCase.php index c1b46bf..1f6b603 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -4,7 +4,7 @@ use PHPUnit\Framework\TestCase as PHPUnitTestCase; use SolutionForest\WorkflowEngine\Core\WorkflowEngine; -use SolutionForest\WorkflowEngine\Tests\Support\InMemoryStorage; +use SolutionForest\WorkflowEngine\Storage\InMemoryStorage; class TestCase extends PHPUnitTestCase { diff --git a/tests/Unit/BuilderExecutionTest.php b/tests/Unit/BuilderExecutionTest.php index 3f1585c..1e1e2e9 100644 --- a/tests/Unit/BuilderExecutionTest.php +++ b/tests/Unit/BuilderExecutionTest.php @@ -6,7 +6,7 @@ use SolutionForest\WorkflowEngine\Core\WorkflowContext; use SolutionForest\WorkflowEngine\Core\WorkflowEngine; use SolutionForest\WorkflowEngine\Core\WorkflowState; -use SolutionForest\WorkflowEngine\Tests\Support\InMemoryStorage; +use SolutionForest\WorkflowEngine\Storage\InMemoryStorage; /** * Test action that records every execution so tests can assert the exact diff --git a/tests/Unit/BuiltInActionsTest.php b/tests/Unit/BuiltInActionsTest.php new file mode 100644 index 0000000..61fc606 --- /dev/null +++ b/tests/Unit/BuiltInActionsTest.php @@ -0,0 +1,162 @@ + 'sent', so a + * workflow could show a delivered confirmation that did not exist. + */ + test('never claims an email was sent', function () { + $action = new FakeEmailAction(['to' => 'user@example.com']); + + $result = $action->execute(contextWith([ + 'to' => 'user@example.com', + 'subject' => 'Hello', + 'template' => 'welcome', + ])); + + expect($result->isSuccess())->toBeTrue(); + expect($result->getData()['email']['sent'])->toBeFalse(); + expect($result->getData()['email']['mock'])->toBeTrue(); + }); + + test('records what would have been sent', function () { + $action = new FakeEmailAction; + + $result = $action->execute(contextWith([ + 'to' => 'user@example.com', + 'subject' => 'Order Confirmed', + 'template' => 'order', + ])); + + $email = $result->getData()['email']; + + expect($email['to'])->toBe('user@example.com'); + expect($email['subject'])->toBe('Order Confirmed'); + expect($email['template'])->toBe('order'); + }); + + test('requires a recipient', function () { + $action = new FakeEmailAction; + + expect($action->canExecute(contextWith([])))->toBeFalse(); + expect($action->canExecute(contextWith(['to' => 'a@b.com'])))->toBeTrue(); + }); + +}); + +describe('DelayAction units', function () { + + /** + * `minutes` and `hours` were documented but never read, so delay(hours: 2) + * silently fell through to the one second default. + */ + test('reports the delay in the unit it was given', function () { + $action = new DelayAction(['microseconds' => 1000]); + + $result = $action->execute(contextWith(['microseconds' => 1000])); + + expect($result->isSuccess())->toBeTrue(); + expect($result->getData()['delayed_microseconds'])->toBe(1000); + }); + + test('minutes are honoured rather than ignored', function () { + // Assert the computed duration without actually sleeping for a minute: + // a zero-valued minutes key still proves the unit is read. + $action = new DelayAction(['minutes' => 0]); + + $result = $action->execute(contextWith(['minutes' => 0])); + + expect($result->isSuccess())->toBeTrue(); + expect($result->getData()['delayed_microseconds'])->toBe(0); + }); + + test('hours are honoured rather than ignored', function () { + $action = new DelayAction(['hours' => 0]); + + $result = $action->execute(contextWith(['hours' => 0])); + + expect($result->isSuccess())->toBeTrue(); + expect($result->getData()['delayed_microseconds'])->toBe(0); + }); + + test('units combine', function () { + $action = new DelayAction(['hours' => 0, 'minutes' => 0, 'microseconds' => 500]); + + $result = $action->execute(contextWith(['hours' => 0, 'minutes' => 0, 'microseconds' => 500])); + + expect($result->getData()['delayed_microseconds'])->toBe(500); + }); + + test('rejects a negative duration', function () { + $action = new DelayAction(['seconds' => -1]); + + $result = $action->execute(contextWith(['seconds' => -1])); + + expect($result->isSuccess())->toBeFalse(); + }); + +}); + +describe('ConditionAction', function () { + + /** + * This action used to carry its own parser accepting "=" and "is", which no + * other part of the engine understood. It now shares one grammar. + */ + test('uses the shared condition grammar', function () { + $action = new ConditionAction(['condition' => 'order.total > 1000']); + + $result = $action->execute(contextWith( + ['condition' => 'order.total > 1000'], + ['order' => ['total' => 1500]] + )); + + expect($result->isSuccess())->toBeTrue(); + expect($result->getData()['result'])->toBeTrue(); + }); + + test('supports boolean operators like every other condition', function () { + $condition = 'order.total > 1000 && order.vip === true'; + $action = new ConditionAction(['condition' => $condition]); + + $result = $action->execute(contextWith( + ['condition' => $condition], + ['order' => ['total' => 500, 'vip' => true]] + )); + + expect($result->getData()['result'])->toBeFalse(); + }); + + test('fails cleanly on a malformed condition', function () { + $action = new ConditionAction(['condition' => 'order.total > ']); + + $result = $action->execute(contextWith(['condition' => 'order.total > '])); + + expect($result->isSuccess())->toBeFalse(); + }); + + test('requires a condition', function () { + $action = new ConditionAction([]); + + $result = $action->execute(contextWith([])); + + expect($result->isSuccess())->toBeFalse(); + }); + +}); diff --git a/tests/Unit/ExecutorIterationTest.php b/tests/Unit/ExecutorIterationTest.php index 0bf9119..8ff294b 100644 --- a/tests/Unit/ExecutorIterationTest.php +++ b/tests/Unit/ExecutorIterationTest.php @@ -6,7 +6,7 @@ use SolutionForest\WorkflowEngine\Core\WorkflowContext; use SolutionForest\WorkflowEngine\Core\WorkflowEngine; use SolutionForest\WorkflowEngine\Core\WorkflowState; -use SolutionForest\WorkflowEngine\Tests\Support\InMemoryStorage; +use SolutionForest\WorkflowEngine\Storage\InMemoryStorage; /** * Records every execution so tests can assert execution order regardless of diff --git a/tests/Unit/ExecutorRetryTest.php b/tests/Unit/ExecutorRetryTest.php index 770ed97..2ce3a94 100644 --- a/tests/Unit/ExecutorRetryTest.php +++ b/tests/Unit/ExecutorRetryTest.php @@ -6,7 +6,7 @@ use SolutionForest\WorkflowEngine\Core\WorkflowContext; use SolutionForest\WorkflowEngine\Core\WorkflowEngine; use SolutionForest\WorkflowEngine\Exceptions\StepExecutionException; -use SolutionForest\WorkflowEngine\Tests\Support\InMemoryStorage; +use SolutionForest\WorkflowEngine\Storage\InMemoryStorage; use SolutionForest\WorkflowEngine\Tests\Support\SpyEventDispatcher; // A test action that fails N times then succeeds diff --git a/tests/Unit/PHP83FeaturesTest.php b/tests/Unit/PHP83FeaturesTest.php index 34b80f8..429fb5f 100644 --- a/tests/Unit/PHP83FeaturesTest.php +++ b/tests/Unit/PHP83FeaturesTest.php @@ -23,7 +23,7 @@ ->version('2.0') ->startWith(LogAction::class, ['message' => 'Starting workflow']) ->then(DelayAction::class, ['seconds' => 1]) - ->email( + ->fakeEmail( template: 'test', to: '{{ user.email }}', subject: 'Test Email' @@ -63,10 +63,12 @@ expect(WorkflowState::PENDING->canTransitionTo(WorkflowState::RUNNING))->toBeTrue(); expect(WorkflowState::RUNNING->canTransitionTo(WorkflowState::COMPLETED))->toBeTrue(); expect(WorkflowState::RUNNING->canTransitionTo(WorkflowState::FAILED))->toBeTrue(); + // FAILED is recoverable: resuming a failed workflow retries it. + expect(WorkflowState::FAILED->canTransitionTo(WorkflowState::RUNNING))->toBeTrue(); // Test invalid transitions expect(WorkflowState::COMPLETED->canTransitionTo(WorkflowState::RUNNING))->toBeFalse(); - expect(WorkflowState::FAILED->canTransitionTo(WorkflowState::RUNNING))->toBeFalse(); + expect(WorkflowState::FAILED->canTransitionTo(WorkflowState::COMPLETED))->toBeFalse(); expect(WorkflowState::CANCELLED->canTransitionTo(WorkflowState::RUNNING))->toBeFalse(); }); @@ -91,7 +93,7 @@ it('can create workflow with common patterns using helper methods', function () { $workflow = WorkflowBuilder::create('helper-test') - ->email( + ->fakeEmail( template: 'welcome', to: 'user@example.com', subject: 'Welcome!' @@ -109,7 +111,7 @@ expect($steps)->toHaveCount(4); // Check email step - expect($steps[0]->getActionClass())->toBe('SolutionForest\\WorkflowEngine\\Actions\\EmailAction'); + expect($steps[0]->getActionClass())->toBe('SolutionForest\\WorkflowEngine\\Actions\\FakeEmailAction'); expect($steps[0]->getConfig()['template'])->toBe('welcome'); // Check delay step @@ -140,7 +142,7 @@ $workflow = WorkflowBuilder::create(name: 'named-args-test') ->description(description: 'Testing named arguments') ->version(version: '1.0') - ->email( + ->fakeEmail( template: 'test', to: 'test@example.com', subject: 'Test Subject', diff --git a/tests/Unit/Support/ConditionEvaluatorBooleanTest.php b/tests/Unit/Support/ConditionEvaluatorBooleanTest.php new file mode 100644 index 0000000..fca710d --- /dev/null +++ b/tests/Unit/Support/ConditionEvaluatorBooleanTest.php @@ -0,0 +1,110 @@ + ['total' => 1500, 'vip' => true]]; + + expect(ConditionEvaluator::evaluate('order.total > 1000 && order.vip === true', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('order.total > 2000 && order.vip === true', $data))->toBeFalse(); + expect(ConditionEvaluator::evaluate('order.total > 1000 && order.vip === false', $data))->toBeFalse(); + }); + + /** + * The regression this whole grammar exists for: the old evaluator swallowed + * "&& order.vip === true" into the right-hand side and string-compared + * "500" against "1000 && ...", which reported TRUE for a 500 dollar order. + */ + test('&& does not silently pass when the first comparison fails', function () { + $data = ['order' => ['total' => 500, 'vip' => true]]; + + expect(ConditionEvaluator::evaluate('order.total > 1000 && order.vip === true', $data))->toBeFalse(); + }); + + test('|| requires only one operand to hold', function () { + $data = ['order' => ['total' => 500, 'vip' => true]]; + + expect(ConditionEvaluator::evaluate('order.total > 1000 || order.vip === true', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('order.total > 1000 || order.vip === false', $data))->toBeFalse(); + }); + + test('&& binds tighter than ||', function () { + // false && false || true => (false && false) || true => true + $data = ['a' => false, 'b' => false, 'c' => true]; + + expect(ConditionEvaluator::evaluate('a && b || c', $data))->toBeTrue(); + }); + + test('parentheses override precedence', function () { + // a && (b || c) => false && true => false + $data = ['a' => false, 'b' => false, 'c' => true]; + + expect(ConditionEvaluator::evaluate('a && (b || c)', $data))->toBeFalse(); + expect(ConditionEvaluator::evaluate('(a || c) && c', $data))->toBeTrue(); + }); + + test('negation applies to groups', function () { + $data = ['a' => false, 'c' => true]; + + expect(ConditionEvaluator::evaluate('!(a && c)', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('!(a || c)', $data))->toBeFalse(); + }); + + test('boolean operators inside quoted strings are not treated as operators', function () { + $data = ['label' => 'fish && chips']; + + expect(ConditionEvaluator::evaluate('label === "fish && chips"', $data))->toBeTrue(); + }); + + test('short-circuits without evaluating the rest', function () { + // The right operand references a key that is absent; && must not need it. + $data = ['enabled' => false]; + + expect(ConditionEvaluator::evaluate('enabled && missing.key === "x"', $data))->toBeFalse(); + }); + + test('rejects a dangling operator', function () { + expect(fn () => ConditionEvaluator::evaluate('a && ', ['a' => true])) + ->toThrow(InvalidWorkflowDefinitionException::class); + }); + + test('rejects unbalanced parentheses', function () { + expect(fn () => ConditionEvaluator::evaluate('(a && b', ['a' => true, 'b' => true])) + ->toThrow(InvalidWorkflowDefinitionException::class); + }); + + test('rejects a chained comparison rather than guessing', function () { + expect(fn () => ConditionEvaluator::evaluate('a > 1 > 2', ['a' => 5])) + ->toThrow(InvalidWorkflowDefinitionException::class); + }); + +}); + +describe('ConditionEvaluator missing keys', function () { + + /** + * null coerces to 0/"" in PHP, so the old evaluator reported that an absent + * key was less than 1000 — quietly running steps gated on data that was + * never set. + */ + test('relational comparisons against an absent key are false', function () { + expect(ConditionEvaluator::evaluate('missing.key < 1000', []))->toBeFalse(); + expect(ConditionEvaluator::evaluate('missing.key > 1000', []))->toBeFalse(); + expect(ConditionEvaluator::evaluate('missing.key >= 0', []))->toBeFalse(); + expect(ConditionEvaluator::evaluate('missing.key <= 0', []))->toBeFalse(); + }); + + test('a key explicitly set to a value still compares normally', function () { + expect(ConditionEvaluator::evaluate('order.total < 1000', ['order' => ['total' => 500]]))->toBeTrue(); + expect(ConditionEvaluator::evaluate('order.total >= 500', ['order' => ['total' => 500]]))->toBeTrue(); + }); + + test('equality against an absent key still works', function () { + expect(ConditionEvaluator::evaluate('missing.key === null', []))->toBeTrue(); + expect(ConditionEvaluator::evaluate('missing.key !== "x"', []))->toBeTrue(); + }); + +}); diff --git a/tests/Unit/WorkflowBuilderAutoIdTest.php b/tests/Unit/WorkflowBuilderAutoIdTest.php index 21f5281..b4f383c 100644 --- a/tests/Unit/WorkflowBuilderAutoIdTest.php +++ b/tests/Unit/WorkflowBuilderAutoIdTest.php @@ -23,8 +23,8 @@ test('email/delay/http/condition sugar methods use monotonic counters', function () { $definition = WorkflowBuilder::create('sugar-ids') - ->email('welcome', 'user@example.com', 'Hi') - ->email('followup', 'user@example.com', 'Hi again') + ->fakeEmail('welcome', 'user@example.com', 'Hi') + ->fakeEmail('followup', 'user@example.com', 'Hi again') ->delay(seconds: 30) ->delay(seconds: 60) ->build(); diff --git a/tests/Unit/WorkflowEngineTest.php b/tests/Unit/WorkflowEngineTest.php index 3719a79..7b2b293 100644 --- a/tests/Unit/WorkflowEngineTest.php +++ b/tests/Unit/WorkflowEngineTest.php @@ -6,7 +6,7 @@ use SolutionForest\WorkflowEngine\Core\WorkflowState; use SolutionForest\WorkflowEngine\Exceptions\InvalidWorkflowDefinitionException; use SolutionForest\WorkflowEngine\Exceptions\WorkflowInstanceNotFoundException; -use SolutionForest\WorkflowEngine\Tests\Support\InMemoryStorage; +use SolutionForest\WorkflowEngine\Storage\InMemoryStorage; beforeEach(function () { $this->storage = new InMemoryStorage; diff --git a/tests/Unit/WorkflowReachabilityTest.php b/tests/Unit/WorkflowReachabilityTest.php new file mode 100644 index 0000000..4627bb7 --- /dev/null +++ b/tests/Unit/WorkflowReachabilityTest.php @@ -0,0 +1,230 @@ +engine = new WorkflowEngine(new InMemoryStorage); + $this->parser = new DefinitionParser; +}); + +describe('multi-root workflows', function () { + + /** + * getFirstStep() used to return a single root, so a second independent + * entry point was never executed — yet the workflow still reported + * COMPLETED, with progress silently stuck below 100%. + */ + test('every entry point runs', function () { + $definition = [ + 'name' => 'Multi Root', + 'steps' => [ + ['id' => 'root_a', 'action' => LogAction::class, 'config' => ['message' => 'a']], + ['id' => 'root_b', 'action' => LogAction::class, 'config' => ['message' => 'b']], + ['id' => 'join', 'action' => LogAction::class, 'config' => ['message' => 'join']], + ], + 'transitions' => [ + ['from' => 'root_a', 'to' => 'join'], + ['from' => 'root_b', 'to' => 'join'], + ], + ]; + + $this->engine->start('multi', $definition); + $instance = $this->engine->getInstance('multi'); + + expect($instance->getState())->toBe(WorkflowState::COMPLETED); + expect($instance->getCompletedSteps())->toContain('root_a', 'root_b', 'join'); + expect($instance->getProgress())->toBe(100.0); + }); + + test('getFirstSteps reports all roots', function () { + $definition = $this->parser->parse([ + 'name' => 'Multi Root', + 'steps' => [ + ['id' => 'root_a', 'action' => LogAction::class], + ['id' => 'root_b', 'action' => LogAction::class], + ['id' => 'join', 'action' => LogAction::class], + ], + 'transitions' => [ + ['from' => 'root_a', 'to' => 'join'], + ['from' => 'root_b', 'to' => 'join'], + ], + ]); + + $ids = array_map(fn ($step) => $step->getId(), $definition->getFirstSteps()); + + expect($ids)->toEqualCanonicalizing(['root_a', 'root_b']); + }); + + test('a single-root workflow still reports one entry point', function () { + $definition = $this->parser->parse([ + 'name' => 'Linear', + 'steps' => [ + ['id' => 'one', 'action' => LogAction::class], + ['id' => 'two', 'action' => LogAction::class], + ], + 'transitions' => [ + ['from' => 'one', 'to' => 'two'], + ], + ]); + + expect($definition->getFirstSteps())->toHaveCount(1); + expect($definition->getFirstStep()->getId())->toBe('one'); + }); + +}); + +describe('unreachable step validation', function () { + + /** + * A step with no incoming transition is an entry point, so it still runs. + * What genuinely cannot be reached is a group of steps that only point at + * each other: every one of them has an incoming transition, so none is a + * root, and nothing outside the group leads in. + */ + test('an isolated cycle is rejected at parse time', function () { + expect(fn () => $this->parser->parse([ + 'name' => 'Island', + 'steps' => [ + ['id' => 'start', 'action' => LogAction::class], + ['id' => 'finish', 'action' => LogAction::class], + ['id' => 'island_a', 'action' => LogAction::class], + ['id' => 'island_b', 'action' => LogAction::class], + ], + 'transitions' => [ + ['from' => 'start', 'to' => 'finish'], + ['from' => 'island_a', 'to' => 'island_b'], + ['from' => 'island_b', 'to' => 'island_a'], + ], + ]))->toThrow(InvalidWorkflowDefinitionException::class, 'unreachable'); + }); + + test('the error names the unreachable steps', function () { + try { + $this->parser->parse([ + 'name' => 'Island', + 'steps' => [ + ['id' => 'start', 'action' => LogAction::class], + ['id' => 'finish', 'action' => LogAction::class], + ['id' => 'lost_a', 'action' => LogAction::class], + ['id' => 'lost_b', 'action' => LogAction::class], + ], + 'transitions' => [ + ['from' => 'start', 'to' => 'finish'], + ['from' => 'lost_a', 'to' => 'lost_b'], + ['from' => 'lost_b', 'to' => 'lost_a'], + ], + ]); + + $this->fail('Expected InvalidWorkflowDefinitionException'); + } catch (InvalidWorkflowDefinitionException $e) { + expect($e->getMessage())->toContain("'lost_a'"); + expect($e->getMessage())->toContain("'lost_b'"); + } + }); + + test('a standalone step with no transitions is an entry point, not an orphan', function () { + $definition = $this->parser->parse([ + 'name' => 'Parallel Branch', + 'steps' => [ + ['id' => 'start', 'action' => LogAction::class], + ['id' => 'middle', 'action' => LogAction::class], + ['id' => 'standalone', 'action' => LogAction::class], + ], + 'transitions' => [ + ['from' => 'start', 'to' => 'middle'], + ], + ]); + + $ids = array_map(fn ($step) => $step->getId(), $definition->getFirstSteps()); + + expect($ids)->toEqualCanonicalizing(['start', 'standalone']); + }); + + test('a converging graph is reachable and accepted', function () { + $definition = $this->parser->parse([ + 'name' => 'Diamond', + 'steps' => [ + ['id' => 'start', 'action' => LogAction::class], + ['id' => 'left', 'action' => LogAction::class], + ['id' => 'right', 'action' => LogAction::class], + ['id' => 'end', 'action' => LogAction::class], + ], + 'transitions' => [ + ['from' => 'start', 'to' => 'left'], + ['from' => 'start', 'to' => 'right'], + ['from' => 'left', 'to' => 'end'], + ['from' => 'right', 'to' => 'end'], + ], + ]); + + expect($definition->getSteps())->toHaveCount(4); + }); + + test('multiple roots are all treated as reachable', function () { + $definition = $this->parser->parse([ + 'name' => 'Two Roots', + 'steps' => [ + ['id' => 'root_a', 'action' => LogAction::class], + ['id' => 'root_b', 'action' => LogAction::class], + ['id' => 'join', 'action' => LogAction::class], + ], + 'transitions' => [ + ['from' => 'root_a', 'to' => 'join'], + ['from' => 'root_b', 'to' => 'join'], + ], + ]); + + expect($definition->getSteps())->toHaveCount(3); + }); + + test('a workflow without transitions is not subject to the check', function () { + $definition = $this->parser->parse([ + 'name' => 'Single', + 'steps' => [ + ['id' => 'only', 'action' => LogAction::class], + ], + ]); + + expect($definition->getSteps())->toHaveCount(1); + }); + +}); + +describe('blocked workflows', function () { + + /** + * A step gated on a prerequisite that never completes can never run. The + * executor used to return leaving the instance in RUNNING, where a + * permanently stuck workflow looks exactly like one still in flight. + */ + test('a workflow blocked on an unmet prerequisite parks in WAITING', function () { + $definition = [ + 'name' => 'Blocked', + 'steps' => [ + ['id' => 'first', 'action' => LogAction::class, 'config' => ['message' => 'go']], + [ + 'id' => 'second', + 'action' => LogAction::class, + 'config' => ['message' => 'never'], + 'prerequisites' => ['first', 'absent_step'], + ], + ], + 'transitions' => [ + ['from' => 'first', 'to' => 'second'], + ], + ]; + + $this->engine->start('blocked', $definition); + $instance = $this->engine->getInstance('blocked'); + + expect($instance->getState())->toBe(WorkflowState::WAITING); + expect($instance->getCompletedSteps())->toContain('first'); + expect($instance->getCompletedSteps())->not->toContain('second'); + }); + +}); diff --git a/tests/Unit/WorkflowRecoveryTest.php b/tests/Unit/WorkflowRecoveryTest.php new file mode 100644 index 0000000..618c7eb --- /dev/null +++ b/tests/Unit/WorkflowRecoveryTest.php @@ -0,0 +1,174 @@ + $config */ + public function __construct(public array $config = [], public mixed $logger = null) {} + + public function execute(WorkflowContext $context): ActionResult + { + self::$attempts++; + + if (self::$shouldFail) { + throw new RuntimeException('transient outage'); + } + + return ActionResult::success(['recovered' => true]); + } + + public function canExecute(WorkflowContext $context): bool + { + return true; + } + + public function getName(): string + { + return 'recoverable'; + } + + public function getDescription(): string + { + return 'Fails until told otherwise'; + } +} + +beforeEach(function () { + RecoverableAction::$shouldFail = true; + RecoverableAction::$attempts = 0; + + $this->storage = new InMemoryStorage; + $this->engine = new WorkflowEngine($this->storage); + $this->definition = [ + 'name' => 'Recovery', + 'steps' => [ + ['id' => 'flaky', 'action' => RecoverableAction::class], + ], + ]; +}); + +describe('resuming a failed workflow', function () { + + test('a failing step leaves the workflow in FAILED with the real error', function () { + expect(fn () => $this->engine->start('wf-1', $this->definition)) + ->toThrow(StepExecutionException::class); + + $instance = $this->engine->getInstance('wf-1'); + + expect($instance->getState())->toBe(WorkflowState::FAILED); + expect($instance->getErrorMessage())->toContain('transient outage'); + }); + + /** + * Previously this threw "Cannot transition workflow from 'failed' to + * 'failed'" — a bookkeeping error raised inside the catch block, which + * destroyed the original cause and made recovery impossible. + */ + test('resume retries the failed step and completes', function () { + expect(fn () => $this->engine->start('wf-1', $this->definition)) + ->toThrow(StepExecutionException::class); + + RecoverableAction::$shouldFail = false; + + $instance = $this->engine->resume('wf-1'); + + expect($instance->getState())->toBe(WorkflowState::COMPLETED); + expect($instance->getData())->toHaveKey('recovered'); + expect(RecoverableAction::$attempts)->toBe(2); + }); + + test('resume clears the stale error message', function () { + expect(fn () => $this->engine->start('wf-1', $this->definition)) + ->toThrow(StepExecutionException::class); + + RecoverableAction::$shouldFail = false; + $instance = $this->engine->resume('wf-1'); + + expect($instance->getErrorMessage())->toBeNull(); + }); + + test('resume that fails again still reports the real error', function () { + expect(fn () => $this->engine->start('wf-1', $this->definition)) + ->toThrow(StepExecutionException::class); + + // Still broken: the second failure must surface as the step error, not + // as an illegal-transition error from the failure handler. + expect(fn () => $this->engine->resume('wf-1')) + ->toThrow(StepExecutionException::class, "Step 'flaky' failed: transient outage"); + + expect($this->engine->getInstance('wf-1')->getState())->toBe(WorkflowState::FAILED); + }); + + test('a failed workflow can be cancelled instead of retried', function () { + expect(fn () => $this->engine->start('wf-1', $this->definition)) + ->toThrow(StepExecutionException::class); + + $instance = $this->engine->cancel('wf-1', 'giving up'); + + expect($instance->getState())->toBe(WorkflowState::CANCELLED); + }); + + test('a cancelled workflow cannot be resumed', function () { + expect(fn () => $this->engine->start('wf-1', $this->definition)) + ->toThrow(StepExecutionException::class); + + $this->engine->cancel('wf-1', 'giving up'); + + expect(fn () => $this->engine->resume('wf-1')) + ->toThrow(InvalidWorkflowStateException::class); + }); + + test('a completed workflow cannot be resumed', function () { + RecoverableAction::$shouldFail = false; + $this->engine->start('wf-2', $this->definition); + + expect(fn () => $this->engine->resume('wf-2')) + ->toThrow(InvalidWorkflowStateException::class); + }); + +}); + +describe('instance ID collisions', function () { + + /** + * start() used to overwrite the stored instance, discarding the earlier + * run's state and history with nothing to indicate it had happened. + */ + test('starting a second workflow with an existing ID is rejected', function () { + RecoverableAction::$shouldFail = false; + $this->engine->start('wf-dup', $this->definition, ['run' => 1]); + + expect(fn () => $this->engine->start('wf-dup', $this->definition, ['run' => 2])) + ->toThrow(InvalidWorkflowStateException::class); + + // The original run is intact. + expect($this->engine->getInstance('wf-dup')->getData()['run'])->toBe(1); + }); + + test('the ID is free again after the instance is deleted', function () { + RecoverableAction::$shouldFail = false; + $this->engine->start('wf-dup', $this->definition, ['run' => 1]); + $this->storage->delete('wf-dup'); + + $this->engine->start('wf-dup', $this->definition, ['run' => 2]); + + expect($this->engine->getInstance('wf-dup')->getData()['run'])->toBe(2); + }); + +}); diff --git a/tests/Unit/WorkflowStateTransitionTest.php b/tests/Unit/WorkflowStateTransitionTest.php index 4b392ae..de4c504 100644 --- a/tests/Unit/WorkflowStateTransitionTest.php +++ b/tests/Unit/WorkflowStateTransitionTest.php @@ -103,11 +103,11 @@ function createInstance(WorkflowState $state): WorkflowInstance ->toThrow(InvalidWorkflowStateException::class); }); - test('FAILED cannot transition to RUNNING', function () { + test('FAILED cannot transition to COMPLETED directly', function () { $instance = createInstance(WorkflowState::PENDING); $instance->setState(WorkflowState::FAILED); - expect(fn () => $instance->setState(WorkflowState::RUNNING)) + expect(fn () => $instance->setState(WorkflowState::COMPLETED)) ->toThrow(InvalidWorkflowStateException::class); }); From ed03cd05491b11f38ab2e9ca5a17f5dd419c9d4a Mon Sep 17 00:00:00 2001 From: LamLam1 Date: Sat, 12 Sep 2026 13:35:45 +0800 Subject: [PATCH 2/2] ci: add PHP 8.5 to the test matrix 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 --- .github/workflows/run-tests.yml | 2 +- CHANGELOG.md | 3 +++ composer.lock | 5 +++-- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index c7d4782..a7fb4de 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -27,7 +27,7 @@ jobs: fail-fast: true matrix: os: [ubuntu-latest] - php: [8.4, 8.3] + php: [8.5, 8.4, 8.3] stability: [prefer-lowest, prefer-stable] name: PHP ${{ matrix.php }} - ${{ matrix.stability }} - ${{ matrix.os }} diff --git a/CHANGELOG.md b/CHANGELOG.md index bfd1112..5ea4ab2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,9 @@ section before upgrading. ### CI +- **PHP 8.5 added to the test matrix.** The library accepts `php: ^8.3`, which + admits 8.5, but it was never tested there. (Installing on 8.5 became possible + once #57 widened the dev tooling constraints.) - `run-tests.yml` and `phpstan.yml` now run on **pull requests**, not just pushes. Combined with `dependabot-auto-merge.yml` auto-merging minor and patch bumps, dependency updates could previously reach `main` without the suite ever running diff --git a/composer.lock b/composer.lock index 0aaa238..9882a4c 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "cf48e6f29095ad11f3d5618093a2b0ae", + "content-hash": "f9ca04ffd07169fea1c74af8a2df7533", "packages": [], "packages-dev": [ { @@ -4287,7 +4287,8 @@ "prefer-stable": true, "prefer-lowest": false, "platform": { - "php": "^8.3" + "php": "^8.3", + "ext-json": "*" }, "platform-dev": {}, "plugin-api-version": "2.9.0"