[Menu items REST API] Perform validation and sanitization separately from saving #23474
Conversation
|
Size Change: +1.68 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
This is not ready for a review yet and needs to be rebased on top of WordPress/wordpress-develop#311 first - validating without using a controller-level |
adamziel
added
[Feature] Navigation Screen
Core REST API Task
adamziel
changed the title
|
This is now ready for discussion. I moved any field-specific validation and sanitization to the schema. This left me with some code that took both the user input and the database data and transformed it into a saveable Code structure wise, I don't love how it creates a synthetic sanitized request property |
|
I also think menu position needs to be handled differently to work with batch processing. It would neatly fit a follow-up PR with some additional test cases once the batch processing support is merged into the core. |
|
This is looking good overall, I'm going to hold off on detailed feedback for a bit while focusing on Beta 1. |
There was a problem hiding this comment.
Thanks again for starting work on this! I've left some feedback.
Now that we have #25096, we could refactor this to be based off that branch for the moment. We will need to do a version check to make sure the user is running at least r48945 which adds support for a route-level validate_callback and if the user isn't, manually calling our validate_callback in the route callback.
What would this look like if we kept prepare_item_for_database and made the validate_callback only do validation? I understand there will be some duplicate work, but how much of an impact would that actually have?
Another thing we need to keep in mind, is that the REST API applies validation before doing permission checks. So we're currently leaking some data at the moment. For the moment, I think we can manually call our permission callbacks before doing the more sensitive validation. But I'm thinking thru what a Core solution might look like.
| @@ -368,64 +478,51 @@ protected function prepare_item_for_database( $request ) { | |||
| $check = rest_validate_value_from_schema( $request[ $api_request ], $schema['properties'][ $api_request ] ); | |||
There was a problem hiding this comment.
Let's remove this double schema validation while we're here. The request data has already been validated and sanitized according to the schema.
| $title = $value['raw']; | ||
| } | ||
|
|
||
| $title = wp_unslash( apply_filters( 'title_save_pre', wp_slash( $title ) ) ); |
There was a problem hiding this comment.
Fairly certain this should be removed: #25095.
| 'sanitize_callback' => static function ( $value ) { | ||
| $sanitized = sanitize_text_field( $value ); | ||
| /** This filter is documented in wp-includes/post.php */ | ||
| $sanitized = wp_unslash( |
| 'sanitize_callback' => static function ( $value, $request ) { | ||
| $sanitized = sanitize_text_field( $value ); | ||
| /** This filter is documented in wp-includes/post.php */ | ||
| $sanitized = wp_unslash( |
| @@ -813,7 +891,18 @@ public function get_item_schema() { | |||
| 'type' => 'string', | |||
| 'context' => array( 'view', 'edit', 'embed' ), | |||
| 'arg_options' => array( | |||
| 'sanitize_callback' => 'sanitize_text_field', | |||
| 'sanitize_callback' => static function ( $value ) { | |||
| $sanitized = sanitize_text_field( $value ); | |||
There was a problem hiding this comment.
I'm not sure this function should be called, it was the previous sanitize_callback, but AFAICT the title attr is not sanitized with this in Core.
| if ( '' === $validated ) { | ||
| // Fail sanitization if URL is invalid. | ||
| return new WP_Error( 'invalid_url', __( 'Invalid URL.', 'gutenberg' ), array( 'status' => 400 ) ); | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| return true; |
| * @param WP_REST_Request $request Request object. | ||
| */ | ||
| return apply_filters( "rest_pre_insert_{$this->post_type}", $prepared_nav_item, $request ); | ||
| $request['prepared_nav_item'] = $prepared_nav_item; |
There was a problem hiding this comment.
I'm really not a fan of us passing data thru the request object like this. What does it look like to stick with a prepare_item_for_database method that does this with already validated data.
There was a problem hiding this comment.
Could not agree more just put the return and filter back. This change is completely unnessary
There was a problem hiding this comment.
This line seems to completely overwrite the existing request. Please remove as it breaks things.
| $sanitized = $value; | ||
| $sanitized = array_map( 'sanitize_html_class', wp_parse_list( $sanitized ) ); | ||
|
|
||
| return array_map( 'sanitize_html_class', $sanitized ); |
There was a problem hiding this comment.
Why does this need change?
| @@ -745,13 +789,33 @@ public function get_item_schema() { | |||
|
|
|||
| $schema['properties']['title'] = array( | |||
| 'description' => __( 'The title for the object.', 'gutenberg' ), | |||
| 'type' => 'object', | |||
| 'type' => array( 'string', 'object' ), | |||
There was a problem hiding this comment.
Can you explain this change?
| 'sanitize_callback' => static function ( $value, $request ) { | ||
| $sanitized = sanitize_text_field( $value ); | ||
| /** This filter is documented in wp-includes/post.php */ | ||
| $sanitized = wp_unslash( | ||
| apply_filters( | ||
| 'content_save_pre', | ||
| wp_slash( $sanitized ) | ||
| ) | ||
| ); | ||
|
|
||
| return $sanitized; | ||
| }, |
There was a problem hiding this comment.
This code has been repeated. Let's just make it a method so we can we use it.
| 'sanitize_callback' => function ( $value ) { | ||
| return array_map( 'sanitize_html_class', wp_parse_list( $value ) ); | ||
| $sanitized = $value; | ||
| $sanitized = array_map( 'sanitize_html_class', wp_parse_list( $sanitized ) ); | ||
|
|
||
| return array_map( 'sanitize_html_class', $sanitized ); | ||
| }, |
| 'sanitize_callback' => static function ( $value ) { | ||
| return intval( $value ); | ||
| }, |
| 'sanitize_callback' => static function ( $value ) { | ||
| return esc_url_raw( $value ); | ||
| }, |
There was a problem hiding this comment.
Why is this required, shouldn't format: uri cover this?
|
|
||
| return true; | ||
| }, | ||
| 'sanitize_callback' => static function ( $value ) { |
| @@ -186,15 +281,25 @@ public function update_item( $request ) { | |||
| return $valid_check; | |||
| } | |||
|
|
|||
| $prepared_nav_item = $this->prepare_item_for_database( $request ); | |||
| $prepared_nav_item = $request['prepared_nav_item']; | |||
There was a problem hiding this comment.
| $prepared_nav_item = $request['prepared_nav_item']; | |
| $prepared_nav_item = $this->sanitize( $request ); |
| @@ -97,7 +181,18 @@ public function create_item( $request ) { | |||
| return new WP_Error( 'rest_post_exists', __( 'Cannot create existing post.', 'gutenberg' ), array( 'status' => 400 ) ); | |||
| } | |||
|
|
|||
| $prepared_nav_item = $this->prepare_item_for_database( $request ); | |||
| $prepared_nav_item = $request['prepared_nav_item']; | |||
There was a problem hiding this comment.
| $prepared_nav_item = $request['prepared_nav_item']; | |
| $prepared_nav_item = $this->sanitize( $request ); |
This is a bad pattern to put an object into state like this. Just run the method again.
| $prepared_nav_item['menu-id'] = absint( $request[ $base ] ); | ||
| } | ||
|
|
||
| // Nav menu title. |
There was a problem hiding this comment.
Title no longer works without this code. Why was this removed?
|
Closing this stale PR, we may need to find another way forward. |
This PR removes the
prepare_item_for_databasemethod in favor of more specific validation and sanitization callbacks, with the ultimate goal being supporting batch requests. Let's discuss in comments!Test plan:
sanitize_callback(see comments on the PR above).