WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 3 months ago

#19733 closed defect (bug) (fixed)

XML-RPC returns invalid dates if the date is zero

Reported by: koke Owned by: westi
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3
Component: XML-RPC Keywords: has-patch mobile needs-unit-tests
Focuses: Cc:

Description

When a post has a 'pending' status, post_date_gmt is set to 0000-00-00 00:00:00 as a marker to update post_date when it's saved (see #5698).

mysql2date then proceeds to turn that date into a negative one, and IXR_Date destroys the thing a bit more, so 0000-00-00 00:00:00 turns into -0001113TT0::0::00, which is an invalid ISO8601 date

Attachments (8)

patch-core-19733.diff (2.4 KB) - added by koke 2 years ago.
patch-core-19733-2.diff (11.8 KB) - added by markoheijnen 2 years ago.
patch-core-19733-3.diff (12.4 KB) - added by markoheijnen 2 years ago.
patch-core-19733-4.diff (12.8 KB) - added by markoheijnen 2 years ago.
patch-core-19733-4.2.diff (12.8 KB) - added by markoheijnen 2 years ago.
patch-core-19733-5.diff (12.3 KB) - added by markoheijnen 2 years ago.
19733.tests.diff (2.6 KB) - added by ericmann 6 months ago.
Unit tests for both _convert_date() and _convert_date_gmt()
19733.tests-subclass.diff (2.5 KB) - added by ericmann 6 months ago.
Same unit tests as proposed before, but using a subclass rather than reflection to access the protected methods.

Download all attachments as: .zip

Change History (26)

koke2 years ago

comment:1 koke2 years ago

  • Keywords has-patch mobile added

Added a patch which sets the post_date_gmt value to the converted post_date for pending and auto-draft posts.

comment:2 daniloercoli2 years ago

  • Cc ercoli@… added

comment:3 DrewAPicture2 years ago

Related #16985 #18998

Last edited 2 years ago by DrewAPicture (previous) (diff)

comment:4 nacin2 years ago

  • Milestone changed from Awaiting Review to 3.4

Looks good to me.

comment:5 koke2 years ago

Also, IXR_Date doesn't like "weird" dates.

  • mysql2date returns -00011130T00:00:00. It looks weird but it makes sense: 0000-00-00 -> -1-12-00 -> -1-11-30
  • IXR_Date works by taking substrings, and the leading '-' is what breaks everything.

Using a NULL for a 'zero' date might make sense, but I wonder how many things would break, probably including XML-RPC :(

comment:6 markoheijnen2 years ago

  • Cc marko@… added

Added a new patch at #18429 that also fixes this issue. Do need to test it tomorrow if it works as it should.

comment:7 westi2 years ago

In [20153]:

XMLRPC: Intoduce a date generation helper method to improve the dates returned over XMLRPC when we have a 0 date stored for drafts.

This improves the ability of clients to work with the new wp Post APIs. See #18429 and #19733 props maxcutler.

comment:8 markoheijnen2 years ago

Added a patch that applies the patch of maxcutler to all the methods. I didn't applied the code of the first patch of this ticket. Is that still needed?

comment:9 maxcutler2 years ago

  • Cc max@… added

comment:10 westi2 years ago

  • Keywords needs-unit-tests added

I would really like to have the last patch in 3.4 but I also really don't want to commit it without us adding test cases because I just found a bunch of bugs in the featured image stuff we applied everywhere by writing the basic test cases for mw_(new|edit)_Post.

If someone wants to write up some tests to cover the date stuff that would be really helpful.

comment:11 markoheijnen2 years ago

I applied the patch of koke in a way that it only applies for incorrect dates. No matter what the post status is.
I also applied it to all the new XML-RPC methods so everything has the same date handling.

A benefit for this patch is that _convert_date and _convert_date_gmt only return an IXR_Date.

comment:12 westi2 years ago

I took a look at the last patch and all looks good apart from:

  • _convert_mysql2date is a level of abstraction too far - lets just call mysql2date directly.
  • $this-> _convert_date( $date_gmt ); - space in this.

@markoheijnen could you give the patch a refresh to address these?

comment:13 markoheijnen2 years ago

Fixed that in patch-core-19733-5.diff

comment:14 westi2 years ago

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

In [20353]:

XMLRPC: Make sure that we always return valid dates when no date is currently set - for example if the post is pending. Fixes #19733 props markoheijnen and koke.

ericmann6 months ago

Unit tests for both _convert_date() and _convert_date_gmt()

ericmann6 months ago

Same unit tests as proposed before, but using a subclass rather than reflection to access the protected methods.

comment:15 ericmann6 months ago

The above two patches add two different approaches to testing _convert_date() and _convert_date_gmt(). The first patch uses reflection to gain access to the protected methods in the XMLRPC server class. The second patch creates a subclass to expose them publicly instead. Both work (and pass) and I leave it to the committer to chose the best approach.

Note: While the Reflection approach is cleaner in code, the subclass approach is easier for new devs to understand. I would recommend committing 19733.tests-subclass.diff.

comment:16 follow-up: markoheijnen6 months ago

I personally would go for 19733.tests.diff​. I think the reflection here is pretty easy to understand. It's a few lines of code and you did add a comment that you want to access a protected method.

I personally would split each test case by having a standard and a null test case.

comment:17 in reply to: ↑ 16 ericmann6 months ago

Replying to markoheijnen:

I personally would go for 19733.tests.diff​. I think the reflection here is pretty easy to understand. It's a few lines of code and you did add a comment that you want to access a protected method.

Easy to understand, but somewhat non-standard. Hence the second patch as an alternative.

I personally would split each test case by having a standard and a null test case.

While we could split them out, there's no real added benefit of doing that. Plus having the standard and null date in the same test method doesn't really have a drawback. That said, if anyone else disagrees it would be trivial to split them.

comment:18 markoheijnen3 months ago

#18681 was marked as a duplicate.

Note: See TracTickets for help on using tickets.