update_metadata() returns true even if meta value not updated

[jdgrimes]

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.

[nofearinc]

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?

[leewillis77]

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.

[leewillis77]

Patch

[nacin]

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.

[nacin]

This ticket also fixes #25015.

[ocean90]

$_newvalue = maybe_serialize( $newvalue );

[ocean90]

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).

[nacin]

In 25583:

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

props leewillis77.
fixes #24933.