Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

istiak-tridip
Copy link
Contributor

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
Copy link

Diddyy commented Nov 21, 2024

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
Copy link
Member

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.

@taylorotwell taylorotwell merged commit d8438ec into laravel:11.x Nov 21, 2024
32 checks passed
@istiak-tridip istiak-tridip deleted the fix-schedule_group branch November 22, 2024 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants