Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Developer draft blog post draft for the Dates and Times issue from https://events.codebasehq.com/projects/event-espresso/tickets/10626

Upcoming changes to the Date Time system

This post is intended to be the more technical complement to the post on our main website blog {link to ee.com blog post}.

Note: until the related code changes are merged into master, you can find them in the BUG-10626-dst-unit-test branch.

Background

We recently discovered the following within our automated unit tests against our date and time system:

  1. Although we test gmt offset conversion to a timezone string via EEH_DTT_Helper_Test::test_get_timezone_string_from_gmt_offset, the primary purpose of that test is just to verify that there are no fatals. Primarily that list of offsets includes what were determined to be invalid offsets within php. However, this list is a hardcoded list of invalid offsets that are not necessarily invalid for every system environment.
  2. There is no where in our unit tests where we test setting the WordPress gmt_offset option to an offset, set the timezone_string option to an empty string, and then test the EE generated dates and times in the model system against what WP returns for it's date methods.

As I began going about correcting the above issues, I started discovering other flaws within our code.

Before getting into the issues uncovered below, keep in mind that one of the reasons EE works with timezone strings as opposed to offsets is because that is primarily how PHP is oriented for its DateTime system. If you want to work with DateTime objects accurately in PHP, then you need to work with timezone strings not offsets.

The fact that WordPress allows users to set a GMT offset for times in their system may be fine for a general blog, but its a huge pain for an application around events because offsets have no location awareness and do not inherently track any DST that might exist for that location. This problem is magnified with software like Event Espresso.

Issue One: DST

EEH_DTT_Helper::get_timezone_string_from_gmt_offset was not considering that a set timezone_string in the database could be in DST.

In WordPress, when a call is made to get_option('gmt_offset') there is actually a default hook added by WordPress core on the pre_get_option_gmt_offset which checks get_option('timezone_string') first and if that’s present, returns the offset for that timezone_string. So even if both gmt_offset and timezone_string are set on a WP install (which is possible, just not via the ui), then the offset on timezone_string gets returned.

Where this is problematic is that offsets are timezone agnostic, however, the offset for a timezone_string could vary depending on whether that timezone is currently in DST or not. So if this method was called with NO offset supplied, and the current set timezone_string on the site was in DST, then the resulting offset used for the initial search in timezone_name_from_abbr could result in an INCORRECT match.

I fixed this so that in this scenario if there is a timezone_string set in the db, we just return that instead of deriving it from what gets returned by WP as the offset.

Issue Two: Offset of +0

EEH_DTT_Helper::get_timezone_string_from_gmt_offset was not properly handling scenarios where an offset of 0 was supplied. For all purposes 0 === UTC so there is no need to go through all the logic that could return something that is 0 but currently only because the site is in DST. If client code is supplying an offset to get a timezone_string, then we assume not DST information.

So this was fixed so that now if this method explicitly receives an offset, the assumption is explicit that the given value has no DST information.

Issue Three: Historical Timezones

EEH_DTT_Helper:;get_timezone_string_from_gmt_offset was returning matches against historical timezones.

The PHP methods timezone_name_from_abbr and timezone_abbreviations_list contain not only current timezone data, but also historical timezone data. I discovered this when running some new unit tests we have setup in the working branch. For the offset -12, that would get flipped by the EE usage of these php methods to +12!!! The reason for this is because although -12 matched a timezone_string using the php methods, the current actual offset in real life for the matched timezone_string (when using that matched timezone_string) to instantiate a DateTimeZone object is +12. So the timezone_string matched historically had an offset of -12 but in current day no longer has that offset.

To fix this, I added some further checks on matched timezone strings to make sure that the current offset for that matched timezone_string equals the incoming offset. If they don't match then that timezone_string match is rejected.

Doing this in turn revealed a number of offsets that are settable via the WordPress UI that have no equivalent current timezone_string matches in PHP! To complicate things, that list of invalid offsets is dynamic and depends on whether the server the site is on has up to date timezone offset maps which in turn is influenced by the server OS and/or PHP version installed. The fixes implemented account for this.

Issue Four: Inability to do certain tests.

I added some comments to the Model_Data_Translator_Test::test_prepare_conditions_query_params_for_models__gmt_datetimes that explains why certain offsets were removed from the list that gets tested. This was done intentionally, because the offsets that get adjusted by EE in the EEH_DTT_Helper::adjust_invalid_gmt_offset with the implemented fixes, are changed to the closest offset with a corresponding current (historically) timezone_string. This means that sometimes, the values for "now" saved to the db will NOT match the value for "now" that is generated by the WordPress current_time function because that function works with offsets directly and does not rely on php's timezones at all (when the only time information on the WP site is gmt_offset which is what this test working against). So this means its pretty much impossible to reliably test comparisons for offsets we convert against the offset WP uses because this could vary between server environments.

Practically speaking, the tests that matter are still covering critical functionality.

What this means

If you are using any code that interacts directly (or indirectly) with our EEH_DTT_Helper::get_timezone_string_from_offset method (or any of the public methods it calls), you need to be aware of how this could change when things are released (as described above).

This also means that for sites using a GMT offset (as opposed to a timezone_string), the resulting values for saved dates and times in the database (when displayed) may not be as expected because the database values were converted using an incorrect offset to begin with.

To fix the above scenario on affected sites, there are a couple options:

  1. You can wait until we release this to production and use the bundled tool that provides a UI for fixing the offset on all saved EE date and time values in the database.
  2. You can manually fix things for affected sites via using a variation of the query found here: https://events.codebasehq.com/redirect?https://github.com/eventespresso/ee-code-snippet-library/blob/master/mysql-queries/update-offset-on-all-datetimes.sql

The good news is that if you have sites that are not using UTC offsets but are using timezone_strings then they will not be affected by any of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.