Make WordPress Core

Opened 6 weeks ago

Last modified 6 weeks ago

#66005 new enhancement

Core Needs a Validating Cache Get Function

Reported by: josephscott Owned by:
Priority: normal Milestone: Awaiting Review
Component: Cache API Version:
Severity: normal Keywords: has-patch has-unit-tests 2nd-opinion
Cc: Focuses:

Description

After ​https://github.com/WordPress/wordpress-develop/pull/13270 I've been thinking about the situation with return values from a cache get.

wp_cache_get() returns whatever the cache backend holds. In most cases a return type is assumed ( or does minimal insufficient checking ), and when a backend returns something else we run into a fatal error situation. We've already seen this happen.

I think WordPress core could do a better job of making this easier for developers with a new function that validates the return value type. After considering different approaches, here is where I am at:

<?php
function wp_cache_get_valid( $key, $group, $type, $callback = '', bool $force = false, bool $delete_invalid = false, ?bool &$valid = null ) {}

My aim is to keep simple cases simple, while having enough flexibility to address more complex cases. For example, this could tighten up wp_count_posts() in ​https://github.com/WordPress/wordpress-develop/blob/7.1/src/wp-includes/post.php#L3464-L3541 -

        $counts = wp_cache_get( $cache_key, 'counts' );
        if ( false !== $counts ) {
                // We may have cached this before every status was registered.
                foreach ( get_post_stati() as $status ) {
                        if ( ! isset( $counts->{$status} ) ) {
                                $counts->{$status} = 0;
                        }
                }

which expects an object, but currently doesn't validate that. With the proposed function it would be a one line change:

        $counts = wp_cache_get_valid( $cache_key, 'counts', 'object' );
        if ( false !== $counts ) {

I've got a PR coming that will expand on the details.

Change History (7)

This ticket was mentioned in ​PR #13339 on ​WordPress/wordpress-develop by ​@josephscott.


6 weeks ago
#1

  • Keywords has-patch has-unit-tests added

https://core.trac.wordpress.org/ticket/66005

The aim is to keep simple improvements simple, while providing enough flexibility to also address more complex situations.

At the core this is a wrapper around wp_cache_get() - with type checking.

function wp_cache_get_valid( $key, $group, $type, $callback = '', bool $force = false, bool $delete_invalid = false, ?bool &$valid = null ) {}
  • Returns the cache contents only when they match an expected $type; a string, or an array meaning "any of these". Otherwise returns false, which callers treat as a miss: regenerate and wp_cache_set().
  • An optional $callback runs after the type check for structural validation (for example, "object with a non-empty ID").
  • $valid (by reference) reports whether contents were found and passed validation, so cached booleans stay usable.
  • Invalid contents are left in place by default for the caller to overwrite; $delete_invalid opts into deleting them.
  • A wp_cache_get_valid_failed action fires when found contents fail validation, giving monitoring tools a signal that they did not have before.
  • Implemented in cache-compat.php purely on top of wp_cache_get() (the wp_cache_get_salted() pattern), so behavior is identical over every drop-in and nothing new is required from cache backends.

There are several cases in WordPress core where this could be appllied to make sure that the return type of cached values is correct.

AI assistance: Yes
Tool(s): Claude Code
Model(s): Fable 5
Used for: I did a lot of rubber ducking with Claude on the wp_cache_get_valid() itself, to narrow down what the implementation should look like. Claude wrote the tests.

#2 @westonruter
6 weeks ago

Related: #51525, the same idea but for filter return values.

​@josephscott commented on ​PR #13339:


6 weeks ago
#3

@westonruter since you merged a few of the fixes related to the issue this is trying to help, you might be interested in this.

​@westonruter commented on ​PR #13339:


6 weeks ago
#4

Yeah, I think this one needs wider discussion for sure. See https://core.trac.wordpress.org/ticket/66005#comment:2

#5 @westonruter
6 weeks ago

  • Keywords 2nd-opinion added

#6 @josephscott
6 weeks ago

  • Keywords 2nd-opinion removed

Thank you for that ticket reference, I had missed that one.

I've read through the comments, especially https://core.trac.wordpress.org/ticket/51525#comment:37 where it was closed. I think this situation is different than #51525 in at least one important way: better type checking for the cache get values will be needed even if wp_cache_get_valid() ( or something like it ) is never added to WordPress.

That pushes the high level version of the question to: does WordPress include a helpful function to centralize the type checking, or does every wp_cache_get() call add follow up code for type checking?

At this point I'm in favor of a helpful function to make this work easier. If that ends up being vetoed then I'll move on and start submitting patches for the individual cases where there are insufficient return value checks that can end up in PHP fatal errors.

#7 @josephscott
6 weeks ago

  • Keywords 2nd-opinion added
Note: See TracTickets for help on using tickets.