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

range(): step exceeds the specified range #88

Closed
pjebs opened this issue Aug 19, 2020 · 7 comments
Closed

range(): step exceeds the specified range #88

pjebs opened this issue Aug 19, 2020 · 7 comments

Comments

@pjebs
Copy link

pjebs commented Aug 19, 2020

@dragonmantank
I'm using v2 of library:

$now = new Carbon();
$cron = CronExpression::factory("41-59/24 5 * * 5");
for ($k = 0 ; $k < 20; $k++){
	$t = $cron->getNextRunDate($now, $k);
	$times[] = $t; // Store next 20 values
}
		

Error:

range(): step exceeds the specified range

/vendor/dragonmantank/cron-expression/src/Cron/AbstractField.php:148
/vendor/dragonmantank/cron-expression/src/Cron/AbstractField.php:53
/vendor/dragonmantank/cron-expression/src/Cron/MinutesField.php:31
/vendor/dragonmantank/cron-expression/src/Cron/CronExpression.php:362
/vendor/dragonmantank/cron-expression/src/Cron/CronExpression.php:199

@pjebs
Copy link
Author

pjebs commented Aug 19, 2020

51-59/57 14 * * 0,1,2,3,4,5,6 also generates the same error.

12-17/43 17 * * 1,3,4,5,6

@pjebs
Copy link
Author

pjebs commented Aug 19, 2020

In PHP 8, this range($rangeStart, $rangeEnd, $step) in AbstractField.php will return a catchable ValueError.

Until then, it needs to handled in the code.

@pjebs
Copy link
Author

pjebs commented Aug 21, 2020

Obviously the cron string is nonsensical - but still valid so it should not throw an error.

@dragonmantank
Copy link
Owner

Obviously the cron string is nonsensical - but still valid

Within the bounds of this library, it's invalid. What happens with a step larger than the range is ambiguous - does it wrap around, does it only run the first, does it truncate? Some libraries handle it, some do not.

This would be the intended error. Your intended step is too large, and will not function, so the error is thrown. It's better to have the error letting you know the cron is invalid than let it "run" with unintended results.

@dragonmantank
Copy link
Owner

dragonmantank commented Aug 21, 2020

Reopening, because 2 years ago I validated the cronie allowed wrap-around steps. Since this library follows cronie, the larger step should wrap around to the beginning and continue counting.

This looks like a break between validation and when we do the stepping on small ranges versus *, so will get that corrected.

@dragonmantank dragonmantank reopened this Aug 21, 2020
@dragonmantank
Copy link
Owner

Sorry this took so long. It's actually a few different subtle issues that dealt wit "how much can break."

So, cronie supports steps larger than a range, but it doesn't actually wrap around. Internally it's using a for() loop so large steps never actually affect the validation, where PHP's range() method has some checks built in.

This library does have, incorrectly, some logic to handle wrapping around. That will be going away. I fixed this particular bug with the correct behavior, so if you specify a range that is smaller than max, but a step that is larger than the small range, you will only ever match against the first value in the range. So given 41-59/24, you will only match minute 41, not 41 + 47 (if I did the wraparound math right).

So given a run date of 2021-08-25 10:00:00, your next run dates are:

  • 2021-08-26 05:41:00
  • 2021-08-27 05:41:00
  • 2021-08-28 05:41:00

This is fixed in master, I will update and close the ticket once I cut a release.

@dragonmantank
Copy link
Owner

This should be fixed with v3.2.2

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

No branches or pull requests

2 participants