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] Introduce Schedule Grouping #53427

Open
wants to merge 12 commits into
base: 11.x
Choose a base branch
from

Conversation

istiak-tridip
Copy link

@istiak-tridip istiak-tridip commented Nov 6, 2024

Description

This PR introduces a new feature allowing related schedules to be grouped, similar to the Route::group method.

With this addition, developers can configure multiple schedules collectively, reducing the need for repetitive configuration of individual schedules.

Looking forward to your feedback. Thanks for all the great work that you guys do! 😍

Current Behavior

Schedule::command('command-one')->everyMinute()->runInBackground()->withoutOverlapping();
Schedule::command('command-two')->everyMinute()->runInBackground()->withoutOverlapping();
Schedule::command('command-three')->everyMinute()->runInBackground()->withoutOverlapping();

With This PR

Schedule::group()
    ->everyMinute()
    ->runInBackground()
    ->withoutOverlapping()
    ->schedules(function () {
        Schedule::command('command-one');
        Schedule::command('command-two');
        Schedule::command('command-three');
    });

Individual schedules can override group configurations as needed:

Schedule::group()
    ->everyMinute()
    ->runInBackground()
    ->withoutOverlapping()
    ->schedules(function () {
        Schedule::command('command-one');
        Schedule::command('command-two');
        Schedule::command('command-three')->everyTenMinutes();  // Override the group's cron expression
    });

Nested grouping is also supported:

Schedule::group()
    ->runInBackground()
    ->withoutOverlapping()
    ->schedules(function () {
        Schedule::group()->everyMinute()->schedules(function () {
            Schedule::command('command-one');
            Schedule::command('command-two');
        });

        Schedule::group()->everyTenMinutes()->schedules(function () {
            Schedule::command('command-three');
            Schedule::command('command-four');
        });
    });

Past similar proposals: #49832 #48046

@cosmastech
Copy link
Contributor

Nice! Glad to see this revived.

@decadence
Copy link
Contributor

decadence commented Nov 7, 2024

Hope this will be merged. I think about this every time I call runInBackground and everyMinute for most events.

@rodrigopedra
Copy link
Contributor

I'd prefer a syntax like this:

Schedule::group(function () {
    Schedule::command('command-one');
    Schedule::command('command-two');
})
    ->runInBackground()
    ->withoutOverlapping()
    ->everyMinute();

@istiak-tridip
Copy link
Author

Thanks for the suggestion, @rodrigopedra!

In my view, the suggested syntax makes the chaining feel a bit less clean and slightly harder to follow. Keeping these configurations at the start helps highlight the group’s behavior, improving readability and clarity overall.

That said, if there’s strong preference for this approach, I’m definitely open to revisiting the implementation.

@rodrigopedra
Copy link
Contributor

My preference is that it would be more inline with the route group feature, mentioned as inspiration for this feature.

Also, in my opinion, it is more declarative.

But let's wait for the maintainer's opinion.

Other than that, I think this is a great addition!

@Rizky92
Copy link

Rizky92 commented Nov 8, 2024

I'd prefer a syntax like this:

Schedule::group(function () {
    Schedule::command('command-one');
    Schedule::command('command-two');
})
    ->runInBackground()
    ->withoutOverlapping()
    ->everyMinute();

I agree! This is more inlined with how Laravel handles fluent route method.

@istiak-tridip
Copy link
Author

@Rizky92 @rodrigopedra I looked into the route group feature again, and it seems that using the syntax you suggested doesn’t apply the group options to the routes.

For example:

Route::group([], function () {
    Route::get('/test', fn () => 'Admin Dashboard')->name('home');
})
    ->name('admin.') // doesn’t affect routes
    ->prefix('/admin'); // doesn’t affect routes

Instead, it needs to be structured like this:

Route::name('admin.')
    ->prefix('/admin')
    ->group(function () {
        Route::get('/test', fn () => 'Admin Dashboard')->name('home');
    });

This setup aligns with my suggested syntax. But let’s see what the maintainers think before making any changes. 🙏

@rodrigopedra
Copy link
Contributor

Having the group at end is fine to me.

On the other hand, your usage example has an empty group() call like a block start, that needs to be
ended with a schedules() call, like a block end.

That is what I find misaligned with the route group feature.

Copy link
Contributor

@stevebauman stevebauman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea! 👍

tests/Integration/Console/Scheduling/ScheduleGroupTest.php Outdated Show resolved Hide resolved
src/Illuminate/Console/Scheduling/ScheduleGroup.php Outdated Show resolved Hide resolved
src/Illuminate/Console/Scheduling/ScheduleGroup.php Outdated Show resolved Hide resolved
@taylorotwell
Copy link
Member

taylorotwell commented Nov 11, 2024

@istiak-tridip Really would love to finally get this in the framework in some form. I am in agreement that consistency with the documented Route group syntax would be awesome where group is at the end and accepts a Closure. Thanks!

@istiak-tridip
Copy link
Author

istiak-tridip commented Nov 12, 2024

@taylorotwell I can think of two ways to implement the Route::group-style syntax:

  1. Implement it similarly to the Router class, using __call to proxy relevant method calls to a "Schedule Grouper." This keeps event attributes separate from the Schedule class, as they are now, maintaining a clean structure but lacking native IDE auto-completion.

  2. Alternatively, use a common trait to extract the relevant methods from the Scheduling\Event class into the Schedule class to ensure auto-completion support.

Let me know which direction you'd prefer!

@morloderex
Copy link
Contributor

@taylorotwell I can think of two ways to implement the Route::group-style syntax:

  1. Implement it similarly to the Router class, using __call to proxy relevant method calls to a "Schedule Grouper." This keeps event attributes separate from the Schedule class, as they are now, maintaining a clean structure but lacking native IDE auto-completion.

  2. Alternatively, duplicate the relevant methods from the Scheduling\Event class into the Schedule class to ensure auto-completion support.

Let me know which direction you'd prefer!

How about extracting the relevant method to a trait and use that trait on both classes? That way, you get auto completion without duplicating the methods?

@istiak-tridip
Copy link
Author

@morloderex Thanks for the reminder! That’s actually what I had planned to do; I just forgot to mention it. I’ve updated my previous reply to reflect this.

@istiak-tridip
Copy link
Author

@taylorotwell I've taken a closer look at the codebase to explore implementing the syntax through the second method if we go that route. I've identified some potential side effects with this approach.

For example, end users could write:

Schedule::runInBackground()->command('command-one');

While that seems fine (similar to routes), issues arise with the callback event. The Schedule::call returns a CallbackEvent instance, which overrides some methods of the parent Event class.

For instance, runInBackground is prohibited for callback event:

Schedule::call('command-one')->runInBackground(); // Throws `RuntimeException`

Allowing runInBackground before knowing the event type prevents us from replicating this behavior.

A solution could be to only allow group to be called after event attribute methods or to add a check in Schedule::call before creating the event. Both options add complexity.

What do you think?

@morloderex
Copy link
Contributor

morloderex commented Nov 12, 2024

@taylorotwell I've taken a closer look at the codebase to explore implementing the syntax through the second method if we go that route. I've identified some potential side effects with this approach.

For example, end users could write:

Schedule::runInBackground()->command('command-one');

While that seems fine (similar to routes), issues arise with the callback event. The Schedule::call returns a CallbackEvent instance, which overrides some methods of the parent Event class.

For instance, runInBackground is prohibited for callback event:

Schedule::call('command-one')->runInBackground(); // Throws `RuntimeException`

Allowing runInBackground before knowing the event type prevents us from replicating this behavior.

A solution could be to only allow group to be called after event attribute methods or to add a check in Schedule::call before creating the event. Both options add complexity.

What do you think?

Wondering if it would work by just making runInBackground return somekind of shadow object which is just to allow chaining in this case then? And not actually make it throw the exception anymore. And then add a note in the docs for this specific method chain?

@istiak-tridip
Copy link
Author

@morloderex That approach is possible, but it would completely change the current behavior, which may cause existing tests to fail and go against end-users' existing expectations. It might also be considered a breaking change, though I'm not entirely sure.

For now, it may be best to wait for feedback from @taylorotwell or other maintainers.

@istiak-tridip
Copy link
Author

@stevebauman @taylorotwell I’ve updated the implementation to follow Route::group syntax, please review 🙏.

Here’s an example of the new Schedule group usage:

Schedule::everyMinute()
    ->runInBackground()
    ->withoutOverlapping()
    ->group(function () {
        Schedule::command('command-one');
        Schedule::command('command-two');
        Schedule::command('command-three')->everyTenMinutes();
    });

Additionally, this implementation allows users to declare schedules similarly to Route, like this:

Schedule::hourly()->command('inspire');

Copy link
Contributor

@stevebauman stevebauman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nits left. Everything LGTM! 👍

We will see what the Laravel team thinks.

src/Illuminate/Console/Scheduling/Schedule.php Outdated Show resolved Hide resolved
Co-authored-by: Steve Bauman <[email protected]>
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.

8 participants