WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#24933 closed defect (bug) (fixed)

update_metadata() returns true even if meta value not updated

Reported by: jdgrimes Owned by: nacin
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.6
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

When update_metadata() updates a meta value with the $prev_value parameter set, the return will be true regardless of whether any rows were affected.

This makes it difficult to use the metadata API in cases in which a race condition should be avoided. For example, we cannot reliably increment a meta value. If we leave $prev_value empty, any intermediate changes will be overridden; if we set it and the value has changed, the update will silently fail (update_metadata() will still return true).

This is inconsistent with update_option(), which returns true only if the number of rows affected is greater than 0.

In short, this behavior makes this function pretty much useless in cases where a race condition needs to be avoided.

My request a way of detecting whether any rows were actually affected or not when this function is called.

Attachments (3)

24933-1.patch (1.0 KB) - added by leewillis77 2 years ago.
Patch
24933.diff (5.7 KB) - added by nacin 2 years ago.
24933.2.diff (6.0 KB) - added by ocean90 2 years ago.
$_newvalue = maybe_serialize( $newvalue );

Download all attachments as: .zip

Change History (11)

comment:1 @nofearinc2 years ago

I can't seem to reproduce that.

I tried to use update_metadata for a post with a key and value. The first insert was true, the next attempts were false. When I change the value, it returns true again and false for all the other attempts.

What is your update call?

comment:2 @leewillis772 years ago

I can reproduce this as follows.

Create a post meta value by calling

update_metadata( 'post', 1, '_test', 'value1' );

Then, call it with a prev_value of value1, e.g.

update_metadata( 'post', 1, '_test', 'value2', 'value1' );

update_metadata() will return true.

Call the same update again. update_metadata() will return true despite the fact that no metadata was updated. Hooks also fire as if the meta has been updated. The attached patch checks the return value from $wpdb->update and only returns true if rows were updated. This solves the original reporter's issue.

The patch also only fires updated_post_meta and updated_postmeta hooks, and invalidates the cache if the update changed rows. This seems logical but could do with a second set of eyes to avoid back-compat headaches.

@leewillis772 years ago

Patch

comment:3 @leewillis772 years ago

  • Cc junk@… added
  • Keywords has-patch added
  • Type changed from enhancement to defect (bug)

comment:4 @nacin2 years ago

  • Milestone changed from Awaiting Review to 3.7
  • Owner set to nacin
  • Status changed from new to accepted

@nacin2 years ago

comment:5 @nacin2 years ago

  • Keywords commit added

24933.diff updates a few option and meta functions to always check the result of the DB query, and to also wait to update any caches before that query is run.

Cache deletion operations (in delete_option() etc) don't need to wait until the query is run.

Just a bunch of code that gets moved around; but, needs review.

comment:6 @nacin2 years ago

This ticket also fixes #25015.

@ocean902 years ago

$_newvalue = maybe_serialize( $newvalue );

comment:7 @ocean902 years ago

24933.diff looks good so far.

I was just a bit confused by wp_cache_set( $option, $_value, 'options' ); vs. wp_cache_set( $option, $newvalue, 'options' );. In 24933.diff​ $_value is the sanitized value now. I'm fine with that, but in update_option() $newvalue still represents the sanitized value. So I changed that in 24933.2.diff to be consistent and less confusing.

Maybe we should also rename $newvalue to $value to be consistent with update_site_option() (Or vice versa).

comment:8 @nacin2 years ago

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

In 25583:

Return false in update_metadata() and update_metadata_by_mid() when the DB query fails.

props leewillis77.
fixes #24933.

Note: See TracTickets for help on using tickets.