framework icon indicating copy to clipboard operation
framework copied to clipboard

[12.x] Fixes for cron expressions when using `between()`

Open JonPurvis opened this issue 1 year ago • 5 comments

[!NOTE]
As part of #50670 I was asked by @driesvints to reattempt my previous PR (https://github.com/laravel/framework/pull/47983) for Laravel 12, so here we go! PR body is the same so I've copied it over

Hey,

I noticed something weird with some of the expressions of my scheduled commands when I run run php artisan schedule list, so I decided to do some digging. It seems like if you're using ->between(), this doesn't get included in the cron expression that shows.

For example, let's take a scheduled command that's meant to run every minute between 10pm and 11pm:

$schedule->command('example')
    ->everyMinute()
    ->timezone('BST')
    ->between('22:00', '23:00');

If I run php artisan schedule:list before 10pm, for example at the time of typing this, it's 21:21, I get:

 * * * * *  php artisan example ............................... Next Due: 20 seconds from now

But that's not correct, because it's actually next due in 36 minutes. With the changes in this PR, if I run php artisan schedule:list again, I get:

* 22-23 * * *  php artisan example ........................ Next Due: 36 minutes from now

I also noticed that if you try and be more specific about the minutes you want to run, e.g.:

$schedule->command('example')
    ->everyMinute()
    ->timezone('BST')
    ->between('22:30', '23:00');

You get the same as before, where it will give me the wrong expression and count down:

 * * * * *  php artisan example ............................... Next Due: 20 seconds from now

But with this PR, it changes it to the below, which includes the hours and minutes.

30-59 22-23 * * *  php artisan example ........................ Next Due: 1 hour from now

I've also amended the output to show if, for example, you've specified it to run every 2 minutes, 10 minutes etc...:

30-35/2 22-23 * * *  php artisan example ..................... Next Due: 1 hour from now

I haven't really delved into this part of the code in the framework before so I'm definitely open to being educated 😄

I've added some supporting tests to cover this new behaviour.

Thanks!

JonPurvis avatar Aug 07 '24 00:08 JonPurvis

Hey @driesvints 👋

Apologies it took me so long to get round to this, I actually had this issue in some more recent work I was doing and then remembered about this! You asked if I would be up for creating this again the L12 branch as a part of https://github.com/laravel/framework/pull/47983, so here we go 😄

JonPurvis avatar Aug 07 '24 00:08 JonPurvis

From a previous attempt:

For example, if an event is scheduled every minute from 1:30-2:30, there is no way to express this in a cron expression.

Please mark as ready for review if you want me to take a look.

taylorotwell avatar Aug 19 '24 03:08 taylorotwell

@taylorotwell I'm gonna leave this as a draft for now, I want to do some more testing with some more timings

JonPurvis avatar Aug 19 '24 08:08 JonPurvis

Hey @taylorotwell

Going to mark this for review as I believe these changes so far bring benefit, but need to have a further discussion regarding the below (I'm also not sure you get notifications for draft PR's):

For example, if an event is scheduled every minute from 1:30-2:30, there is no way to express this in a cron expression.

That is correct, there's no way to express this is crontab, the only thing I can think of doing is making the framework able to split something like this into two lines when you run php artisan schedule:list, so taking that example above, the output could be:

30-59 1 * * *  php artisan inspire ...................... Next Due: 1 hour from now
0-29 2 * * *  php artisan inspire ....................... Next Due: 1 hour from now

instead of

* 01-02 * * *  php artisan inspire .................. Next Due: 39 minutes from now

I'm not 100% how I'd go about doing the above, it's merely just a thought at this point.

JonPurvis avatar Aug 20 '24 23:08 JonPurvis

Yeah, I'm not really sure - I'm not sure trying to convert to cron expressions is the best way to solve this "Next Due" problem.

taylorotwell avatar Aug 21 '24 16:08 taylorotwell

I'm going to go back to the drawing board on this one and try and come up with a new way of handling the issue. I'll close this PR for now so it's not just sat there!

JonPurvis avatar Sep 16 '24 00:09 JonPurvis