Fix TaskScheduler DateTime → DateTimeImmutable & restore broken tests - #54
Merged
alexander-schranz merged 4 commits intoMay 19, 2026
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
- TaskScheduler::scheduleTask now converts the \DateTime returned by
Cron\CronExpression::getNextRunDate() to \DateTimeImmutable before
calling TaskExecutionRepositoryInterface::create(), which contractually
requires \DateTimeImmutable. Without this, every cron-driven schedule
fails with a TypeError when the repository enforces the parameter type.
- TaskExecution::getResult() returns null when the result column was
never set, instead of false (unserialize(null) === false in PHP 8).
- Repair pre-existing syntax errors ("new \\DateTimeImmutable))" and
"new \\DateTimeImmutable)'...')") that were left in the test suite by
the sed-based DateTime → DateTimeImmutable migration in php-task#52, which
made the whole tests/Unit/Execution and tests/Unit/Storage suites
un-loadable.
- Update remaining tests that still called the removed findAll() method
to use findAllPaginated(1).
ubuntu-20.04 has been retired; jobs hang waiting for a runner. See: https://github.blog/changelog/2025-01-15-github-actions-ubuntu-20-runner-image-brownout-dates-and-other-breaking-changes/
Copy link
Copy Markdown
There was a problem hiding this comment.
Pull request overview
This PR fixes a runtime TypeError in cron-based scheduling by normalizing cron-derived schedule times to \DateTimeImmutable, and restores the unit test suite by repairing migration-introduced syntax errors and updating tests to the current repository API.
Changes:
- Convert cron-derived \DateTime values to \DateTimeImmutable before calling TaskExecutionRepositoryInterface::create().
- Adjust TaskExecution::getResult() to return null when the result was never set (avoids unserialize(null) === false on PHP 8).
- Repair broken unit tests (syntax fixes + findAll() → findAllPaginated(1) updates).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file| File | Description |
|---|---|
| src/Task/Scheduler/TaskScheduler.php | Normalizes mutable cron schedule times to \DateTimeImmutable to satisfy repository contract. |
| src/Task/Execution/TaskExecution.php | Returns null for unset results before unserializing stored values. |
| tests/Unit/Scheduler/TaskSchedulerTest.php | Updates mock expectations to match immutable schedule time passed to repository. |
| tests/Unit/Execution/TaskExecutionTest.php | Fixes syntax errors introduced by prior DateTimeImmutable migration. |
| tests/Unit/Storage/ArrayStorage/ArrayTaskRepositoryTest.php | Replaces removed findAll() usage with findAllPaginated(1). |
| tests/Unit/Storage/ArrayStorage/ArrayTaskExecutionRepositoryTest.php | Fixes syntax errors and updates findAll() usage to findAllPaginated(1). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
Comment thread
tests/Unit/Storage/ArrayStorage/ArrayTaskRepositoryTest.php
Comment thread
tests/Unit/Storage/ArrayStorage/ArrayTaskExecutionRepositoryTest.php
Outdated
Comment thread
src/Task/Execution/TaskExecution.php
- TaskExecutionInterface::getResult() PHPDoc now documents the null return possibility introduced by the unset-result fix. - Rename leftover testFindAll() methods to testFindAllPaginatedUnbounded() so the test name matches the API being exercised.
Accidentally included in the previous commit; add it to .gitignore.
alexander-schranz
approved these changes
May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's in this PR?
TaskScheduler::scheduleTask now converts the mutable \DateTime returned by Cron\CronExpression::getNextRunDate() to \DateTimeImmutable before passing it to TaskExecutionRepositoryInterface::create(). The interface declares the parameter as \DateTimeImmutable, so every cron-driven schedule currently raises TypeError when the implementation enforces the contract.
TaskExecution::getResult() returns null when the result column was never set, instead of false (since unserialize(null) === false on PHP 8).
Pre-existing syntax errors introduced by the sed-based DateTime → DateTimeImmutable migration in Enhance doctrine compatability #52 are repaired (new \DateTimeImmutable)) and new \DateTimeImmutable)'...')). These were making the whole tests/Unit/Execution and tests/Unit/Storage/ArrayStorage suites un-loadable.
Remaining test calls to the removed findAll() method are updated to findAllPaginated(1).
Why?
CI in php-task/TaskBundle#60 hit TypeError: TaskExecutionRepository::create(): Argument #2 ($scheduleTime) must be of type DateTimeImmutable, DateTime given, called in vendor/php-task/php-task/src/Task/Scheduler/TaskScheduler.php on line 110 on every PHP version. Per @alexander-schranz' review, the conversion belongs on the caller side rather than in every TaskExecutionRepository implementation — that's this PR.
The other fixes are needed so the test suite here actually runs and stays green; they were never caught because the ubuntu-20.04 runner is unavailable, which is being addressed separately.