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

Daylight savings time considerations #116

Open
mc0 opened this issue Feb 22, 2016 · 4 comments
Open

Daylight savings time considerations #116

mc0 opened this issue Feb 22, 2016 · 4 comments

Comments

@mc0
Copy link

mc0 commented Feb 22, 2016

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

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

mc0 commented Feb 23, 2016

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

dilab commented Aug 20, 2017

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

@dilab
Copy link

dilab commented Aug 26, 2017

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)

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

3 participants