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

[redsweater]

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.

[redsweater]

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.

[redsweater]

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.

[redsweater]

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.

[redsweater]

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.

[redsweater]

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]

Fixes to wp_update_post to prevent accidentally stripping the date from posts

[redsweater]

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

[redsweater]

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.

[rachelbaker]

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

[redsweater]

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

[redsweater]

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

[rachelbaker]

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

[redsweater]

@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 :)

[redsweater]

@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.

[redsweater]

Updated patch that attempts to address the bug more conservatively

[redsweater]

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]

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

[redsweater]

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]

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

[redsweater]

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]

Update to remove cruft from the wp_editPost test case.

[kirasong]

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

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

[rachelbaker]

Refreshed patch and made minor style adjustments

[rachelbaker]

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

[DrewAPicture]

Improved inline comments

[DrewAPicture]

@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.

[rachelbaker]

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

[rachelbaker]

In 37044:

XMLRPC: Unit tests left off [r37043]

See #35874.

Props redsweater, rachelbaker