From 12683ae9531edba1cf7f5856bbae9d2160f1d21b Mon Sep 17 00:00:00 2001 From: Achraf AAMRI <36072352+achrafAa@users.noreply.github.com> Date: Thu, 1 May 2025 23:27:30 +0100 Subject: [PATCH 1/7] Add exception handling for failed scheduled commands - Throw an exception for scheduled commands that fail in the foreground, providing the exit code in the message. - Introduce a new test suite to verify the behavior of scheduled commands, ensuring that events are dispatched correctly for failed, successful, and background tasks. --- .../Console/Scheduling/ScheduleRunCommand.php | 5 + .../Scheduling/ScheduleRunCommandTest.php | 123 ++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php diff --git a/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php b/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php index 75cb579925cf..9a92a4ade40f 100644 --- a/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php +++ b/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php @@ -2,6 +2,7 @@ namespace Illuminate\Console\Scheduling; +use Exception; use Illuminate\Console\Application; use Illuminate\Console\Command; use Illuminate\Console\Events\ScheduledTaskFailed; @@ -196,6 +197,10 @@ protected function runEvent($event) round(microtime(true) - $start, 2) )); + if ($event->exitCode != 0 && ! $event->runInBackground) { + throw new Exception("Scheduled command [{$event->command}] failed with exit code [{$event->exitCode}]."); + } + $this->eventsRan = true; } catch (Throwable $e) { $this->dispatcher->dispatch(new ScheduledTaskFailed($event, $e)); diff --git a/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php b/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php new file mode 100644 index 000000000000..43309e46ab21 --- /dev/null +++ b/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php @@ -0,0 +1,123 @@ +app->make(Schedule::class); + $task = $schedule->exec('exit 1') + ->everyMinute() + ->withoutOverlapping(); + + // Make sure it will run regardless of schedule + $task->when(function () { + return true; + }); + + // Execute the scheduler + $this->artisan('schedule:run'); + + // Verify the event sequence + Event::assertDispatched(ScheduledTaskStarting::class); + Event::assertDispatched(ScheduledTaskFinished::class); + Event::assertDispatched(ScheduledTaskFailed::class, function ($event) use ($task) { + return $event->task === $task && + $event->exception->getMessage() === 'Scheduled command [exit 1] failed with exit code [1].'; + }); + } + + /** + * @throws BindingResolutionException + */ + public function test_failing_command_in_background_does_not_trigger_event() + { + Event::fake([ + ScheduledTaskStarting::class, + ScheduledTaskFinished::class, + ScheduledTaskFailed::class, + ]); + + // Create a schedule and add the command + $schedule = $this->app->make(Schedule::class); + $task = $schedule->exec('exit 1') + ->everyMinute() + ->runInBackground(); + + // Make sure it will run regardless of schedule + $task->when(function () { + return true; + }); + + // Execute the scheduler + $this->artisan('schedule:run'); + + // Verify the event sequence + Event::assertDispatched(ScheduledTaskStarting::class); + Event::assertDispatched(ScheduledTaskFinished::class); + Event::assertNotDispatched(ScheduledTaskFailed::class); + } + + /** + * @throws BindingResolutionException + */ + public function test_successful_command_does_not_trigger_event() + { + Event::fake([ + ScheduledTaskStarting::class, + ScheduledTaskFinished::class, + ScheduledTaskFailed::class, + ]); + + // Create a schedule and add the command + $schedule = $this->app->make(Schedule::class); + $task = $schedule->exec('exit 0') + ->everyMinute() + ->withoutOverlapping(); + + // Make sure it will run regardless of schedule + $task->when(function () { + return true; + }); + + // Execute the scheduler + $this->artisan('schedule:run'); + + // Verify the event sequence + Event::assertDispatched(ScheduledTaskStarting::class); + Event::assertDispatched(ScheduledTaskFinished::class); + Event::assertNotDispatched(ScheduledTaskFailed::class); + } +} From 4ed5cdf284184a5cfc079911853a4a23af8f8f8b Mon Sep 17 00:00:00 2001 From: Achraf AAMRI <36072352+achrafAa@users.noreply.github.com> Date: Thu, 1 May 2025 23:33:16 +0100 Subject: [PATCH 2/7] Add test for command without explicit return --- .../Scheduling/ScheduleRunCommandTest.php | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php b/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php index 43309e46ab21..809d99be3481 100644 --- a/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php +++ b/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php @@ -120,4 +120,35 @@ public function test_successful_command_does_not_trigger_event() Event::assertDispatched(ScheduledTaskFinished::class); Event::assertNotDispatched(ScheduledTaskFailed::class); } + + /** + * @throws BindingResolutionException + */ + public function test_command_with_no_explicit_return_does_not_trigger_event() + { + Event::fake([ + ScheduledTaskStarting::class, + ScheduledTaskFinished::class, + ScheduledTaskFailed::class, + ]); + + // Create a schedule and add the command that just performs an action without explicit exit + $schedule = $this->app->make(Schedule::class); + $task = $schedule->exec('true') + ->everyMinute() + ->withoutOverlapping(); + + // Make sure it will run regardless of schedule + $task->when(function () { + return true; + }); + + // Execute the scheduler + $this->artisan('schedule:run'); + + // Verify the event sequence + Event::assertDispatched(ScheduledTaskStarting::class); + Event::assertDispatched(ScheduledTaskFinished::class); + Event::assertNotDispatched(ScheduledTaskFailed::class); + } } From cdc08e39d3ece3d7e49c53647685c62b4976f8ac Mon Sep 17 00:00:00 2001 From: Achraf AAMRI <36072352+achrafAa@users.noreply.github.com> Date: Thu, 1 May 2025 23:33:21 +0100 Subject: [PATCH 3/7] add test for no explicit return --- package-lock.json | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 package-lock.json diff --git a/package-lock.json b/package-lock.json new file mode 100644 index 000000000000..89dbe2e3ef6e --- /dev/null +++ b/package-lock.json @@ -0,0 +1,6 @@ +{ + "name": "laravel-framework", + "lockfileVersion": 3, + "requires": true, + "packages": {} +} From 59eb6c40e1f602b39f52ceeca151d3b0a5fd34cd Mon Sep 17 00:00:00 2001 From: Achraf AAMRI <36072352+achrafAa@users.noreply.github.com> Date: Thu, 1 May 2025 23:43:37 +0100 Subject: [PATCH 4/7] Revert "add test for no explicit return" This reverts commit cdc08e39d3ece3d7e49c53647685c62b4976f8ac. --- package-lock.json | 6 ------ 1 file changed, 6 deletions(-) delete mode 100644 package-lock.json diff --git a/package-lock.json b/package-lock.json deleted file mode 100644 index 89dbe2e3ef6e..000000000000 --- a/package-lock.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "name": "laravel-framework", - "lockfileVersion": 3, - "requires": true, - "packages": {} -} From eb7be29c680bc556f05dc151f87dfa235d639a36 Mon Sep 17 00:00:00 2001 From: Achraf AAMRI <36072352+achrafAa@users.noreply.github.com> Date: Thu, 1 May 2025 23:52:45 +0100 Subject: [PATCH 5/7] Add tests for background command execution with and without explicit return --- .../Scheduling/ScheduleRunCommandTest.php | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php b/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php index 809d99be3481..2eefda43dbd8 100644 --- a/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php +++ b/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php @@ -151,4 +151,66 @@ public function test_command_with_no_explicit_return_does_not_trigger_event() Event::assertDispatched(ScheduledTaskFinished::class); Event::assertNotDispatched(ScheduledTaskFailed::class); } + + /** + * @throws BindingResolutionException + */ + public function test_successful_command_in_background_does_not_trigger_event() + { + Event::fake([ + ScheduledTaskStarting::class, + ScheduledTaskFinished::class, + ScheduledTaskFailed::class, + ]); + + // Create a schedule and add the command + $schedule = $this->app->make(Schedule::class); + $task = $schedule->exec('exit 0') + ->everyMinute() + ->runInBackground(); + + // Make sure it will run regardless of schedule + $task->when(function () { + return true; + }); + + // Execute the scheduler + $this->artisan('schedule:run'); + + // Verify the event sequence + Event::assertDispatched(ScheduledTaskStarting::class); + Event::assertDispatched(ScheduledTaskFinished::class); + Event::assertNotDispatched(ScheduledTaskFailed::class); + } + + /** + * @throws BindingResolutionException + */ + public function test_command_with_no_explicit_return_in_background_does_not_trigger_event() + { + Event::fake([ + ScheduledTaskStarting::class, + ScheduledTaskFinished::class, + ScheduledTaskFailed::class, + ]); + + // Create a schedule and add the command that just performs an action without explicit exit + $schedule = $this->app->make(Schedule::class); + $task = $schedule->exec('true') + ->everyMinute() + ->runInBackground(); + + // Make sure it will run regardless of schedule + $task->when(function () { + return true; + }); + + // Execute the scheduler + $this->artisan('schedule:run'); + + // Verify the event sequence + Event::assertDispatched(ScheduledTaskStarting::class); + Event::assertDispatched(ScheduledTaskFinished::class); + Event::assertNotDispatched(ScheduledTaskFailed::class); + } } From 6f78ed8c87d38feb6a52a4bebb3fac25c21bf751 Mon Sep 17 00:00:00 2001 From: Achraf AAMRI <36072352+achrafAa@users.noreply.github.com> Date: Fri, 2 May 2025 07:53:27 +0100 Subject: [PATCH 6/7] move the check after set-ting the eventsRan to true --- src/Illuminate/Console/Scheduling/ScheduleRunCommand.php | 8 ++------ .../Console/Scheduling/ScheduleRunCommandTest.php | 9 +++------ 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php b/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php index 9a92a4ade40f..4c91bc5df8d5 100644 --- a/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php +++ b/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php @@ -97,10 +97,6 @@ public function __construct() /** * Execute the console command. * - * @param \Illuminate\Console\Scheduling\Schedule $schedule - * @param \Illuminate\Contracts\Events\Dispatcher $dispatcher - * @param \Illuminate\Contracts\Cache\Repository $cache - * @param \Illuminate\Contracts\Debug\ExceptionHandler $handler * @return void */ public function handle(Schedule $schedule, Dispatcher $dispatcher, Cache $cache, ExceptionHandler $handler) @@ -197,11 +193,11 @@ protected function runEvent($event) round(microtime(true) - $start, 2) )); + $this->eventsRan = true; + if ($event->exitCode != 0 && ! $event->runInBackground) { throw new Exception("Scheduled command [{$event->command}] failed with exit code [{$event->exitCode}]."); } - - $this->eventsRan = true; } catch (Throwable $e) { $this->dispatcher->dispatch(new ScheduledTaskFailed($event, $e)); diff --git a/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php b/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php index 2eefda43dbd8..1ede51aa3f24 100644 --- a/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php +++ b/tests/Integration/Console/Scheduling/ScheduleRunCommandTest.php @@ -39,8 +39,7 @@ public function test_failing_command_in_foreground_triggers_event() // Create a schedule and add the command $schedule = $this->app->make(Schedule::class); $task = $schedule->exec('exit 1') - ->everyMinute() - ->withoutOverlapping(); + ->everyMinute(); // Make sure it will run regardless of schedule $task->when(function () { @@ -104,8 +103,7 @@ public function test_successful_command_does_not_trigger_event() // Create a schedule and add the command $schedule = $this->app->make(Schedule::class); $task = $schedule->exec('exit 0') - ->everyMinute() - ->withoutOverlapping(); + ->everyMinute(); // Make sure it will run regardless of schedule $task->when(function () { @@ -135,8 +133,7 @@ public function test_command_with_no_explicit_return_does_not_trigger_event() // Create a schedule and add the command that just performs an action without explicit exit $schedule = $this->app->make(Schedule::class); $task = $schedule->exec('true') - ->everyMinute() - ->withoutOverlapping(); + ->everyMinute(); // Make sure it will run regardless of schedule $task->when(function () { From 1a556d2241d771bc2840fd1eead01252cdb3c2e1 Mon Sep 17 00:00:00 2001 From: Achraf AAMRI <36072352+achrafAa@users.noreply.github.com> Date: Fri, 2 May 2025 08:23:58 +0100 Subject: [PATCH 7/7] redo pint removing doc blocks for handle --- src/Illuminate/Console/Scheduling/ScheduleRunCommand.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php b/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php index 4c91bc5df8d5..8a2a30bac64d 100644 --- a/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php +++ b/src/Illuminate/Console/Scheduling/ScheduleRunCommand.php @@ -97,6 +97,10 @@ public function __construct() /** * Execute the console command. * + * @param \Illuminate\Console\Scheduling\Schedule $schedule + * @param \Illuminate\Contracts\Events\Dispatcher $dispatcher + * @param \Illuminate\Contracts\Cache\Repository $cache + * @param \Illuminate\Contracts\Debug\ExceptionHandler $handler * @return void */ public function handle(Schedule $schedule, Dispatcher $dispatcher, Cache $cache, ExceptionHandler $handler)