search

dragonmantank / cron-expression

CRON for PHP: Calculate the next or previous run date and determine if a CRON expression is due
MIT License
4.57k stars 124 forks source link

Update CronExpression.php #128

Open afechser-bamboo opened 2 years ago

afechser-bamboo commented 2 years ago

If the DayOfMonth is set to L the usort reorders the $combined and then $nth points to the current date instead of the last day of the month.

dragonmantank commented 2 years ago

Do you happen to have an example of when this happens, or example an expression and expected next run dates? While all the tests pass with the fix, I'm not sure I 100% understand the problem.

afechser-bamboo commented 2 years ago

It seems to be when a cron expression like "0 20 L 6,12 ?", "0/5 L ?", or " LW * ?" is used. Basically anytime there is an L or LW in the Day Of Month spot.

The $combined variable contains something like [ "Last Day of Month Date Object", "Todays Date Object"] in the code the $nth variable is set to 0 so you would return $combined[$nth] which would point to "Last Day of Month Date Object" but the usort section changes the $combined array to ["Todays Date Object", "Last Day of Month Date Object"] which then makes $combined[$nth] return "Todays Date Object" causing the cron expression to run everyday instead of only on the last day of the month. Also the code I committed doesn't handle the "LW" case but it would have a similar result.

dragonmantank commented 2 years ago

Dug into this some more, and I think I have a potential fix in dev-master, if you can test it.

Day of Week and Day of Month are handled kind of weird.

When it comes to *, it doesn't mean what it means for all the other fields. In all the other fields, * means "anything in the range of this field", so * in the Minute field is the same as 0-59, * in the Hour field is the same as 0-23, etc. We were handling DOW and DOM the same.

The original cronie code actually handles this differently. * for DOM or DOW actually means IGNORE. So if you set * for the DOW field you shouldn't take it into account determining the next run date/should I run calculations, use DOM. The same applies vice-versa. We weren't doing that though, we were combining the results all the time.

If you want these fields to behave like the other fields, you should actually set the physical ranges (1-31 or 0-6). Then the OR rule for those fields kicks in (we match/calculate a run date that matches EITHER the DOM or DOW).

The fix should now handle this situation properly. Can you test and see if this works for you?

Some implementations also allow ? for those two fields, which is a non-standard character. It is effectively analogous to *, so the fix should work whether you supply * or ?.