Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#35874 closed defect (bug) (fixed)

XMLRPC: Draft posts are published immediately when changed to published and future-dated at once

Reported by: redsweater's profile redsweater Owned by: rachelbaker's profile rachelbaker
Milestone: 4.5 Priority: normal
Severity: major Version: 4.4
Component: XML-RPC Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

A serious issue exists since r34572, in which a post that is edited via the XMLRPC API, such that its post_status changes from draft to publish, and at the same time its post_date is changed, will have the post_date disregarded because of "date reset" logic within wp_update_post().

A very negative impact of this is in a scenario where a user is confident that a draft post will change from draft status to future-published status. This behavior was reliable prior to r34572, but since that change, the behavior will result in an unexpectedly public, published post.

To reproduce the bug, using an XMLRPC client app such as my MarsEdit:

  1. Publish a new draft post with no date specified.
  2. Edit the draft post, changing the publish status to "Published" AND the date to some future date, e.g. tomorrow.
  3. Publish the edits.

The result is a published post with a post_date of the current time.

Although the bug is revealed by r34572, the issue lies deeper within faulty logic in wp_udpate_post(). The behavior prior to r34572 in which new posts were always assigned a baseline post_date, prevented the logic in wp_update_post from ever "resetting" the date. Thus the draft post created in step 1 above was assigned a post_date prior to r34572, but is now left with an empty date.

Subsequently, when the edits in step 2 above are made, this logic is met and the date is reset, even though the post being submitted for edit has expilicit post_date and post_date_gmt values set upon it:

// Drafts shouldn't be assigned a date unless explicitly done so by the user.
if ( isset( $post['post_status'] ) && in_array($post['post_status'], array('draft', 'pending', 'auto-draft')) && empty($postarr['edit_date']) &&
		 ('0000-00-00 00:00:00' == $post['post_date_gmt']) )
	$clear_date = true;
else
	$clear_date = false;

See how the logic in this case tests for the presence of a $postarredit_date? value, which is as it happens always empty in the case of XMLRPC API edits. It seems the test for "are we editing the date" needs to be more nuanced than merely testing for this value.

So it is the confluence of the following circumstnaces that lead to this bug:

  • A post already exists in the database
  • It does not have a specific date set
  • It has been saved as a draft, pending, or auto-draft status
  • It is not determined that its date is being edited right now

I think the fix for this can be as simple as rethinking the logic in the code cited above, perhaps allowing that EITHER the presence of a an 'edit_date' boolean, OR the presence of a non-empty post_date or post_date_gmt should circumvent the date reset.

Attachments (9)

fix-update-post.patch (1.3 KB) - added by redsweater 8 years ago.
Fixes to wp_update_post to prevent accidentally stripping the date from posts
test-update-post.patch (1.0 KB) - added by redsweater 8 years ago.
Tests to cover the fixes from this ticket's patch.
fix-and-test-update-post.patch (2.3 KB) - added by redsweater 8 years ago.
Fixes to wp_update_post to prevent accidentally stripping the date from posts.
fix-and-test-update-post-2.patch (1.8 KB) - added by redsweater 8 years ago.
Updated patch that attempts to address the bug more conservatively
xmlrpc-based-fix-and-test.patch (3.1 KB) - added by redsweater 8 years ago.
A new patch and test addressing the problem from the XMLRPC component.
xmlrpc-based-fix-and-test-2.patch (5.5 KB) - added by redsweater 8 years ago.
Update the patch to include a fix and test coverage for wp_editPost as well
xmlrpc-based-fix-and-test-3.patch (5.4 KB) - added by redsweater 8 years ago.
Update to remove cruft from the wp_editPost test case.
35874.diff (4.9 KB) - added by rachelbaker 8 years ago.
Refreshed patch and made minor style adjustments
35874.2.diff (4.9 KB) - added by DrewAPicture 8 years ago.
Improved inline comments

Download all attachments as: .zip

Change History (41)

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


8 years ago

#2 @redsweater
8 years ago

