[wp-trac] [WordPress Trac] #56468: sanitize_option() does not handle deprecated timezones correctly
WordPress Trac
noreply at wordpress.org
Tue Aug 30 19:19:24 UTC 2022
#56468: sanitize_option() does not handle deprecated timezones correctly
--------------------------------+-------------------------------------
Reporter: jrf | Owner: (none)
Type: defect (bug) | Status: new
Priority: normal | Milestone: 6.1
Component: Options, Meta APIs | Version:
Severity: normal | Keywords: has-patch needs-testing
Focuses: |
--------------------------------+-------------------------------------
= 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
[https://www.php.net/manual/en/datetimezone.listidentifiers.php
timezone_identifiers_list()] function.
This list only contains the timezone identifiers which are valid as of the
last time the [https://www.iana.org/time-zones 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 [https://github.com/php
/php-
src/commits/master?before=06f713e80f0bad93352469316adb14d277fc6c1d+35&branch=master&path%5B%5D=ext&path%5B%5D=date&qualified_name=refs%2Fheads%2Fmaster
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 [https://github.com/php/php-
src/commit/2fbea217c2ed1195895ebf8f011ffa636b441a56 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 [http://mm.icann.org/pipermail/tz-
announce/2022-August/000071.html 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.
* https://3v4l.org/Hpe2R
* https://3v4l.org/Hpe2R/rfc#vgit.master
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
<?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
[https://www.php.net/manual/en/datetimezone.listidentifiers.php#refsect1-datetimezone
.listidentifiers-returnvalues `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
[https://core.trac.wordpress.org/newticket 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
;-)
--
Ticket URL: <https://core.trac.wordpress.org/ticket/56468>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac
mailing list