New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST API: Avoid resetting parent field in draft update #11513

Merged
merged 3 commits into from Nov 14, 2018

Conversation

Projects
None yet
2 participants
@aduth
Member

aduth commented Nov 5, 2018

Related: #10753

This pull request seeks to resolve an issue where if a draft page is assigned a parent, the parent will become unset the next time an autosave occurs. This occurs because while the revision's parent is typically assigned to the post for which the revision is to be created, in this flow a revision is not created and instead the post itself is updated.

Testing instructions:

Verify that an autosave for a page assigned a parent does not reset the parent value send the parent value in its save request.

  1. Navigate to Pages > Add New
  2. Set a Parent page
  3. Add a title
  4. Save the page
  5. Reload
  6. Change the title
  7. Wait for autosave
  8. Once autosave occurs, reload observe in the Developer Tools Network tab that a parent value was not sent with the request.

Note that the parent field is still assigned to the value chosen in Step 2.

Verify there are no other regressions in the behavior of autosave.

Ensure tests pass:

npm run test-unit
npm run test-unit-php
npm run test-e2e

Follow-up Task:

Needs Trac ticket / patch for core 5.0 branch if deemed an appropriate solution.

Related:

@@ -188,6 +188,10 @@ public function create_item( $request ) {
$user_id = get_current_user_id();
if ( ( 'draft' === $post->post_status || 'auto-draft' === $post->post_status ) && $post->post_author == $user_id ) {
// To avoid resetting the draft's parent, clear the parent of the
// prepared revision which is assigned the draft's own ID.
unset( $prepared_post->post_parent );

This comment has been minimized.

@danielbachhuber

danielbachhuber Nov 8, 2018

Member

Is this truly necessary, or are you simply adding it to guard against a client causing a similar bug in the future?

The reason I ask is more from the perspective of: why is post_parent a special case? Is it a special case simply because Gutenberg sent it, or is there some additional functional value?

If the former, then we'd need to guard against all extraneous post fields. Which means it probably only makes sense to whitelist the fields we do allow: title, excerpt, content.

Let me know if this doesn't make sense or I missed something.

This comment has been minimized.

@aduth

aduth Nov 8, 2018

Member

I don't see it as something specific to Gutenberg: In sending a payload to the autosave endpoint, I consider that the parent_id value I send should correspond to the post for which we're creating an autodraft revision. That the REST API interprets the fact that it's a draft and thus doesn't need to operate on a revision should change its treatment of the parent field. It's a bit of an overloaded use of the parent field.

we'd need to guard against all extraneous post fields. Which means it probably only makes sense to whitelist the fields we do allow: title, excerpt, content.

I agree. This would also help avoid setting misleading expectations about the ability to send other fields for this specific draft condition, which won't work for when creating/updating a revision†.

It still needs to accept parent_id, but it only becomes relevant when creating a revision in knowing where to attach the revision, not for the update flow here.

I've not actually confirmed whether this behavior takes place

This comment has been minimized.

@danielbachhuber

danielbachhuber Nov 12, 2018

Member

@aduth As it turns out, it looks like you flagged this issue 5 months ago: https://core.trac.wordpress.org/ticket/43316#comment:89


@adamsilverstein Do you recall why you introduced this conditional logic?

if ( ( 'draft' === $post->post_status || 'auto-draft' === $post->post_status ) && $post->post_author == $user_id ) {
	// Draft posts for the same author: autosaving updates the post and does not create a revision.
	// Convert the post object to an array and add slashes, wp_update_post expects escaped array.
	$autosave_id = wp_update_post( wp_slash( (array) $prepared_post ), true );
} else {
	// Non-draft posts: create or update the post autosave.
	$autosave_id = $this->create_post_autosave( (array) $prepared_post );
}

Ideally, we'd prefer to use $this->create_post_autosave() for both instances. Barring that, we'll need to replicate the behavior of _wp_post_revision_data() prior to calling wp_update_post().

@danielbachhuber danielbachhuber added this to the 4.4 milestone Nov 14, 2018

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Nov 14, 2018

@aduth What do you think about:

  1. Backing out of the PHP changes on this pull request.
  2. Keeping this pull request exclusively JavaScript changes.
  3. Solving the PHP issues with https://core.trac.wordpress.org/ticket/43316.

I'd be fine to only fix the PHP issues in trunk, and have that conversation on the Trac ticket.

@aduth

This comment has been minimized.

Member

aduth commented Nov 14, 2018

@danielbachhuber Works for me. I'll push up a revised branch shortly.

@aduth

This comment has been minimized.

Member

aduth commented Nov 14, 2018

With foresight of this moment, it was just a revert of one of two commits, done in e7a2360 . I'll keep it in the branch's history for posterity's sake. It'll be squashed into master as a single commit.

@danielbachhuber danielbachhuber self-requested a review Nov 14, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 14, 2018

Removing the "Fixes" signal, since the issue described in #10753 will still persist until the backend stops considering the parent argument from the registered path.

Related: https://core.trac.wordpress.org/ticket/43316#comment:128

@aduth aduth merged commit 9d958e0 into master Nov 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the fix/10753-autodraft-parent branch Nov 14, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

REST API: Avoid resetting parent field in draft update (WordPress#11513)
* REST API: Unset post parent in updating autosave draft

* Saving: Avoid passing unused parent body parameter

* Revert "REST API: Unset post parent in updating autosave draft"

This reverts commit c95deeb.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment