Opened 11 years ago
Last modified 3 months ago
#22192 new defect (bug)
update_metadata() and update_option() strict checks can cause false negatives
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Options, Meta APIs | Keywords: | has-patch 2nd-opinion |
Focuses: | performance | Cc: |
Description
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.
Attachments (6)
Change History (22)
#3
follow-up:
↓ 4
@
10 years ago
- Cc atimmermans@… added
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.
#4
in reply to:
↑ 3
@
10 years ago
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 toTests_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)
#5
@
10 years ago
22192.2.diff updates the test with the notes of duck_.
#8
@
8 years ago
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.
#9
@
8 years ago
- Keywords has-patch 2nd-opinion added; needs-patch needs-unit-tests removed
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.
#10
@
8 years ago
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.)
Related: #20712