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

Wrong nextRunDate for * rules #153

Closed
decadence opened this Issue Jun 14, 2017 · 15 comments

Comments

Projects
None yet
4 participants
@decadence

decadence commented Jun 14, 2017

Thanks for library.
But it gives wrong result for * rules (0 0 1 */3 * should run 1,4,7,10 month, in fact it runs on 3,6,9,12).

Please see this discussion for more info.

@dragonmantank

This comment has been minimized.

Show comment
Hide comment
@dragonmantank

dragonmantank Jun 14, 2017

Collaborator

The expression should fire on the 0th second of the 0th minute of the 1st hour of every 3rd month (3,6,9,12), so that would be intended.

The / character is not a step value starting from the beginning of the of the option, and adding on, but an indicator of which element to fire on. So with an array of [1,2,3,4,5,6,7,8,9,10,11,12], you would fire on every 3rd element.

If you want to fire on 1,4,7,10, you are better off using 0 0 1 1,4,7,10 *, as there is no shortcut to allow you to start at 1 and then increment from there.

Collaborator

dragonmantank commented Jun 14, 2017

The expression should fire on the 0th second of the 0th minute of the 1st hour of every 3rd month (3,6,9,12), so that would be intended.

The / character is not a step value starting from the beginning of the of the option, and adding on, but an indicator of which element to fire on. So with an array of [1,2,3,4,5,6,7,8,9,10,11,12], you would fire on every 3rd element.

If you want to fire on 1,4,7,10, you are better off using 0 0 1 1,4,7,10 *, as there is no shortcut to allow you to start at 1 and then increment from there.

@decadence

This comment has been minimized.

Show comment
Hide comment
@decadence

decadence Jun 14, 2017

Sorry but you're wrong.
It's inconsistent with real cron rules where / is start + N exactly (beginning of the of the option). And it allows to step from first position and we don't have to enumerate all values (what if I have 1000 of them?)

This lib is "CRON for PHP" so it should follow CRON rules I suppose.

Proof

decadence commented Jun 14, 2017

Sorry but you're wrong.
It's inconsistent with real cron rules where / is start + N exactly (beginning of the of the option). And it allows to step from first position and we don't have to enumerate all values (what if I have 1000 of them?)

This lib is "CRON for PHP" so it should follow CRON rules I suppose.

Proof

@dragonmantank

This comment has been minimized.

Show comment
Hide comment
@dragonmantank

dragonmantank Jun 14, 2017

Collaborator

“At 00:00 on day-of-month 1 in every 3rd month.”

January, April, July, October are not every three months.

Collaborator

dragonmantank commented Jun 14, 2017

“At 00:00 on day-of-month 1 in every 3rd month.”

January, April, July, October are not every three months.

@decadence

This comment has been minimized.

Show comment
Hide comment
@decadence

decadence Jun 14, 2017

Why? 4 - 1 = 3.
screenshot_1

decadence commented Jun 14, 2017

Why? 4 - 1 = 3.
screenshot_1

@decadence

This comment has been minimized.

Show comment
Hide comment
@decadence

decadence Jun 14, 2017

Then start position can be set with 2-12/3.
In current situation we can't set start from 1 while cron allows to do it.

decadence commented Jun 14, 2017

Then start position can be set with 2-12/3.
In current situation we can't set start from 1 while cron allows to do it.

@dragonmantank

This comment has been minimized.

Show comment
Hide comment
@dragonmantank

dragonmantank Jun 14, 2017

Collaborator

As I'm getting conflicting information depending on the source I'm looking at, I'm going to go through the cronie source and see exactly what it's doing. I'll re-open this ticket for now.

Changing the range to 1-12/3 yields the result you are expecting, so the naive modulus check we do for * is probably incorrect for ranges that do not start with 0.

At best this will get fixed in the current master branch and slated for the next release. A quick round of testing shows that it will break existing behavior on the day of month as well, as that range is also starts with 1. As this package is included in Laravel, that's a pretty massive change that could break a lot of functioning systems.

I doubt the impending v2.0.0 library, which requires PHP 7, will be compatible with Laravel any time soon, as Laravel master still requires 5.6. v2.0.0 will have a much improved validation system, as there are other areas where v1 was very lax on checks.

In the meantime, you should be able to use 0 0 1 1-12/3 *, as the v1.2.0 library does have support for specifying the ranges.

Collaborator

dragonmantank commented Jun 14, 2017

As I'm getting conflicting information depending on the source I'm looking at, I'm going to go through the cronie source and see exactly what it's doing. I'll re-open this ticket for now.

Changing the range to 1-12/3 yields the result you are expecting, so the naive modulus check we do for * is probably incorrect for ranges that do not start with 0.

At best this will get fixed in the current master branch and slated for the next release. A quick round of testing shows that it will break existing behavior on the day of month as well, as that range is also starts with 1. As this package is included in Laravel, that's a pretty massive change that could break a lot of functioning systems.

I doubt the impending v2.0.0 library, which requires PHP 7, will be compatible with Laravel any time soon, as Laravel master still requires 5.6. v2.0.0 will have a much improved validation system, as there are other areas where v1 was very lax on checks.

In the meantime, you should be able to use 0 0 1 1-12/3 *, as the v1.2.0 library does have support for specifying the ranges.

