Wrong nextRunDate for * rules
decadence opened this issue · 15 comments
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.
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.
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.
“At 00:00 on day-of-month 1 in every 3rd month.”
January, April, July, October are not every three months.
Why? 4 - 1 = 3.
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.
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.
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)
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.
This is exactly what I learned recently about cron *
I'm glad my investigation was helpful.
@dragonmantank any updates on this or v2?
@leeoniya I just pushed a big update for a bunch of validation fixes, so this is next on the radar.
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!
@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 :)
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.
@dragonmantank respect for your bug-hunting and -solving skills