WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 6 months ago

#22192 new defect (bug)

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

Reported by: nacin Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch 2nd-opinion
Focuses: performance Cc:
PR Number:

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)

22192.1.diff (1.7 KB) - added by atimmer 6 years ago.
22192.2.diff (1.7 KB) - added by atimmer 6 years ago.
22192.diff (2.0 KB) - added by boonebgorges 5 years ago.
22192.3.diff (2.7 KB) - added by boonebgorges 5 years ago.
22192.4.diff (3.6 KB) - added by boonebgorges 5 years ago.
22192.5.diff (4.2 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (18)

#1 @knutsp
7 years ago

  • Cc knut@… added

#3 follow-up: @atimmer
6 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.

@atimmer
6 years ago

#4 in reply to: ↑ 3 @duck_
6 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 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
6 years ago

#5 @atimmer
6 years ago

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

#6 @nacin
6 years ago

  • Component changed from Performance to Options and Meta
  • Focuses performance added

#8 @boonebgorges
5 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.

@boonebgorges
5 years ago

@boonebgorges
5 years ago

#9 @boonebgorges
5 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.

@boonebgorges
5 years ago

@boonebgorges
5 years ago

#10 @boonebgorges
5 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.)

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by boone. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.