@dragonmantank dragonmantank reopened this Jun 14, 2017

@decadence

This comment has been minimized.

Show comment
Hide comment
@decadence

decadence Jun 14, 2017

Thanks. You're right, it's different for non zero based date components.
In fact next Laravel release will require PHP 7 (master branch, not 5.4)

decadence commented Jun 14, 2017

Thanks. You're right, it's different for non zero based date components.
In fact next Laravel release will require PHP 7 (master branch, not 5.4)

@dragonmantank

This comment has been minimized.

Show comment
Hide comment
@dragonmantank

dragonmantank Jun 16, 2017

Collaborator

I learned two things - I hate digging through other people's C code and we are doing * expansions wrong.

According to the cronie source code, * gets expanded out to the list for each type, based on their range. That range is then used to determine the stepping, and that stepping is different for zero-based ranges versus one-based ranges.

Since this changes a fundamental bit of logic, this will definitely only be going into v2.x and I'm going to treat it as a BC-breaking change. The library has been out too long and too many people may be relying on the bad logic, effectively turning this from a bug to a "feature," in a way.

Thank you @decadence for bringing this up.

Collaborator

dragonmantank commented Jun 16, 2017

I learned two things - I hate digging through other people's C code and we are doing * expansions wrong.

According to the cronie source code, * gets expanded out to the list for each type, based on their range. That range is then used to determine the stepping, and that stepping is different for zero-based ranges versus one-based ranges.

Since this changes a fundamental bit of logic, this will definitely only be going into v2.x and I'm going to treat it as a BC-breaking change. The library has been out too long and too many people may be relying on the bad logic, effectively turning this from a bug to a "feature," in a way.

Thank you @decadence for bringing this up.

@decadence

This comment has been minimized.

Show comment
Hide comment
@decadence

decadence Jun 16, 2017

This is exactly what I learned recently about cron *
I'm glad my investigation was helpful.

decadence commented Jun 16, 2017

This is exactly what I learned recently about cron *
I'm glad my investigation was helpful.

@leeoniya

This comment has been minimized.

Show comment
Hide comment
@leeoniya

leeoniya Aug 16, 2017

@dragonmantank any updates on this or v2?

leeoniya commented Aug 16, 2017

@dragonmantank any updates on this or v2?

@dragonmantank

This comment has been minimized.

Show comment
Hide comment
@dragonmantank

dragonmantank Aug 16, 2017

Collaborator

@leeoniya I just pushed a big update for a bunch of validation fixes, so this is next on the radar.

Collaborator

dragonmantank commented Aug 16, 2017

@leeoniya I just pushed a big update for a bunch of validation fixes, so this is next on the radar.

@dragonmantank

This comment has been minimized.

Show comment
Hide comment
@dragonmantank

dragonmantank Aug 18, 2017

Collaborator

@leeoniya @decadence

This should now be fixed in the latest master. Sorry this took so long, I spent a bunch of time getting the validation fixed, which made this fix easier.

The / operator should now be much more robust than before, and should be working for fields that now have a range starting at 1 and a step of */X.

I'd appreciate it if you could take a look. The unit tests pass, but some better real world testing would be great. I'll leave this issue open for a while.

Thanks!

Collaborator

dragonmantank commented Aug 18, 2017

@leeoniya @decadence

This should now be fixed in the latest master. Sorry this took so long, I spent a bunch of time getting the validation fixed, which made this fix easier.

The / operator should now be much more robust than before, and should be working for fields that now have a range starting at 1 and a step of */X.

I'd appreciate it if you could take a look. The unit tests pass, but some better real world testing would be great. I'll leave this issue open for a while.

Thanks!

@decadence

This comment has been minimized.

Show comment
Hide comment
@decadence

decadence Sep 9, 2017

@dragonmantank Thanks for your work. I can't check it because I don't use your library directly in any project but I believe you handled it right :)

decadence commented Sep 9, 2017

@dragonmantank Thanks for your work. I can't check it because I don't use your library directly in any project but I believe you handled it right :)

@dragonmantank

This comment has been minimized.

Show comment
Hide comment
@dragonmantank

dragonmantank Oct 12, 2017

Collaborator

2.x has been released over at https://github.com/dragonmantank/cron-expression with the latest fixes (more info available at http://ctankersley.com/2017/10/12/cron-expression-update/).

If you are using this package with Laravel (as I'm assuming most of you are in this issue chain), I'll be working with them to get this updated in their next release.

Collaborator

dragonmantank commented Oct 12, 2017

2.x has been released over at https://github.com/dragonmantank/cron-expression with the latest fixes (more info available at http://ctankersley.com/2017/10/12/cron-expression-update/).

If you are using this package with Laravel (as I'm assuming most of you are in this issue chain), I'll be working with them to get this updated in their next release.

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp Nov 1, 2017

@dragonmantank respect for your bug-hunting and -solving skills 👍 Will have a look at the new fork. The "only" major change is that expressions as mentioned in this issue are now properly evaluated? No other migration steps required?

holtkamp commented Nov 1, 2017

@dragonmantank respect for your bug-hunting and -solving skills 👍 Will have a look at the new fork. The "only" major change is that expressions as mentioned in this issue are now properly evaluated? No other migration steps required?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment