Skip to content
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

Editor: Move auto-draft status change to the server. #16814

Merged

Conversation

@epiqueras
Copy link
Contributor

commented Jul 30, 2019

Extracted from #16761

Description

This PR moves auto-draft status changing to the server in preparation for refactoring the editor store to rely entirely on core/data entities.

It does this by filtering inserted post data to unset any auto-draft assigned post title. The status of an auto-draft should be read from its post_status and not inferred via its title. A post with an explicit title should be created with draft status, not with auto-draft status. It will also update an existing post's status to draft if currently an auto-draft. This is intended to ensure that a post which is explicitly updated should no longer be subject to auto-draft purge.

How has this been tested?

It was verified that saving still works as expected.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@epiqueras epiqueras added this to the Future milestone Jul 30, 2019

@epiqueras epiqueras self-assigned this Jul 30, 2019

}
return $data;
}
add_filter( 'wp_insert_post_data', 'gutenberg_filter_wp_insert_post_data', 10, 2 );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 30, 2019

Contributor

This seems right for me but I'd like to check with the REST API people if this is legit as if I understand properly. I don't see why people would want to save "auto-drafts" using the REST API (it's an internal status). Any API request saving auto-drafts will create drafts with this change. Also, can we limit this change to the REST API instead of a global wp_insert_post_data filter?

cc @kadamwhite @TimothyBJacobs

This comment has been minimized.

Copy link
@adamsilverstein

adamsilverstein Jul 30, 2019

Contributor

Also, can we limit this change to the REST API instead of a global wp_insert_post_data filter?

The rest_pre_insert_{post_type} filter would likely work for this - it is applied to the prepared post data before calling update_post.

Can we limit the behavior to Gutenberg? or is this the expected behavior for all API updates to auto-draft posts? If so, does this change belong in the controller itself?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 30, 2019

Contributor

My gut feeling tells me that this should be the expected behavior for all API updates to auto-draft posts and that it belongs to the controller. That filter could be a way to test it in Gutenberg early before the Core update.

This comment has been minimized.

Copy link
@aduth

aduth Jul 30, 2019

Member

Also, can we limit this change to the REST API instead of a global wp_insert_post_data filter?

Can we limit the behavior to Gutenberg? or is this the expected behavior for all API updates to auto-draft posts? If so, does this change belong in the controller itself?

The change as I considered it in #16761 is based on the assumption that there should be no circumstance that a post be considered as auto-draft after an explicit save. For this reason, I don't think it's something which should be exclusive to either REST API updates or the Gutenberg plugin, but rather apply to any post update.

This comment has been minimized.

Copy link
@adamsilverstein

adamsilverstein Jul 30, 2019

Contributor

there should be no circumstance that a post be considered as auto-draft after an explicit save

Makes sense 👍

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jul 31, 2019

Author Contributor

Separately, I'm a bit confused as to why a title is being disallowed for an auto-draft. I understand that the code should check against the post's status instead of the title to determine if it is an auto-draft or not, but I don't see why that means unsetting it here.

To avoid saving it, the default "Auto Draft" title, when saving for the first time without having edited the title.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 31, 2019

Contributor

What if the title was changed? We should reset only if it's equal to "Auto Draft" right?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 31, 2019

Contributor

Actually, I think the fix for that one (the title) shouldn't be on the "save" hook but more on the "auto draft" creation. I don't understand why we assign "Auto Draft" as a title in the first place?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jul 31, 2019

Author Contributor

You are right. This should work:

if ( 'auto-draft' === $postarr['post_status'] ) {
	if ( ! empty( $postarr['ID'] ) ) {
		$data['post_status'] = 'draft';
	} else {
		$data['post_title'] = '';
	}
}

Post ID will be empty when creating right?

This comment has been minimized.

Copy link
@aduth

aduth Jul 31, 2019

Member

Aside: A benefit of having these two as separate commits a74c59c f5748c0 in the original #16761 was in optimizing not for concise code as it exists in Gutenberg, but for clarity in an eventual merge into core that each of these changes be considered independently.

Respectively, I expect likely here:

https://github.com/WordPress/wordpress-develop/blob/a8a4c09f33356475f4a3f3a9f5bdacd0c25bcb42/src/wp-includes/post.php#L4077-L4091

...and here:

https://github.com/WordPress/wordpress-develop/blob/a8a4c09f33356475f4a3f3a9f5bdacd0c25bcb42/src/wp-admin/includes/post.php#L669

@epiqueras epiqueras force-pushed the update/move-auto-draft-status-change-to-the-server branch from cec71bf to c1b0fd8 Jul 31, 2019

@epiqueras epiqueras force-pushed the update/move-auto-draft-status-change-to-the-server branch from c1b0fd8 to c46e62e Jul 31, 2019

@youknowriad
Copy link
Contributor

left a comment

I think we should follow-up with Core Tickets about these two changes but it does make sense to at least move forward with this in Gutenberg for now.

@epiqueras epiqueras merged commit fea9b4b into master Aug 1, 2019

1 of 3 checks passed

Filter opened Filter opened
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@epiqueras epiqueras added this to Done in Phase 2 via automation Aug 1, 2019

@epiqueras epiqueras deleted the update/move-auto-draft-status-change-to-the-server branch Aug 1, 2019

@kadamwhite

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Following up on the conversation above:

The change as I considered it in #16761 is based on the assumption that there should be no circumstance that a post be considered as auto-draft after an explicit save. For this reason, I don't think it's something which should be exclusive to either REST API updates or the Gutenberg plugin, but rather apply to any post update.

I understand this reasoning, but introducing this kind of "magic" into such a low level method does concern me. I'd conceptually prefer making this change at the REST API controller level, as that's a fairly consistent place for us to introduce filtering and transformations on the data being saved.

If committers with more familiarity with auto-draft are comfortable with changing things in this location then it's unarguably the most central place to introduce such a change; but I'd want to make sure no plugins or themes are using auto-draft explicitly, and I'd want validation from a larger group of contributors that there's no situation we'd ever want wp_insert_post to result in an auto-draft.

@adamsilverstein

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

I tend to agree with the concern about changing this low level function vs altering the REST API handling. Searching the plugin and theme repos I see a ton of uses of auto-draft, we would have to look more carefully to gauge impact:

https://wpdirectory.net/search/01DHQ4NF8XT1MR966K3D4053TQ
https://wpdirectory.net/search/01DHQ4VCAXGC0HB1NTBN2RTYW2

@mmtr

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

I believe this is breaking the WP_Customize_Nav_Menus::insert_auto_draft_post function in WordPress core, since that function creates a new auto draft post with a title explicitly set that is being removed by the gutenberg_filter_wp_insert_post_data filter.

Any feature or plugin using that function will be likely affected.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

I'm trying to see if we can remove the filter without any e2e test failures, but I can't even build master successfully since #17004 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.