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 337 forks source link

Daylight savings time considerations #116

Open mc0 opened 8 years ago

mc0 commented 8 years ago

I didn't notice anything in the source handling daylight savings time. Cron's consideration for this is that any time changes less than 3 hours should be considered daylight savings time.

Not handling this would mean that hour-based runs could run more than they are suppose to. For example if something were scheduled for 2AM it would run twice when rolling back.

See: https://www.pantz.org/software/cron/croninfo.html

dragonmantank commented 8 years ago

I want to say that this is outside the scope of the library most likely.

The Cron program does a few different things, which includes collecting and managing all of the system and user crontab entries, parsing those crontab entries, psuedo-remembering when stuff ran last, calling programs, and all of the logic around making that stuff run. In Cron's case, it has to be very cognizant of time shifting due to daylight savings time or even system clocks being updated.

cron-expression handles only a portion of that, which is determining if a cron expression meets the criteria of needing to run right now. It doesn't handle anything else. So if you have a cron expression that says "0 1 * * *", we check to see if it's 1am and tell if the expression matches the current time. What you do with that information is up to your application. If 1am happens twice, we'll evaluate that as true twice because your application asked us to check it.

I'll leave this open for a while unless I'm missing something, but this sounds like more of a job for the overall application to handle, not the library.

mc0 commented 8 years ago

@dragonmantank I don't think that's a bad viewpoint. This may seem like a nitpick but it'd be nice if the documentation at least explained it doesn't handle daylight savings time adjustments. That's not to say everything a library does or doesn't do should be explained but DST seems like an important subject in this case. At least one project that uses cron-expression seems to assume cron-expression handles DST to a certain extent (by comparing it to Cron): Jobby.

Although, this looks like it is actually a bug (and is partially how I'd implement a solution in an application without reinventing the wheel):

require 'vendor/autoload.php';
$cron = Cron\CronExpression::factory('0 3 * * *');
date_default_timezone_set('America/New_York');
echo $cron->getPreviousRunDate(new DateTime('2016-03-13 01:00:00'))->format('Y-m-d H:i:s T');
// 2016-03-12 03:00:00 EST
echo $cron->getPreviousRunDate(new DateTime('2016-03-13 02:00:00'))->format('Y-m-d H:i:s T');
// PHP Warning:  Uncaught exception 'RuntimeException' with message 'Impossible CRON expression' in .../cron-expression/src/Cron/CronExpression.php:379
echo $cron->getPreviousRunDate(new DateTime('2016-03-13 03:00:00'))->format('Y-m-d H:i:s T');
// PHP Warning:  Uncaught exception 'RuntimeException' with message 'Impossible CRON expression' in .../cron-expression/src/Cron/CronExpression.php:379
echo $cron->getPreviousRunDate(new DateTime('2016-03-13 04:00:00'))->format('Y-m-d H:i:s T');
// 2016-03-13 03:00:00 EDT

I'd be open to submitting a PR but I also know there is a bunch of experience in those 60 lines of code for getRunDate.

dilab commented 7 years ago

The least we can do is to have a section in the document on how to handle timezone(Daylight savings)?

dilab commented 7 years ago

What I ended up doing is something like:

date_default_timezone_set('target_tz');

$cronExpression = CronExpression::factory(
    '0 6 * * *'
);

$result = $cronExpression->getPreviousRunDate()->format('Y-m-d H:i:s');

date_default_timezone_set('UTC'); 

This way, I get desired prev run date without affecting the global timezone (UTC)