From 4757546045466c7bc656f9998137a4f9b5138a53 Mon Sep 17 00:00:00 2001 From: Rachel Baker Date: Thu, 3 Nov 2016 20:04:59 +0000 Subject: [PATCH] REST API: Modify the structure of our DELETE responses to be more explicit. Add the `deleted` property to the root of the Response object to communicate if the delete action was successful. Move the state of the resource prior to the delete request under a new `previous` property. As a result DELETE responses are now structured like so: `{ deleted: true, previous: { ... } }` Also includes helpful information to DELETE requests for resources that are not trashable. Props timmydcrawford, rmccue, jnylen0. Fixes #38494. git-svn-id: https://develop.svn.wordpress.org/trunk@39126 602fd350-edb4-49c9-b593-d223f7449a82 --- .../class-wp-rest-comments-controller.php | 10 ++-- .../class-wp-rest-posts-controller.php | 9 +++- .../class-wp-rest-revisions-controller.php | 25 ++++++++-- .../class-wp-rest-terms-controller.php | 8 ++- .../class-wp-rest-users-controller.php | 9 +++- .../rest-api/rest-attachments-controller.php | 4 ++ .../rest-api/rest-categories-controller.php | 13 +++-- .../rest-api/rest-comments-controller.php | 9 ++-- .../tests/rest-api/rest-posts-controller.php | 5 +- .../rest-api/rest-revisions-controller.php | 16 ++++++ .../tests/rest-api/rest-tags-controller.php | 16 ++++-- .../tests/rest-api/rest-users-controller.php | 50 +++++++++++-------- 12 files changed, 129 insertions(+), 45 deletions(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php index 52b91fcf30..e6f79203fb 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php @@ -83,6 +83,7 @@ class WP_REST_Comments_Controller extends WP_REST_Controller { 'permission_callback' => array( $this, 'delete_item_permissions_check' ), 'args' => array( 'force' => array( + 'type' => 'boolean', 'default' => false, 'description' => __( 'Whether to bypass trash and force deletion.' ), ), @@ -738,14 +739,15 @@ class WP_REST_Comments_Controller extends WP_REST_Controller { $request->set_param( 'context', 'edit' ); - $response = $this->prepare_item_for_response( $comment, $request ); - if ( $force ) { + $previous = $this->prepare_item_for_response( $comment, $request ); $result = wp_delete_comment( $comment->comment_ID, true ); + $response = new WP_REST_Response(); + $response->set_data( array( 'deleted' => true, 'previous' => $previous->get_data() ) ); } else { // If this type doesn't support trashing, error out. if ( ! $supports_trash ) { - return new WP_Error( 'rest_trash_not_supported', __( 'The comment does not support trashing.' ), array( 'status' => 501 ) ); + return new WP_Error( 'rest_trash_not_supported', __( 'The comment does not support trashing. Set force=true to delete.' ), array( 'status' => 501 ) ); } if ( 'trash' === $comment->comment_approved ) { @@ -753,6 +755,8 @@ class WP_REST_Comments_Controller extends WP_REST_Controller { } $result = wp_trash_comment( $comment->comment_ID ); + $comment = get_comment( $comment->comment_ID ); + $response = $this->prepare_item_for_response( $comment, $request ); } if ( ! $result ) { diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php index ce5d3d91b8..c313784504 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php @@ -101,6 +101,7 @@ class WP_REST_Posts_Controller extends WP_REST_Controller { 'permission_callback' => array( $this, 'delete_item_permissions_check' ), 'args' => array( 'force' => array( + 'type' => 'boolean', 'default' => false, 'description' => __( 'Whether to bypass trash and force deletion.' ), ), @@ -757,15 +758,17 @@ class WP_REST_Posts_Controller extends WP_REST_Controller { $request->set_param( 'context', 'edit' ); - $response = $this->prepare_item_for_response( $post, $request ); // If we're forcing, then delete permanently. if ( $force ) { + $previous = $this->prepare_item_for_response( $post, $request ); $result = wp_delete_post( $id, true ); + $response = new WP_REST_Response(); + $response->set_data( array( 'deleted' => true, 'previous' => $previous->get_data() ) ); } else { // If we don't support trashing for this type, error out. if ( ! $supports_trash ) { - return new WP_Error( 'rest_trash_not_supported', __( 'The post does not support trashing.' ), array( 'status' => 501 ) ); + return new WP_Error( 'rest_trash_not_supported', __( 'The post does not support trashing. Set force=true to delete.' ), array( 'status' => 501 ) ); } // Otherwise, only trash if we haven't already. @@ -776,6 +779,8 @@ class WP_REST_Posts_Controller extends WP_REST_Controller { // (Note that internally this falls through to `wp_delete_post` if // the trash is disabled.) $result = wp_trash_post( $id ); + $post = $this->get_post( $id ); + $response = $this->prepare_item_for_response( $post, $request ); } if ( ! $result ) { diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php index 4c21d0195c..ed1ec2dc08 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php @@ -93,6 +93,13 @@ class WP_REST_Revisions_Controller extends WP_REST_Controller { 'methods' => WP_REST_Server::DELETABLE, 'callback' => array( $this, 'delete_item' ), 'permission_callback' => array( $this, 'delete_item_permissions_check' ), + 'args' => array( + 'force' => array( + 'type' => 'boolean', + 'default' => false, + 'description' => __( 'Required to be true, as resource does not support trashing.' ), + ), + ), ), 'schema' => array( $this, 'get_public_item_schema' ), )); @@ -220,6 +227,16 @@ class WP_REST_Revisions_Controller extends WP_REST_Controller { * @return true|WP_Error True on success, or WP_Error object on failure. */ public function delete_item( $request ) { + $force = isset( $request['force'] ) ? (bool) $request['force'] : false; + + // We don't support trashing for this resource type. + if ( ! $force ) { + return new WP_Error( 'rest_trash_not_supported', __( 'Revisions do not support trashing. Set force=true to delete.' ), array( 'status' => 501 ) ); + } + + $revision = $this->get_post( $request['id'] ); + $previous = $this->prepare_item_for_response( $revision, $request ); + $result = wp_delete_post( $request['id'], true ); /** @@ -234,11 +251,13 @@ class WP_REST_Revisions_Controller extends WP_REST_Controller { */ do_action( 'rest_delete_revision', $result, $request ); - if ( $result ) { - return true; - } else { + if ( ! $result ) { return new WP_Error( 'rest_cannot_delete', __( 'The post cannot be deleted.' ), array( 'status' => 500 ) ); } + + $response = new WP_REST_Response(); + $response->set_data( array( 'deleted' => true, 'previous' => $previous->get_data() ) ); + return $response; } /** diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-terms-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-terms-controller.php index 4e39c8fa56..cfe3f6dbc8 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-terms-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-terms-controller.php @@ -116,6 +116,7 @@ class WP_REST_Terms_Controller extends WP_REST_Controller { 'permission_callback' => array( $this, 'delete_item_permissions_check' ), 'args' => array( 'force' => array( + 'type' => 'boolean', 'default' => false, 'description' => __( 'Required to be true, as resource does not support trashing.' ), ), @@ -566,14 +567,14 @@ class WP_REST_Terms_Controller extends WP_REST_Controller { // We don't support trashing for this resource type. if ( ! $force ) { - return new WP_Error( 'rest_trash_not_supported', __( 'Resource does not support trashing.' ), array( 'status' => 501 ) ); + return new WP_Error( 'rest_trash_not_supported', __( 'Terms do not support trashing. Set force=true to delete.' ), array( 'status' => 501 ) ); } $term = get_term( (int) $request['id'], $this->taxonomy ); $request->set_param( 'context', 'view' ); - $response = $this->prepare_item_for_response( $term, $request ); + $previous = $this->prepare_item_for_response( $term, $request ); $retval = wp_delete_term( $term->term_id, $term->taxonomy ); @@ -581,6 +582,9 @@ class WP_REST_Terms_Controller extends WP_REST_Controller { return new WP_Error( 'rest_cannot_delete', __( 'The resource cannot be deleted.' ), array( 'status' => 500 ) ); } + $response = new WP_REST_Response(); + $response->set_data( array( 'deleted' => true, 'previous' => $previous->get_data() ) ); + /** * Fires after a single term is deleted via the REST API. * diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php index e62b6481a3..9fece46997 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php @@ -85,6 +85,7 @@ class WP_REST_Users_Controller extends WP_REST_Controller { 'permission_callback' => array( $this, 'delete_item_permissions_check' ), 'args' => array( 'force' => array( + 'type' => 'boolean', 'default' => false, 'description' => __( 'Required to be true, as resource does not support trashing.' ), ), @@ -114,6 +115,7 @@ class WP_REST_Users_Controller extends WP_REST_Controller { 'permission_callback' => array( $this, 'delete_current_item_permissions_check' ), 'args' => array( 'force' => array( + 'type' => 'boolean', 'default' => false, 'description' => __( 'Required to be true, as resource does not support trashing.' ), ), @@ -653,7 +655,7 @@ class WP_REST_Users_Controller extends WP_REST_Controller { // We don't support trashing for this type, error out. if ( ! $force ) { - return new WP_Error( 'rest_trash_not_supported', __( 'Users do not support trashing.' ), array( 'status' => 501 ) ); + return new WP_Error( 'rest_trash_not_supported', __( 'Users do not support trashing. Set force=true to delete.' ), array( 'status' => 501 ) ); } $user = get_userdata( $id ); @@ -670,7 +672,7 @@ class WP_REST_Users_Controller extends WP_REST_Controller { $request->set_param( 'context', 'edit' ); - $response = $this->prepare_item_for_response( $user, $request ); + $previous = $this->prepare_item_for_response( $user, $request ); /** Include admin user functions to get access to wp_delete_user() */ require_once ABSPATH . 'wp-admin/includes/user.php'; @@ -681,6 +683,9 @@ class WP_REST_Users_Controller extends WP_REST_Controller { return new WP_Error( 'rest_cannot_delete', __( 'The resource cannot be deleted.' ), array( 'status' => 500 ) ); } + $response = new WP_REST_Response(); + $response->set_data( array( 'deleted' => true, 'previous' => $previous->get_data() ) ); + /** * Fires immediately after a user is deleted via the REST API. * diff --git a/tests/phpunit/tests/rest-api/rest-attachments-controller.php b/tests/phpunit/tests/rest-api/rest-attachments-controller.php index 40148a07ba..3fceb44e4e 100644 --- a/tests/phpunit/tests/rest-api/rest-attachments-controller.php +++ b/tests/phpunit/tests/rest-api/rest-attachments-controller.php @@ -730,6 +730,10 @@ class WP_Test_REST_Attachments_Controller extends WP_Test_REST_Post_Type_Control $response = $this->server->dispatch( $request ); $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + $request->set_param( 'force', 'false' ); + $response = $this->server->dispatch( $request ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + // Ensure the post still exists $post = get_post( $attachment_id ); $this->assertNotEmpty( $post ); diff --git a/tests/phpunit/tests/rest-api/rest-categories-controller.php b/tests/phpunit/tests/rest-api/rest-categories-controller.php index 4212f5f708..ff24dbb960 100644 --- a/tests/phpunit/tests/rest-api/rest-categories-controller.php +++ b/tests/phpunit/tests/rest-api/rest-categories-controller.php @@ -725,16 +725,21 @@ class WP_Test_REST_Categories_Controller extends WP_Test_REST_Controller_Testcas $response = $this->server->dispatch( $request ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'Deleted Category', $data['name'] ); + $this->assertTrue( $data['deleted'] ); + $this->assertEquals( 'Deleted Category', $data['previous']['name'] ); } - public function test_delete_item_force_false() { + public function test_delete_item_no_trash() { wp_set_current_user( self::$administrator ); $term = get_term_by( 'id', $this->factory->category->create( array( 'name' => 'Deleted Category' ) ), 'category' ); + $request = new WP_REST_Request( 'DELETE', '/wp/v2/categories/' . $term->term_id ); - // force defaults to false $response = $this->server->dispatch( $request ); - $this->assertEquals( 501, $response->get_status() ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + + $request->set_param( 'force', 'false' ); + $response = $this->server->dispatch( $request ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); } public function test_delete_item_invalid_taxonomy() { diff --git a/tests/phpunit/tests/rest-api/rest-comments-controller.php b/tests/phpunit/tests/rest-api/rest-comments-controller.php index 0775073fb7..a8526c893a 100644 --- a/tests/phpunit/tests/rest-api/rest-comments-controller.php +++ b/tests/phpunit/tests/rest-api/rest-comments-controller.php @@ -1888,12 +1888,14 @@ class WP_Test_REST_Comments_Controller extends WP_Test_REST_Controller_Testcase 'comment_post_ID' => self::$post_id, 'user_id' => self::$subscriber_id, )); - $request = new WP_REST_Request( 'DELETE', sprintf( '/wp/v2/comments/%d', $comment_id ) ); + $request = new WP_REST_Request( 'DELETE', sprintf( '/wp/v2/comments/%d', $comment_id ) ); + $request->set_param( 'force', 'false' ); $response = $this->server->dispatch( $request ); $this->assertEquals( 200, $response->get_status() ); + $data = $response->get_data(); - $this->assertEquals( self::$post_id, $data['post'] ); + $this->assertEquals( 'trash', $data['status'] ); } public function test_delete_item_skip_trash() { @@ -1910,7 +1912,8 @@ class WP_Test_REST_Comments_Controller extends WP_Test_REST_Controller_Testcase $response = $this->server->dispatch( $request ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( self::$post_id, $data['post'] ); + $this->assertTrue( $data['deleted'] ); + $this->assertNotEmpty( $data['previous']['post'] ); } public function test_delete_item_already_trashed() { diff --git a/tests/phpunit/tests/rest-api/rest-posts-controller.php b/tests/phpunit/tests/rest-api/rest-posts-controller.php index eff261c2f7..cdd5ec0abe 100644 --- a/tests/phpunit/tests/rest-api/rest-posts-controller.php +++ b/tests/phpunit/tests/rest-api/rest-posts-controller.php @@ -2008,12 +2008,14 @@ class WP_Test_REST_Posts_Controller extends WP_Test_REST_Post_Type_Controller_Te wp_set_current_user( self::$editor_id ); $request = new WP_REST_Request( 'DELETE', sprintf( '/wp/v2/posts/%d', $post_id ) ); + $request->set_param( 'force', 'false' ); $response = $this->server->dispatch( $request ); $this->assertNotInstanceOf( 'WP_Error', $response ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); $this->assertEquals( 'Deleted post', $data['title']['raw'] ); + $this->assertEquals( 'trash', $data['status'] ); } public function test_delete_item_skip_trash() { @@ -2027,7 +2029,8 @@ class WP_Test_REST_Posts_Controller extends WP_Test_REST_Post_Type_Controller_Te $this->assertNotInstanceOf( 'WP_Error', $response ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'Deleted post', $data['title']['raw'] ); + $this->assertTrue( $data['deleted'] ); + $this->assertNotEmpty( $data['previous'] ); } public function test_delete_item_already_trashed() { diff --git a/tests/phpunit/tests/rest-api/rest-revisions-controller.php b/tests/phpunit/tests/rest-api/rest-revisions-controller.php index 5d0836660c..3c88c6fa0e 100644 --- a/tests/phpunit/tests/rest-api/rest-revisions-controller.php +++ b/tests/phpunit/tests/rest-api/rest-revisions-controller.php @@ -184,11 +184,27 @@ class WP_Test_REST_Revisions_Controller extends WP_Test_REST_Controller_Testcase public function test_delete_item() { wp_set_current_user( self::$editor_id ); $request = new WP_REST_Request( 'DELETE', '/wp/v2/posts/' . self::$post_id . '/revisions/' . $this->revision_id1 ); + $request->set_param( 'force', true ); $response = $this->server->dispatch( $request ); $this->assertEquals( 200, $response->get_status() ); $this->assertNull( get_post( $this->revision_id1 ) ); } + public function test_delete_item_no_trash() { + wp_set_current_user( self::$editor_id ); + + $request = new WP_REST_Request( 'DELETE', '/wp/v2/posts/' . self::$post_id . '/revisions/' . $this->revision_id1 ); + $response = $this->server->dispatch( $request ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + + $request->set_param( 'force', 'false' ); + $response = $this->server->dispatch( $request ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + + // Ensure the revision still exists + $this->assertNotNull( get_post( $this->revision_id1 ) ); + } + public function test_delete_item_no_permission() { wp_set_current_user( self::$contributor_id ); $request = new WP_REST_Request( 'DELETE', '/wp/v2/posts/' . self::$post_id . '/revisions/' . $this->revision_id1 ); diff --git a/tests/phpunit/tests/rest-api/rest-tags-controller.php b/tests/phpunit/tests/rest-api/rest-tags-controller.php index 568b15addf..df2c030d49 100644 --- a/tests/phpunit/tests/rest-api/rest-tags-controller.php +++ b/tests/phpunit/tests/rest-api/rest-tags-controller.php @@ -625,16 +625,21 @@ class WP_Test_REST_Tags_Controller extends WP_Test_REST_Controller_Testcase { $response = $this->server->dispatch( $request ); $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'Deleted Tag', $data['name'] ); + $this->assertTrue( $data['deleted'] ); + $this->assertEquals( 'Deleted Tag', $data['previous']['name'] ); } - public function test_delete_item_force_false() { + public function test_delete_item_no_trash() { wp_set_current_user( self::$administrator ); $term = get_term_by( 'id', $this->factory->tag->create( array( 'name' => 'Deleted Tag' ) ), 'post_tag' ); + $request = new WP_REST_Request( 'DELETE', '/wp/v2/tags/' . $term->term_id ); - // force defaults to false $response = $this->server->dispatch( $request ); - $this->assertEquals( 501, $response->get_status() ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + + $request->set_param( 'force', 'false' ); + $response = $this->server->dispatch( $request ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); } public function test_delete_item_invalid_term() { @@ -667,7 +672,8 @@ class WP_Test_REST_Tags_Controller extends WP_Test_REST_Controller_Testcase { $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'Deleted Tag', $data['name'] ); + $this->assertTrue( $data['deleted'] ); + $this->assertEquals( 'Deleted Tag', $data['previous']['name'] ); } public function grant_delete_term( $caps, $cap ) { diff --git a/tests/phpunit/tests/rest-api/rest-users-controller.php b/tests/phpunit/tests/rest-api/rest-users-controller.php index 7dd254052e..09e11a6693 100644 --- a/tests/phpunit/tests/rest-api/rest-users-controller.php +++ b/tests/phpunit/tests/rest-api/rest-users-controller.php @@ -1154,7 +1154,29 @@ class WP_Test_REST_Users_Controller extends WP_Test_REST_Controller_Testcase { $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'Deleted User', $data['name'] ); + $this->assertTrue( $data['deleted'] ); + $this->assertEquals( 'Deleted User', $data['previous']['name'] ); + } + + public function test_delete_item_no_trash() { + $user_id = $this->factory->user->create( array( 'display_name' => 'Deleted User' ) ); + + $this->allow_user_to_manage_multisite(); + wp_set_current_user( self::$user ); + + $userdata = get_userdata( $user_id ); // cache for later + + $request = new WP_REST_Request( 'DELETE', sprintf( '/wp/v2/users/%d', $user_id ) ); + $response = $this->server->dispatch( $request ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + + $request->set_param( 'force', 'false' ); + $response = $this->server->dispatch( $request ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + + // Ensure the user still exists + $user = get_user_by( 'id', $user_id ); + $this->assertNotEmpty( $user ); } public function test_delete_current_item() { @@ -1170,37 +1192,25 @@ class WP_Test_REST_Users_Controller extends WP_Test_REST_Controller_Testcase { $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'Deleted User', $data['name'] ); - } - - public function test_delete_item_no_trash() { - $user_id = $this->factory->user->create( array( 'display_name' => 'Deleted User' ) ); - - $this->allow_user_to_manage_multisite(); - wp_set_current_user( self::$user ); - - $userdata = get_userdata( $user_id ); // cache for later - $request = new WP_REST_Request( 'DELETE', sprintf( '/wp/v2/users/%d', $user_id ) ); - $response = $this->server->dispatch( $request ); - $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); - - // Ensure the user still exists - $user = get_user_by( 'id', $user_id ); - $this->assertNotEmpty( $user ); + $this->assertTrue( $data['deleted'] ); + $this->assertEquals( 'Deleted User', $data['previous']['name'] ); } public function test_delete_current_item_no_trash() { - $user_id = $this->factory->user->create( array( 'role' => 'administrator' ) ); + $user_id = $this->factory->user->create( array( 'role' => 'administrator', 'display_name' => 'Deleted User' ) ); wp_set_current_user( $user_id ); $user = wp_get_current_user(); update_site_option( 'site_admins', array( $user->user_login ) ); - $userdata = get_userdata( $user_id ); // cache for later $request = new WP_REST_Request( 'DELETE', '/wp/v2/users/me' ); $response = $this->server->dispatch( $request ); $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + $request->set_param( 'force', 'false' ); + $response = $this->server->dispatch( $request ); + $this->assertErrorResponse( 'rest_trash_not_supported', $response, 501 ); + // Ensure the user still exists $user = get_user_by( 'id', $user_id ); $this->assertNotEmpty( $user );