WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 7 months ago

#40136 closed defect (bug) (fixed)

REST API: Issues with dates and DST

Reported by: afercia Owned by: jnylen0
Milestone: 4.7.4 Priority: normal
Severity: major Version: 4.7.3
Component: REST API Keywords: has-patch commit fixed-major
Focuses: Cc:

Description (last modified by jnylen0)

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

Attachments (3)

40136.diff (2.0 KB) - added by nerrad 7 months ago.
change how gmt is shimmed and revert test back to original state
40136.2.diff (2.0 KB) - added by nerrad 7 months ago.
refresh of patch with removal of incorrect @group tag
40136.3.diff (2.0 KB) - added by nerrad 7 months ago.
refresh of patch for following WP coding standards re spacing

Download all attachments as: .zip

Change History (21)

#1 @ocean90
7 months ago

#40141 was marked as a duplicate.

#2 @jnylen0
7 months ago

  • Owner set to jnylen0
  • Resolution set to fixed
  • Status changed from new to closed

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.

#3 @jnylen0
7 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

[40284] needs backport to 4.7 as well.

This ticket was mentioned in Slack in #core-restapi by nerrad. View the logs.


7 months ago

#6 @nerrad
7 months ago

  • Keywords needs-refresh needs-patch dev-feedback added; fixed-major removed
  • Severity changed from normal to major

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.

Last edited 7 months ago by nerrad (previous) (diff)

#7 @nerrad
7 months ago

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.

#8 @nerrad
7 months ago

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.

#9 @jnylen0
7 months ago

  • Description modified (diff)
  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 4.7.4
  • Status changed from reopened to assigned
  • Summary changed from Failing rest-api test test_empty_post_date_gmt_shimmed_using_post_date to REST API: Issues with dates and DST
  • Version set to 4.7.3

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

#10 @jnylen0
7 months ago

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.

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


7 months ago

@nerrad
7 months ago

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

#12 @nerrad
7 months ago

  • Keywords has-patch dev-feedback added; needs-refresh needs-patch removed

@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
7 months ago

refresh of patch with removal of incorrect @group tag

@nerrad
7 months ago

refresh of patch for following WP coding standards re spacing

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


7 months ago

#14 @swissspidy
7 months ago

  • Keywords commit added; dev-feedback removed

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


7 months ago

#16 @swissspidy
7 months ago

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

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.

#17 @swissspidy
7 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#18 @swissspidy
7 months ago

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

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.

Note: See TracTickets for help on using tickets.