Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 4 years ago

#35053 closed defect (bug) (fixed)

XML-RPC when post with date_created_gmt, its post_date will gmt date not local date

Reported by: hnle's profile hnle Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: XML-RPC Keywords: has-patch commit revert
Focuses: Cc:

Description

Since [34681] (fix of #30429), mw_newPost mw_editPost using date_created_gmt (like WP for Android App) is broken because its post_date will GMT date not Local date.

Android App request sample: https://gist.github.com/hinaloe/88bc64ec00b2ce79f502

In App, I selected 12-14 20:00 (+09:00) so date_created_gmt is 20151214T11:00:00.
but in WP, <post_date | post_date_gmt> is <2015-12-14 11:00:00 | 2015-12-14 02:00:00>

[34681] was fixed when using dateCreated but effected to gmt...

Attachments (3)

35053.patch (4.7 KB) - added by hnle 8 years ago.
35053-2.patch (16.5 KB) - added by dossy 8 years ago.
35053-3.patch (15.6 KB) - added by dossy 8 years ago.

Download all attachments as: .zip

Change History (39)

@hnle
8 years ago

#1 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.4.1

#2 @hnle
8 years ago

(Note reported in forum by @bbb_kuro) thx.

#3 @hnle
8 years ago

  • Keywords has-patch has-unit-tests added

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


8 years ago

#5 follow-up: @redsweater
8 years ago

The same commit [34681] also changed the behavior of dates specified with the "date_created" key, such that the date is set on the server to the dateCreated value without regard for its time zone information. For example, a post that is edited and set to a date:

<dateCreated>
	<value><dateTime.iso8601>20151215T15:19:49Z</dateTime.iso8601></value>
</dateCreated>

Comes back from the server as the same date and time with the Z stripped:

<member><name>post_date</name><value><dateTime.iso8601>20151215T15:19:49</dateTime.iso8601></value></member>

This has the effect of causing posts to be scheduled or set as having been published at a time that is as many hours off from GMT as the user's local time zone.

The patch supplied with this ticket does not yet fix the behavior for the dateCreated case.

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

#6 @hnle
8 years ago

  • Keywords needs-patch added; has-patch removed

Yes, I also it is in the mind.

It is because iso8601_to_datetime's result ($timezone == 'user') does'nt take into account the time zone.

Which the better way or other?

  • Change iso8601_to_datetime
  • revert and use iso8601_to_datetime second arg 'gmt' : It should meen the way of [34681] was wrong. (but good effect to think)

:)

Oh…… dateCreated has not always timezone value……ok. sure.

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

#7 @bcarney
8 years ago

I'm just an end user that is using the Syndicate Out plugin. I can attest that this bug has impacted our use of that plugin - all posts on the "Master" site are now being syndicated as "Scheduled" instead of "Published".

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


8 years ago

#9 @jorbin
8 years ago

  • Owner set to wonderboymusic
  • Status changed from new to assigned

@wonderboymusic @justdaiv @smerriman this looks to be related to [34681] which you all worked on, can you take a look?

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


8 years ago

@dossy
8 years ago

#11 in reply to: ↑ 5 @dossy
8 years ago

  • Keywords needs-testing added; needs-patch removed

Replying to redsweater:

The same commit [34681] also changed the behavior of dates specified with the "date_created" key, such that the date is set on the server to the dateCreated value without regard for its time zone information. For example, a post that is edited and set to a date: [...]

The patch supplied with this ticket does not yet fix the behavior for the dateCreated case.

I added new tests to cover this case (dateCreated value is an ISO-8601 date with a timezone specified) and updated the code to handle it correctly in attachment:35053-2.patch, which builds on top of @hnle's previous work.

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


8 years ago

#13 @redsweater
8 years ago

I confirmed that @dossy's revised patch addresses the problem I reported in comment #5. From my perspective as a WordPress client developer, this is a good improvement to the API that restores functionality to its historic behavior.

#14 @markoheijnen
8 years ago

I do like that we have failing tests for 4.4 now. When I only reverted src/wp-includes/class-wp-xmlrpc-server.phpserver.php all tests still passes. Which seems to make sense looking at the code.

#15 @dossy
8 years ago

@markoheijnen - is there anything else that you feel needs to be done for this patch to be accepted? While it's all still fresh in my head ... ;-)

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


8 years ago

#17 @dd32
8 years ago

I'm hesitant to change iso8601_to_datetime() to use date functions directly, especially in a point release.

FYI WordPress also sets the datetimezone to UTC during bootstrap, so it can be assumed to be set to that if it makes life easier.

Why is there try{} catch{} handling in the unit tests wrapping iso8601_to_datetime()? If those datetimes can throw exceptions they need to be caught inside iso8601_to_datetime().

