dev!: return WP_Error from WP_Ability methods#23
Conversation
|
Looks like my IDE |
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
I left some initial feedback. Thank you for taking the time to refactor the code.
| return true; | ||
| } | ||
|
|
||
| $valid_input = rest_validate_value_from_schema( $input, $input_schema ); |
There was a problem hiding this comment.
It looks like it could return the value here because rest_validate_value_from_schema returns true or WP_Error.
There was a problem hiding this comment.
Thinking a bit more about it after seeing the updated unit tests, we should remap these WP_Errors so the names are more tied to the abilty, for example:
rest_invalid_type -> ability_input_invalid_type
Could we replace the prefix?
|
@emdashcodes, this could use some sanity check from you. |
| return $result; | ||
| } | ||
|
|
||
| $output_validation = $this->validate_output( $ability, $result ); |
There was a problem hiding this comment.
Both $this->validate_input() and $this->validate_output() calls should no longer be needed as they are handled internally inside $ability->execute() with similar WP_Errors. We would need to differentiate mostly the error names to the proper status code in the response: 400 vs 500.
| } | ||
|
|
||
| return true; | ||
| return is_wp_error( $valid_output ) ? $valid_output : true; |
There was a problem hiding this comment.
Similar feedback as for input handling since we use rest_validate_value_from_schema which happens to be tied to much to REST API at this level.
I noticed similar issues when running |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #23 +/- ##
========================================
Coverage ? 92.48%
Complexity ? 99
========================================
Files ? 7
Lines ? 519
Branches ? 0
========================================
Hits ? 480
Misses ? 39
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Resolving merge conflicts on mobile is hard but I think I made it 😆 I plan to land changes and look into my feedback separately. This PR largely addresses the issue but we still can add some final touches to reuse the builtin schema validation inside |
2 participants
What
This PR updates various
WP_Abilitymethods to return an explicitWP_Errorobject instead of logging a_doing_it_wrong()and returningnullWhy
Refactor for #14
How
WP_Error,nullis a valid response fromexecute(), and the test has been updated accordingly. If we want, we could mapnullto anability_unknown_execution_erroror something.WP_Ability::get_permissions()error, so as not to leak a potentially sensitive error to a user without the proper permissions.