update_metadata() and update_option() strict checks can cause false negatives

[nacin]

Given this:

add_post_meta( $post_id, 'key', 1 );
update_post_meta( $post_id, 'key', 1 );

The update should not work, because they are the same. However, the meta cache will have "1" as a string, and then it will strict compare it to 1 as an integer. Thus, an unnecessary update will run.

Best I can tell, this could also affect update_option().

It is quite common to use integers and booleans directly into these functions. They should be smart enough to recognize that "1" == 1 == true and "0" == 0 == false, and that any numeric string is also equal to a properly cast integer.

Unit tests needed.

Ticket from which this was spun: #22189, saving navigation menus is slow.

[SergeyBiryukov]

Related: #20712

[atimmer]

​22192.1.diff adds a test to test for the amount of queries executed after adding a meta values and right after updating it with the same value.

[duck_]

Replying to atimmer:

​22192.1.diff adds a test to test for the amount of queries executed after adding a meta values and right after updating it with the same value.

Thanks for the tests :) A few notes though:

• These tests shouldn't be in Tests_Meta_Query they belong more to Tests_Meta in tests/meta.php
• Can we use the num_queries property of $wpdb? If not then the reference to $wpdb should disappear.
• Debug cruft left-over in query_counter(). (Though the entire function might go per bullet above)

[atimmer]

​22192.2.diff updates the test with the notes of duck_.

[SergeyBiryukov]

Related: #31047

[boonebgorges]

See https://core.trac.wordpress.org/ticket/31047#comment:7 for a description of what I think the problem is and how it should probably be solved.

[boonebgorges]

22192.3.diff​ has some unit tests that demonstrate what's happening in the case of update_option(), with a partial suggested solution. (update_metadata() will be very similar.) The short story is that the strict equality check between old value and new value is getting tripped up by inconsistent type handling in our cache and in our database handlers.

There are really two different sets of cases. The first is the "normal" case, where the cache is populated from the database. In this case, scalar values will become strings, which means that we only need to cast the *new* value to a string to do the comparison. The second case is where you call add_option() and then immediately call update_option(). In this case, add_option() updates the cache in a way that respects the type of the original data, so the old/new value comparison needs to cast *both sides* to get a properly loose comparison.

The is_scalar()/(string) dance in 22192.3.diff fixes most of these issues. However, (string) false (or strval( false )) resolves to an empty string, *not* '0', so the tests involving the boolean false are failing. If we want to embrace the spirit of nacin's original bug report, we could introduce special handling for false. My personal thinking is that casting between string '1' and int 1 is pretty obvious from an API point of view, but casting bools is not, and it may not be a great developer experience to juggle booleans in the background. Maybe others feel differently?

This is a potentially breaking change in certain edge cases, so it'd be nice to get a second opinion on it.

[boonebgorges]

22192.5.diff​ is a more comprehensive solution (again, for the case of options). I've added unit tests that demonstrate the difference between the way types are juggled when set directly from add_option() and when pulled from the database.

I've also added the logic that would be necessary to get all the tests to pass - that is, to get 1 == '1' == true and 0 == '0' == false. It's pretty ugly. The fact that (string) false evaluates to an empty string means, in part, that add_option( 'foo', false ) results in a db row where the 'value' field is an empty string. So, in order for update_option( 'foo', $value ) to result in a cache hit when $value is falsey, we need to do a special check against empty strings. This will result in false cache hits when an option has been explicitly set to an empty string that is not meant to be falsey, which suggests that we should be fixing this on the way into the database (ie false should be converted to 0). But this latter suggestion would likely cause even more breakage, which is why I'm recommending limiting the empty-string/false equivalence only when doing the cache check. (The remaining option is to exclude booleans from the type juggling, which is what I suggested earlier. Anyone saving booleans through these functions is asking for trouble.)

[SergeyBiryukov]

#57894 was marked as a duplicate.

[domainsupport]

Is there any chance this patch can be signed off? We're getting many false negatives per day on one of our sites because of this bug which is causing un-necessary calls to $wpdb->insert() in update_option().

Is there a reason that this is still open 10 years later?

Thanks,

Oliver

[azaozz]

Related: #55942. Seems the patch needed for here would be different after the changes there (storing of PHP types in the DB) are implemented.