Make WordPress Core

Opened 2 years ago

Closed 22 months ago

#56468 closed defect (bug) (fixed)

sanitize_option() does not handle deprecated timezones correctly

Reported by: jrf's profile jrf Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch needs-testing has-unit-tests
Focuses: Cc:

Description

TL;DR

When saving WordPress native options, the sanitize_options() function validates the value for the timezone_string option against a list of valid timezone identifiers as retrieved via the PHP native timezone_identifiers_list() function.

This list only contains the timezone identifiers which are valid as of the last time the timezone database was updated on a particular PHP version.

This means that if a timezone has been deprecated since then and the user has updated their PHP version, the timezone for their website will be reset to the default value for the timezone_string option (which may be set based on the locale), when the option is saved again.

Note: this is one of those bugs which users will run into, but then cannot reproduce again, so this may have plagued some users over time, but I couldn't find any bug reports for it and am not that surprised there aren't any as, as outlined above, it requires a particular combination of circumstances to reproduce. Still annoying for those users whom it affects.

Reproduction scenario

  • Given that a user, while running WP on PHP 7.2, has set their timezone to "Enderbury" (under "Pacific") in the options dropdown in the Options -> General page (and has kept the "Site language" setting at "English (United States)").
  • This will have been saved to the options table under the key timezone_string with the value Pacific/Enderbury.
  • That user has subsequently updated their PHP version to PHP 7.4.
  • The next time they visit the Options -> General page, the saved option cannot be matched with an option in the list of valid options and no option will be selected in the dropdown (not even "Select a city").
  • Now if they don't notice that the timezone isn't set in the dropdown (because they only wanted to change something else and didn't even look at it) and then click "Save changes", the originally chosen timezone for their site will be lost and the timezone will be reset to 0.

Note: I'm using Enderbury as an example case here, in practice, there are currently six different timezone strings which have been deprecated since PHP 5.6 and for which this issue applies.

Long version

Okay, get a drink and a snack, cause this is going to be a long read.

How the issue was discovered

While working on making WordPress compatible with PHP 8.2, approximately 15 (extra) tests suddenly started failing when I updated PHP 8.2 from beta2 to beta3.

Note: these were hard test failures, not tests erroring out on deprecation notices or other PHP notices/warnings!

Based on the names of the test classes/test functions involved, it became clear that these test failures had something to do with Date/Time, either with the PHP native extension or with the handling of it in WP itself.

Test results showed that the timezone handling should be the first thing to look at:

4) Tests_Date_DateI18n::test_should_handle_zero_timestamp
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'1970-01-01T00:00:00+03:00'
+'1970-01-01T00:00:00+00:00'

At this time, it was not clear whether this was a regression/bug in PHP 8.2 or a bug in WP itself.

Investigation

Step 1

First off, I needed to isolate the failures and some of the tests which were failing in a run of the complete test suite, were not failing when run in isolation.

So looking closer at the affected tests and the test suite as a whole, it became clear that while most of the database tables get a reset/clear out between tests, the wp_options table is not one of those tables.

I may be missing something here (as some of the wiring in the test suite is pretty complicated and that wiring was not my focus for this ticket), if so, please post a link to where the test suite resets the options table, but I couldn't find it in a quick search.

So first thing to do, was to make sure that all tests which set a value for timezone_string would always reset that value to '' after the test was run.

[Follow up action, out of scope]
While the included patch will fix this for the timezone_string option, a review of the complete test suite should be done to do the same for all other options which tests set. Alternatively a solution should be designed to reset the value of all options after each test run.

Step 2

Next, I had a look at the PHP source code and the commit history for the PHP Date/Time extension to see if that would be able to give me a clue as to what the culprit might be.

The only relevant commit between PHP 8.2.0-beta2 and PHP 8.2.0-beta3 appeared to be an update of the timezone database included with PHP.

So the next step was to figure out what had changed with that database update.

Digging into the release announcement of the latest version of the timezone database, it turned out that the timezone name for Europe/Kiev no longer exists and had been replaced with Europe/Kyiv.

Now, the WP Core test suite contains quite a number of tests related to date/time and timezones, most of which have been written by the awesome Andrey "Rarst" Savchenko !
@rarst, being from Kyiv himself, used the Europe/Kiev timezone in most places when a test needed to test with an alternative timezone, so 1 + 1 = 2, now we're getting somewhere... or are we ?

Step 3

The next thing to verify was whether PHP supports old timezone names and if so, if it still supported the old timezone name Europe/Kiev. If not, this would a regression/bug to be reported to PHP itself.

So, I tried to reproduce the issue in isolation, but no matter what I tried, in native PHP, I couldn't reproduce the behaviour I was seeing.

Okay, so not a bug in PHP. PHP appears to handle deprecated timezone names without any problems whatsoever. Massive kudos to Derick Rethans for that !

So, now what ?

Step 4

While all the failing tests were related to the Date/Time component, they were all testing different WP functions, so was there a common end-point which could be determined as the culprit ?

As these were test failures, not test errors, there was no backtrace to go on, so this required lots of manual digging through the code, adding var_dump()s all over the place and running the tests against multiple different PHP versions, to see if I could identify the function path were the timezone was no longer being respected...

Long story short, turned out, no matter which test I looked at, it always started failing at the first call to a WP native Date/Time function.

So what were those functions receiving as input and why was the timezone not respected ?

Step 5

A couple of strategically placed var_dumps()s and test runs later, I'd found the problem: when running on PHP 8.2beta3, the Europe/Kiev timezone name would no longer be saved to the options database... Oh dear.

So instead of focusing on the Date/Time component, I now needed to start digging into the Options component of WP.

After some digging and checking on the filters being hooked in for the update_option() function, everything pointed to the sanitize_options() function and in particular to this code:

<?php
case 'timezone_string':
    $allowed_zones = timezone_identifiers_list();
    if ( ! in_array( $value, $allowed_zones, true ) && ! empty( $value ) ) {
        $error = __( 'The timezone you have entered is not valid. Please select a valid timezone.' );
    }
    break;

Src: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/formatting.php#L4925

Step 6

Now, let's take a look at what the PHP `timezone_identifiers_list()` function returns....

Returns the array of timezone identifiers. Only non-outdated items are returned. To get all, including outdated timezone identifiers, use the DateTimeZone::ALL_WITH_BC as value for $countryCode.

Okay, so we were comparing a deprecated timezone name against an array containing only non-deprecated timezone names and invalidating the timezone to save when it is using a deprecated name.

Well, that explains a lot. Let's see if using that DateTimeZone::ALL_WITH_BC value would make a difference...

Uh-oh.. turns out the PHP docs had an error and the documented way of getting the list including outdated timezone identifiers didn't work.... https://3v4l.org/iBYXH
Better fix the docs to help the next person: https://github.com/php/doc-en/pull/1781

Step 7

Right, so after all that, a one-line, 25 character, change could fix everything ? Yup!

Well, nearly. While making that one-line change would fix the saving of the option when update_option() was called directly (and yes, all test failures were gone after that one change), what about if an actual user would use the Admin Panel and visit the "Options" -> "General" page ?

In that case, the previously saved option would not be found in the drop-down list of options to choose from, so no option would be "selected" and with no option being selected, a "Save Changes" from the admin page would still reset the timezone_string option to 0.

In other words, all uses of the PHP timezone_identifiers_list() function in WP Core would need a review, but at least we now know what to look for.

Culprit found, solution ready to be pulled.

Conclusions (PR available/upcoming)

This is not a PHP 8.2 bug. This is a PHP cross-version bug in WordPress, which can be encountered in multiple PHP versions depending on the timezone string which was used.

While there are only a few timezones (6) which have been deprecated between PHP 5.6.20 and PHP 8.2.0, WordPress should still allow for these as otherwise the timezone for a site can be reset to 0/UTC once a site upgrades to a newer PHP version.

This also means that the names of deprecated timezones as well as their possible replacements need to be accounted for in the continents-cities.php list, which is used as the basis for the timezone name translations.
Both the old as well as the new names can be encountered, it just depends on the PHP version WordPress is being run on.

