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

Update CronExpression.php #128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

afechser-bamboo
Copy link

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.

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
Copy link
Owner

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
Copy link
Author

afechser-bamboo commented Jan 5, 2022

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
Copy link
Owner

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 ?.

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.

2 participants