#38553 closed defect (bug) (fixed)
Posts Controller handle_terms throws warning
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | has-patch has-unit-tests commit |
Focuses: | Cc: |
Description
If a malformatted value is passed as a term field ( categories, tags ) to the WP_REST_Posts_Controller, the handle_terms method is currently throwing a warning:
PHP Warning: array_map(): Argument #2 should be an array in /srv/www/wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php on line 1053
To reproduce this warning, you can issue a curl request like:
curl -X POST -H "Authorization: Basic redacted" -H "Content-Type: multipart/form-data;" -F "title=rad post3" -F "content=blah blah blah" -F "categories=2,1" "http://src.wordpress-develop.dev/wp-json/wp/v2/posts"
I have attached a simple fix, but should we possibly consider either parsing comma-separated strings into arrays, or returning an error on malformed inputs like this? In the wpcom API, it is possible to send in a comma-separated string - so it seems like some sort of standard needs to emerge here.
I haven't tested other array inputs to see if similar warnings might be thrown elsewhere.
Attachments (4)
Change History (19)
#2
follow-up:
↓ 3
@
8 years ago
Thanks @ChopinBach - here is an updated diff with a test case. Your suggested solution does work well with a comma-separated string, though it doesn't handle something like
categories=[1,2,3]
Either way, I think this second diff is better than the first since it does work with comma-sep values.
#3
in reply to:
↑ 2
@
8 years ago
Replying to timmydcrawford:
it doesn't handle something like
categories=[1,2,3]
I think that is expected, because that is equivalent to a string not an array literal in x-www-form-urlencoded
, so it is like saying 'left bracket 1,2,3 right bracket'
. If you want to pass an array via query params you have to do ?categories[0]=1&categories[1]=2
at least that is how PHP can handle array input for x-www-form-urlencoded
requests; not sure if other languages do things differently. We could do something to make it work for [1,2,3]
, but it would probably over complicate things, even though that looks better to me too.
#4
follow-up:
↓ 5
@
8 years ago
I'm generally in favor of comma-separated strings and array syntax over [1,2,3]
, I haven't ever seen that in the wild and the others feel comparatively common. Supporting array syntax and comma separated equally seems reasonable though.
#5
in reply to:
↑ 4
@
8 years ago
Replying to kadamwhite:
I'm generally in favor of comma-separated strings and array syntax over
[1,2,3]
, I haven't ever seen that in the wild and the others feel comparatively common. Supporting array syntax and comma separated equally seems reasonable though.
+1 - I think supporting csv like in the last patch is ideal. The bracketed syntax was tried by someone else at Automattic who found the error, so that is why I brought it up.
#6
follow-up:
↓ 8
@
8 years ago
- Keywords has-patch has-unit-tests added
[1,2,3]
would work if sending a raw JSON body. That's more an issue with our developer tools (hint: you can test our integration of the content endpoints at https://developer.wordpress.com/docs/api/console/ right now!)
Definitely +1 on this patch, this is a nice simple fix.
Other things we should think about (in separate tickets):
- How can we expose the schema information about what type(s) each argument accepts? Currently it's not visible in the discovery response, the documentation, or the code (at least, not immediately obvious to me).
- Are there other array inputs that should get similar treatment? Let's be consistent about the way we handle these if possible.
#8
in reply to:
↑ 6
;
follow-up:
↓ 9
@
8 years ago
Replying to jnylen0:
Other things we should think about (in separate tickets):
- How can we expose the schema information about what type(s) each argument accepts? Currently it's not visible in the discovery response, the documentation, or the code (at least, not immediately obvious to me).
I agree that each argument's type should be exposed in the discovery process, so that you can do validation client side. For taxonomy term settings for a post they are exposed in the discovery under the schema
portion. Every non-readonly element in the schema is translated to an argument for create/update
requests via WP_REST_Controller::get_endpoint_args_for_item_schema()
.
- Are there other array inputs that should get similar treatment? Let's be consistent about the way we handle these if possible.
The main array parameters we have are the various include
, exclude
params and the roles
param. The include
/exclude
params are supposed to be an array of ids and currently use the wp_parse_id_list
as their sanitize_callback
, without a validate_callback
, to support arrays and csv lists.
After reviewing our code some more, I believe I came across the actual root cause of the problem. Around line 1776 in the post controller, we need to add arg_options => array( 'sanitize_callback' => 'wp_parse_id_list' )
to the schema, making it consistent with how other id lists are handled. This way on create and update requests we will auto sanitize our request arguments. Then the terms line can be changed to $terms = $request[ $base ]
, or just replace the reference to $terms
entirely. I think that would work, but haven't tested.
#9
in reply to:
↑ 8
@
8 years ago
Replying to ChopinBach:
After reviewing our code some more, I believe I came across the actual root cause of the problem. Around line 1776 in the post controller, we need to add
arg_options => array( 'sanitize_callback' => 'wp_parse_id_list' )
to the schema, making it consistent with how other id lists are handled.
Great solution, and confirmed it works in the next diff.
#10
@
8 years ago
For taxonomy term settings for a post they are exposed in the discovery under the
schema
portion.
Thanks for pointing this out. I think there is not much to do regarding documentation, then, unless we want to indicate that a value can be either an array or a comma-delimited string. I'm not sure if this is something we would want to do or if it is something that JSON Schema even supports.
I looked through the other array parameters in all endpoints and here are the other ones that do not support comma-separated strings:
- Taxonomy "exclude" parameter immediately below the one already patched in
fix-terms-3.diff
(should be addressed in this ticket) - User
roles
parameter (#38577)
Finally I think we should support filtering post slugs this way as well (#38579).
#12
@
8 years ago
- Keywords commit added
In 38553.diff:
- Update the patch for compatibility with [39046]
- Add support for
_exclude
- Fix and add more unit tests
Patch is ready to go, pending review of #38586 by @rmccue.
#14
@
8 years ago
@pento I don't think this should have be required now I committed https://core.trac.wordpress.org/changeset/39061 (which I think should probably have been in https://core.trac.wordpress.org/ticket/38586 anyway). Does that sound right to you?
@timmydcrawford Great catch on this! To solve two problems at once, instead of changing the conditional to check if it is an array, only change the line where
$terms
is assigned to read$terms = wp_parse_id_list( $request[ $base ] );
That should enable the csv list support for updating/creating posts with multiple categories. I haven't tested it, but it should work. Adding in a test case would be nice too. I will test it myself tomorrow and see if I can help out.