Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#24933 closed defect (bug) (fixed)

update_metadata() returns true even if meta value not updated

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


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 11 years ago.
24933.diff (5.7 KB) - added by nacin 11 years ago.
24933.2.diff (6.0 KB) - added by ocean90 11 years ago.
$_newvalue = maybe_serialize( $newvalue );

Download all attachments as: .zip

Change History (11)

#1 @nofearinc
11 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?

#2 @leewillis77
11 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.

11 years ago


#3 @leewillis77
11 years ago

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

#4 @nacin
11 years ago

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

11 years ago

#5 @nacin
11 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.

#6 @nacin
11 years ago

This ticket also fixes #25015.

11 years ago

$_newvalue = maybe_serialize( $newvalue );

#7 @ocean90
11 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).

#8 @nacin
11 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.