Fix handling of deprecated timezones (Trac 56468) #3155
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
costdev
requested changes
Sep 1, 2022
There was a problem hiding this comment.
Most of these comments are regarding the same issue. I've just marked each instance to make it easier to locate each change if it's necessary. If the change isn't necessary, then I apologise for the completely unnecessary resolve conversation clicks I've put you through! 😂
The source changes look good at a glance, but I'll try to get the PR tested and will get back to you if I have any queries/concerns.
| 'gmt_offset' => 3, | ||
| ), | ||
| array( | ||
| 'timezone_string' => '', | ||
| 'gmt_offset' => 3, | ||
| ), | ||
| // @ticket 56468. | ||
| 'Deprecated timezone string and no gmt offset set' => array( |
There was a problem hiding this comment.
Suggested change
| 'Deprecated timezone string and no gmt offset set' => array( | |
| 'Deprecated timezone string and no GMT offset set' => array( |
Also, genuine question about something trivial:
Should datasets begin with uppercase, or lowercase?
is8601 to datetime correct time with Deprecated timezone string and no GMT offset set
versus
is8601 to datetime correct time with deprecated timezone string and no GMT offset set
There was a problem hiding this comment.
There is currently no standard set for this. I normally would use sentence case, though it depends on the codebase.
|
|
||
| wp_cache_delete( 'alloptions', 'options' ); | ||
|
|
||
| $result = get_option( 'timezone_string' ); |
There was a problem hiding this comment.
Is there a scenario in which timezone_string might already be set to America/Buenos_Aires due a faulty test earlier in the class/test suite?
There was a problem hiding this comment.
Not that I can think off with the resets I added.
The `options` table is **not** reset after each test/test class, so if an option is changed during a tests, it should be reset to the default value _after_ the test. This commit does so for those tests which didn't have such resetting in place yet, while one or more tests in the class _do_ change the value of the `timezone_string` option. Follow up on Trac#45821, [45857]
The `Europe/Kiev` timezone has become deprecated in PHP 8.2 and been replaced with `Europe/Kyiv`. These tests are testing WP date/time functionality. They are **not** testing whether WP/PHP can handle deprecated timezone names correctly. To ensure the tests stay "pure" and test what the original purpose was for these tests, I'm replacing the use of `Europe/Kiev` within these tests now with the `Europe/Helsinki` timezone, which is within the same timezone as `Europe/Kyiv`. This should ensure that these tests run without issue and test what they are supposed to be testing on every supported PHP version (unless at some point in the future `Europe/Helsinki` would be renamed, but that's a bridge to cross if and when). Note: separate tests should/will be added to ensure that relevant date/time related functions handle a deprecated timezone correctly, but that is not something _these_ tests are supposed to be testing.
This commit adds tests in select places to safeguard that these date/time related functions will continue to behave as expected when the `timezone_string` option has been set to an outdated/deprecated timezone name. The timezone string I'm using in these tests, `America/Buenos_Aires`, is a timezone string which was already deprecated in PHP 5.6.20 (the current minimum PHP version), so using this timezone string, we can safely test the handling of deprecated timezone names on all supported PHP versions. See: https://3v4l.org/Holsr#v5.6.20
Based on a two-way comparison between the available timezone city names in PHP 5.6.20 and PHP 8.2.0. Lists of available timezone names retrieved using the PHP `timezone_identifiers_list()` function. See: https://3v4l.org/ro1vY/rfc#vgit.master Note: both spellings of `Kiev`/`Kyiv` need to be in the list to allow it to work PHP cross-version. The "old" version - `Kiev` - will be used as the basis to find the localized name for the timezone drop down lists on PHP 5.6 - 8.1, while the corrected spelling - `Kyiv` - will be used when to find the localized name for the timezone drop down lists on PHP 8.2 and up. Previous: Trac#52861 / [50555].
This fixes a bug where if the `timezone_string` would be set to timezone name which has since been deprecated, the option value would be "lost" when saving the value again, as the comparison being done to verify whether it is a valid timezone name would only take "current" timezone names into account and would invalidate deprecated timezone names. By passing the `DateTimeZone::ALL_WITH_BC` constant as the `$timezoneGroup` parameter to the PHP native `timezone_identifiers_list()` function, a timezone name list is retrieved containing both current and deprecated timezone names, preventing the invalidation of the option value. See the extensive write-up about this in Trac#56468. Also see: https://www.php.net/manual/en/datetimezone.listidentifiers.php Includes adding a dedicated test to the data provider used in the `Tests_Option_SanitizeOption` test class. Note: I have made this a _named data set_, even though the other data sets are unnamed, to make sure it is clear what this data set is testing. Adding test names for the original data sets in this data provider would be a great future improvement, but is outside of the scope of this commit/ticket.
…ezone_string` This fixes a bug where if the default `timezone_string` would be set to a deprecated timezone name due to a localization providing an outdated timezone name string, this localized timezone string would be discarded and an empty string would be set as the timezone value instead. By passing the `DateTimeZone::ALL_WITH_BC` constant as the `$timezoneGroup` parameter to the PHP native `timezone_identifiers_list()` function, a timezone name list is retrieved containing both current and deprecated timezone names, preventing the invalidation of the option value. See the extensive write-up about this in Trac#56468. Also see: https://www.php.net/manual/en/datetimezone.listidentifiers.php Includes an expansion of the translators comment to encourage translators to use "old" names over "new" names. Includes adding a dedicated test to the `Tests_Admin_IncludesSchema` test class.
This fixes a bug where if the `timezone_string` would be set to timezone name which has since been deprecated, no option would be (pre-)selected in the generated dropdown list and when the form using the dropdown list would be submitted, the "old", originally saved value would be lost as the form would submit without a value being selected for the `timezone_string` field. The fix is a little hacky: it basically checks ahead of generating the actual dropdown list whether the `$selected_zone` value would be recognized and set to "selected" and if not, verifies the value _is_ a valid value, but outdated timezone name value and if so, adds an extra dropdown entry to the top of the list with the original value and sets this value to "selected". See the extensive write-up about this in Trac#56468. Also see: https://www.php.net/manual/en/datetimezone.listidentifiers.php Note: there are no pre-existing tests at all for this method and adding a complete set of tests for this method is outside the scope of this ticket, so this fix does not contain any tests.
Underneath the time zone selector on the Options/General page, a small snippet of info about the selected time zone is displayed. This information would be missing if the timezone would be set to a deprecated timezone value, even though PHP is perfectly capable of generating that information, including for deprecated timezones. By passing the `DateTimeZone::ALL_WITH_BC` constant as the `$timezoneGroup` parameter to the PHP native `timezone_identifiers_list()` function, a timezone name list is retrieved containing both current and deprecated timezone names, preventing the condition from failing when the current timezone is a deprecated one. See the extensive write-up about this in Trac#56468. Also see: https://www.php.net/manual/en/datetimezone.listidentifiers.php As this is an admin/output page, no tests are available or can be set up without jumping through a lot of hoops.
jrfnl
force-pushed
the
trac-56468/fix-handling-deprecated-timezones
branch
from
a2a6347
to
45e9502
Compare
Thanks for reviewing! I've fixed up those commits which needed it. Left a reply for everything else (and for some which I fixed up as well).
You're a ⭐ ! |
|
Thanks for getting those all in @SergeyBiryukov ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tests: reset timezone related options if the tests change them
The
optionstable is not reset after each test/test class, so if an option is changed during a tests, it should be reset to the default value after the test.This commit does so for those tests which didn't have such resetting in place yet, while one or more tests in the class do change the value of the
timezone_stringoption.Follow up on Trac#45821, [45857]
Tests: replace the timezone used in tests
The
Europe/Kievtimezone has become deprecated in PHP 8.2 and been replaced withEurope/Kyiv.These tests are testing WP date/time functionality. They are not testing whether WP/PHP can handle deprecated timezone names correctly.
To ensure the tests stay "pure" and test what the original purpose was for these tests, I'm replacing the use of
Europe/Kievwithin these tests now with theEurope/Helsinkitimezone, which is within the same timezone asEurope/Kyiv.This should ensure that these tests run without issue and test what they are supposed to be testing on every supported PHP version (unless at some point in the future
Europe/Helsinkiwould be renamed, but that's a bridge to cross if and when).Note: separate tests should/will be added to ensure that relevant date/time related functions handle a deprecated timezone correctly, but that is not something these tests are supposed to be testing.
Tests: add tests with deprecated timezone strings
This commit adds tests in select places to safeguard that these date/time related functions will continue to behave as expected when the
timezone_stringoption has been set to an outdated/deprecated timezone name.The timezone string I'm using in these tests,
America/Buenos_Aires, is a timezone string which was already deprecated in PHP 5.6.20 (the current minimum PHP version), so using this timezone string, we can safely test the handling of deprecated timezone names on all supported PHP versions.See: https://3v4l.org/Holsr#v5.6.20
I18N: Update list of continents and cities for the timezone selection
Based on a two-way comparison between the available timezone city names in PHP 5.6.20 and PHP 8.2.0.
Lists of available timezone names retrieved using the PHP
timezone_identifiers_list()function.See: https://3v4l.org/ro1vY/rfc#vgit.master
Note: both spellings of
Kiev/Kyivneed to be in the list to allow it to work PHP cross-version.The "old" version -
Kiev- will be used as the basis to find the localized name for the timezone drop down lists on PHP 5.6 - 8.1, while the corrected spelling -Kyiv- will be used when to find the localized name for the timezone drop down lists on PHP 8.2 and up.Previous: Trac#52861 / [50555].
sanitize_option(): fix bug in sanitization of timezone_string
This fixes a bug where if the
timezone_stringwould be set to timezone name which has since been deprecated, the option value would be "lost" when saving the value again, as the comparison being done to verify whether it is a valid timezone name would only take "current" timezone names into account and would invalidate deprecated timezone names.By passing the
DateTimeZone::ALL_WITH_BCconstant as the$timezoneGroupparameter to the PHP nativetimezone_identifiers_list()function, a timezone name list is retrieved containing both current and deprecated timezone names, preventing the invalidation of the option value.See the extensive write-up about this in Trac#56468.
Also see: https://www.php.net/manual/en/datetimezone.listidentifiers.php
Includes adding a dedicated test to the data provider used in the
Tests_Option_SanitizeOptiontest class.Note: I have made this a named data set, even though the other data sets are unnamed, to make sure it is clear what this data set is testing.
Adding test names for the original data sets in this data provider would be a great future improvement, but is outside of the scope of this commit/ticket.
populate_options(): fix bug in sanitization of localized default
timezone_stringThis fixes a bug where if the default
timezone_stringwould be set to a deprecated timezone name due to a localization providing an outdated timezone name string, this localized timezone string would be discarded and an empty string would be set as the timezone value instead.By passing the
DateTimeZone::ALL_WITH_BCconstant as the$timezoneGroupparameter to the PHP nativetimezone_identifiers_list()function, a timezone name list is retrieved containing both current and deprecated timezone names, preventing the invalidation of the option value.See the extensive write-up about this in Trac#56468.
Also see: https://www.php.net/manual/en/datetimezone.listidentifiers.php
Includes an expansion of the translators comment to encourage translators to use "old" names over "new" names.
Includes adding a dedicated test to the
Tests_Admin_IncludesSchematest class.wp_timezone_choice(): fix bug in timezone dropdown list creation
This fixes a bug where if the
timezone_stringwould be set to timezone name which has since been deprecated, no option would be (pre-)selected in the generated dropdown list and when the form using the dropdown list would be submitted, the "old", originally saved value would be lost as the form would submit without a value being selected for thetimezone_stringfield.The fix is a little hacky: it basically checks ahead of generating the actual dropdown list whether the
$selected_zonevalue would be recognized and set to "selected" and if not, verifies the value is a valid value, but outdated timezone name value and if so, adds an extra dropdown entry to the top of the list with the original value and sets this value to "selected".See the extensive write-up about this in Trac#56468.
Also see: https://www.php.net/manual/en/datetimezone.listidentifiers.php
Note: there are no pre-existing tests at all for this method and adding a complete set of tests for this method is outside the scope of this ticket, so this fix does not contain any tests.
Options/General page: minor tweak to support deprecated timezones
Underneath the time zone selector on the Options/General page, a small snippet of info about the selected time zone is displayed.
This information would be missing if the timezone would be set to a deprecated timezone value, even though PHP is perfectly capable of generating that information, including for deprecated timezones.
By passing the
DateTimeZone::ALL_WITH_BCconstant as the$timezoneGroupparameter to the PHP nativetimezone_identifiers_list()function, a timezone name list is retrieved containing both current and deprecated timezone names, preventing the condition from failing when the current timezone is a deprecated one.See the extensive write-up about this in Trac#56468.
Also see: https://www.php.net/manual/en/datetimezone.listidentifiers.php
As this is an admin/output page, no tests are available or can be set up without jumping through a lot of hoops.
Trac ticket: https://core.trac.wordpress.org/ticket/56468
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.