WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#28601 closed defect (bug) (fixed)

wp.newPost fatals when a post_date field is included in the data

Reported by: dllh Owned by: SergeyBiryukov
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: XML-RPC Keywords: needs-patch
Focuses: Cc:

Description

In _insert_post(), we assume that $post_data['post_date'] and $post_data['post_date_gmt'] are already IXR_Date objects, when they're not. These are converted in the wp_editPost() function but are not converted either in wp_newPost() or in _insert_post(). The result is that if you submit an xmlrpc request that includes post_date or post_date_gmt, you get this fatal error:

Fatal error: Call to a member function getIso() on a non-object in /root/wordpress-develop/src/wp-includes/class-wp-xmlrpc-server.php on line 1200

I'm attaching a patch that fixes it and a couple of unit tests that demonstrate the error. To repro:

  1. Apply the unit test patch but not the xmlrpc patch.
  2. Run the tests.

You'll see the fatal error as the tests run. Apply the xmlrpc patch and run the tests again, and the error is gone.

I've basically copied the method that was already being used in wp_editPost(), with some checks for existence of the data added, since there's not a certainty that it'll be submitted.

Attachments (2)

class-wp-xmlrpc-server.php.patch (1.6 KB) - added by dllh 5 years ago.
Patches the xml-rpc server code to fix the fatal
unit-test-xmlrpc-wp_newPost.patch (1.3 KB) - added by dllh 5 years ago.
Unit tests to demonstrate the issue

Download all attachments as: .zip

Change History (14)

@dllh
5 years ago

Patches the xml-rpc server code to fix the fatal

@dllh
5 years ago

Unit tests to demonstrate the issue

#1 @dllh
5 years ago

  • Keywords has-patch added

Note: I wrote the unit tests to demonstrate the issue and not necessarily to be added to the unit test suite. I'm not sure they're really all that useful as ongoing tests. It was just an easy way to show that the fatal occurred before the patch and went away after.

This ticket was mentioned in IRC in #wordpress-dev by dllh. View the logs.


5 years ago

#3 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.0

#4 follow-up: @SergeyBiryukov
5 years ago

I wonder if these checks should go into _insert_post() method instead, where most of the validation happens.

Last edited 5 years ago by SergeyBiryukov (previous) (diff)

#5 in reply to: ↑ 4 @dllh
5 years ago

Replying to SergeyBiryukov:

I wonder if these checks should go into _insert_post() method instead, where most of the validation happens.

That was actually my first impulse, and the first version of the patch (not attached). I moved it out of that function with these thoughts in mind:

  • Maybe somebody had a good reason for putting the validation in wp_editPost(), so following that model was perhaps a good first step.
  • It's possible somebody extends the xml-rpc server with custom code that does its own checks. If they're convertng in their function (because we're not) and we reconvert in _insert_post(), we'll introduce another error. Introducing higher in the stack seemed both consistent with past code and safer going forward. Of course, we could add an is_a() check or something to prevent double conversion.

I'd be happy to rework the patch to move the validation into _insert_post() if you think that's better, though. What say you?

#6 @SergeyBiryukov
5 years ago

  • Keywords commit added

I see, thanks for the clarification. Looks good to me.

Related: [19848], [20153].

#7 @SergeyBiryukov
5 years ago

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

In 28854:

XML-RPC: Make sure wp.newPost does not produce a fatal error when a post_date field is included in the data.

props dllh.
fixes #28601.

#8 @westi
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

While this fix introduces some consistency is also breaks some clients.

Previously this worked fine:

<member><name>post_date_gmt</name><value><dateTime.iso8601>20140630T08:46:25</dateTime.iso8601></value></member>

This gets parsed into an IXR_Date and then we try and treat is as a string :(

I think we need to make new and edit post accept both dateTime.iso8601 in the structs and dates as strings.

The get function will return dateTime.iso8601 which make a strong ease of use argument for supporting it in the other methods.

#9 @SergeyBiryukov
5 years ago

  • Keywords needs-patch added; has-patch commit removed

#10 @westi
5 years ago

In 29061:

XMLRPC: Improve the test case from [28854] so that it truely tests an invalid date string like the test name implies. See #28601.

#11 @westi
5 years ago

In 29062:

XMLRPC: Extend the test cases from [28854] so that we also test valid date based strings. See #28601.

#12 @westi
5 years ago

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

In 29063:

XMLRPC: Restore support in wp.newPost for dates to be supplied in the structured dateTime.iso8601 format as well as still supporting dates specified as strings.

Fixes #28601.

Note: See TracTickets for help on using tickets.