REST API: 'array' type meta not updating properly

[caercam]

#43392 introduced support for array and object type for meta. However, updating meta with array values sometimes fails due to WP_REST_Meta_Fields::update_meta_value() still expecting a string value.

This can be observe when updating multiple meta values with some values not changing, for instance updating post meta

{
    foo : [ 'bar' ],
    new : [ 'value' ],
}

Where foo meta already exists with bar value.

Following happen:

1. WP_REST_Meta_Fields::update_meta_value() tries to compare the new meta value with the old value in order to avoid needless calls to update_metadata().
2. https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L355 fails to compare old and new value as the new value is now an "Array" string instead of [ 'foo' => 'bar' ] array, and does not return as it should.
3. update_metadata() is called, compares the new and old values with casting and returns false since values are actually the same.
4. WP_REST_Meta_Fields::update_meta_value() return an error and everything stops.

So what was supposed to be a simple check for existing meta to avoid unnecessary updates ends up with REST API returning an error 500. String casting of sanitize_meta() return value in https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L355 should be removed to avoid this.

[TimothyBlynJacobs]

Assigning to myself to look into this.

To clarify something, the check isn't to avoid a needless update. If we make a call to the update metadata API with an identical value, the function will return false. We have no way of distinguishing that false return value from an actual error.

[TimothyBlynJacobs]

Thanks much for reporting this @caercam!

I've uploaded a patch with a failing test.

Unfortunately, while removing the string cast does fix the bug, it causes the boolean sanitization test cases for test_update_value_return_success_with_same_value to fail. We might need to adjust based on whether the registered meta type is scalar.

[TimothyBlynJacobs]

Added a patch that checks the meta type before casting to a string. We have to cast to a string because the return value for scalar types from get_metadata will always be a string. And if a sanitize callback turns that value into a non-string type, the comparison check would fail.

This was fixed in #42069 for the REST API. However, it looks like it might also need to be fixed in the update_metadata primitive. Right now, it doesn't get caught by the early same value return check for the same reasons. It looks like it is rejected at the database level because when the value is stored as a string, there is no change from the original value. Though something for another ticket I think.

Could you give this a test @caercam? Also looping in @kadamwhite.

[caercam]

Thanks for the clarification @TimothyBlynJacobs, didn't stop to think about booleans. Patch works perfectly for me!

[caercam]

By the way, a quick and dirty fix for this until it's released is to use the update_{$meta_type}_metadata filter to short-circuit update_metadata() before it compares the identical values and return true instead to allow the request to proceed as expected.

[kadamwhite]

In 45903:

REST API: Only cast scalar types to string when comparing new & old meta values.

Newly-supported array and object meta types should not be cast to strings.

Props TimothyBlynJacobs, caercam.
Fixes #47928.

[kadamwhite]

@caercam Thanks for raising this. If you see any other issues with metadata handling, let us know!