Invalid meta causes warning in WP_Test_REST_Post_Meta_Fields

[timmydcrawford]

While testing the terms endpoints I was trying to figure out how to properly submit meta values to the API through trial and error, and noticed that if the meta input is not an array, a warning is thrown in the logs:

[29-Nov-2016 23:48:58 UTC] PHP Warning:  array_key_exists() expects parameter 2 to be array, string given in /srv/www/wordpress-develop/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php on line 128

We fixed a similar issue in #38553 but I don't believe we can utilize a similar fix here, so in the attached diff I opted to just check that the value is an array. Additionally, the doc block for update_value currently says the first attribute is a WP_REST_Request object, when it is actually the meta argument from the request.

I couldn't find much documentation on meta in the terms endpoints, so I also opted to add some test coverage on the subject as well.

[jnylen0]

Thanks for the fix, nice catch on $request vs $meta as well!

• I think passing a non-array for meta should return an error.
• Can you add a test for passing a non-array also?

[rachelbaker]

A few comments on the proposed solution in fix-meta-warnings.diff​:

1. Checking the meta value type so far down in the request processing means that if I am updating a full object (as in the title and content for a post IN ADDITION to the meta values) the post's title and content would be updated on the post object, but I would receive a WP_Error for my request. We should do this check near the beginning of the create_item() and update_item() methods in the object controllers.

2. We cannot create a new error message because we are in string freeze for 4.7. Instead, the WP_Error message should match the strings we use with the rest_invalid_param errors.

3. The unit tests included by @timmydcrawford do not appear to be testing anything in the scope of this ticket. I agree with @jnylen0 that we should unit tests for sending meta values that are "invalid" in a request, but we should also move the unrelated tests to a new ticket. @timmydcrawford would you be able to create a new ticket with your tests?

[rachelbaker]

Moves the meta array check into WP_REST_Request->has_valid_params() and includes unit tests

[rachelbaker]

@joehoyle What do you think about my approach in 38989.2.diff​? There is a limitation in that any request with a meta property now has to be an array. The benefit is that we don't have to add is_array checks in 8 different methods and remember to include the check in any other endpoints that would use meta.

[joehoyle]

@rachelbaker hmm I'm not a big fan of this approach. Special casing meta is not nice, and I think this is just a bit too special / one off. I think we should be able to add an arg_options here ​https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L393 to specify a custom validate callback for this schema item.

[rachelbaker]

Our schema ignores the sanitization or validation callbacks in top-level parameters (like the meta object parameter). After discussing with @joehoyle in person, we agree that special-casing meta as proposed in 38989.2.diff​ is a hacky workaround the real problem.

Instead, we decided for 4.7 we would ignore the meta property in a request if it is not an array, which is better than returning a PHP Warning which results in unparsable JSON data. Updated patch incoming...

For 4.8 @joehoyle wants to fix this in register_rest_field. @joehoyle would you mind creating a ticket for the underlying problem for solving in 4.8?

[rachelbaker]

Ignore the meta property in a request if it is not an array before trying to update meta

[rachelbaker]

In 38989.3.diff​ ignore the meta values of a request if attempting to update meta when meta is not an array. This fixes the bug where currently a PHP warning is returned if in a create/update request `'meta' is not an array.

I would like a 2nd opinion on the possible perception of "breakage" if we implement the longer term fix @joehoyle has in mind.

Explanation of what is going on in this ticket

Send a request to update a post with request data like so:

{ 
'content': "Look at me changing the post_content.", 
'meta': "this is not an array or object of meta keys and values!" 
}

Current behavior:

• your post would update, so the content would now be "Look at me changing the post_content.".
• the invalid string in the meta property would trigger a PHP Warning which would result in invalid JSON data for your response (if a site is set to display PHP warnings)
• the client would have an indication that something went wrong...

Behavior with 38989.3.diff​) applied:

• your post would update, so the content would now be "Look at me changing the post_content.".
• the invalid string in the meta property would be ignored, and not trigger an error
• the client would have no indication that something went wrong. :(

Proposed behavior with fixing the underlying problem in 4.8:

• the client would receive an error noting that meta was an invalid param in the request
• the post would not update

[jnylen0]

Use sanitize_callback to verify that meta is an array

[jnylen0]

Replying to rachelbaker:

Our schema ignores the sanitization or validation callbacks in top-level parameters

I think the real issue is that meta is declared with type object, which we don't validate at all yet, no? The full fix will be to support object in schemas in a future version.

Replying to joehoyle:

I think we should be able to add an arg_options here ​https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L393 to specify a custom validate callback for this schema item.

Good suggestion, this seems to be working quite well in 38989.4.diff​.

Replying to rachelbaker:

I would like a 2nd opinion on the possible perception of "breakage" if we implement the longer term fix @joehoyle has in mind.

I think we should avoid this future behavior change if possible.

[rachelbaker]

@jnylen0 Your 38989.4.diff​ looked good, I only had minor feedback:

• let's use the validate_callback instead because we aren't really sanitizing the array
• rename the callback check_ to closely match the naming convention of our custom callbacks

In the interest of time I did both of these in 38989.5.diff​ - is this acceptable? If you agree, I will go ahead and apply the commit keyword so we can move this forward.

[rachelbaker]

Use the validate_callback and renamed the callback method check_meta_is_array()

[pento]

38989.5.diff​ Looks good, let roll with it.

[rachelbaker]

Sitting next to @jnylen0 and got his verbal +1 with the changes in 38989.5.diff​ so I am considering this dev-reviewed.

[rachelbaker]

In 39436:

REST API: Return a WP_Error if meta property is not an array.

Fixes bug where a PHP Warning is currently thrown if a client sends a request where meta is not an array value.

Props timmydcrawford, jnylen0, rachelbaker, pento.
Fixes #38989.

[rachelbaker]

In 39437:

REST API: Return a WP_Error if meta property is not an array.

Fixes bug where a PHP Warning is currently thrown if a client sends a request where meta is not an array value.

Merges [39436] onto 4.7 branch.

Props timmydcrawford, jnylen0, rachelbaker, pento.
Fixes #38989 for 4.7.