WordPress.org

Make WordPress Core

Opened 9 months ago

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

Download all attachments as: .zip

Change History (11)

comment:1 nofearinc9 months 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 leewillis779 months 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.

leewillis779 months ago

Patch

comment:3 leewillis779 months ago

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

comment:4 nacin9 months ago

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

nacin7 months ago

comment:5 nacin7 months 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 nacin7 months ago

This ticket also fixes #25015.

ocean907 months ago

$_newvalue = maybe_serialize( $newvalue );

comment:7 ocean907 months 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 nacin7 months 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.