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

DST and getNextRunDate skips day #154

Open
einarsozols opened this issue Mar 14, 2023 · 13 comments
Open

DST and getNextRunDate skips day #154

einarsozols opened this issue Mar 14, 2023 · 13 comments

Comments

@einarsozols
Copy link

einarsozols commented Mar 14, 2023

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

holtkamp commented Mar 15, 2023

Probably related to #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
Copy link
Author

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: v3.2.0...dragonmantank:cron-expression:v3.2.1

@holtkamp
Copy link

holtkamp commented Mar 17, 2023

Indeed, I suspect this function

public function increment(DateTimeInterface &$date, $invert = false, $parts = null): FieldInterface
{
$originalTimestamp = (int) $date->format('U');
// Change timezone to UTC temporarily. This will
// allow us to go back or forwards and hour even
// if DST will be changed between the hours.

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

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

@dragonmantank
Copy link
Owner

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

curunoir commented Mar 4, 2024

Hello, is this issue resolved in 3.3.3 ?

@daum
Copy link

daum commented Mar 11, 2024

Don't believe so we just hit it.

@dragonmantank
Copy link
Owner

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

@curunoir
Copy link

Thanks for the updates. :)

@plsoucy-tapclicks
Copy link

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

daum commented Mar 18, 2024

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

@dragonmantank
Copy link
Owner

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 #181 as it will impact the overall solution?

@sc0ttdav3y
Copy link

We had this happen last October for Australia/Sydney, and we're expecting it to happen again this weekend. We're on 3.3.3. I only discovered this issue today. We'll try out 3.2.0 as a workaround for now.

@dragonmantank, thank you for this library and your work in maintaining it ❤️. If you had to drop 7.4 to simplify the internals, I'd be fine with that, and I say this as an engineer that still maintains some legacy 7.4 systems.

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

8 participants