REST API: Issues with dates and DST

[afercia]

Edit: As indicated by @nerrad below, the original test was correct and this is a problem with the API logic.

Not sure why this test fails for some people and doesn't fail for other people. Probably related to difference in Day Saving Time across the world (currently, North America is already in DST, Europe not).

For me (GMT+1 and currently no DST here), it is failing:

1) WP_Test_REST_Posts_Controller::test_empty_post_date_gmt_shimmed_using_post_date
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2016-02-23T18:00:00'
+'2016-02-23T17:00:00'

As pointed out by @nerrad ​on Slack the hour difference expected in the test actually differs depending on DST.

Test was introduced in [40115].

[ocean90]

#40141 was marked as a duplicate.

[jnylen0]

In 40284:

REST API: Fix DST handling in a test.

The time chosen for test_empty_post_date_gmt_shimmed_using_post_date falls within US daylight savings time. This may cause the test to fail depending on whether the current locale has DST.

The fix is to specify a numeric gmt_offset rather than a timezone_string.

Unprops jnylen0.
Fixes #40136.

[jnylen0]

[40284] needs backport to 4.7 as well.

[ocean90]

Related: [39167]

[nerrad]

So I did some diving to find out why tests using the posts_dates_provider are passing is because the way the tests run, the _date_ used in the tests are NOT in DST (i.e. _December 12_ is not in DST) and so all the related conversions happening in post controller/wp_posts api are working explicitly with the provided date. So those tests don’t need changed.

However, I’ve reversed my position and I think the original tests for test_empty_post_date_gmt_shimmed_using_post_date were actually correct and the fail was a valid fail. The problem is this line of code (found in WP_REST_Posts_Controller::prepare_item_for_response:

$post_date_gmt = date( 'Y-m-d H:i:s', strtotime( $post->post_date ) - ( get_option( 'gmt_offset' ) * 3600 ) );

That line is NOT considering the provided date when determining whether there should be a DST offset or not. In other words, if the provided date was 12-12 (in America/Chicago) it should NOT have a DST for the conversion. If the provided date was 3-14 in America/Chicago it _should_ have DST.

Instead, that function is ALWAYS providing whatever state of DST exists for the _current_time_ .

So that line that was introduced in [40115] needs fixed. The unit test was actually working correctly and exposing a valid fail.

[nerrad]

This is a pretty significant flaw if the api is being used to import/migrate posts for instance and the migration just happens to be occurring where the current date on the server things are being processed is in DST and dates for posts being migrated are for dates _not_ in DST.

[nerrad]

I should probably explain more why this is a problem.

On sites where a timezone_string is defined, the timezone_string overrides any call to get_option( 'gmt_offset' ) and provides the offset derived from the timezone_string (via a pre_option filter callback set in default-filters.php). To get this offset, the _current time_ is used.

[jnylen0]

Yes, I agree this is a problem, and while there are currently no failing tests as a result of [40284], we definitely want to get this fixed in 4.7.4 :(

Something similar to the existing wp_timezone_override_offset code should work, but using the actual date instead of the current date: https://core.trac.wordpress.org/browser/tags/4.7.2/src/wp-includes/functions.php?marks=4504,4505,4509#L4490

Because WP doesn't store dates in UTC for drafts, there will still be cases where it's impossible to determine a correct date_gmt based on the date (there are brief windows where the same date can occur twice in timezones with DST, and we will just have to pick one of the two corresponding UTC dates).

[jnylen0]

The existing function get_gmt_from_date looks like it will probably do what we want. As @nerrad noted in Slack, we should make sure there are adequate tests for DST conversions. These tests are probably best written against get_gmt_from_date directly, for the most part.

[nerrad]

change how gmt is shimmed and revert test back to original state

[nerrad]

@jnylen0 the attached patch uses get_gmt_from_date for getting the date and I reverted the test back to its original state. Locally after reverting the test and running it, it failed. After making the change for the gmt shim in the posts controller, it passed. So we should be good to go.

Regarding adding additional dst tests, it looks like it's already well covered in tests/formatting/date.php

[nerrad]

refresh of patch with removal of incorrect @group tag

[nerrad]

refresh of patch for following WP coding standards re spacing

[swissspidy]

In 40324:

REST API: Use get_gmt_from_date() when preparing a draft post for response.

This prevents wrong dates when dealing with DST, see [40115] and [40284].

Props nerrad.
Fixes #40136.

[swissspidy]

In 40325:

REST API: Use get_gmt_from_date() when preparing a draft post for response.

This prevents wrong dates when dealing with DST, see [40115] and [40284].

Props nerrad.
Fixes #40136.

Merges [40284] and [40324] to the 4.7 branch.