#34659 closed enhancement (worksforme)
Whitelist for validation functions which only accept one argument
Reported by: | danielbachhuber | Owned by: | rmccue |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | has-patch needs-testing |
Focuses: | Cc: |
Description
WP_REST_Request
supports a validate_callback
for arguments. When it calls the validate_callback
, it passes additional contextual arguments:
$valid_check = call_user_func( $arg['validate_callback'], $param, $this, $key );
However, some validation functions like is_numeric
only support one argument:
1) Tests_REST_Request::test_validate_callback_only_pass_value is_numeric() expects exactly 1 parameter, 3 given
I propose we add a filterable whitelist of validation functions that should only be passed one argument.
We may want to do the same for sanitization callbacks.
Originally:
Attachments (4)
Change History (34)
#2
in reply to:
↑ 1
@
9 years ago
Replying to TimothyBlynJacobs:
The idea of whitelisting functions seems quite a bit kludgy. Most of my sanitization functions, for example, also only accept one parameter. And whitelisting all of those doesn't seem ideal. Plus, what about functions that only need access to the first two?
In each of these cases, do the functions error or exhibit unexpected behavior when additional arguments are passed? If not, and they silently discard the extra arguments, then you wouldn't need to whitelist them.
Why not specify the number of arguments to be passed to the sanitization function the same way we do for hooks?
Could, although that's a different type of complexity. What would the syntax look like?
#3
follow-ups:
↓ 4
↓ 5
@
9 years ago
In most cases it would be fine, but it'd be a problem with functions that use a variable number of arguments.
I'd imagine something like this. Though you're right that'd get clunky as well. Especially for a object function name pair.
$arg['validate_callback'] = array( 'myfunction', 1 );
#4
in reply to:
↑ 3
@
9 years ago
Replying to TimothyBlynJacobs:
I'd imagine something like this. Though you're right that'd get clunky as well. Especially for a object function name pair.
$arg['validate_callback'] = array( 'myfunction', 1 );
Yeah, that's pretty clunky. I can't say that I'm a fan.
@rmccue could I get your thoughts on the patch provided? I'm happy to update it to handle the sanitization callback as well.
#5
in reply to:
↑ 3
@
9 years ago
Replying to TimothyBlynJacobs:
In most cases it would be fine, but it'd be a problem with functions that use a variable number of arguments.
I'd imagine something like this. Though you're right that'd get clunky as well. Especially for a object function name pair.
$arg['validate_callback'] = array( 'myfunction', 1 );
I'm not sure if you were pointing it out here - but this would conflict with the array( 'MyClass', 'myMethod' )
syntax for the callbacks.
I'm not a big fan of a whitelist, I'm +1 for specific rest validate functions for those tricky ones line is_numeric()
This ticket was mentioned in Slack in #core by joehoyle. View the logs.
9 years ago
#7
@
9 years ago
- Keywords has-unit-tests added
34659.2.diff supports whitelisting specific functions to only receive one argument when used as sanitization or validation callbacks
On Slack, we also explored a couple other options:
- Only ever passing one argument to the validation / sanitization callback. But, the extra context can be helpful, as we're using in the plugin itself.
- Introducing an ancillary
validate_callback_single
argument, which would only pass one argument to the callback. However, this doesn't seem very glamorous.
This ticket was mentioned in Slack in #core by danielbachhuber. View the logs.
9 years ago
#9
@
9 years ago
This looks good to me - I think in the case that we decided to change to :
Only ever passing one argument to the validation / sanitization callback. But, the extra context can be helpful, as we're using in the plugin itself.
At some point, the user-land code would still be compatible, so I think the whitelist is the best way to go.
#10
@
9 years ago
- Keywords commit added
- Owner set to johnbillion
- Status changed from new to assigned
This ticket was mentioned in Slack in #core by danielbachhuber. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.
9 years ago
#15
@
9 years ago
- Keywords 4.5-early added; commit removed
- Milestone changed from 4.4 to Future Release
I'm unconvinced on the whitelist approach, and I'd rather not lock us into using it for future versions. As painful as using this stuff in the meantime may be (with wrapper functions)... bumping to next release.
This ticket was mentioned in Slack in #core-restapi by brianhogg. View the logs.
9 years ago
#17
@
9 years ago
Since we can't go back on having validate_callback and sanitize_callback functions being passed 3 parameters without breaking things for those that are using them, whitelisting core WP and PHP functions that either return a different value then when passed just the argument value or throws a warning seems like the only reasonable approach. Functions like strip_tags also return NULL when passed 3 parameters.
The only alternative I see is doing some kind of reflection to only pass in the number of parameters a function accepts, which would likely cause performance issues and be unnecessary (and undesired) for functions that dynamically fetch arguments.
This ticket was mentioned in Slack in #core-restapi by danielbachhuber. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.
8 years ago
#21
@
8 years ago
- Keywords needs-patch added; has-patch has-unit-tests removed
I want to solve this with sanitize_callback_args
and validate_callback_args
, which are used as a number in array_slice
. This matches the $accepted_args
parameter to add_filter
, and is completely backwards compatible, plus doesn't involve some weird special whitelist for specific functions.
This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.
8 years ago
#23
@
8 years ago
After discussing with @rachelbaker added a validate_callback_single_arg
and sanitize_callback_single_arg
. When set to true
will only pass the value and nothing else to the callback function.
@
8 years ago
Adding an alternative way of doing this more akin to the plugins API, via {sanitize/validate}_callback_args
parameter. Allows you to specify the number of args to pass into your callback. Based on the work of @brianhogg and the idea/pattern of @rmccue.
#25
@
8 years ago
@joshlevinson the decision on _single_arg
was that a) people would likely either want just the value or the three params and b) its not really clear what passing "2" would do or passing sanitize/validate_callback_args
with a value greater than 3 (error/warning? no effect?)
If there's a case we missed where one would just want 2 args passed without using an anonymous function for the callback (ie. value and the request object, but without the key) could go with the args count approach?
#26
@
8 years ago
- Keywords needs-testing added
- Version set to 4.7
Patch here looks good to me, but requires some testing. Also need to decide whether this is for 4.7.2 or 4.8.
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
8 years ago
#28
@
4 years ago
- Milestone Future Release deleted
- Resolution set to worksforme
- Status changed from assigned to closed
Now that our minimum PHP version is 5.6, it seems like this is far better served by using an anonymous function. I don't think it's worth introducing the complexity to WP_REST_Request
anymore.
If someone feels strongly about this and wants to own it, feel free to reopen.
#29
@
3 years ago
(just reading from REST API Handbook)
Is it a good idea to use func_get_arg() to get the number of arguments passed?
This functions seems available since PHP 4, so compatibility might not be a problem.
With this, less explicit work to do by specifying the number of arguments. While development teams could dynamically adjust inside the core.
Not sure if there is other concerns that I did not aware of.
The idea of whitelisting functions seems quite a bit kludgy. Most of my sanitization functions, for example, also only accept one parameter. And whitelisting all of those doesn't seem ideal. Plus, what about functions that only need access to the first two?
Why not specify the number of arguments to be passed to the sanitization function the same way we do for hooks?