-
Notifications
You must be signed in to change notification settings - Fork 50
Add filters for input and output validation #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #150 +/- ##
============================================
+ Coverage 80.46% 80.48% +0.02%
Complexity 178 178
============================================
Files 20 20
Lines 1474 1476 +2
Branches 120 120
============================================
+ Hits 1186 1188 +2
Misses 288 288
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:
|
536188f to
59215aa
Compare
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks so much for this PR @priethor , 🙇♂️ I love the use case!
Only reviewed from mobile (I'm sick ATM) and had
a quick question:
Both filters run AFTER default WordPress validation, allowing overrides
Can you share your reasoning here?
If the goal is to BYO validation, I would assume we'd want the filter to be a precheck and return early instead of running validation you know you're throwing out in favor of another parser/validation. I personally don't feel strongly either way, but since it was intentional enough for you to call out, I am curious.
(FWIW since it's a public method, a bypass hook could still be used to override/recover from a WP_Error, just with admittedly not too great ergonomics. Ironically my unrelated comment below recommends making those ergonomics slightly worse 😅)
|
Thanks, @justlevine !
I'm glad you asked, since I gave this a few thoughts. I considered adding a pre-validation filter to short-circuit it entirely, similar to how the REST works, but I think there are cases where extenders might want to extend the validation output rather than completely replace it. For example, I could see a case where extenders want to apply the core validation to verify the input structure and, when it succeeds, apply an extra dynamic filter (e.g., checking an API) to detect forbidden words. I don't feel very strongly either way, but it seemed to me that the potential computational cost of applying the core validation first is worth the tradeoff for the flexibility it provides. Another option that could make sense is to provide both pre- and post-filters. |
|
Trac ticket, since core is the source of truth: core.trac.wordpress.org/ticket/64311 |
What
Closes #149 by adding validation filters to the Abilities API to allow custom validation strategies.
Why
Current validation uses only WordPress REST's JSON Schema subset (Draft 4), whereas extenders might need support for modern JSON Schema features.
How
wp_ability_validate_inputfilter inWP_Ability::validate_input()wp_ability_validate_outputfilter inWP_Ability::validate_output()Testing Instructions
input_schemaandoutput_schemawp_ability_validate_inputto use custom validationAlternatively, run the new seven tests against
trunkand verify they fail.