The current tests using Europe/Kiev should be updated to use an alternative timezone name which hasn't changed between PHP 5.6.20 and PHP 8.2.0. I'm proposing to use Europe/Helsinki as that is a timezone name in the same general timezone as Europe/Kiev and would cause the least amount of churn in the tests.

Additionally, tests should be added in strategic places to ensure that WordPress correctly handles deprecated timezone names.

Caveats

The one situation not covered by the patch is when someone would use one of the (11) new timezone names and subsequently would downgrade their PHP version to a version in which that timezone name does not exist yet.

In my opinion, that is not a situation we can account for, nor should we.

Potential future scope

It could be considered to add a "translation" table to transition deprecated timezone names to their replacement name.
This translation could then be run before displaying the Options->General page and in select other places and could update the timezone name automagically to the correct name for the more recent PHP version before saving it to the database.

If anyone thinks this is a good idea/should be done, I invite them to open a separate ticket to discuss this further. I consider this outside of the scope of this ticket.

Recommendation

While I'm confident about the proposed fixes and the coverage created by the updated and additional tests, I have not manually tested the patch.

I would recommend that someone follow the reproduction scenario posted at the top of this post both on trunk as well as with the patch in place, to confirm the bug and verify the fix.

P.S.: If you made it this far: congrats! and I hope you enjoyed the read ;-)

Change History (45)

#1 @costdev
2 years ago

P.S.: If you made it this far: congrats! and I hope you enjoyed the read ;-)

Thanks! I did and the drink and snack were enjoyed too!

I had some prior awareness that you'd encountered this issue and glad to hear you tracked it down!

I'll test the patch when it's up and, per your request, I'll also take a look at the test suite to see when options might be cleared

#2 @costdev
2 years ago

On the wp_options table:

I wrote a test method with a couple of datasets. On the first dataset, it calls update_option( 'howdy', 'admin' );, then asserts that the value is 'admin'.

The first dataset passes, and the rest fail. So the options are reset between tests.

How?

No drink or snack for you, this is quick!

Boop: tear_down() contains the line: $wpdb->query( 'ROLLBACK' );. Commented out to confirm it's the specific point where it's cleared - and it is.

Last edited 2 years ago by costdev (previous) (diff)

This ticket was mentioned in PR #3155 on WordPress/wordpress-develop by jrfnl.


2 years ago
#3

  • Keywords has-unit-tests added

### Tests: reset timezone related options if the tests change them

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]

### Tests: replace the timezone used in tests

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.

### 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_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

### 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/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].

### sanitize_option(): fix bug in sanitization of timezone_string

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.

### populate_options(): fix bug in sanitization of localized default timezone_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.

### wp_timezone_choice(): fix bug in timezone dropdown list creation

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.

### 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.

https://i0.wp.com/user-images.githubusercontent.com/663378/187558126-0546375b-b092-41cb-b335-1c617b24fc2c.png

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.

Trac ticket: https://core.trac.wordpress.org/ticket/56468

#4 @jrf
2 years ago

I've opened GitHub PR#3155 to address this issue.

I'll test the patch when it's up

@costdev I look forward to your feedback!

Boop: tear_down() contains the line: $wpdb->query( 'ROLLBACK' );. Commented out to confirm it's the specific point where it's cleared - and it is.

Interesting for sure, but that doesn't explain why I still saw problems running the tests in isolation. Either way, the "resetting of the options in test fixture" bit is in a separate commit, so could be dropped if it turns out to not be needed, though there is "prior art" for the same.

jrfnl commented on PR #3155:


2 years ago
#5

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! 😂

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).

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.

You're a ⭐ !

#6 @marcyoast
23 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/3155.diff

Environment

  • OS: macOS 12.6
  • Web Server: Nginx
  • PHP: 7.2.34 - 7.4.29
  • WordPress: 6.1-alpha-53344-src
  • Browser: Brave 1.43.89 Chromium: 105.0.5195.102 (Official Build) (x86_64)
  • Theme: Twenty Twenty-Two
  • Active Plugins:
    • none

Actual Results

  • The issue is resolved with the patch.

Additional Notes

  • Without the patch instead of no timezone, Abidjan is selected instead.

#7 @jrf
23 months ago

  • Keywords commit added

Thank you @marcyoast for testing and confirming the issue and that the patch fixes it.

As the patch has now also been confirmed via manual testing, I'm adding the commit keyword.

#8 @SergeyBiryukov
23 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#9 @SergeyBiryukov
23 months ago

In 54207:

Tests: Reset timezone-related options if the tests change them.

The options table is not explicitly reset after each test or test class, so if an option is changed during a test, it should be reset to the default value after the test.

This commit does so for those tests which did not have such resetting in place yet, while one or more tests in the class do change the value of the timezone_string option.

Note: The test suite executes a ROLLBACK query after each test, which should reset the options table in theory, however that appears to not always be enough, as some timezone-related tests can fail in a complete test suite run, while not failing when run in isolation. This commit aims to improve stability of the tests.

Follow-up to [45857] / #45821.

Props jrf, costdev.
See #56468.

#10 @SergeyBiryukov
23 months ago

In 54217:

Tests: Replace the timezone used in date/time tests.

The Europe/Kiev timezone has been deprecated in PHP 8.2 and replaced with Europe/Kyiv.

The tests updated in this commit are testing the WordPress date/time functionality. They are not testing whether WP or PHP can handle deprecated timezone names correctly.

To ensure the tests follow the original purpose, the use of Europe/Kiev within these tests is now replaced 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.

Follow-up to [45853], [45856], [45876], [45882], [45887], [45908], [45914], [46577], [46154], [46580], [46864], [46974], [54207].

Props jrf, costdev.
See #56468.

#11 @SergeyBiryukov
23 months ago

In 54227:

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 have been retrieved using the PHP timezone_identifiers_list() function.

See: timezone_identifiers_list() output and comparison.

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 dropdown lists on PHP 5.6 to 8.1.
  • The corrected spelling — Kyiv — will be used to find the localized name on PHP 8.2 and up.

Follow-up to [50555], [54207], [54217].

Props jrf, costdev.
See #56468.

#12 @SergeyBiryukov
23 months ago

In 54229:

Date/Time: Correct sanitization of timezone_string in sanitize_option().

This fixes a bug where if the timezone_string is set to a 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 ticket #56468.

Also see: PHP Manual: timezone_identifiers_list().

Includes adding a dedicated test to the data provider used in the Tests_Option_SanitizeOption test class.

Note: The new data set is named, 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.

Follow-up to [18323], [33119], [54207], [54217], [54227].

Props jrf, costdev.
See #56468.

#13 @SergeyBiryukov
23 months ago

In 54230:

Tests: Add tests with deprecated timezone strings.

This commit adds tests in select places to ensure that these date/time related functions continue to behave as expected when the timezone_string option is set to an outdated/deprecated timezone name.

The timezone string used 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: timezone_identifiers_list() output for PHP 5.6.20.

Follow-up to [54207], [54217], [54227], [54229].

Props jrf, costdev.
See #56468.

#14 @SergeyBiryukov
23 months ago

In 54232:

Date/Time: Correct sanitization of localized default timezone_string in populate_options().

This fixes a bug where if the default timezone_string is 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 ticket #56468.

Also see: PHP Manual: timezone_identifiers_list().

Includes:

  • Expanding the translators comment to encourage translators to use “old” names over “new” names.
  • Adding a dedicated test to the Tests_Admin_IncludesSchema test class.

Follow-up to [54207], [54217], [54227], [54229], [54230].

Props jrf, costdev.
See #56468.

#15 @SergeyBiryukov
23 months ago

In 54233:

Date/Time: Correct timezone dropdown list creation in wp_timezone_choice().

This fixes a bug where if the timezone_string is set to a 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 is 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 that the value is a valid but outdated timezone name 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 ticket #56468.

Also see: PHP Manual: timezone_identifiers_list().

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.

Follow-up to [54207], [54217], [54227], [54229], [54230], [54232].

