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 saving and PHP8.1 #133

Open
nyamsprod opened this issue Jan 11, 2022 · 5 comments
Open

Daylight saving and PHP8.1 #133

nyamsprod opened this issue Jan 11, 2022 · 5 comments

Comments

@nyamsprod
Copy link

In PHP8.1 some improvements around daylight saving did land and they are messing with how the next run is being calculated. The
DaylightSavingsTest test suite is having many issues.

Excerpt from this blog post since I fail to find the correct commit/entry in the php changelog

Improved management of DateTime and daylight saving time transitions
Just a little section to tell you about some improvements about transitions to Daylight saving time.
When you’re trying to create February 29th on a non-leap year, PHP will “round” your date so it’s a valid and existing one. This RFC propose to apply this behaviour to hours that don’t exist because of Daylight saving time transitions: it will “round” the unexisting time to a valid one. This will result in a way much simpler management of dates and times in PHP.
Here is a concrete example: in France, the next time change to daylight saving time will happen on March 27th 2022. We’ll go from 2:00 am to 3:00 am directly, which means 2:30 am won’t exist. If you try to create a Datetime for March 27th, 2:30 am, PHP will automatically “round” it to March 27th, 3:30 am.
Note that this RFC has been created… 10 years ago, back in 2011!

see https://medium.com/geekculture/php-8-1-is-coming-2nd-round-of-upcoming-features-e1c5ddd14b38

I believe this makes the following tests fail.

2) Cron\Tests\DaylightSavingsTest::testOffsetIncrementsNextRunDate
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
-2021-03-28T02:00:00.000000+0100
+2021-04-04T01:00:00.000000+0100

There are other similar error since I do not want to change the tests maybe something needs to happen here in the code to selectively prevent any changes in PHP8.1 🤔 .

Also some other tests just end up in an infinite loop

Cron\Tests\DaylightSavingsTest::testOffsetIncrementsPreviousRunDate
2) Cron\Tests\DaylightSavingsTest::testOffsetIncrementsPreviousRunDate
RuntimeException: Impossible CRON expression

/Users/i.nyamagana.butera/dev/alt-cron-expression/src/Cron/CronExpression.php:461
/Users/i.nyamagana.butera/dev/alt-cron-expression/src/Cron/CronExpression.php:226
/Users/i.nyamagana.butera/dev/alt-cron-expression/tests/Cron/DaylightSavingsTest.php:95
@nyamsprod
Copy link
Author

nyamsprod commented Jan 12, 2022

OK I read the PHP 8.1 changelog and indeed seems the whole DateInterval and DateTimeZone calculation where re-implemented and moreso fixed. Hence the reason why most of the Timezone issue this package has do not show up in my fork which is PHP8.1+ only .

For complete reference look at https://www.php.net/ChangeLog-8.php#PHP_8_1 the Date part. 🤔

@dragonmantank
Copy link
Owner

Currently as of v3.2.4 all of the existing tests seem to work with the DST patch that was applied (and subsequently refined) in v3.2.1, and currently work from PHP 7.2 to 8.1.

Can you take a look at the newest release and see if you are still having issues on PHP 8.1?

@rgson
Copy link

rgson commented Jul 19, 2022

I'm seeing test failures in DaylightSavingsTest with v3.3.1 and PHP 8.1. Tests pass on PHP 7.4.

> php --version
PHP 8.1.7 (cli) (built: Jun 25 2022 07:57:04) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.7, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.7, Copyright (c), by Zend Technologies

> git status +short
HEAD detached at v3.3.1
nothing to commit, working tree clean

> vendor/bin/phpunit
PHPUnit 9.5.21 #StandWithUkraine

...............................................................  63 / 167 ( 37%)
............................................................... 126 / 167 ( 75%)
.......FF..F.F...........................                       167 / 167 (100%)

Time: 00:00.029, Memory: 8.00 MB

There were 4 failures:

1) Cron\Tests\DaylightSavingsTest::testOffsetDecrementsNextRunDateAllowCurrent
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
-2020-10-25T01:00:00.000000+0000
+2020-10-25T01:00:00.000000+0100

[...]/tests/Cron/DaylightSavingsTest.php:125

2) Cron\Tests\DaylightSavingsTest::testOffsetDecrementsNextRunDateDisallowCurrent
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
-2020-10-25T01:00:00.000000+0000
+2020-10-25T01:00:00.000000+0100

[...]/tests/Cron/DaylightSavingsTest.php:159

3) Cron\Tests\DaylightSavingsTest::testOffsetDecrementsMultipleRunDates
Failed asserting that an array contains DateTimeImmutable Object &00000000000000fd0000000000000000 (
    'date' => '2020-11-08 01:00:00.000000'
    'timezone_type' => 3
    'timezone' => 'Europe/London'
).

[...]/tests/Cron/DaylightSavingsTest.php:248

4) Cron\Tests\DaylightSavingsTest::testOffsetDecrementsEveryOtherHour
Failed asserting that an array contains DateTimeImmutable Object &00000000000000fe0000000000000000 (
    'date' => '2020-10-25 07:00:00.000000'
    'timezone_type' => 3
    'timezone' => 'Europe/London'
).

[...]/tests/Cron/DaylightSavingsTest.php:367

FAILURES!
Tests: 167, Assertions: 775, Failures: 4.

@athos-ribeiro
Copy link

Here is a reproducer:

#!/bin/bash

docker_build() {
  docker build - <<EOF
FROM composer:latest as composer
FROM php:${1}
COPY --from=composer /usr/bin/composer /usr/bin/composer
RUN apt update
RUN apt install -y git unzip
RUN git clone https://github.com/dragonmantank/cron-expression
workdir /cron-expression
RUN composer --no-plugins install
RUN composer --no-plugins test
EOF

}

for TAG in 7.4 8.0.22 8.1.0 8.1.5 8.1.6 8.1.7 8.1.8 8.1.9 8.2-rc-buster 8.1; do
  if docker_build ${TAG} > /dev/null 2>&1; then
    echo "${TAG}: SUCCESS"
  else
    echo "${TAG}: FAILURE"
  fi
done

Which outputs:

7.4: SUCCESS
8.0.22: SUCCESS
8.1.0: FAILURE
8.1.5: FAILURE
8.1.6: SUCCESS
8.1.7: FAILURE
8.1.8: FAILURE
8.1.9: FAILURE
8.2-rc-buster: FAILURE
8.1: FAILURE

By going through https://www.php.net/ChangeLog-8.php, I see several changes to the date implementation throughout the patch releases.

This seems to be the commit that fixed 8.1.6:
php/php-src@e6c4988

There is a related php upstream issue at php/php-src#9165

athos-ribeiro added a commit to athos-ribeiro/cron-expression that referenced this issue Sep 1, 2022
As discussed in dragonmantank#133, the PHP 8.1's date extension daylight saving
APIs have been suffering with instabilities. Let's skip the affected
tests until php/php-src#9165 is resolved.

Signed-off-by: Athos Ribeiro <[email protected]>
dragonmantank pushed a commit that referenced this issue Sep 6, 2022
As discussed in #133, the PHP 8.1's date extension daylight saving
APIs have been suffering with instabilities. Let's skip the affected
tests until php/php-src#9165 is resolved.

Signed-off-by: Athos Ribeiro <[email protected]>

Signed-off-by: Athos Ribeiro <[email protected]>
@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?

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

4 participants