It looks like my analysis is out of date, since I was looking specifically at the state of the source tree when r34572 was committed. The buggy behavior is still present in trunk tip, but the exact source code responsible for the issue doesn't match my citation above.

I'll look closer at trunk tip and try to zero in on it.

#3 @redsweater
8 years ago

OK, updated analysis good as of just now with latest trunk: the problem specifically applies to clients who specify the date with "date_created" and leave "date_created_gmt" empty. MarsEdit is one such client.

The problematic code is here, in wp_insert_post():

if ( empty( $postarr['post_date_gmt'] ) || '0000-00-00 00:00:00' == $postarr['post_date_gmt'] ) {
	if ( ! in_array( $post_status, array( 'draft', 'pending', 'auto-draft' ) ) ) {
		$post_date_gmt = get_gmt_from_date( $post_date );
	} else {
		$post_date_gmt = '0000-00-00 00:00:00';
	}
} else {
	$post_date_gmt = $postarr['post_date_gmt'];
}

The problem this date resetting code hinges on the presence of 'post_date_gmt' which may be absent. In fact, code just prior to this block has standardized and validated the date by assigning to $post_date a value that is either taken directly from $postarrpost_date? or else derived from $postarrpost_date_gmt?.

At this point the canonical date for any further validation or date-reset logic should be the value in the local $post_date variable.

#4 @redsweater
8 years ago

It's all a little messy in this date logic. Previously in the function there is a block of code that derives the $post_date, but it has an outdated comment about ignoring draft and pending posts:

/*
 * If the post date is empty (due to having been new or a draft) and status
 * is not 'draft' or 'pending', set date to now.
 */
if ( empty( $postarr['post_date'] ) || '0000-00-00 00:00:00' == $postarr['post_date'] ) {
	if ( empty( $postarr['post_date_gmt'] ) || '0000-00-00 00:00:00' == $postarr['post_date_gmt'] ) {
		$post_date = current_time( 'mysql' );
	} else {
		$post_date = get_date_from_gmt( $postarr['post_date_gmt'] );
	}
} else {
	$post_date = $postarr['post_date'];
}

The fact that $post_date and $post_date_gmt variables are derived in this function makes it a little hard to wrap head around. Currently the date handling is mostly standardized around converting everything to $post_date, but then some dependencies on a GMT version of the date are intermingled.

#5 @redsweater
8 years ago

I am currently endeavoring to rework the logic of this date related stuff to eliminate the bug reported here while hopefully also making it easier to maintain and express the desired behavior.

#6 @redsweater
8 years ago

  • Keywords has-patch has-unit-tests added

Although this bug affects XMLRPC (and probably REST API), the fix is actually in another component (Posts?).

I'm attaching a patch for a fix and a patch for the tests to cover the buggy behavior reported in this bug.

I was wrong about the code in wp_insert_post needing to be reworked to address this bug. I have refactored that but will report it as a separate ticket.

@redsweater
8 years ago

Fixes to wp_update_post to prevent accidentally stripping the date from posts

@redsweater
8 years ago

Tests to cover the fixes from this ticket's patch.

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


8 years ago

#8 @redsweater
8 years ago

FWIW the bug here is even a little wider-spread than I thought: in general, the dateCreated flag will be ignored for an updated Draft or Pending post, if it doesn't already have a date associated with it.

So for example, independent of switching a post to "Published" status, simply trying to set a date on an existing Draft that was previously published without a date will fail.

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


8 years ago

#10 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.5

#11 @rachelbaker
8 years ago

@redsweater would you mind combining your patches, so the code fix and the unit test are in one patch file?

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

#12 @rachelbaker
8 years ago

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

@redsweater
8 years ago

Fixes to wp_update_post to prevent accidentally stripping the date from posts.

#13 @redsweater
8 years ago

Let me know if I can do anything else! Thanks.

#14 @rachelbaker
8 years ago

  • Version changed from trunk to 4.4

#15 @rachelbaker
8 years ago

@redsweater Looking at your patch, couldn't this be solved by setting the post_date_gmt from post_date in the XML-RPC methods?

