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 issue with PHP 8.1 #120

Closed
joelstein opened this issue Oct 24, 2022 · 13 comments · Fixed by #121
Closed

Daylight Savings Time issue with PHP 8.1 #120

joelstein opened this issue Oct 24, 2022 · 13 comments · Fixed by #121

Comments

@joelstein
Copy link
Contributor

joelstein commented Oct 24, 2022

On Nov 6, 2022, Daylight Savings Time switches just before the 2 AM hour in the America/Chicago timezone.

I can't get this library to give me the correct time stamp or time zone when iterating to the 1 AM hour just before the time change.

For example:

$timezone = new \DateTimeZone('America/Chicago');

$rrule = new RRule([
  'FREQ' => 'WEEKLY',
  'DTSTART' => new \DateTime('2022-10-30T01:00', $timezone),
  'COUNT' => 2,
]);
echo $rrule[1]->format('c T U') . "\n";

$date = new \DateTime('2022-10-30T01:00:00', $timezone);
$date->modify('+1 week');
echo $date->format('c T U') . "\n";

$date = new \DateTime('2022-10-30T01:00:00', $timezone);
$date->add(new \DateInterval('P1W'));
echo $date->format('c T U') . "\n";

The output is:

2022-11-06T01:00:00-06:00 CST 1667718000
2022-11-06T01:00:00-05:00 CDT 1667714400
2022-11-06T01:00:00-05:00 CDT 1667714400

