[wp-trac] [WordPress Trac] #38203: Remove `absint` on object IDs in `delete_metadata`, etc

[WordPress Trac]

#38203: Remove `absint` on object IDs in `delete_metadata`, etc
--------------------------------+------------------------------
 Reporter:  peterwilsoncc       |       Owner:
     Type:  defect (bug)        |      Status:  new
 Priority:  normal              |   Milestone:  Awaiting Review
Component:  Options, Meta APIs  |     Version:  2.9
 Severity:  normal              |  Resolution:
 Keywords:  2nd-opinion         |     Focuses:
--------------------------------+------------------------------
Changes (by johnjamesjacoby):

 * keywords:  has-patch commit => 2nd-opinion


Comment:

 **Research**

 I put a few hours of investigation into this today. It's really hard not
 to do a deeper dive into all `absint()` and `intval()` usages everywhere,
 because numeric values get changed in so many places.

 Looking at the numerous usages of both functions, it seems at first like a
 flawed design, but there's actually a lot more going on that makes this
 surprisingly tricky to explain.

 As was stated in the ticket description, the 2 problems are:

 * `absint()` will turn negative numbers into their positive (absolute)
 equivalent
 * `intval()` will not change floats (really large numbers, beyond
 `PHP_INT_MAX` and `PHP_INT_MIN`) and those constants have different values
 based on system configuration (32/64 bit, etc...)

 ----

 **Case 1**

 The following code will save to post ID `1` and not `-1`, thanks to
 `absint()`:

 {{{
 update_post_meta( -1, 'foo', 'bar' );
 }}}

 ** Case 2 **

 Imagine that `PHP_INT_MAX` has a value of `9223372036854775807`, which is
 pretty standard:

 {{{
 $id_max     = 9223372036854775807;
 $id_max_one = $id_max + 1;
 }}}

 And we want to use them directly:

 {{{
 var_dump( $id_max     );
 var_dump( $id_max_one );
 }}}

 Will result in:

 {{{
 int   9223372036854775807
 float 9.2233720368548E+18
 }}}

 Once we cast them as `int`s, weird stuff starts happening:

 {{{
 var_dump( (int) $id_max     );
 var_dump( (int) $id_max_one );
 }}}

 The float comes out as flipped into the minimum negative integer:

 {{{
 int  9223372036854775807
 int -9223372036854775808
 }}}

 What we've learned here is that `(int)` or `intval()` alone are not enough
 to always get a trustworthy ID value.

 ----

 **Discovery**

 Most of the `ID` type columns (for posts/comments/users/terms) in
 WordPress are `BIGINT(20)` (and that does not mean what I think most
 people would feel safe assuming it means, which is some number up to 20
 digits in length. That is not the case. The `20` is simply a display hint,
 usually for padding zeroes.)

 [https://dev.mysql.com/doc/refman/5.7/en/integer-types.html
 Minimum/Maximum values] are dependent on whether the column is signed or
 unsigned, and while WordPress single-site database tables were normalized
 in #8751, it's multisite tables all remain `signed` when they should be
 `unsigned`. The most likely reason for this is `signed` is assumed if
 `unsigned` is not explicitly passed, and way back when those tables were
 new, the single-site tables were still "in a way" as my wife likes to say.

 ----

 **Conclusion**

 Knowing that post/comment/user/term `ID` columns are `unsigned`, I believe
 `absint()` is the correct call to make here, because the database simply
 cannot store negative numbers in `BIGINT(20) unsigned` columns.

 This means my comment above on being able to store `-1` in the
 `wp_sitemeta:site_id` column, is unintended behavior, and should be
 changed.

 **To-do's**

 * Educate community about why negative numbers are not allowed for object
 IDs
 * A new ticket should be opened to update `$ms_global_tables` database
 tables to alter the relevant `signed` columns into `unsigned` columns.
 This may involve an upgrade routine, which needs to obey
 `wp_should_upgrade_global_tables()`.
 * Awareness campaign about breaking changes to multisite global tables, in
 the very off chance someone is storing negative values where they can no
 longer exist.
 * Educate multisite plugin developers about the above breaking changes
 * Close this ticket as a wontfix
 * BONUS: Many plugins with custom database tables do not use `unsigned`
 integers, including BuddyPress, WooCommerce, EDD, and more. They should
 all be updated at their earliest convenience as well.

 ----

 With all of that said, I'd like to make a motion to close this ticket as
 wontfix.

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