Warn about calling `_get_non_cached_ids()` with invalid ids

[tillkruess]

Currently the _get_non_cached_ids() function doesn't validate its $object_ids argument contains only int[]. This can lead to call like:

wp_cache_get_multiple( [ null, false, true, 1, 2, 3 ], 'posts' );

or even worse:

wp_cache_get_multiple( [ 1, 2, array() ], 'posts' );

Which is impossible to translate to it's expected return value:

[
    1 => 1,
    2 => 2,
    array() => 0
];

Similar to #56198 we should check the given $object_ids and at least call _doing_it_wrong().

[tillkruess]

Maybe we can turn WP_Object_Cache::is_valid_key() into a global helper function and use it in _get_non_cached_ids()?

[spacedmonkey]

How about a simplier solution like this?

function _get_non_cached_ids( $object_ids, $cache_key ) {
        $non_cached_ids = array();
        $object_ids = array_filter( $object_ids, 'is_scalar' );

[48055] for extra context.

[tillkruess]

@spacedmonkey: That would allow bool keys, which often groups false into the same key unexpectedly.

[spacedmonkey]

Replying to tillkruess:

@spacedmonkey: That would allow bool keys, which often groups false into the same key unexpectedly.

$object_ids = array_filter( $object_ids, function( $object_id ) {
   return is_scalar($object_id) && !is_bool( $object_id );
});

How about this?

[peterwilsoncc]

Does $object_ids = array_filter( $object_ids, 'is_numeric' /* or 'is_int' */ ); work?

[tillkruess]

Yeah, I think is_numeric() would be strict-ish enough.

Most edge cases like 42.0 or "1337e0" are so rare, we only really need to protect against false and null and other gross type mismatches.

Should we just do an array_filter() or also throw a little user notice?

Remove entries from $object_ids array that are not integers or a string contain an integer.

Trac ticket: https://core.trac.wordpress.org/ticket/57593

[tillkruess]

I've opened a PR with a simple fix.

Since _get_non_cached_ids() is no longer marked as private, should we inform the user that a non-integer was given using a _doing_it_wrong() call, ​like we did here? Otherwise this may lead to silent misbehavior.

[peterwilsoncc]

Replying to tillkruess:

Since _get_non_cached_ids() is no longer marked as private, should we inform the user that a non-integer was given using a _doing_it_wrong() call, ​like we did here? Otherwise this may lead to silent misbehavior.

Yes, let's go with consistency.

[tillkruess]

@peterwilsoncc Added: ​https://github.com/WordPress/wordpress-develop/pull/4076/files

I don't understand the yoda failure.

Oh, some additional tests would be helpful here too.

Something to ensure that the expected failures throw the notice and that everything that should be valid does not. IS that something you have bandwidth to help out with?

🤦 Looks like an off-by-one error with the yoda note. */ is definitely not triggering it. Let's suppress the error with a note it's a false positive.

🤦 Looks like an off-by-one error with the yoda note. */ is definitely not triggering it. Let's suppress the error with a note it's a false positive.

How? 😬

Hey Till,

After chatting in Slack, I took the liberty of pushing a few changes to the tests:

• moved the tests to a dedicated file for the function -- apparently it hasn't been tested directly in the past
• expanded the data provider for invalid object IDs to include a few extra types.
• switched the group name to fake-group to avoid collisions/bad luck causing the tests to fail
• added a test for valid ids to make sure they were returned as expected (ie, integers)

I've also confirmed on trunk that wp eval "var_dump( _get_non_cached_ids( array( 500, '501' ), 'invalidgroup' ) );" returns integers and this doesn't change any return types. It does not, so that's nice :)

@SergeyBiryukov @spacedmonkey @felixarntz Read for review ✌️

@tillkruss That is not what I had in mind. I had this in mind. ​https://github.com/WordPress/wordpress-develop/commit/b9d0ad5d504f77e0f417ec0aa458eee1ba602a5c. This means no static callbacks and we can unit tests directly against, _sanaitize_cache_id.

@tillkruss That is not what I had in mind. I had this in mind. ​b9d0ad5. This means no static callbacks and we can unit tests directly against, _sanaitize_cache_id.

I'm so confused, where did this commit go?!

[spacedmonkey]

Marking as ready to commit. Assigning to myself as committer, will commit as part of 6.3 release.

[spacedmonkey]

In 55543:

Cache API: Add a warning when calling _get_non_cached_ids with invalid ids.

Sanitize the array of ids passed to the _get_non_cached_ids function and add a _doing_it_wrong call, if an invalid type is passed.

Props tillkruess, spacedmonkey, peterwilsoncc, flixos90, SergeyBiryukov.
Fixes #57593.

[SergeyBiryukov]

In 55549:

Docs: Fix typo in _validate_cache_id() description.

Includes:

• Capitalizing "ID" in a consistent way.
• Expanding the comment on not using filter_var().
• Adding a @covers tag for the function in unit tests.
• Minor tweak to the _doing_it_wrong() message.

Follow-up to [53818], [55543].

See #57593.

Committed https://core.trac.wordpress.org/changeset/55543

[flixos90]

Covered in dev note ​https://make.wordpress.org/core/2023/07/17/improvements-to-the-cache-api-in-wordpress-6-3/