Doesn't the America/New_York timezone respect DST? If that's the case, adding 5hrs isn't always correct. Probably best to use a UTC+x timezone for tests so it's explicit.

#18 @dossy
8 years ago

get_gmt_from_date() and get_date_from_gmt() were already using PHP's Date/Time functions, so I figured it would be okay to use in iso8601_to_datetime() as well.

Similarly, yes, the date/time functions can throw exceptions, and the other WP functions that I mentioned above don't catch them, either, so I was trying to stay consistent with what's already in the codebase. I agree, personally, that the functions should catch and handle the exceptions, but I didn't want to reach into those other functions to change their implementations - it seems weird that one set of functions would catch exceptions while the others don't.

And, yes, America/New_York has DST, but because our tests have specific dates in the tests, we already know if they're during the DST time of year, or not, therefore there's no need to use UTC-5.

#19 follow-up: @dd32
8 years ago

And, yes, America/New_York has DST, but because our tests have specific dates in the tests, we already know if they're during the DST time of year, or not, therefore there's no need to use UTC-5.

Right, Got it. That's fine then.

Given the extent of this patch compared to the original commit, the lack of any committer with a reasonable grasp of the issues at play, and the fact that the timezone handling issues are a long standing issues (See #30429 and #25399).
Is there any good reason why this should be pushed into 4.4.1 in preference to a revert of behaviour to pre-4.4? The behaviour previous was well known as buggy, but consistent, this has simply changed it to be buggy in a different way.

#20 in reply to: ↑ 19 ; follow-up: @smerriman
8 years ago

Replying to dd32:

The behaviour previous was well known as buggy, but consistent, this has simply changed it to be buggy in a different way.

I can see now why what I thought was the fix is flawed and both new and old are buggy, but I disagree that the previous approach was consistent. As per #30429, with the old approach it was impossible to get a consistent value for post_date and post_date_gmt in the database if you pass the post_date parameter (and not post_date_gmt), unless your site was running in the GMT timezone. One of them was guaranteed to be incorrect.

With the current approach if post_date is used, it is consistent and correct when a local time is passed to post_date, and buggy if you try to pass things in another timezone format (as per redsweater's comment). I'm probably missing a use-case but I would have thought that's where you'd use post_date_gmt; you're using post_date specifically because you do want a local time.

However, my approach was never meant to affect when post_date_gmt was passed. Perhaps a simpler quick-fix would be to apply the previous logic only when post_date_gmt is empty.

#21 in reply to: ↑ 20 @dossy
8 years ago

Replying to smerriman:
[...]

With the current approach if post_date is used, it is consistent and correct when a local time is passed to post_date, and buggy if you try to pass things in another timezone format (as per redsweater's comment). [...]

And, the patch I provided fixes this, as @redsweater confirmed.

Replying to dd32:

[...] Is there any good reason why this should be pushed into 4.4.1 in preference to a revert of behaviour to pre-4.4? The behaviour previous was well known as buggy, but consistent, this has simply changed it to be buggy in a different way.

AFAICT, the patch I submitted implements the correct behavior in all cases currently described. If I missed a case, describe it and I'll work on implementing the test for it. I don't agree that it's "simply changed ... to b buggy in different way." The key improvement that I implemented in iso8601_to_datetime() is to properly use the PHP Date/Time functions to coerce the date either to GMT or to local timezone (per get_option( 'timezone_string ')'s setting).

My implementation might even coincidentally fix #25399, but I'm not completely sure since there's no tests that document exactly what the breakage is. I'll read #20328 more carefully and try to decipher exactly what @nacin might be referring to as still broken. It looks like the code all uses the old get_date_from_gmt() and get_gmt_from_date() functions and could probably benefit from the changes in iso8601_to_datetime() ...

#22 follow-up: @nacin
8 years ago

Some questions:

  • What bug is worse: #30429 or #35053 (this ticket)? It's possible we fixed a major bug and caused a minor one, so a revert may not be worth the tradeoff, but I don't know.
  • Is this bug a regression? As in, did it work fine prior to [34681]?

This sounds like a revert of [34681], assuming it is a regression and it's an equally bad bug — local used to be broken, now that's fixed but GMT is broken. I'd rather GMT work, since it's always worked. And the patch to make this work is a) not ready (for one, exceptions should absolutely be trapped within iso8601_to_datetime()), b) as it stands, too big and too hairy for a point release.

#23 @nacin
8 years ago

@wonderboymusic, any thoughts here?

#24 @redsweater
8 years ago

@nacin Just to clarify the severity of the regression: since [34681], in my tests, both "local" (dateCreated) and GMT (date_created_gmt) are broken. I still use the local field in MarsEdit, which is where my customers noticed that the time zone information, which could always optionally be supplied, was being ignored.

