WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 13 days ago

#42069 new defect (bug)

Saving metadata fails (randomly) if equal value already exists

Reported by: JVel Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8.2
Component: REST API Keywords: has-patch
Focuses: rest-api Cc:

Description

When using js rest api to save page.

Duplicate value test in wp-includes/rest-api/fields/class-wp-rest-meta-fields.php line 303 fails and causes error "Could not update meta value in database."

if ( $old_value[0] === $meta_value ) {

Problem seems to be that $old_value is not going through wp_slash but new $meta_value is.

I get it working by changing code to test against $value instead.

if ( $old_value[0] === $value ) {

Attachments (3)

42069.1.diff (3.9 KB) - added by dcavins 4 months ago.
Improve reliability of updating meta via REST API.
42069.2.diff (4.6 KB) - added by dcavins 13 days ago.
Update tests to use new REST fake server. Add test of single-slash case.
42069.2.2.diff (4.6 KB) - added by dcavins 13 days ago.
Update tests to use new REST fake server. Add test of single-quote case.

Download all attachments as: .zip

Change History (10)

#1 @pilou69
8 months ago

Having an issue with this too in 4.8.2.

It's not random, it always happens when the meta is not modified and has any characters that must be slashed.

We found 3 ways to work around this:

  1. Apply modification above, but it overrides the core :(
    if ( $old_value[0] === $value ) {
    
  1. Register meta fields as single => false. But that seems very ugly.
  1. Create custom endpoint:

create a class extending WP_REST_Meta_Fields, and override update_meta_value function with patch applied above

extend WP_REST_Posts_Controller (WP_REST_{$type}_Controller),)

override in you custom endpoint controller $this->meta with your Class with the patch

@dcavins
4 months ago

Improve reliability of updating meta via REST API.

#2 @dcavins
4 months ago

  • Keywords has-patch added

I too ran into this problem today and was able to identify two cases where WP_REST_Meta_Fields::update_meta_value() was failing to interpret the values as identical, but update_metadata() was returning false (because the values were considered equal by that function), so the REST request was getting a 500 error back. These two cases are problematic:

  • Text containing characters that are slashed or converted to entities, like " or <.
  • Booleans, like a checkbox, that, when unchecked, pass the integer 0.

In the first case, the comparison in WP_REST_Meta_Fields::update_meta_value() tries to strict compare &lt; to <, and, in the second case, the string '0' (all saved meta is strings) is compared to the incoming sanitized value of (int) 0.

I've created a diff with tests showing these failures and a proposed patch to improve the comparison in WP_REST_Meta_Fields::update_meta_value() so that it catches more cases that update_metadata() or $wpdb->query() will consider equal.

If there are other cases that I can write tests for, please describe the situation in detail in your comments. I'd like to find all failures and make sure that the fix works for all of them.

Thanks for your comments!

#3 @dcavins
4 months ago

Also note that #42810 is related.

#4 follow-up: @MattGeri
4 weeks ago

I'm encountering this issue too. I've looked at the patch and my suggestion would be that you remove the sanitization from the old value as it's already unslashed and sanitized. Sanitization and unslashing should only be applied to $meta_value.

So the conditional could read something like this, to satisfy all the usecases and tests:

if ( $old_value[0] === sanitize_meta( $meta_key, wp_unslash( $meta_value ), $meta_type ) ) {

Let me know your thoughts and if you agree but can't make the changes to the patch, I'd be happy to.

#5 in reply to: ↑ 4 @dcavins
4 weeks ago

Replying to MattGeri: Thanks for your comments. Your change is close to what TimothyBlynJacobs suggested in https://core.trac.wordpress.org/attachment/ticket/42810/42810.diff

Can you offer a use case that fails in his previous patch, so that we can explain the necessity of adding wp_unslash()?

I'm fuzzy on the details, but it seems like one of my test cases was failing with the previous patch, probably the '0' (from saved meta) === 0 (incoming sanitized POST) case, which is why I added the sanitize step to the saved value as well. It's easy to test, but hard to remember something from a few months ago.

Thanks!

#6 @MattGeri
2 weeks ago

No worries. I think it was actually the other test that would have been failing.

The issue is that $old_value[0] gets returned unslashed from get_metadata, but $meta_value is wrapped in slashes earlier in the method. So it's trying to compare slashed against unslashed data, which the other solution does not take in to account.

The data in meta.php > update_metadata() gets unslashed on line 167 this is before the same comparison as the rest function happens to check if an update should occur.

Edit: So an example usecase would be text with a single quote in it.

Last edited 2 weeks ago by MattGeri (previous) (diff)

@dcavins
13 days ago

Update tests to use new REST fake server. Add test of single-slash case.

#7 @dcavins
13 days ago

Hi @MattGeri- I've tested your suggestion, and it works for textured text, but fails when updating booleans (test_update_value_return_success_response_when_updating_boolean_values_no_change() here: https://core.trac.wordpress.org/attachment/ticket/42069/42069.2.diff)

I've added a test that includes a single quote per your suggestion. Any other feedback is welcome--it'd be great to have comprehensive tests of cases we know fail.

Without the change, all three of my test cases fail. :(

@dcavins
13 days ago

Update tests to use new REST fake server. Add test of single-quote case.

Note: See TracTickets for help on using tickets.