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

Update the names of timezones #2871

Closed
wants to merge 1 commit into from

Conversation

rgson
Copy link
Contributor

@rgson rgson commented Oct 19, 2023

Some timezones have had their names changed. The newer names are recommended in newer versions of PHP [1].

Asia/Calcutta -> Asia/Kolkata
Europe/Kiev   -> Europe/Kyiv
US/Pacific    -> America/Los_Angeles

[1] https://www.php.net/manual/en/timezones.others.php


The old names cause test failures on systems where legacy time zone data is not installed.

Some timezones have had their names changed. The newer names are
recommended in newer versions of PHP [1].

    Asia/Calcutta -> Asia/Kolkata
    Europe/Kiev   -> Europe/Kyiv
    US/Pacific    -> America/Los_Angeles

[1] https://www.php.net/manual/en/timezones.others.php
@kylekatarnls
Copy link
Collaborator

IIRC, those tests aim to check a variety of timezones including deprecated ones. It also sounds like we don't have an updated timezone DB on Windows on GitHub Actions.

Maybe we can instead have a check of the timezone version of check which timezones are available and use a set of TZ based on what the OS supports.

May I ask what kind of annoyance is it? I.E. What is the need to run those unit tests, are you developing extensions for Carbon or something?

@rgson
Copy link
Contributor Author

rgson commented Oct 19, 2023

IIRC, those tests aim to check a variety of timezones including deprecated ones.

I see what you mean. The changes to the testDateSerializationReflectionCompatibility tests can be reverted. Only one instance of US/Pacific causes a test failure for me: the one testing __wakeup.

All mentions of Europe/Kiev and Asia/Calcutta seem (to me) to be unrelated to the tests' purposes.

Maybe we can instead have a check of the timezone version of check which timezones are available and use a set of TZ based on what the OS supports.

In cases where the exact time zone doesn't matter, you could also consider using another time zone whose name has not changed. Possibly even one that practically means the same offset.

May I ask what kind of annoyance is it? I.E. What is the need to run those unit tests, are you developing extensions for Carbon or something?

I maintain a package of this library for Debian (and Ubuntu, etc.). The tests run as part of that build process and Debian's routine QA processes.

The new test failures were noticed in Debian unstable [1], where legacy time zones are no longer installed by default.

In the end, my problem with this was resolved by simply installing the missing time zones.

This PR was more of a long-term solution as PHP's documentation also advises against using the legacy time zones. But considering the failures in GitHub Actions, it may be too early for you to conveniently switch then.

[1] https://bugs.debian.org/1052778

@kylekatarnls
Copy link
Collaborator

We'll make those tests pass whatever the running system support old, new or both timezone names via #2886

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 this pull request may close these issues.

2 participants