WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 4 weeks ago

Last modified 4 weeks ago

#47928 closed defect (bug) (fixed)

REST API: 'array' type meta not updating properly

Reported by: caercam Owned by: TimothyBlynJacobs
Milestone: 5.3 Priority: normal
Severity: normal Version: trunk
Component: REST API Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

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

Attachments (3)

47928.diff (1.4 KB) - added by TimothyBlynJacobs 4 weeks ago.
47928.2.diff (2.6 KB) - added by TimothyBlynJacobs 4 weeks ago.
47928.3.diff (3.6 KB) - added by TimothyBlynJacobs 4 weeks ago.

Download all attachments as: .zip

Change History (11)

#1 @TimothyBlynJacobs
4 weeks ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to TimothyBlynJacobs
  • Status changed from new to assigned

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.

#2 @TimothyBlynJacobs
4 weeks ago

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.

#3 @TimothyBlynJacobs
4 weeks ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

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.

#4 @caercam
4 weeks ago

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

#5 @caercam
4 weeks ago

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.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


4 weeks ago

#7 @kadamwhite
4 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#8 @kadamwhite
4 weeks ago

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

Note: See TracTickets for help on using tickets.