[wp-trac] [WordPress Trac] #22192: update_metadata() and update_option() strict checks can cause false negatives

[WordPress Trac]

#22192: update_metadata() and update_option() strict checks can cause false
negatives
-----------------------------------+-----------------------------
 Reporter:  nacin                  |       Owner:
     Type:  defect (bug)           |      Status:  new
 Priority:  normal                 |   Milestone:  Future Release
Component:  Options, Meta APIs     |     Version:
 Severity:  normal                 |  Resolution:
 Keywords:  has-patch 2nd-opinion  |     Focuses:  performance
-----------------------------------+-----------------------------
Changes (by boonebgorges):

 * keywords:  needs-patch needs-unit-tests => has-patch 2nd-opinion


Comment:

 [attachment: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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/22192#comment:9>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform