From 8a0215ca57f1214de73c7f98388820da35494628 Mon Sep 17 00:00:00 2001 From: benkhalife Date: Fri, 25 Sep 2026 03:38:58 -0700 Subject: [PATCH 1/4] fix: OR day-of-month and day-of-week when both are restricted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isDue() combined all five cron fields with AND, so an expression like "0 9 1 * 1" (meant to mean "9am on the 1st of the month OR every Monday", per standard cron) only matched on the rare day that was both at once — a silent behavior mismatch for anyone coming from standard crontab semantics. Track whether the day-of-month/day-of-week fields were literally restricted (not the bare "*"), and OR them together when both are; otherwise keep the existing AND (the unrestricted field is always true, so the two agree, no special-casing needed). --- src/CronExpression.php | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/src/CronExpression.php b/src/CronExpression.php index f710ba8..db15dd3 100644 --- a/src/CronExpression.php +++ b/src/CronExpression.php @@ -26,6 +26,14 @@ class CronExpression /** @var array Expanded valid values per field, in field order. */ private array $fields; + /** + * Whether the day-of-month / day-of-week field was literally restricted + * (i.e. not the bare "*" wildcard). Standard cron ORs these two fields + * together when both are restricted at once; see isDue(). + */ + private bool $dayOfMonthRestricted; + private bool $dayOfWeekRestricted; + public function __construct(string $expression) { $parts = preg_split('/\s+/', trim($expression)); @@ -36,6 +44,9 @@ public function __construct(string $expression) ); } + $this->dayOfMonthRestricted = $parts[2] !== '*'; + $this->dayOfWeekRestricted = $parts[4] !== '*'; + $this->fields = []; foreach ($parts as $index => $part) { $range = self::FIELD_RANGES[$index]; @@ -51,11 +62,24 @@ public function __construct(string $expression) public function isDue(\DateTimeInterface $at): bool { + $dayOfMonthMatches = in_array((int) $at->format('j'), $this->fields[2], true); + $dayOfWeekMatches = in_array((int) $at->format('w'), $this->fields[4], true); + + // Standard cron: when BOTH day-of-month and day-of-week are + // restricted (neither is "*"), the day matches if EITHER one does + // (OR), not only when both do at once (AND) — e.g. "0 9 1 * 1" means + // 9am on the 1st of the month OR every Monday, not only on the rare + // day that's both. When at most one of them is restricted, the + // unrestricted field is always true, so AND and OR agree; AND is + // kept for that case since it needs no special-casing. + $dayMatches = ($this->dayOfMonthRestricted && $this->dayOfWeekRestricted) + ? ($dayOfMonthMatches || $dayOfWeekMatches) + : ($dayOfMonthMatches && $dayOfWeekMatches); + return in_array((int) $at->format('i'), $this->fields[0], true) && in_array((int) $at->format('G'), $this->fields[1], true) - && in_array((int) $at->format('j'), $this->fields[2], true) && in_array((int) $at->format('n'), $this->fields[3], true) - && in_array((int) $at->format('w'), $this->fields[4], true); + && $dayMatches; } /** From c7fc0a296413101dbc0d7d500c9a089f6d1b763d Mon Sep 17 00:00:00 2001 From: benkhalife Date: Fri, 25 Sep 2026 03:39:05 -0700 Subject: [PATCH 2/4] test: cover the day-of-month/day-of-week OR interaction 4 tests: both-restricted fields OR together (e.g. "0 9 1 * 1"), either field alone still acts as AND against a wildcard sibling, and a step field (e.g. "*/2") still counts as "restricted" for this rule. --- tests/CronExpressionTest.php | 60 ++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/CronExpressionTest.php b/tests/CronExpressionTest.php index 0dfd41c..640d68c 100644 --- a/tests/CronExpressionTest.php +++ b/tests/CronExpressionTest.php @@ -85,6 +85,66 @@ public function testAllFieldsMustMatchSimultaneously(): void $this->assertFalse($cron->isDue($this->dt('2024-07-01 14:30:00'))); } + // ========================================================================= + // Day-of-month / day-of-week interaction (standard cron semantics) + // ========================================================================= + + /** + * Standard cron: when BOTH day-of-month and day-of-week are restricted + * (neither is "*"), the day matches if EITHER one does (OR) — e.g. + * "0 9 1 * 1" means 9am on the 1st of the month OR every Monday, not + * only on the rare day that happens to be both at once. + */ + public function testDayOfMonthAndDayOfWeekAreOredWhenBothRestricted(): void + { + $cron = new CronExpression('0 9 1 * 1'); + + // 1st of the month, a Thursday: matches via day-of-month alone. + $this->assertTrue($cron->isDue($this->dt('2026-10-01 09:00:00'))); + + // A Monday that isn't the 1st: matches via day-of-week alone. + $this->assertTrue($cron->isDue($this->dt('2026-10-05 09:00:00'))); + + // Both at once: OR is inclusive, still matches. + $this->assertTrue($cron->isDue($this->dt('2026-06-01 09:00:00'))); + + // Neither (a Friday that isn't the 1st): no match. + $this->assertFalse($cron->isDue($this->dt('2026-10-02 09:00:00'))); + } + + public function testDayOfMonthAloneStillActsAsAndWhenDayOfWeekIsWildcard(): void + { + $cron = new CronExpression('0 0 1 * *'); + $this->assertTrue($cron->isDue($this->dt('2024-06-01 00:00:00'))); + $this->assertFalse($cron->isDue($this->dt('2024-06-02 00:00:00'))); + } + + public function testDayOfWeekAloneStillActsAsAndWhenDayOfMonthIsWildcard(): void + { + $cron = new CronExpression('0 0 * * 1'); + // 2024-01-08 is a Monday, 2024-01-09 a Tuesday. + $this->assertTrue($cron->isDue($this->dt('2024-01-08 00:00:00'))); + $this->assertFalse($cron->isDue($this->dt('2024-01-09 00:00:00'))); + } + + /** + * A step field (e.g. every-2-days) is not the literal "*" wildcard, so + * it still counts as "restricted" and triggers the OR rule against a + * restricted day-of-week — matching standard cron, which looks at the + * literal field text, not whether it happens to cover every value. + */ + public function testSteppedDayOfMonthCountsAsRestrictedForOrLogic(): void + { + $cron = new CronExpression('0 0 */2 * 1'); + + // 2024-01-08: day 8 (even, not in the odd 1,3,5... every-2 sequence) + // but a Monday — matches via day-of-week (OR). + $this->assertTrue($cron->isDue($this->dt('2024-01-08 00:00:00'))); + + // 2024-01-02: day 2 (even, no match) and a Tuesday (no match either). + $this->assertFalse($cron->isDue($this->dt('2024-01-02 00:00:00'))); + } + public function testDayOfWeekZeroMeansSunday(): void { $cron = new CronExpression('0 0 * * 0'); From ae2c30a18a6f536bdb976b7c42aad481ac60ab5f Mon Sep 17 00:00:00 2001 From: benkhalife Date: Fri, 25 Sep 2026 03:39:13 -0700 Subject: [PATCH 3/4] fix: resolve [Class::class, method] for non-static scheduled tasks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [ClassName::class, 'method'] only worked when "method" was static: is_callable() is false for a class-string paired with an instance method, so the documented array-callback form threw "Scheduled task callback is not callable." for the common case of a regular (non-static) method. Instantiate the class ourselves in that case, same as 'Class@method' already did, so both forms work identically regardless of whether the method is static. Since there's no dependency container here, that instantiation still only supports a no-argument constructor — but a class that needs one now fails with a clear, actionable RuntimeException pointing at the [$instance, 'method'] workaround, instead of a bare ArgumentCountError leaking out of `new $class()`. Documented both the supported forms and this limitation on ScheduleEvent's constructor. --- src/ScheduleEvent.php | 46 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/src/ScheduleEvent.php b/src/ScheduleEvent.php index dfe7dc4..4840f98 100644 --- a/src/ScheduleEvent.php +++ b/src/ScheduleEvent.php @@ -18,8 +18,17 @@ class ScheduleEvent private ?string $name = null; /** - * @param callable|string|array $callback Closure, [object|class, method], - * 'Class@method', or a global function name. + * @param callable|string|array $callback Any of: + * - a Closure + * - [$instance, 'method'] — an already-built object; works for any + * visibility/dependencies, since you built it + * - [ClassName::class, 'method'] or 'Class@method' — the class name + * as a string. Both forms instantiate the class with `new + * ClassName()` (no constructor arguments) and work whether + * "method" is static or not. A class whose constructor requires + * arguments can NOT be referenced this way — build it yourself and + * pass [$instance, 'method'] instead. + * - a global function name */ public function __construct($callback) { @@ -169,12 +178,23 @@ private static function invoke($callback): mixed { if (is_string($callback) && str_contains($callback, '@')) { [$class, $method] = explode('@', $callback, 2); + $callback = [$class, $method]; + } + + // [ClassName::class, 'method'] with the class given as a string + // (not an already-built object) only works out of the box for a + // *static* method — is_callable() is false for an instance method + // there, since there is no instance to call it on. Instantiate the + // class ourselves so this form works the same way whether "method" + // is static or not, exactly like 'Class@method' already did. + if (is_array($callback) && count($callback) === 2 && is_string($callback[0]) && !is_callable($callback)) { + [$class, $method] = $callback; if (!class_exists($class)) { throw new \RuntimeException("Scheduled task class '$class' not found."); } - $callback = [new $class(), $method]; + $callback = [self::instantiate($class), $method]; } if (!is_callable($callback)) { @@ -184,6 +204,26 @@ private static function invoke($callback): mixed return call_user_func($callback); } + /** + * Build a task class from its name alone. Since there is no dependency + * container here, this only works for a class with a no-argument + * constructor; a class that needs dependencies must be constructed by + * the caller and registered as an instance instead (see the class + * docblock). + */ + private static function instantiate(string $class): object + { + try { + return new $class(); + } catch (\ArgumentCountError $e) { + throw new \RuntimeException( + "Scheduled task class '$class' requires constructor arguments, so it can't be " . + "referenced by class name alone. Construct it yourself and register the instance " . + "instead, e.g. Schedule::call([new $class(...your dependencies...), 'method'])." + ); + } + } + private function inferName(): string { if (is_string($this->callback)) { From 3e2fdcb134cbac58bfb760630959bb535b8f4b68 Mon Sep 17 00:00:00 2001 From: benkhalife Date: Fri, 25 Sep 2026 03:39:19 -0700 Subject: [PATCH 4/4] test: cover callback resolution (array/string, static vs instance, deps) 5 tests: [Class::class, method] now works for non-static methods, still works for static ones, 'Class@method' still works, a class needing constructor arguments fails with an actionable message, and the documented workaround (pre-built instance) bypasses that limit. --- tests/ScheduleTest.php | 91 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 2 deletions(-) diff --git a/tests/ScheduleTest.php b/tests/ScheduleTest.php index 2c17724..882587a 100644 --- a/tests/ScheduleTest.php +++ b/tests/ScheduleTest.php @@ -189,7 +189,71 @@ public function testRunExecutesATaskImmediatelyIgnoringItsDueCheck(): void } // ========================================================================= - // 4. loadFromDirectory(): discovery + per-file error isolation + // 4. Callback resolution: [Class::class, 'method'], 'Class@method', + // static vs. instance methods, and constructor dependencies + // ========================================================================= + + /** + * [ClassName::class, 'method'] with a *non-static* method used to fail + * (is_callable() is false for a class-string paired with an instance + * method — there's no instance to call it on). It's now instantiated + * the same way 'Class@method' already was. + */ + public function testArrayFormWithClassStringWorksForNonStaticMethod(): void + { + $event = Schedule::call([ScheduleTestTarget::class, 'ok'])->name('array-non-static'); + $result = Schedule::run($event); + + $this->assertSame(['name' => 'array-non-static', 'status' => 'ran', 'error' => null], $result); + } + + public function testArrayFormWithClassStringStillWorksForStaticMethod(): void + { + $event = Schedule::call([ScheduleTestStaticTarget::class, 'ok'])->name('array-static'); + $result = Schedule::run($event); + + $this->assertSame(['name' => 'array-static', 'status' => 'ran', 'error' => null], $result); + } + + public function testAtSyntaxStringFormStillWorksForNonStaticMethod(): void + { + $event = Schedule::call(\Tests\ScheduleTestTarget::class . '@ok')->name('at-syntax'); + $result = Schedule::run($event); + + $this->assertSame(['name' => 'at-syntax', 'status' => 'ran', 'error' => null], $result); + } + + /** + * There is no dependency container here, so a class-name callback can + * only be instantiated with no constructor arguments. That failure + * mode must surface as a clear, actionable error — not a raw + * ArgumentCountError leaking out of `new $class()`. + */ + public function testClassWithRequiredConstructorArgumentsFailsWithActionableMessage(): void + { + $event = Schedule::call([ScheduleTestNeedsDependency::class, 'run'])->name('needs-dependency'); + $result = Schedule::run($event); + + $this->assertSame('failed', $result['status']); + $this->assertStringContainsString('requires constructor arguments', $result['error']); + $this->assertStringContainsString('ScheduleTestNeedsDependency', $result['error']); + } + + /** + * The documented workaround for the case above: build the instance + * yourself and register it directly. + */ + public function testPreBuiltInstanceBypassesTheConstructorLimitation(): void + { + $service = new ScheduleTestNeedsDependency('a-real-dependency'); + $event = Schedule::call([$service, 'run'])->name('pre-built-instance'); + $result = Schedule::run($event); + + $this->assertSame(['name' => 'pre-built-instance', 'status' => 'ran', 'error' => null], $result); + } + + // ========================================================================= + // 5. loadFromDirectory(): discovery + per-file error isolation // ========================================================================= public function testLoadFromDirectoryRegistersTasksFromEveryFile(): void @@ -243,7 +307,7 @@ public function testABrokenFileDoesNotPreventOtherFilesFromLoading(): void } // ========================================================================= - // 5. runDue(): due-filtering + per-task failure isolation + overlap lock + // 6. runDue(): due-filtering + per-task failure isolation + overlap lock // ========================================================================= public function testRunDueOnlyRunsTasksThatAreDueAtGivenTime(): void @@ -330,3 +394,26 @@ public function ok(): bool return true; } } + +class ScheduleTestStaticTarget +{ + public static function ok(): bool + { + return true; + } +} + +class ScheduleTestNeedsDependency +{ + private string $dependency; + + public function __construct(string $dependency) + { + $this->dependency = $dependency; + } + + public function run(): string + { + return $this->dependency; + } +}