mtdowling / cron-expression

CRON for PHP: Calculate the next or previous run date and determine if a CRON expression is due
http://mtdowling.com/blog/2012/06/03/cron-expressions-in-php/
MIT License
4.89k stars 339 forks source link

cron-expression throws an error when checking an expired cron expression #76

Closed ghoastfalcon closed 9 years ago

ghoastfalcon commented 9 years ago

I have noticed that I get a "Impossible CronExpression" error any time I have a cron expression that has expired or is in the past.

A little background on how we are using cron-expression. We have maintenance schedules which are scheduled via cron expressions, which makes perfect sense as many are recurring on a set schedule. However, there are times when the maintenance schedule is just for a couple of hours early in the morning and it is a one time thing. That is when we start running into issues.

Example: If today's date/time is 1/29/2015 12:00 and we have a cron expression for a maintenance schedule of "* 1-4 29 1 * 2015" we will get a "Impossible CronExpression" error thrown when checking it through the IsDue() method. Which I assume is because that expression represents a time in the past.

For entering a new cronExpression for a time in the past, an error or warning like this makes sense, but not for a cronExpression that, when entered, was/is valid and as time goes it becomes a time in the past. That should not error out I would think. Any ideas as to why this is happening? And how one would go about fixing it?

Any help on this issue would be most appreciated.

Thanks,

mtdowling commented 9 years ago

A cron expression that is no longer valid will always throw an error. There could potentially be more checks that quickly see if an expression is invalid before iterating over possible matches, but it would still result in the same error.

For entering a new cronExpression for a time in the past, an error or warning like this makes sense, but not for a cronExpression that, when entered, was/is valid and as time goes it becomes a time in the past. That should not error out I would think.

I'm not sure what you're suggesting. Are you saying that this library should know when you created a cron expression and then behave differently?

ghoastfalcon commented 9 years ago

Thanks for responding so quickly.

Ahhhh, I see. Disregard that part, the library should not necessarily know when you enter the expression. I was not thinking about it in the proper way. If it will always throw an error when checking for an expression in the past, then it would not matter "when" the expression was entered.

Although I am curious on why it throws an error for an expression that is in the past, rather than returning false. Throwing an error implies that it always expects an expression in the future or at least during that interval.

For example if now() = 1/29/2015 12:00 and the expression is "* 1-22 29 1 * 2015" that would still be valid and work just fine, but if we were to have "* 1-4 29 1 * 2015" that would result in error. Perhaps it should return a warning or false, meaning the interval is no longer applicable and will not return a isDue(), because it will never be due now, as it is in the past.

Thanks again for your help on this.

swekaj commented 9 years ago

This is actually the same problem as #60:

$ php -a
Interactive mode enabled

php > require('.../vendor/autoload.php');
php > $c = Cron\CronExpression::factory('1 1 1 1 * 2014');
php > var_dump($c->isDue());
PHP Warning:  Uncaught exception 'RuntimeException' with message 'Impossible CRON expression' in .../vendor/mtdowling/cron-expression/src/Cron/CronExpression.php:321
Stack trace:
#0 .../vendor/mtdowling/cron-expression/src/Cron/CronExpression.php(157): Cron\CronExpression->getRunDate('2015-01-29 19:1...', 0, false, true)
#1 .../vendor/mtdowling/cron-expression/src/Cron/CronExpression.php(252): Cron\CronExpression->getNextRunDate('2015-01-29 19:1...', 0, true)
#2 php shell code(1): Cron\CronExpression->isDue()
#3 {main}
  thrown in .../vendor/mtdowling/cron-expression/src/Cron/CronExpression.php on line 321

It seems the catch on line 251 isn't actually catching, which is weird.

swekaj commented 9 years ago

Well I feel silly. The exception wasn't being caught because I was on 1.0.3, not 1.0.4. Ignore me, and I'm pretty sure that's what @ghoastfalcon was experiencing as well.

ghoastfalcon commented 9 years ago

Indeed, I was using 1.0.3. I'll update to 1.04 and see how that goes. Thanks guys!

DerManoMann commented 9 years ago

I like the idea of being able to distinguish between a really invalid cron line and an expired one. Would it be possible to use a different exception for that. I am using the code also in an environment where cron lines might be limited and expire. Being able to show a different message depending on whether the line is actually invalid or expired would help a lot.

swekaj commented 9 years ago

That does happen. If you provide an invalid cron expression, you get an InvalidArgumentException. If the expression is not currently due, isDue() returns false. If you want to check to see if it will never be due again, you can use getNextRunDate() and catch the exception thrown. If a RuntimeException is thrown then the cron expression will never be due in the future and is thus expired.

mtdowling commented 9 years ago

Should be fixed in https://github.com/mtdowling/cron-expression/issues/92