Remove `absint` on object IDs in `delete_metadata`, etc

[peterwilsoncc]

Absint is run on the object ID in the functions called with a meta key: delete_metadata(), get_metadata(), add_metadata(), update_metadata() and metadata_exists().

This leads to unexpected behavior in the event a negative or floating object ID is passed.

Related #37746, #37738, #33372.

[johnjamesjacoby]

Just ran into this, working on #35773.

intval() should be sufficient here.

[johnjamesjacoby]

Prefer intval() over absint() in meta.php

[johnjamesjacoby]

38203.patch replaces absint() assignments with intval() instead. My reasoning for this is:

• Post/Comment/User IDs are passed through intval() in many locations already
• Mutating the $object_id away from the intended value reduces trust in the API
• $mid touches also already use intval() on the meta_id

[johnjamesjacoby]

Screenshot of wp_sitemeta values that led me here

[peterwilsoncc]

@johnjamesjacoby, thanks a tonne. I've been pondering @dd32's comments on #39053 before doing much about this. I guess I should make a decision and one or the other in.

[johnjamesjacoby]

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 ints, 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.)

​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.

---

TL;DR

absint() keeps the PHP layer more closely aligned with the BIGINT(20) unsigned ID columns in every single-site database table. The multisite database tables need fixing.

My recommendation is to close this ticket as wontfix.

[johnjamesjacoby]

I've opened #40418 to discuss the signed vs unsigned issue with the multisite database table columns.

[peterwilsoncc]

Replying to johnjamesjacoby:

Thanks so much for the research.

I'd like to keep the ticket open as I still think absint() is invalid in this context, passing $object_id = -4 shouldn't update object 4. intval() combined with checking for a positive number is a better but problematic in its own ways.