Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#39256 closed defect (bug) (fixed)

REST API: Multiple issues with setting dates of posts

Reported by: jnylen0's profile jnylen0 Owned by: jnylen0's profile jnylen0
Milestone: 4.7.3 Priority: normal
Severity: major Version: 4.7
Component: REST API Keywords: has-unit-tests has-patch fixed-major
Focuses: Cc:

Description

Setting the date of a post while creating or updating it is broken in at least two ways.


First, the value that comes back from the API does not contain a timezone (some recent discussion about this in Slack). This is not great, but possibly reasonable, since these values are stored in the database without a timezone offset.

However, when these values are pushed back to the date field of a post via the API, they are interpreted as GMT by the `rest_parse_date` function.

This means that whenever you set the date of a post based on the value that came back from the API, it ends up offset by the site's gmt_offset. Example requests (read in reverse order):

https://nylen.io/rest-api-post-dates-offset.png


Second, it's impossible to update the date of a draft - the request succeeds but the date is not changed. I haven't yet looked into why this is broken. Creating a draft with a specific date seems to work, though I think it may break core's flag for updating a post's date when the post changes (see "complication" below).


To address the first issue, we need to stop interpreting dates without timezones as UTC, and I think we should also include our best guess for the timezone offset with each date value. (For date this will be the site's gmt_offset, which is not currently available anywhere in the API; for date_gmt this will be zero).

One complication in addressing the second issue is that WP core uses a zero/null date_gmt to indicate that the date field should be updated when a draft is saved. See also #38883 and ticket:5698#comment:14

I'll work on a full patch next, but since it seems likely we'll need to discuss the best approach, I'm starting this ticket off with failing test cases first. We also need tests for rest_parse_date, rest_get_date_with_gmt, and any other related methods.

Attachments (5)

39256.diff (5.4 KB) - added by jnylen0 8 years ago.
Test cases for setting post dates during create and update (12/20 failing)
39256.2.diff (10.1 KB) - added by jnylen0 8 years ago.
Fix issues with timezone conversion; add a whole bunch of tests
39256.3.diff (11.0 KB) - added by jnylen0 8 years ago.
Allow updating the date of a draft post
39256.4.diff (17.2 KB) - added by jnylen0 8 years ago.
Deprecate rest_get_date_with_gmt; add tests for comments
39256.5.diff (15.5 KB) - added by jnylen0 8 years ago.
Do not deprecate rest_get_date_with_gmt

Download all attachments as: .zip

Change History (22)

@jnylen0
8 years ago

Test cases for setting post dates during create and update (12/20 failing)

#1 @jnylen0
8 years ago

  • Milestone changed from 4.7.1 to 4.7.2

This is the #​1 API issue on my mind at the moment, because it is pretty severely broken in 4.7, but unfortunately not going to make it into 4.7.1.

@jnylen0
8 years ago

Fix issues with timezone conversion; add a whole bunch of tests

#2 @jnylen0
8 years ago

In 39256.2.diff:

  1. Fix the first issue described in the original report. The problem was that a value for the date field without a timezone offset was being interpreted as a UTC date during parsing.
  1. Improve behavior when specifying dates with timezone offsets (do not remove the timezone offset to force a date into UTC). Previously, you could set date_gmt to e.g. 2016-12-12T18:00:00-01:00... and this would be interpreted as 2016-12-12T18:00:00 UTC. I can't think of a reason why this is what you would want.
  1. Tests for all of the above cases (and some extra ones for the rest_parse_date function). Currently there are basically no tests for REST API date handling.

Still left to do:

  • Allow updating the date of a draft. I'll address that in this ticket.
  • Figure out a way to work around the zero/null date_gmt terribleness with drafts. I have some further ideas on this, but this work can happen on #38883 after this ticket is done.

This latest patch has 28 test cases for updating post dates using the API (6 failing; 22 passing).

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


8 years ago

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


8 years ago

#5 follow-up: @aduth
8 years ago

  • The $has_timezone regular expression could incorrectly match e.g. +99:99, and does not match a valid -0500 (omitting : permitted). More accurate option might be #(Z|[+-]([0-1]\d|2[0-4])(:?[0-5]\d)?)$#
  • rest_get_date_with_gmt had previously been the only function to call rest_parse_date with a second parameter. Since it no longer does, should we remove that argument altogether?

#6 @adamsilverstein
8 years ago

This looks good so far, the test cases are very helpful. Thanks for your work on this.

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


8 years ago

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


8 years ago

#9 in reply to: ↑ 5 @jnylen0
8 years ago

Replying to aduth:

The $has_timezone regular expression could incorrectly match e.g. +99:99 ...

I am not super worried about this.

... and does not match a valid -0500 (omitting : permitted). More accurate option might be #(Z|[+-]([0-1]\d|2[0-4])(:?[0-5]\d)?)$#

The regex is based on the existing regex in rest_parse_date. I'd prefer to preserve the existing behavior for now, and address this in a separate ticket.

Note that REST API date parsing is more strict than ISO8601 in other ways as well: we require dashes in between year, month, and day, and colons in between hour, minute, and second, whereas the standard does not.

rest_get_date_with_gmt had previously been the only function to call rest_parse_date with a second parameter. Since it no longer does, should we remove that argument altogether?

I was hoping to get more feedback on this specific point by now, but we may need to deprecate rest_get_date_with_gmt and replace it with a new function.

Last edited 8 years ago by jnylen0 (previous) (diff)

@jnylen0
8 years ago

Allow updating the date of a draft post

#10 @jnylen0
8 years ago

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

39256.3.diff addresses the second issue in the original report (allows updating the date of a draft).

From https://codex.wordpress.org/Function_Reference/wp_update_post#Scheduling_posts:

If you are trying to use wp_update_post() to schedule an existing draft, it will not work unless you pass $my_post->edit_date = true. WordPress will ignore the post_date when updating drafts unless edit_date is true.

I believe this is close to commit-ready. I plan to commit next week, in particular please give feedback on the approach needed to change the signature and behavior of rest_get_date_with_gmt.

@jnylen0
8 years ago

Deprecate rest_get_date_with_gmt; add tests for comments

#11 @jnylen0
8 years ago

  • Keywords has-patch added; needs-patch removed

In 39256.4.diff:

  • Deprecate rest_get_date_with_gmt; add and use a new function (rest_parse_date_with_utc) instead.
  • Add tests for creating and updating comments as well as posts.

#12 follow-up: @jnylen0
8 years ago

39256.4.diff is ready for commit, but it would be nice to modify rest_get_date_with_gmt in place rather than deprecating it. I think this should be fine, @dd32 do you agree?

Plugin usage is very light:

Matches  Plugin                             Active installs
=======  ======                             ===============
      4  rest-api                                   40,000+
      4  wptoandroid                                    30+
      5  custom-contact-forms                       60,000+
      2  appmaker-wp-mobile-app-manager                 50+
      4  appmaker-woocommerce-mobile-app-manager       200+

The rest-api plugin is a no-op in 4.7+, and the custom-contact-forms plugin does not load the API endpoints in 4.7+.

#13 in reply to: ↑ 12 ; follow-up: @dd32
8 years ago

Replying to jnylen0:

39256.4.diff is ready for commit, but it would be nice to modify rest_get_date_with_gmt in place rather than deprecating it. I think this should be fine, @dd32 do you agree?

@jnylen0 It looks like altering the function to simply handle a timezone on the input string is sane, without deprecating or adding an extra flag.
It doesn't appear that any code which would be using that function would expect that the timezone data would be discarded, so seems fine.

@jnylen0
8 years ago

Do not deprecate rest_get_date_with_gmt

#14 in reply to: ↑ 13 @jnylen0
8 years ago

  • Keywords commit added

Replying to dd32:

It looks like altering the function to simply handle a timezone on the input string is sane, without deprecating or adding an extra flag.

39256.5.diff does just that. I'll commit this tomorrow in the absence of further comments.

Last edited 8 years ago by jnylen0 (previous) (diff)

#15 @jnylen0
8 years ago

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

In 40101:

REST API: Fix multiple issues with setting dates of posts and comments.

This commit modifies the rest_get_date_with_gmt function to correctly parse local and UTC timestamps with or without timezone information.

It also ensures that the REST API can edit the dates of draft posts by setting the edit_date flag to wp_update_post.

Overall this commit ensures that post and comment dates can be set and updated as expected.

Fixes #39256.

#16 @jnylen0
8 years ago

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

Reopening for backport to 4.7 branch.

#17 @SergeyBiryukov
8 years ago

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

In 40114:

REST API: Fix multiple issues with setting dates of posts and comments.

This commit modifies the rest_get_date_with_gmt function to correctly parse local and UTC timestamps with or without timezone information.

It also ensures that the REST API can edit the dates of draft posts by setting the edit_date flag to wp_update_post.

Overall this commit ensures that post and comment dates can be set and updated as expected.

Props jnylen0.
Merges [40101] to the 4.7 branch.
Fixes #39256.

Note: See TracTickets for help on using tickets.