My first attempt to work around the bug was to see if switching to date_created_gmt would give me a better experience, but of course because of the initial bug described here by @hnle, that path is also not working right.

In short: since [34681], I believe it's impossible to specify a date with useful time zone information through the API.

My feeling is you should revert [34681] and to apply @dossy's valuable efforts to creating a bona fide fix for both issues in 4.4.2 or a later release.

Version 1, edited 8 years ago by redsweater (previous) (next) (diff)

#25 in reply to: ↑ 22 @dossy
8 years ago

Replying to nacin:

[...] And the patch to make this work is a) not ready (for one, exceptions should absolutely be trapped within iso8601_to_datetime()),

Actually, I just realized - I originally wrote the code using the PHP Date/Time object oriented interface, which throws exeptions. I later changed the code to exclusively use the procedural interface, which returns errors instead of throwing exceptions. So, my actual implementation of iso8601_to_datetime() no longer throws exceptions, but I never update the unit tests to reflect this. I can go ahead and update the tests, if that's your objection ...

(FWIW, get_gmt_from_date() and get_date_from_gmt() both use the object oriented interface, c.f. new DateTimeZone(...), $datetime->setTimezone(...) and $datetime->format(...), all of which could throw exceptions and are not caught.)

b) as it stands, too big and too hairy for a point release.

So, 4.5 then? In the meantime, revert all the partial attempts to fix date handling in 4.4 which fix one bug and introduce another?

@dossy
8 years ago

#26 follow-up: @dossy
8 years ago

Oh, now I remember why I used the try/catch pattern in the tests I wrote: I needed to ensure that update_option( 'timezone_string', $tz ); got called after a failed PHPUnit assertion, to restore the pre-test timezone_string value, since the tests modify it.

Really, I should have done this inside PHPUnit's setUp() and tearDown(). I've uploaded a fresh patch that improves the tests this way, see 35053-3.patch.

#27 in reply to: ↑ 26 ; follow-up: @nacin
8 years ago

  • Keywords has-patch commit revert added; has-unit-tests needs-testing removed

Thank you for the feedback, @redsweater and @dossy. Marking [34681] for revert.

Replying to dossy:

Oh, now I remember why I used the try/catch pattern in the tests I wrote: I needed to ensure that update_option( 'timezone_string', $tz ); got called after a failed PHPUnit assertion, to restore the pre-test timezone_string value, since the tests modify it.

Really, I should have done this inside PHPUnit's setUp() and tearDown(). I've uploaded a fresh patch that improves the tests this way, see 35053-3.patch.

This actually isn't necessary. You're right that tearDown() is normally the correct approach, but our own WP_UnitTestCase rolls back DB writes as well as the local object cache, so you don't need to restore option values.

#28 in reply to: ↑ 27 @dossy
8 years ago

Replying to nacin:
[...]

This actually isn't necessary. You're right that tearDown() is normally the correct approach, but our own WP_UnitTestCase rolls back DB writes as well as the local object cache, so you don't need to restore option values.

Ah, that's very cool. I just assumed that since @hnle's original patch stashed and restored the timezone_string option value that it needed to be done explicitly in each test.

#29 @wonderboymusic
8 years ago

I am going to defer to anyone who actually still uses XML-RPC. A lot of these methods didn't/still don't have exhaustive unit tests, so I think we should do whatever causes the least amount of harm. If we revert, let's also keep this ticket open, fix the underlying issues, and fill out the unit tests.

#30 @nacin
8 years ago

In 36163:

XML-RPC: Revert [34681] as it broke date handling.

props dossy, hnle, redsweater.
see #35053, #30429 (original ticket).

#31 @nacin
8 years ago

In 36164:

XML-RPC: Revert [34681] as it broke date handling.

Merges [36163] to the 4.4 branch.

props dossy, hnle, redsweater.
see #35053, #30429 (original ticket).

#32 @nacin
8 years ago

@dossy Want to spin work here off into a new ticket, so we can close this one as fixed for 4.4.1?

#33 @dossy
8 years ago

@nacin - Sure, but as I understand it, this bug was introduced in [34681], so reverting that "fixes" this bug. Really, we should re-open #30429 as that's no longer fixed after reverting [34681].

Do you still want me to create another bug? There's also #25399, which I actually suspect my improved iso8601_to_datetime()will also fix, but I need to figure out what are appropriate unit tests to demonstrate the bugs that are relevant for that ticket based on the discussion from #20328.

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


8 years ago

#35 @ocean90
8 years ago

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

#36 @Rarst
4 years ago

I've revisited patch in #30429 since related functions in WP 5.3 like iso8601_to_datetime() got fixed up, which put a complex system of errors depending on each other out of sync.

If anyone closely familiar with XMLRPC would appreciate a look, please! :)

Note: See TracTickets for help on using tickets.