laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.62k stars 11.03k forks source link

[11.x] Fix attribute inheritance for nested scheduled groups #53626

Closed istiak-tridip closed 9 hours ago

istiak-tridip commented 11 hours ago

Description

This PR resolves a bug in which the attributes of the parent schedule group were not applied to the nested group's events. @rcerljenko reported the issue in #53608.

Example of unexpected behavior:

Schedule::onOneServer()->group(function () {
    Schedule::command('command-one');
    Schedule::runInBackground()->group(function () {
        // The `onOneServer` attribute is not applied to this event
        Schedule::command('command-two');
    });
});

P.S.

While debugging, I encountered two behaviors that I’m unsure how to handle, and I’d appreciate feedback.

Behavior One

Both examples below throw fatal exceptions because no PendingEventAttribute is set before creating a group.

// Example #1
Schedule::group(function () { // throws an exception here
    Schedule::command('command-one');
});

// Example #2
Schedule::daily()->group(function () {
    Schedule::command('command-one');
    Schedule::group(function () { // throws an exception here
        Schedule::command('command-two');
    });
});

What should be done, as I think groups should not be created without common attributes?

  1. Move the group method to the PendingEventAttribute class to prevent calling Schedule::group directly
  2. Catch the fatal exceptions and throw a new exception with a clear message
  3. Keep the current behavior

Behavior Two

Schedule::daily()->group(function () {
    Schedule::command('command-one');
    Schedule::command('command-two')->hourly();
});

The frequency of the second event becomes this:

Schedule::command('command-two')->daily()->hourly();

The hourly() method with the group's daily() results in a cron expression like 0 0 * * *, whereas the expected expression is 0 * * * *.

Again what should be done when modifying group events?

  1. Keep the current behavior
  2. Somehow reset the expression to * * * * * so that the hourly() generates correct frequency

@stevebauman @rodrigopedra @taylorotwell Tagging you for your valuable input during the initial PR and hoping to get your feedback on this one as well.

Diddyy commented 10 hours ago

I have no idea where you find the things in the framework that need fixing but you seem to be pushing out some ace work recently. 🔥

taylorotwell commented 9 hours ago

Hey @istiak-tridip ...

For the first situation, I would catch the fatal exception and throw an informative error message.

For the second situation, I think the expression would reset to hourly so that you could in theory group a bunch of tasks but opt out to some custom customization for a scheduled task when you need to.