#16 @redsweater
8 years ago

@rachelbaker The whole mess of code makes my head hurt, so I tried to solve it in as straight-forward a way as I could without rethinking it too much. My thinking was it's better to fix it in the core method because that would protect against a similar bug affecting other clients to that function. For example, maybe the REST API will use it? Also, I don't completely understand the convention of the 00'd out date strings, nor am I confident it's a 100% reliable way to test for whatever case they are trying to cover here, but the fact that extending the test to cover post_date keeps the logic identical except for also extending it to that field, made me most comfortable in submitting a patch I could defend :)

#17 @redsweater
8 years ago

@rachelbaker To underscore why it's probably most appropriate to fix the bug in wp_insert_post, consider that at least by the contract/documentation of wp_insert_post, post_date is a primary, expected parameter to the function. I think that's shifting over time since the XMLRPC API is favoring use of post_date_gmt, but unless we want to document away the validity of passing post_date to wp_insert_post, I think it should be safeguarded within the function.

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


8 years ago

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


8 years ago

@redsweater
8 years ago

Updated patch that attempts to address the bug more conservatively

#20 @redsweater
8 years ago

Please disregard fix-and-test-update-post-2.patch, as it turns out I have an approach to address the bug in XMLRPC after all.

@redsweater
8 years ago

A new patch and test addressing the problem from the XMLRPC component.

#21 @redsweater
8 years ago

Please consider the updated patch https://core.trac.wordpress.org/attachment/ticket/35874/xmlrpc-based-fix-and-test.patch.

The key insight here is that the test in wp_update_post that determines whether to clear the date (the crux of this problem) consults among its tests the value of $postarredit_date? which is always missing on calls originating from XMLRPC. The value appears to be a flag set by the wp-admin interface specifically to indicate whether a change of date was specified by the user.

It seems reasonable then, to adapt XMLRPC to supply this value when it makes edits, so that the same logic that prevents dates being reset for wp-admin edits will also apply to scenarios where the API can tell that an explicit date has been specified.

@redsweater
8 years ago

Update the patch to include a fix and test coverage for wp_editPost as well

#22 @redsweater
8 years ago

Since my app, MarsEdit, uses the mw_editPost method currently, my focus was on getting that working. However, the same bug affects wp_editPost, so I added a fix for that case and an associated unit test for wp_editPost as well.

@redsweater
8 years ago

Update to remove cruft from the wp_editPost test case.

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


8 years ago

#24 @kirasong
8 years ago

  • Severity changed from normal to major

Raising priority due to @rachelbaker's comments in #core.

@dd32 @westi @markoheijnen @ocean90 @SergeyBiryukov Mind taking a look?

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


8 years ago

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


8 years ago

@rachelbaker
8 years ago

Refreshed patch and made minor style adjustments

#27 @rachelbaker
8 years ago

  • Keywords commit added

#28 @rachelbaker
8 years ago

In 35874.diff I refreshed and ready to go. I asked @DrewAPicture to clarify the inline comment verbiage for me.

@DrewAPicture
8 years ago

Improved inline comments

#29 @DrewAPicture
8 years ago

@rachelbaker Based on my reading of the ticket, unit tests, et al, 35874.2.diff attempts to give a simpler, more direct explanation of what's happening. Let me know if I got that wrong.

#30 @rachelbaker
8 years ago

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

In 37043:

XMLRPC: Fix bug where draft posts couldn’t be published in the future, and would publish immediately.

Resolves bug introduced in [r34572], in which editing a Post via the XMLRPC API with a draft post_status, where the post_status changes from draft->publish with a future post_date set for the publish action, will have the future post_date disregarded and the Post will be published immediately. The expected behavior is that the post_date is used to schedule the Post to be published in the future.

Fixes #35874.

Props redsweater, rachelbaker, DrewAPicture

#31 @rachelbaker
8 years ago

In 37044:

XMLRPC: Unit tests left off [r37043]

See #35874.

Props redsweater, rachelbaker

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


5 years ago

Note: See TracTickets for help on using tickets.