Fix/schedule cron review findings

src/CronExpression.php

@@ -26,6 +26,14 @@ class CronExpression
     /** @var array<int, int[]> 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;
     }
 
     /**

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)) {

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');

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;
+    }
+}