Add DELETE endpoint #5

dlh01 · 2017-05-25T04:43:16Z

This adds delete_item() and delete_item_permissions_check() methods. There are a few (at least) pieces that will probably need to be refactored. They're marked with TODOs, but the highlights:

The custom logic for trashing or deleting a changeset is currently copied from _wp_customize_publish_changeset(). There might be a need for a function or WP_Customize_Manager method to house that logic.
On that topic, there are points within delete_item() at which the post data stored in the $wp_customize instance becomes outdated. A way to trash or delete posts within WP_Customize_Manager so that the properties can be updated (similar to what happens now at the end of save_changeset_post()) might help address that, if it needs to be.
current_user_can() checks should eventually use the delete_post meta cap.

westonruter · 2017-05-25T04:48:21Z

wp-includes/rest-api/endpoints/class-wp-rest-customize-changesets-controller.php

+		if ( $request['force'] ) {
+			$previous = $this->prepare_item_for_response( $post, $request );
+
+			// TODO: At this point $wp_customize will no longer have up-to-date post data.


Maybe then after this point it should do $GLOBALS['wp_customize'] = null;?

I suppose WP_Customize_Manager should be listening for post deletion action to then clear the cached post ID.

We may want to consider updating \WP_Customize_Manager::changeset_data() to eliminate the if ( isset( $this->_changeset_data ) ) { check entirely so that \WP_Customize_Manager::get_changeset_post_data() will always be called. Either that, or we should add delete_post and save_post_customize_changeset (or better yet, clean_post_cache) actions in the constructor to unset \WP_Customize_Manager::$_changeset_data.

That being said, all of the changeset data should actually be abstracted into a separate class to make it easier to work with changesets irrespective of a manager. See https://core.trac.wordpress.org/ticket/40527

I'd recommend work to be done as part of that ticket.

OK, I'll look into submitting a patch on that ticket.

westonruter · 2017-05-25T04:51:12Z

wp-includes/rest-api/endpoints/class-wp-rest-customize-changesets-controller.php

+		/** This action is documented in wp-includes/post.php */
+		do_action( 'trashed_post', $manager->changeset_post_id() );
+
+		// TODO: At this point $wp_customize will no longer have up-to-date post data.


What won't be up to date here? If the ID is the only thing that the Customizer is holding on to.

You're right, I was thinking at the time that the Customizer stored the post object. It does have the $_changeset_data, though, which would be at least potentially outdated if someone modified the post during one of the actions above.

You're absolutely right. See #5 (comment)

westonruter · 2017-05-25T04:53:05Z

wp-includes/rest-api/endpoints/class-wp-rest-customize-changesets-controller.php

+		// TODO: At this point $wp_customize will no longer have up-to-date post data.
+		$result = get_post( $manager->changeset_post_id() );
+
+		if ( ! $result ) {


Is this right? If the status was set to trash then the $result should always be a WP_Post.

As with above, this is mostly to defend against unexpected behavior during the actions, but, yes, under most circumstances it should still be a post.

I was thinking it might not hurt to also check ( 'trash' === get_post_status() ), too?

OK, good points. The 'trash' === get_post_status() check would need to be done right after the clean_post_cache call, right?

It could be. I was thinking it would occur right here, as in:

if ( ! $result || ( 'trash' !== get_post_status( $result ) ) )

which could even be shortened to if ( 'trash' !== get_post_status( $result ) )

since it seemed to me that if the status is not trash at this point for whatever reason, then we know we would need to return the rest_cannot_delete error.

Yes, you're right. Though I don't think the shortened version would work since if $result is false then get_post_status() would try to use $GLOBALS['post'].

Also, I think this conditional should happen right after the calls to $wpdb->update() and clean_post_cache() right? Is the only reason that the failure could occur is for there to be a DB failure?

I don't think the shortened version would work

Oops, yes, you're correct.

Is the only reason that the failure could occur is for there to be a DB failure?

I'm not sure. I'm thinking of a hypothetical case where someone is listening for trashed_post or an action in wp_transition_post_status() and changing the status to something other than trash in certain circumstances based on client requirements.

It also occurs to me that all of the trash-post logic is still copied from _wp_customize_publish_changeset(). Will this logic be abstracted into a new function or method? If so, the location of the conditional in this method might become moot. For example:

_wp_customize_trash_changeset( $manager->changeset_post_id() ); $result = get_post( $manager->changeset_post_id() ); if ( ! $result || 'trash' !== get_post_status( $result ) ) { ... }

@dlh01 good point. So essentially there could be some plugin logic that prevents changesets from being trashed? And yes, the logic for trashing should absolutely be factored out into another function like you have for _wp_customize_trash_changeset here. Really this should be put into a WP_Customize_Changeset class, perhaps, as proposed in https://core.trac.wordpress.org/ticket/40527

So essentially there could be some plugin logic that prevents changesets from being trashed?

Yeah, as part of preserving an audit trail, perhaps.

westonruter · 2017-05-25T04:53:48Z

wp-includes/rest-api/endpoints/class-wp-rest-customize-changesets-controller.php

+			);
+		}
+
+		// TODO: Use 'delete_post'.


Yes, the post type caps need to be fleshed out in core.

Where would be the best place to begin that effort?

Probably a new Trac ticket.

Some prior discussions on that:

https://wordpress.slack.com/archives/C0381N237/p1481851138000475

https://wordpress.slack.com/archives/D0G14JH09/p1489768662369477 (our DM convo)

Separate "Save" & "Publish" xwp/wp-customize-snapshots#4

Only user with customize_publish cap can publish snapshot xwp/wp-customize-snapshots#74

https://core.trac.wordpress.org/ticket/38900#comment:7

OK, I'll create a ticket.

dlh01 · 2017-06-07T03:09:33Z

I added a temporary _wp_customize_trash_changeset() function and opened #40922 to begin investigating the post type capabilities.

With work towards resolving the other TODOs potentially occurring in https://core.trac.wordpress.org/ticket/40527, is there anything else you'd like to see happen in this PR?

westonruter · 2017-06-09T15:47:35Z

It looks good other than the PHPCS issues in the Travis build. I'd say good to merge with a 👍 from @miina.

dlh01 · 2017-06-10T23:43:52Z

@miina, could you advise about what to do about the PHP 5.2 error?

miina · 2017-06-11T18:24:58Z

wp-includes/rest-api/endpoints/class-wp-rest-customize-changesets-controller.php

+			);
+		}
+
+		if ( 'trash' === get_post_status( $manager->changeset_post_id() ) ) {


@westonruter Do I understand correctly that if revisions are enabled, the published changeset status could also be publish, not just trash? @dlh01 Thinking that if that's the case probably it shouldn't be possible to delete a changeset that's already with status publish either?

That's true, if revisions are enabled then a changeset keeps its publish status and doesn't get set to trash. I guess it remains to be seen if we should allow published changesets to be trashed in a UI. I suppose post revisions aren't able to be deleted in the UI? That being the case, perhaps there should be no API for trashing published changesets which are essentially revisions of an entire site's settings?

That being the case, perhaps there should be no API for trashing published changesets which are essentially revisions of an entire site's settings?

While I'm not sure I have an opinion yet about this specific behavior, it sounds to me like it would need to be enforced in non-REST contexts, too. Would it then go in the general WP_Customize_Changeset or whatever comes out of https://core.trac.wordpress.org/ticket/40527?

@dlh01 yes, that makes sense.

miina · 2017-06-11T18:26:09Z

@dlh01 Added workarounds for PHP 5.2 issues for the date and for tests.

dlh01 · 2017-06-15T03:28:21Z

Thanks @miina!

miina · 2017-06-27T08:28:08Z

@dlh01 Is this ready for merging or are there still things you're working on?

dlh01 · 2017-06-27T13:21:45Z

I think the remaining issues that were raised in comments are due to be handled in other core tickets, in which case it's ready for merging if you're OK with it.

miina · 2017-06-27T13:24:08Z

Looks like the other issues are not requirements for this to work and the changes from other issues could be implemented later, seems ready for merging.

dlh01 added 14 commits May 1, 2017 01:11

Simplify delete_item_without_permission() setup

8c9e40a

Update expected error when attempting to delete invalid UUID

bff7528

Start delete_item_permissions_check()

061aabe

Merge branch 'master' into delete-endpoint

4436be5

Use more-specific instanceof check for $wp_customize

288d756

Remove merge conflict cruft

b41db1f

Ensure prepare_item_for_response() returns WP_REST_Response

cfd5dc3

Fix "andpoints"

7b8beaf

Use 'customize' for now

24e3eb5

Use ensure_customize_manager()

608a337

First pass at delete_item()

0f0dfdf

Clean up delete tests

51b991e

Note another point at which $wp_customize becomes outdated

a973890

Move delete methods down a bit

4395ccf

westonruter reviewed May 25, 2017

View reviewed changes

dlh01 added 4 commits June 5, 2017 01:01

Use delete_post

3a526e0

Temporarily abstract changeset-trashing logic

36a1c2a

Merge branch 'master' into delete-endpoint

c1f03e4

Revert submodule update

e2a8dbc

dlh01 added 4 commits June 9, 2017 12:34

Remove namespace

2e0f009

Address PHPCS errors

a7f1a19

Add period

93905b5

Update custom callback for WP_REST_Response parameter

c559f1a

miina added 4 commits June 11, 2017 19:28

Add support for PHP 5.2

20491f3

Fix phpcs.

dbb3491

Another fix for 5.2

28f859b

Fix assertJsonStringEqualsJsonString for PHP 5.2

3c3bf4c

miina reviewed Jun 11, 2017

View reviewed changes

miina merged commit ca40b50 into master Jun 29, 2017

miina deleted the delete-endpoint branch June 29, 2017 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DELETE endpoint #5

Add DELETE endpoint #5

dlh01 commented May 25, 2017

westonruter May 25, 2017

westonruter May 25, 2017

westonruter May 26, 2017

dlh01 Jun 3, 2017

westonruter May 25, 2017

dlh01 May 25, 2017

westonruter May 26, 2017

westonruter May 25, 2017

dlh01 May 25, 2017

westonruter May 26, 2017

dlh01 Jun 3, 2017

westonruter Jun 3, 2017

dlh01 Jun 4, 2017

westonruter Jun 4, 2017

dlh01 Jun 7, 2017

westonruter May 25, 2017

dlh01 May 25, 2017

westonruter May 26, 2017

dlh01 Jun 3, 2017

dlh01 commented Jun 7, 2017

westonruter commented Jun 9, 2017

dlh01 commented Jun 10, 2017

miina Jun 11, 2017

westonruter Jun 11, 2017

dlh01 Jun 15, 2017

westonruter Jun 20, 2017

miina commented Jun 11, 2017

dlh01 commented Jun 15, 2017

miina commented Jun 27, 2017

dlh01 commented Jun 27, 2017

miina commented Jun 27, 2017

Add DELETE endpoint #5

Add DELETE endpoint #5

Conversation

dlh01 commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlh01 commented Jun 7, 2017

westonruter commented Jun 9, 2017

dlh01 commented Jun 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina commented Jun 11, 2017

dlh01 commented Jun 15, 2017

miina commented Jun 27, 2017

dlh01 commented Jun 27, 2017

miina commented Jun 27, 2017