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
@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
#6
@
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.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
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 ) {}$type; a string, or an array meaning "any of these". Otherwise returnsfalse, which callers treat as a miss: regenerate andwp_cache_set().$callbackruns after the type check for structural validation (for example, "object with a non-emptyID").$valid(by reference) reports whether contents were found and passed validation, so cached booleans stay usable.$delete_invalidopts into deleting them.wp_cache_get_valid_failedaction fires when found contents fail validation, giving monitoring tools a signal that they did not have before.cache-compat.phppurely on top ofwp_cache_get()(thewp_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.