From 7f049d3ac168dc0461f2322aa2af1a10fd1e734e Mon Sep 17 00:00:00 2001 From: Timothy Jacobs Date: Tue, 2 Feb 2021 17:26:06 +0000 Subject: [PATCH] REST API: Return detailed error information from request validation. Previously, only the first error message for each parameter was made available. Now, all error messages for a parameter are concatenated. Additionally, the detailed error for each parameter is made available in a new `details` section of the validation error. Each error is formatted following the standard REST API error formatting. The `WP_REST_Server::error_to_response` method has been abstracted out into a standalone function `rest_convert_error_to_response` to allow for reuse by `WP_REST_Request`. The formatted errors now also contain an `additional_data` property which contains the additional error data provided by `WP_Error::get_all_error_data`. Props dlh, xkon, TimothyBlynJacobs. Fixes #46191. git-svn-id: https://develop.svn.wordpress.org/trunk@50150 602fd350-edb4-49c9-b593-d223f7449a82 --- src/wp-includes/rest-api.php | 54 +++++++ .../rest-api/class-wp-rest-request.php | 24 ++- .../rest-api/class-wp-rest-server.php | 32 +--- .../rest-api/rest-attachments-controller.php | 4 +- tests/phpunit/tests/rest-api/rest-request.php | 145 ++++++++++++++++++ tests/phpunit/tests/rest-api/rest-server.php | 19 ++- 6 files changed, 234 insertions(+), 44 deletions(-) diff --git a/src/wp-includes/rest-api.php b/src/wp-includes/rest-api.php index 536efea696..6eed4e2b08 100644 --- a/src/wp-includes/rest-api.php +++ b/src/wp-includes/rest-api.php @@ -3182,3 +3182,57 @@ function rest_get_endpoint_args_for_schema( $schema, $method = WP_REST_Server::C return $endpoint_args; } + + +/** + * Converts an error to a response object. + * + * This iterates over all error codes and messages to change it into a flat + * array. This enables simpler client behaviour, as it is represented as a + * list in JSON rather than an object/map. + * + * @since 5.7.0 + * + * @param WP_Error $error WP_Error instance. + * + * @return WP_REST_Response List of associative arrays with code and message keys. + */ +function rest_convert_error_to_response( $error ) { + $status = array_reduce( + $error->get_all_error_data(), + function ( $status, $error_data ) { + return is_array( $error_data ) && isset( $error_data['status'] ) ? $error_data['status'] : $status; + }, + 500 + ); + + $errors = array(); + + foreach ( (array) $error->errors as $code => $messages ) { + $all_data = $error->get_all_error_data( $code ); + $last_data = array_pop( $all_data ); + + foreach ( (array) $messages as $message ) { + $formatted = array( + 'code' => $code, + 'message' => $message, + 'data' => $last_data, + ); + + if ( $all_data ) { + $formatted['additional_data'] = $all_data; + } + + $errors[] = $formatted; + } + } + + $data = $errors[0]; + if ( count( $errors ) > 1 ) { + // Remove the primary error. + array_shift( $errors ); + $data['additional_errors'] = $errors; + } + + return new WP_REST_Response( $data, $status ); +} diff --git a/src/wp-includes/rest-api/class-wp-rest-request.php b/src/wp-includes/rest-api/class-wp-rest-request.php index e972c5fa25..267ce4e01d 100644 --- a/src/wp-includes/rest-api/class-wp-rest-request.php +++ b/src/wp-includes/rest-api/class-wp-rest-request.php @@ -802,7 +802,8 @@ class WP_REST_Request implements ArrayAccess { $order = $this->get_parameter_order(); - $invalid_params = array(); + $invalid_params = array(); + $invalid_details = array(); foreach ( $order as $type ) { if ( empty( $this->params[ $type ] ) ) { @@ -825,10 +826,12 @@ class WP_REST_Request implements ArrayAccess { continue; } + /** @var mixed|WP_Error $sanitized_value */ $sanitized_value = call_user_func( $param_args['sanitize_callback'], $value, $this, $key ); if ( is_wp_error( $sanitized_value ) ) { - $invalid_params[ $key ] = $sanitized_value->get_error_message(); + $invalid_params[ $key ] = implode( ' ', $sanitized_value->get_error_messages() ); + $invalid_details[ $key ] = rest_convert_error_to_response( $sanitized_value )->get_data(); } else { $this->params[ $type ][ $key ] = $sanitized_value; } @@ -841,8 +844,9 @@ class WP_REST_Request implements ArrayAccess { /* translators: %s: List of invalid parameters. */ sprintf( __( 'Invalid parameter(s): %s' ), implode( ', ', array_keys( $invalid_params ) ) ), array( - 'status' => 400, - 'params' => $invalid_params, + 'status' => 400, + 'params' => $invalid_params, + 'details' => $invalid_details, ) ); } @@ -894,13 +898,15 @@ class WP_REST_Request implements ArrayAccess { * * This is done after required checking as required checking is cheaper. */ - $invalid_params = array(); + $invalid_params = array(); + $invalid_details = array(); foreach ( $args as $key => $arg ) { $param = $this->get_param( $key ); if ( null !== $param && ! empty( $arg['validate_callback'] ) ) { + /** @var bool|\WP_Error $valid_check */ $valid_check = call_user_func( $arg['validate_callback'], $param, $this, $key ); if ( false === $valid_check ) { @@ -908,7 +914,8 @@ class WP_REST_Request implements ArrayAccess { } if ( is_wp_error( $valid_check ) ) { - $invalid_params[ $key ] = $valid_check->get_error_message(); + $invalid_params[ $key ] = implode( ' ', $valid_check->get_error_messages() ); + $invalid_details[ $key ] = rest_convert_error_to_response( $valid_check )->get_data(); } } } @@ -919,8 +926,9 @@ class WP_REST_Request implements ArrayAccess { /* translators: %s: List of invalid parameters. */ sprintf( __( 'Invalid parameter(s): %s' ), implode( ', ', array_keys( $invalid_params ) ) ), array( - 'status' => 400, - 'params' => $invalid_params, + 'status' => 400, + 'params' => $invalid_params, + 'details' => $invalid_details, ) ); } diff --git a/src/wp-includes/rest-api/class-wp-rest-server.php b/src/wp-includes/rest-api/class-wp-rest-server.php index d7f8ae9a41..ed94151228 100644 --- a/src/wp-includes/rest-api/class-wp-rest-server.php +++ b/src/wp-includes/rest-api/class-wp-rest-server.php @@ -196,41 +196,13 @@ class WP_REST_Server { * list in JSON rather than an object/map. * * @since 4.4.0 + * @since 5.7.0 Converted to a wrapper of {@see rest_convert_error_to_response()}. * * @param WP_Error $error WP_Error instance. * @return WP_REST_Response List of associative arrays with code and message keys. */ protected function error_to_response( $error ) { - $error_data = $error->get_error_data(); - - if ( is_array( $error_data ) && isset( $error_data['status'] ) ) { - $status = $error_data['status']; - } else { - $status = 500; - } - - $errors = array(); - - foreach ( (array) $error->errors as $code => $messages ) { - foreach ( (array) $messages as $message ) { - $errors[] = array( - 'code' => $code, - 'message' => $message, - 'data' => $error->get_error_data( $code ), - ); - } - } - - $data = $errors[0]; - if ( count( $errors ) > 1 ) { - // Remove the primary error. - array_shift( $errors ); - $data['additional_errors'] = $errors; - } - - $response = new WP_REST_Response( $data, $status ); - - return $response; + return rest_convert_error_to_response( $error ); } /** diff --git a/tests/phpunit/tests/rest-api/rest-attachments-controller.php b/tests/phpunit/tests/rest-api/rest-attachments-controller.php index 33dd61b674..c023033369 100644 --- a/tests/phpunit/tests/rest-api/rest-attachments-controller.php +++ b/tests/phpunit/tests/rest-api/rest-attachments-controller.php @@ -455,9 +455,7 @@ class WP_Test_REST_Attachments_Controller extends WP_Test_REST_Post_Type_Control $request->set_param( 'status', 'publish' ); $request->set_param( 'context', 'edit' ); $response = rest_get_server()->dispatch( $request ); - $data = $response->get_data(); - $this->assertCount( 3, $data ); - $this->assertSame( 'rest_invalid_param', $data['code'] ); + $this->assertErrorResponse( 'rest_invalid_param', $response ); } public function test_get_items_private_status() { diff --git a/tests/phpunit/tests/rest-api/rest-request.php b/tests/phpunit/tests/rest-api/rest-request.php index 6952ee5c53..54afd39c7c 100644 --- a/tests/phpunit/tests/rest-api/rest-request.php +++ b/tests/phpunit/tests/rest-api/rest-request.php @@ -462,6 +462,78 @@ class Tests_REST_Request extends WP_UnitTestCase { $this->assertSame( 'rest_invalid_param', $valid->get_error_code() ); } + /** + * @ticket 46191 + */ + public function test_sanitize_params_error_multiple_messages() { + $this->request->set_url_params( + array( + 'failparam' => '123', + ) + ); + $this->request->set_attributes( + array( + 'args' => array( + 'failparam' => array( + 'sanitize_callback' => function () { + $error = new WP_Error( 'invalid', 'Invalid.' ); + $error->add( 'invalid', 'Super Invalid.' ); + $error->add( 'broken', 'Broken.' ); + + return $error; + }, + ), + ), + ) + ); + + $valid = $this->request->sanitize_params(); + $this->assertWPError( $valid ); + $data = $valid->get_error_data(); + + $this->assertInternalType( 'array', $data ); + $this->assertArrayHasKey( 'params', $data ); + $this->assertArrayHasKey( 'failparam', $data['params'] ); + $this->assertEquals( 'Invalid. Super Invalid. Broken.', $data['params']['failparam'] ); + } + + /** + * @ticket 46191 + */ + public function test_sanitize_params_provides_detailed_errors() { + $this->request->set_url_params( + array( + 'failparam' => '123', + ) + ); + $this->request->set_attributes( + array( + 'args' => array( + 'failparam' => array( + 'sanitize_callback' => function () { + return new WP_Error( 'invalid', 'Invalid.', 'mydata' ); + }, + ), + ), + ) + ); + + $valid = $this->request->sanitize_params(); + $this->assertWPError( $valid ); + + $data = $valid->get_error_data(); + $this->assertArrayHasKey( 'details', $data ); + $this->assertArrayHasKey( 'failparam', $data['details'] ); + $this->assertEquals( + array( + 'code' => 'invalid', + 'message' => 'Invalid.', + 'data' => 'mydata', + ), + $data['details']['failparam'] + ); + } + public function test_sanitize_params_with_null_callback() { $this->request->set_url_params( array( @@ -652,6 +724,79 @@ class Tests_REST_Request extends WP_UnitTestCase { $this->assertSame( 'This is not valid!', $error_data['params']['someotherparams'] ); } + + /** + * @ticket 46191 + */ + public function test_invalid_params_error_multiple_messages() { + $this->request->set_url_params( + array( + 'failparam' => '123', + ) + ); + $this->request->set_attributes( + array( + 'args' => array( + 'failparam' => array( + 'validate_callback' => function () { + $error = new WP_Error( 'invalid', 'Invalid.' ); + $error->add( 'invalid', 'Super Invalid.' ); + $error->add( 'broken', 'Broken.' ); + + return $error; + }, + ), + ), + ) + ); + + $valid = $this->request->has_valid_params(); + $this->assertWPError( $valid ); + $data = $valid->get_error_data(); + + $this->assertInternalType( 'array', $data ); + $this->assertArrayHasKey( 'params', $data ); + $this->assertArrayHasKey( 'failparam', $data['params'] ); + $this->assertEquals( 'Invalid. Super Invalid. Broken.', $data['params']['failparam'] ); + } + + /** + * @ticket 46191 + */ + public function test_invalid_params_provides_detailed_errors() { + $this->request->set_url_params( + array( + 'failparam' => '123', + ) + ); + $this->request->set_attributes( + array( + 'args' => array( + 'failparam' => array( + 'validate_callback' => function () { + return new WP_Error( 'invalid', 'Invalid.', 'mydata' ); + }, + ), + ), + ) + ); + + $valid = $this->request->has_valid_params(); + $this->assertWPError( $valid ); + + $data = $valid->get_error_data(); + $this->assertArrayHasKey( 'details', $data ); + $this->assertArrayHasKey( 'failparam', $data['details'] ); + $this->assertEquals( + array( + 'code' => 'invalid', + 'message' => 'Invalid.', + 'data' => 'mydata', + ), + $data['details']['failparam'] + ); + } + public function _return_wp_error_on_validate_callback() { return new WP_Error( 'some-error', 'This is not valid!' ); } diff --git a/tests/phpunit/tests/rest-api/rest-server.php b/tests/phpunit/tests/rest-api/rest-server.php index f27811819a..b47cacd245 100644 --- a/tests/phpunit/tests/rest-api/rest-server.php +++ b/tests/phpunit/tests/rest-api/rest-server.php @@ -407,7 +407,7 @@ class Tests_REST_Server extends WP_Test_REST_TestCase { $message = 'Test error message for the API'; $error = new WP_Error( $code, $message ); - $response = rest_get_server()->error_to_response( $error ); + $response = rest_convert_error_to_response( $error ); $this->assertInstanceOf( 'WP_REST_Response', $response ); // Make sure we default to a 500 error. @@ -424,7 +424,7 @@ class Tests_REST_Server extends WP_Test_REST_TestCase { $message = 'Test error message for the API'; $error = new WP_Error( $code, $message, array( 'status' => 400 ) ); - $response = rest_get_server()->error_to_response( $error ); + $response = rest_convert_error_to_response( $error ); $this->assertInstanceOf( 'WP_REST_Response', $response ); $this->assertSame( 400, $response->get_status() ); @@ -443,7 +443,7 @@ class Tests_REST_Server extends WP_Test_REST_TestCase { $error = new WP_Error( $code, $message, array( 'status' => 400 ) ); $error->add( $code2, $message2, array( 'status' => 403 ) ); - $response = rest_get_server()->error_to_response( $error ); + $response = rest_convert_error_to_response( $error ); $this->assertInstanceOf( 'WP_REST_Response', $response ); $this->assertSame( 400, $response->get_status() ); @@ -456,6 +456,19 @@ class Tests_REST_Server extends WP_Test_REST_TestCase { $this->assertSame( array( 'status' => 403 ), $error->error_data[ $code2 ] ); } + /** + * @ticket 46191 + */ + public function test_error_to_response_with_additional_data() { + $error = new WP_Error( 'test', 'test', array( 'status' => 400 ) ); + $error->add_data( 'more_data' ); + + $response = rest_convert_error_to_response( $error ); + $this->assertSame( 400, $response->get_status() ); + $this->assertEquals( 'more_data', $response->get_data()['data'] ); + $this->assertEquals( array( array( 'status' => 400 ) ), $response->get_data()['additional_data'] ); + } + public function test_rest_error() { $data = array( 'code' => 'wp-api-test-error',