Abilities API: add filters for input and output validation #10557
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
priethor
changed the title
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…r output even if no schema is provided
| if ( empty( $input_schema ) ) { | ||
| if ( null === $input ) { | ||
| return true; | ||
| } | ||
|
|
||
| return new WP_Error( |
There was a problem hiding this comment.
Note that we don't fire the input hook when the $input_schema is empty but we do for the output:
- For inputs, non-empty inputs REQUIRE a schema by design
- For outputs, an empty schema always successfully validates even if the output is not empty
Because of this design decision, it might make sense not to allow hooking into the input in these cases?
| * @param true|WP_Error $is_valid The validation result from default validation. | ||
| * @param mixed $input The input data being validated. | ||
| * @param string $ability_name The name of the ability. |
There was a problem hiding this comment.
Extra space can be removed:
| * @param true|WP_Error $is_valid The validation result from default validation. | |
| * @param mixed $input The input data being validated. | |
| * @param string $ability_name The name of the ability. | |
| * @param true|WP_Error $is_valid The validation result from default validation. | |
| * @param mixed $input The input data being validated. | |
| * @param string $ability_name The name of the ability. |
| * @param mixed $input The input data being validated. | ||
| * @param string $ability_name The name of the ability. | ||
| */ | ||
| return apply_filters( 'wp_ability_validate_input', $is_valid, $input, $this->name ); |
There was a problem hiding this comment.
I'm wary of plugins doing the right thing here. This would be safer:
Consider the similar logic in the Customizer:
wordpress-develop/src/wp-includes/class-wp-customize-setting.php
Lines 602 to 622 in d676a07
So think this could rather be, with even more hardening to handle the case where a plugin erroneously returns false:
| return apply_filters( 'wp_ability_validate_input', $is_valid, $input, $this->name ); | |
| $validity = apply_filters( 'wp_ability_validate_input', $is_valid, $input, $this->name ); | |
| if ( ! is_wp_error( $validity ) || ! $validity->has_errors() ) { | |
| $validity = true; | |
| } | |
| return $validity; |
Or maybe returning false should result in a generic invalidity WP_Error being created.
There was a problem hiding this comment.
Good catch.
Or maybe returning false should result in a generic invalidity WP_Error being created.
That seems more straightforward to me; do you see any downsides to that?
There was a problem hiding this comment.
I don't see any downsides. Ideally a plugin would provide an error message, but better to err on the side of marking a non-true response as being an error than to accidentally let something which a plugin intends to be an error to be considered valid.
| sprintf( | ||
| /* translators: %1$s ability name, %2$s error message. */ | ||
| __( 'Ability "%1$s" has invalid output. Reason: %2$s' ), | ||
| esc_html( $this->name ), |
There was a problem hiding this comment.
Is HTML escaping appropriate here? It could be rendered into JSON in which case the HTML entities would be wrong.
| esc_html( $this->name ), | |
| $this->name, |
| * @param mixed $output The output data being validated. | ||
| * @param string $ability_name The name of the ability. | ||
| */ | ||
| return apply_filters( 'wp_ability_validate_output', $is_valid, $output, $this->name ); |
There was a problem hiding this comment.
Ditto above:
| return apply_filters( 'wp_ability_validate_output', $is_valid, $output, $this->name ); | |
| $validity = apply_filters( 'wp_ability_validate_output', $is_valid, $output, $this->name ); | |
| if ( ! is_wp_error( $validity ) || ! $validity->has_errors() ) { | |
| $validity = true; | |
| } | |
| return $validity; |
2 participants
Trac ticket: https://core.trac.wordpress.org/ticket/64311
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.