This library produces the wrong time (it's 1 hour late).

Yet, PHP's DateTime::modify and DateTime::add show the correct time.

Why is this?

@rlanvin
Copy link
Owner

rlanvin commented Oct 25, 2022

Hmm interesting, I copy/pasted your code to test this and got (tested with the latest master and various PHP versions on a Debian system):

2022-11-06T01:00:00-05:00 CDT 1667714400
2022-11-06T01:00:00-05:00 CDT 1667714400
2022-11-06T01:00:00-05:00 CDT 1667714400

What version of the library and of PHP are you using? Are you on Windows or Linux? It might be related to your setup.

@joelstein
Copy link
Contributor Author

Wow, that is interesting indeed.

I tested on PHP 8.1.9 and 8.1.11 on two environments: Ubuntu 18.04 and my local Mac.

I wonder if there's an external library that PHP depends on to calculate timezone offsets and daylight savings time rules?

@rlanvin
Copy link
Owner

rlanvin commented Oct 25, 2022

Ah ah! I tested on PHP 8.1 with a Docker image and got:

2022-11-06T01:00:00-06:00 CST 1667718000
2022-11-06T01:00:00-05:00 CDT 1667714400
2022-11-06T01:00:00-05:00 CDT 1667714400

So up to PHP 8.0 it works just fine, and then something changed in PHP 8.1... At least we're narrowing this down.

@joelstein
Copy link
Contributor Author

I think PHP's date/time logic is powered by timelib.

PHP 8.1.11 (current stable) includes timelib 2021.16, but there's a newer version, 2021.17, with several commits since the previous version. It was merged with php-src on 9/14, so I guess it will be available in PHP 8.1.12.

I don't know how to test or apply the new timelib version in case it resolves this issue.

But I guess the point of this issue is to understand why PHP's native modify and add functions produce the correct result, but the php-rrule library produces the incorrect result. Any ideas?

@rlanvin
Copy link
Owner

rlanvin commented Oct 25, 2022

Hmm at the moment no, I have no idea, but I'm testing. I changed the recurrence to DAILY to check, and switch back to central standard time do work, but with 1 day difference.

$timezone = new \DateTimeZone('America/Chicago');

$rrule = new RRule([
    'FREQ' => 'DAILY',
    'DTSTART' => new \DateTime('2022-10-30T01:00', $timezone),
    'COUNT' => 14,
]);

foreach ($rrule as $occurrence) {
    echo $occurrence->format('c T U') . "\n";
}

with php 8.1

2022-10-30T01:00:00-05:00 CDT 1667109600
2022-10-31T01:00:00-05:00 CDT 1667196000
2022-11-01T01:00:00-05:00 CDT 1667282400
2022-11-02T01:00:00-05:00 CDT 1667368800
2022-11-03T01:00:00-05:00 CDT 1667455200
2022-11-04T01:00:00-05:00 CDT 1667541600
2022-11-05T01:00:00-05:00 CDT 1667628000
2022-11-06T01:00:00-06:00 CST 1667718000 <-- 
2022-11-07T01:00:00-06:00 CST 1667804400
2022-11-08T01:00:00-06:00 CST 1667890800
2022-11-09T01:00:00-06:00 CST 1667977200
2022-11-10T01:00:00-06:00 CST 1668063600
2022-11-11T01:00:00-06:00 CST 1668150000
2022-11-12T01:00:00-06:00 CST 1668236400

with php 8.0

2022-10-30T01:00:00-05:00 CDT 1667109600
2022-10-31T01:00:00-05:00 CDT 1667196000
2022-11-01T01:00:00-05:00 CDT 1667282400
2022-11-02T01:00:00-05:00 CDT 1667368800
2022-11-03T01:00:00-05:00 CDT 1667455200
2022-11-04T01:00:00-05:00 CDT 1667541600
2022-11-05T01:00:00-05:00 CDT 1667628000
2022-11-06T01:00:00-05:00 CDT 1667714400
2022-11-07T01:00:00-06:00 CST 1667804400 <--
2022-11-08T01:00:00-06:00 CST 1667890800
2022-11-09T01:00:00-06:00 CST 1667977200
2022-11-10T01:00:00-06:00 CST 1668063600
2022-11-11T01:00:00-06:00 CST 1668150000
2022-11-12T01:00:00-06:00 CST 1668236400

@rlanvin
Copy link
Owner

rlanvin commented Oct 25, 2022

Interestingly, I do not observe the same behaviour for another timezone like Europe/Berlin, the results are identical with both versions:

$timezone = new \DateTimeZone('Europe/Berlin');

$rrule = new RRule([
    'FREQ' => 'DAILY',
    'DTSTART' => new \DateTime('2022-10-28T02:00', $timezone),
    'COUNT' => 6,
]);

foreach ($rrule as $occurrence) {
    echo $occurrence->format('c T U') . "\n";
}

with php 8.1

2022-10-28T02:00:00+02:00 CEST 1666915200
2022-10-29T02:00:00+02:00 CEST 1667001600
2022-10-30T02:00:00+01:00 CET 1667091600
2022-10-31T02:00:00+01:00 CET 1667178000
2022-11-01T02:00:00+01:00 CET 1667264400
2022-11-02T02:00:00+01:00 CET 1667350800

with php 8.0

2022-10-28T02:00:00+02:00 CEST 1666915200
2022-10-29T02:00:00+02:00 CEST 1667001600
2022-10-30T02:00:00+01:00 CET 1667091600
2022-10-31T02:00:00+01:00 CET 1667178000
2022-11-01T02:00:00+01:00 CET 1667264400
2022-11-02T02:00:00+01:00 CET 1667350800

I tried with both 2AM and 3AM as a start time just in case, because the summer time switch is between 2am and 3am, but the behaviour is exactly the same.

@rlanvin
Copy link
Owner

rlanvin commented Oct 25, 2022

This code reproduces the problem, by essentially reproducing what is happening inside the library when it calculates occurrences. The library doesn't use modify or add to calculate occurrences (it's too slow and generally impractical) - it uses arithmetic and then convert into DateTime objects.

$timezone = new \DateTimeZone('America/Chicago');

$date = new \DateTime('2022-10-30T01:00:00', $timezone);
$date->modify('+1 week');
echo $date->format('c T U') . "\n";

