WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 4 weeks ago

#42069 new defect (bug)

Saving metadata fails (randomly) if equal value already exists

Reported by: JVel Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.8.2
Component: REST API Keywords: has-patch needs-testing
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 (4)

42069.1.diff (3.9 KB) - added by dcavins 5 months ago.
Improve reliability of updating meta via REST API.
42069.2.diff (4.6 KB) - added by dcavins 6 weeks 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 6 weeks ago.
Update tests to use new REST fake server. Add test of single-quote case.
42069.diff (3.2 KB) - added by flixos90 4 weeks ago.

Download all attachments as: .zip

Change History (17)

#1 @pilou69
9 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
5 months ago

Improve reliability of updating meta via REST API.

#2 @dcavins
5 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
5 months ago

Also note that #42810 is related.

#4 follow-up: @MattGeri
2 months 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
2 months 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
7 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 7 weeks ago by MattGeri (previous) (diff)

@dcavins
6 weeks ago

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

#7 @dcavins
6 weeks 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
6 weeks ago

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

#8 @adamsilverstein
4 weeks ago

  • Milestone changed from Awaiting Review to 5.0

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


4 weeks ago

@flixos90
4 weeks ago

#10 @flixos90
4 weeks ago

  • Keywords needs-testing added

I cannot replicate this issue unfortunately. The tests you wrote all passed for me without actually using the changes in the code. @dcavins @MattGeri @adamsilverstein Can you provide some more background on your environment? I wonder if it's some PHP version-related detail or similar.

Regarding the patch, I just uploaded 42069.diff with optimized tests using a data provider, which reduces the necessary code and at the same time tests a lot more combinations. There's one test for checking updating a value with exactly the same value, and another one for checking updating a value with another value that essentially means the same (like 0/1 or false/true for booleans). My new patch only includes the tests as they passed without the code change. I'm not at all saying that change is wrong, it just did not fix a problem for me, so I excluded it for now. Of course, as we determine where exactly this problem is coming from, we can add the fix back as needed. Please use the new patch going forward and iterate from there.

Last edited 4 weeks ago by flixos90 (previous) (diff)

#11 @adamsilverstein
4 weeks ago

@flixos90

Nice work on the unit test cleanup! I pushed the changes from 42069.diff to a branch/fork to run in travis and see some meta tests failing, surprising these don't fail for you locally. are you sure the new tests are running, possibly you need to run build before testing?

https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/396971510

#12 @flixos90
4 weeks ago

@adamsilverstein I did make sure all of this is there. Must be something in my setup, maybe I can dive in in a couple days. Did you check whether bringing back the actual code change makes all those tests pass? My general feeling is, if so, we can commit this. Those tests should catch enough of these cases.

#13 @dcavins
4 weeks ago

Thanks very much for the unit test expansion, @flixos90! I haven't seen or used the dataProvider functionality and look forward to simplifying many of my unit tests in the future.

I also tested the new tests, and I'm not seeing any failures on my local machine, either. However, when I create a build, I'm still seeing a 500 response code when updating textured text via REST, so something must have changed in the test environment that is masking the failures.

Thanks again for taking a look at this issue.

Note: See TracTickets for help on using tickets.