Props jrf, costdev, marcyoast.
See #56468.

#16 @SergeyBiryukov
23 months ago

In 54237:

Date/Time: Minor tweak to support deprecated timezones on General Settings screen.

Underneath the timezone selector on the General Settings screen, a small snippet of info about the selected time zone is displayed.

This information would be missing if the timezone is 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 ticket #56468.

Also see: PHP Manual: timezone_identifiers_list().

Note: As this is an admin/output page, no pre-existing tests are available.

Follow-up to [54207], [54217], [54227], [54229], [54230], [54232], [54233].

Props jrf, costdev, marcyoast.
See #56468.

#17 @SergeyBiryukov
23 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

SergeyBiryukov commented on PR #3155:


23 months ago
#18

Thanks for the PR! Merged in r54207, r54217, r54227, r54229, r54230, r54232, r54233, and r54237.

jrfnl commented on PR #3155:


23 months ago
#19

Thanks for getting those all in @SergeyBiryukov !

This ticket was mentioned in Slack in #core by sergey. View the logs.


22 months ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


22 months ago

#22 @desrosj
22 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this because there appear to be failures in older branches related to this issue. The Docker containers were updated yesterday after 2 months of the workflow being disabled due to inactivity (normally, they're regenerated weekly to ensure the latest release within each PHP version is used), and that seems to have surfaced this.

I'm not sure if any portions of the test related commits attached here should be backported yet, but this will need to be addressed in some way.

#23 @jrf
22 months ago

there appear to be failures in older branches related to this issue

@desrosj Could you point me to the errors you are seeing ?

The only thing I can think of offhand would be that the timezone database update in PHP may also have been applied to PHP 8.0 and 8.1 (which are still actively maintained).

In that case, yes, older WP branches would start failing on PHP 8.0/8.1 on the same tests as we saw failing on PHP 8.2.

IIRC though most PHP-specific (non-security) related fixes, are normally not backported, so why would this be any different ? (other than that the failing tests are annoying)

#24 @jrf
22 months ago

Think I've found the error logs and this confirms to me what I said above.

#25 @jrf
22 months ago

Backporting [54217] would get rid of the failing tests, though it wouldn't actually solve anything related to this issue in older WP versions. It literally just prevents the tests from failing on the issue.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


22 months ago

#31 follow-up: @peterwilsoncc
22 months ago

I've created PRs for the 5.6 through 6.0 branches backporting [54217]. Each of the PRs now have passing tests.

The 5.6, 5.7 and 5.8 branches required conflict resolutions so will need to be applied manually. I think this was due to the logic preventing the tests running on PHP 8.1.

For the legacy branches I think the bug can remain but fixing the tests will ease the way for maintenance and security releases.

This ticket was mentioned in PR #3464 on WordPress/wordpress-develop by @peterwilsoncc.


22 months ago
#32

https://core.trac.wordpress.org/ticket/56468

{{{sh
REV=54217
SOURCE_PR=3464

svn switch '/branches/6.0'
svn up --ignore-externals . > /dev/null
svn merge --record-only -c$REV '
/trunk' .
gh pr diff $SOURCE_PR --repo WordPress/WordPress-Develop | patch -p1
LOG=$(svn log -r$REV '/trunk' | grep -v '\-------' | tail -n +3)
BRANCH=$(svn info | grep '
URL:' | egrep -o '(tags|branches)/[/]+|trunk' | egrep -o '[/]+$')
echo -en "$LOG\n\nMerges [$REV] to the $BRANCH branch." | pbcopy
echo ""
pbpaste
echo ""
}}}

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


22 months ago

#34 in reply to: ↑ 31 @jrf
22 months ago

Replying to peterwilsoncc:

I've created PRs for the 5.6 through 6.0 branches backporting [54217]. Each of the PRs now have passing tests.

Thanks for setting up these PRs @peterwilsoncc ! I've done a quick visual review of each of them and approved them all.

The 5.6, 5.7 and 5.8 branches required conflict resolutions so will need to be applied manually. I think this was due to the logic preventing the tests running on PHP 8.1.

Actually, looks like it was due to the public visibility keyword having been added to a number of the test methods involved. All good though.

For the legacy branches I think the bug can remain but fixing the tests will ease the way for maintenance and security releases.

Agreed.

#35 @peterwilsoncc
22 months ago

In 54512:

Tests: Replace the timezone used in date/time tests.

The Europe/Kiev timezone has been deprecated in PHP 8.2 and replaced with Europe/Kyiv.

The tests updated in this commit are testing the WordPress date/time functionality. They are not testing whether WP or PHP can handle deprecated timezone names correctly.

To ensure the tests follow the original purpose, the use of Europe/Kiev within these tests is now replaced 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.

Follow-up to [45853], [45856], [45876], [45882], [45887], [45908], [45914], [46577], [46154], [46580], [46864], [46974], [54207].

Props jrf, costdev, SergeyBiryukov.
Merges [54217] to the 6.0 branch.
See #56468.

#36 @peterwilsoncc
22 months ago

In 54513:

Tests: Replace the timezone used in date/time tests.

The Europe/Kiev timezone has been deprecated in PHP 8.2 and replaced with Europe/Kyiv.

The tests updated in this commit are testing the WordPress date/time functionality. They are not testing whether WP or PHP can handle deprecated timezone names correctly.

To ensure the tests follow the original purpose, the use of Europe/Kiev within these tests is now replaced 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.

Follow-up to [45853], [45856], [45876], [45882], [45887], [45908], [45914], [46577], [46154], [46580], [46864], [46974], [54207].

Props jrf, costdev, SergeyBiryukov.
Merges [54217] to the 5.9 branch.
See #56468.

#37 @peterwilsoncc
22 months ago

In 54514:

Tests: Replace the timezone used in date/time tests.

The Europe/Kiev timezone has been deprecated in PHP 8.2 and replaced with Europe/Kyiv.

The tests updated in this commit are testing the WordPress date/time functionality. They are not testing whether WP or PHP can handle deprecated timezone names correctly.

To ensure the tests follow the original purpose, the use of Europe/Kiev within these tests is now replaced 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.

Follow-up to [45853], [45856], [45876], [45882], [45887], [45908], [45914], [46577], [46154], [46580], [46864], [46974], [54207].

Props jrf, costdev, SergeyBiryukov.
Merges [54217] to the 5.8 branch.
See #56468.

#38 @peterwilsoncc
22 months ago

In 54515:

Tests: Replace the timezone used in date/time tests.

The Europe/Kiev timezone has been deprecated in PHP 8.2 and replaced with Europe/Kyiv.

The tests updated in this commit are testing the WordPress date/time functionality. They are not testing whether WP or PHP can handle deprecated timezone names correctly.

To ensure the tests follow the original purpose, the use of Europe/Kiev within these tests is now replaced 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.

Follow-up to [45853], [45856], [45876], [45882], [45887], [45908], [45914], [46577], [46154], [46580], [46864], [46974], [54207].

Props jrf, costdev, SergeyBiryukov.
Merges [54217] to the 5.7 branch.
See #56468.

#39 @peterwilsoncc
22 months ago

In 54516:

Tests: Replace the timezone used in date/time tests.

The Europe/Kiev timezone has been deprecated in PHP 8.2 and replaced with Europe/Kyiv.

The tests updated in this commit are testing the WordPress date/time functionality. They are not testing whether WP or PHP can handle deprecated timezone names correctly.

To ensure the tests follow the original purpose, the use of Europe/Kiev within these tests is now replaced 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.

Follow-up to [45853], [45856], [45876], [45882], [45887], [45908], [45914], [46577], [46154], [46580], [46864], [46974], [54207].

Props jrf, costdev, SergeyBiryukov.
Merges [54217] to the 5.6 branch.
See #56468.

#45 @peterwilsoncc
22 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Thanks for the quick reviews and approvals, Juliette!

I've backported the timezone changes for the tests in the branches 5.6 - 6.0. The PHP 8.x runs are now passing in each.

I've reclosed this ticket as fixed.

Note: See TracTickets for help on using tickets.