// the lib calculates occurrences using the day of the year (0 through 365), which is "z"
$date = \DateTime::createFromFormat(
    'Y z',
    $date->format('Y z'),
    $date->getTimezone()
);
// the lib then sets the time separately
$date->setTime(1,0,0);
echo $date->format('c T U') . "\n";

with PHP 8.0

2022-11-06T01:00:00-05:00 CDT 1667714400
2022-11-06T01:00:00-05:00 CDT 1667714400

with PHP 8.1

2022-11-06T01:00:00-05:00 CDT 1667714400
2022-11-06T01:00:00-06:00 CST 1667718000

@rlanvin rlanvin changed the title Daylight Savings Time issue Daylight Savings Time issue with PHP 8.1 Oct 25, 2022
@rlanvin
Copy link
Owner

rlanvin commented Oct 25, 2022

After testing a bit, my hypothesis is that because the "1am" time on the 6th of November in Chicago timezone literally exists twice (once in CDT, and then once again at 2am CDT / 1am CST), the ->setTime(1,0,0) has undefined behaviour - that clearly changed between 8.0 and 8.1 (or indeed, maybe changed in the underlying timelib).

But it seems modify is not bug free either. The behaviour with Europe/Berlin timezone is reversed with the test code:

with php 8.0, they both become winter time (in America/Chicago they both stayed in summer time)

modify +1 week  2022-10-30T02:00:00+01:00 CET 1667091600
lib creation    2022-10-30T02:00:00+01:00 CET 1667091600

with php 8.1 the modify +1 week stays in summer time 🤷

modify +1 week  2022-10-30T02:00:00+02:00 CEST 1667088000
lib creation    2022-10-30T02:00:00+01:00 CET 1667091600

@joelstein
Copy link
Contributor Author

Thank you so much for such timely responses, @rlanvin. I greatly appreciate the work you put into building this excellent library.

I couldn't find the cause of this weird DST issue, but we devised a workaround for our use case. We map events to hours on a weekly schedule, and the Nov 6 at 1 AM hour gave us trouble. Our workaround was to use a local, week-focused date format (l-H:i) to match things together, without the timezone offset.

Anyway, thanks again!

@joelstein
Copy link
Contributor Author

Ah, as you suspected, in PHP 8.1.0, the behavior of setTime changed:

The behaviour with double existing hours (during the fall-back DST transition) changed. Previously PHP would pick the second occurrence (after the DST transition), instead of the first occurrence (before DST transition).

@joelstein
Copy link
Contributor Author

I figured it out! It's an issue with createFromFormat. From the docs:

All fields are initialised with the current date/time.

If we omit time from the format, the current time is assumed.

$timezone = new \DateTimeZone('America/Chicago');

\DateTime::createFromFormat('Y z', '2022 309', $timezone);
// 2022-11-06T06:17:11-06:00 CST

\DateTime::createFromFormat('Y z H:i:s', '2022 309 00:00:00', $timezone);
// 2022-11-06T00:00:00-05:00 CDT 

Without the time, depending on the time of day that we calculate the date, it would have a different timezone offset.

To fix this, we should set the time to 00:00:00 in the format and adjust with setTime.

@rlanvin
Copy link
Owner

rlanvin commented Nov 6, 2022

Oh wow, great find! That definitely explains why PHP 8.1 is returning different results.

Your solution looks promising, especially since I had a lot of issues in the past with the fact that current time is assumed, but it's not obvious why it would solve the issue (I would need to test a bit to wrap my head around it). Thanks for submitting a PR - I think I'd like to add some unit tests for this bug to make sure the behaviour is now the same for all versions of PHP. Would you like to add them? If not, I'll have a look myself but probably a bit later this month.

@joelstein
Copy link
Contributor Author

I added a test in 045bbf1, but I'm not sure if that's what you are looking for. I'm happy to write some more tests if it helps.

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

Successfully merging a pull request may close this issue.

2 participants