fix: UTC DateTime + remove dead compensation code#38
Conversation
- Replace all bare `new \DateTime` (7 occurrences) with
`new \DateTime('now', new \DateTimeZone('UTC'))` across
WorkflowEngine, WorkflowInstance, LogAction, DelayAction.
- Remove Step::$compensationAction, getCompensationAction(),
hasCompensation(), toArray() 'compensation' key, and the
compensationAction named arg in WorkflowDefinition::processSteps().
Saga/rollback is planned for a future version.
- 116 package tests all pass.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 631b0ba2b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private readonly ?string $timeout = null, | ||
| private readonly int $retryAttempts = 0, | ||
| private readonly ?string $compensationAction = null, | ||
| private readonly array $conditions = [], | ||
| private readonly array $prerequisites = [] |
There was a problem hiding this comment.
This constructor change removes the compensationAction slot from a public class that is instantiated directly by consumers, so upgrades can now fail at runtime with Unknown named parameter $compensationAction (or argument-type shifts for positional calls). The same commit also removes compensation accessors, so existing integrations that read compensation metadata now hit undefined-method fatals. If this removal is intended, it should be staged as a deprecation first (or done in a major release) to avoid breaking current users on a fix update.
Useful? React with 👍 / 👎.
| return ActionResult::success([ | ||
| 'logged_message' => $processedMessage, | ||
| 'logged_at' => (new \DateTime)->format('c'), | ||
| 'logged_at' => (new \DateTime('now', new \DateTimeZone('UTC')))->format('c'), |
There was a problem hiding this comment.
logged_at is now forced to UTC, but the same log entry still includes $context->toArray() where executed_at comes from WorkflowContext’s default process timezone and is formatted without an offset. On hosts not set to UTC, one record can contain mismatched clock bases, making event ordering and debugging unreliable. Consider deriving both fields from the same timezone-normalized source (or normalizing WorkflowContext serialization to UTC ISO-8601).
Useful? React with 👍 / 👎.
2 participants
Summary
new \DateTimecalls explicitly UTC-aware so workflow timestamps don't silently track whatever timezone the PHP process happens to run in.Step::$compensationAction,getCompensationAction(),hasCompensation(), thetoArray()['compensation']key, and thecompensationActionnamed arg inWorkflowDefinition::processSteps()). Saga / rollback is planned for a later version; until then this surface was just noise that consumers could wire up but never actually fired.What changed
UTC DateTime (7 call sites)
Replaces bare
new \DateTime()withnew \DateTime('now', new \DateTimeZone('UTC'))across:Core/WorkflowEngine.php(×2)Core/WorkflowInstance.php(×3 —startedAt,updatedAtbumps,completedAt)Actions/LogAction.php(×1 — event timestamp)Actions/DelayAction.php(×1 — delay wake time)PHPDoc examples elsewhere were left alone since they're not executing code. Two
WorkflowInstance::fromArraycall sites that parse stored ISO-8601 strings (new \DateTime($data['created_at'])) already carry their own timezone and don't produce "now", so they're unchanged.Dead compensation code
Step::$compensationActionand friends were never read byExecutor— consumers could assign compensation actions, but nothing ever rolled them back. Removing the surface avoids implying a contract we don't honor. Whoever picks up saga support later can re-introduce this with the actual dispatch path in the same PR.Test plan
composer test— 116 tests, 277 assertions, all greencomposer analyze— PHPStan level 6 clean (via CI)Notes
Origin of this patch: the commit has been sitting in a downstream consumer of the package. Pushing it up so the two stay in sync.