Posts Controller handle_terms throws warning

[timmydcrawford]

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.

[ChopinBach]

@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.

[timmydcrawford]

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.

[ChopinBach]

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.

[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.

[timmydcrawford]

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.

[jnylen0]

[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.

[ChopinBach]

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.

[timmydcrawford]

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.

[jnylen0]

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).

[pento]

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.

[pento]

In 39055:

REST API: Allow a CSV list of term IDs to be passed to /posts.

[39048] added CSV support to array types, this change explicitly parses term lists as IDs, and adds tests.

Props timmydcrawford, pento.
Fixes #38553.

[joehoyle]

@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?

[pento]

The only difference is that wp_parse_id_list() removes duplicates - it's not wildly necessary, the MySQL query optimiser will remove them when it gets to there, there's certainly no performance difference, it just makes for slightly nicer queries.

I'm not fussed either way.