dragonmantank / cron-expression

CRON for PHP: Calculate the next or previous run date and determine if a CRON expression is due
MIT License
4.51k stars 117 forks source link

DST and getNextRunDate skips day #154

Open einarsozols opened 1 year ago

einarsozols commented 1 year ago

America/New_York time zone changed to daylight saving time at 2:00 AM on Sunday, Mar 12, 2023.

// php version 8.1.16, package version 3.3.2
$date = new DateTime('2023-03-11 08:00:00', new DateTimeZone('America/New_York'));
$cron = new CronExpression('30 07 * * *');
$nextDate = $cron->getNextRunDate($date);

Expected date: 2023-03-12 07:30:00.0 America/New_York (-04:00) Returned date: 2023-03-13 07:30:00.0 America/New_York (-04:00)

It works fine for other days, for example, if check would happen every day at 08:00 starting from 8th march:

2023-03-09 07:30:00.0 America/New_York (-05:00)
2023-03-10 07:30:00.0 America/New_York (-05:00)
2023-03-11 07:30:00.0 America/New_York (-05:00)
// Missing 12th March
2023-03-13 07:30:00.0 America/New_York (-04:00)
2023-03-14 07:30:00.0 America/New_York (-04:00)
2023-03-15 07:30:00.0 America/New_York (-04:00)
holtkamp commented 1 year ago

Probably related to https://github.com/dragonmantank/cron-expression/issues/133

Encountered similar behaviour.

Europe/Amsterdam changes to Daylight Saving Time at 02:00 AM on Sunday, Mar 26, 2023 / 2023-03-26 02:00

When a weekday is indicated in the CRON expression, for example Sunday (day 7), not a day is skipped, but a week:

   public function testIssueEnteringDateTimeAmsterdamOnSunday(): void
    {
        $expression = "* 12 * * 7";
        $cron = new CronExpression($expression);
        $tz = new \DateTimeZone("Europe/Amsterdam");

        $dtExpected = $this->createDateTimeExactly("2023-03-26 12:00+02:00", $tz);

        $dtCurrent = \DateTimeImmutable::createFromFormat("!Y-m-d H:i:s", "2023-03-26 00:00:00", $tz);
        $dtActual = $cron->getNextRunDate($dtCurrent, 0, true, $tz->getName());
        $this->assertEquals($dtExpected, $dtActual);
    }

results in:

3) Cron\Tests\DaylightSavingsTest::testIssueEnteringDateTimeAmsterdamOnSunday
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
-2023-03-26T12:00:00.000000+0200
+2023-04-02T12:00:00.000000+0200

When a range of hours like 12-15 is indicated in the CRON expression, not a day is skipped, but an hour:

    public function testIssueEnteringDateTimeRangeAmsterdam(): void
    {
        $expression = "* 12-15 * * *";
        $cron = new CronExpression($expression);
        $tz = new \DateTimeZone("Europe/Amsterdam");

        $dtExpected = $this->createDateTimeExactly("2023-03-26 12:00+02:00", $tz);

        $dtCurrent = \DateTimeImmutable::createFromFormat("!Y-m-d H:i:s", "2023-03-26 00:00:00", $tz);
        $dtActual = $cron->getNextRunDate($dtCurrent, 0, true, $tz->getName());
        $this->assertEquals($dtExpected, $dtActual);
    }

results in:

1) Cron\Tests\DaylightSavingsTest::testIssueEnteringDateTimeRangeAmsterdam
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
-2023-03-26T12:00:00.000000+0200
+2023-03-26T13:00:00.000000+0200
einarsozols commented 1 year ago

I tested package 3.3.2 with php 8.1, 7.4, 7.2 All of them have the same issue as I described in my issue.

Tried to test php 8.1 with different package versions.

3.3.2: FAIL 3.3.1: FAIL 3.3.0: FAIL 3.2.4: FAIL 3.2.3: FAIL 3.2.2: FAIL 3.2.1: FAIL 3.2.0: SUCCESS

So I guess the problem is somewhere here: https://github.com/dragonmantank/cron-expression/compare/v3.2.0...dragonmantank:cron-expression:v3.2.1

holtkamp commented 1 year ago

Indeed, I suspect this function https://github.com/dragonmantank/cron-expression/blob/782ca5968ab8b954773518e9e49a6f892a34b2a8/src/Cron/HoursField.php#L123-L129

Apparently UTC does not use/apply/consider DST: https://www.worldtimeserver.com/learn/does-utc-observe-daylight-saving-time

UTC has no DST because it serves as a constant frame of reference in which other time zones are relative. Take note that Coordinated Universal Time does not change with a change of seasons. However, its local time or civil time can vary depending on the time zone jurisdiction.

This "feature" seems to be used here, but might have unexpected side-effects?

Dr-Shadow commented 1 year ago

@dragonmantank Would be it possible to get a fix for the next daylight saving time schedule which would be around October/November ?

dragonmantank commented 10 months ago

3.2.1 did introduce a change to DST handling, as there were instances were it was skipping (particularly in +1 timezones and the London timezone). There was also a change in how DST is handled in PHP ~8.0 sometime, which broke how it was handled in prior versions.

I'll see what I can do since this is coming up, but part of the fix might be just dropping older PHP support to simplify DST handling.

curunoir commented 4 months ago

Hello, is this issue resolved in 3.3.3 ?

daum commented 3 months ago

Don't believe so we just hit it.

dragonmantank commented 3 months ago

Yeah, unfortunately I was working on it last week but you fix one time change bug, and it affects something else.

curunoir commented 3 months ago

Thanks for the updates. :)

plsoucy-tapclicks commented 3 months ago

If we are only using this in North American time zones, would downgrading to 3.2.0 be a good way to prevent the issue until you find a long-term fix? Just looking for options to make sure we don't get impacted on the next DST change.

Thanks for your help!

daum commented 3 months ago

We downgraded to that and I tested out a few different scenarios using timecop and it worked for us!

dragonmantank commented 2 months ago

I believe I have a fix for this, it is due to how time math was being handled. Before I release a fix, could I get your thoughts on https://github.com/dragonmantank/cron-expression/discussions/181 as it will impact the overall solution?