WP_Post::get_instance() and four others cache a miss while cache addition is suspended

[bejignesh]

[63354] swapped wp_cache_add() for wp_cache_set() in get_instance() on WP_Post, WP_Term, WP_Comment, WP_Site and WP_Network. Only add() checks wp_suspend_cache_addition(), so all five now write to the object cache while additions are suspended.

On a running site, cache cleared, wp_suspend_cache_addition( true ):

                             trunk      [63353]
WP_Post::get_instance()      PRESENT    MISS
WP_Term::get_instance()      PRESENT    MISS
WP_Comment::get_instance()   PRESENT    MISS
wp_cache_add() control       MISS       MISS

The [63353] column is the same script with only those five files reverted.

Scope is narrow. The only caller in core is the widget preview path in WP_Widget, and what gets cached there is the raw row, so nothing renders wrong. What broke is the contract, not a site. Worth noting that the drop-in in tests/phpunit/includes/object-cache.php checks the flag in add() only, so with a persistent cache these writes reach the shared backend.

Nothing failed because nothing covers this, which is what #57263 and #60017 are open about.

The set() was right: an unusable cached value has to be replaced and add() will not overwrite. The two do not conflict. Replacing an existing entry is not an addition, so only a genuine miss needs to defer to the flag. Making set() itself honour the flag was ruled a breaking change on #55999, so I have not touched that.

Trunk only, 7.1 still has wp_cache_add(). Patch coming.

wp_cache_add() returns early when wp_suspend_cache_addition() is on and wp_cache_set() does not, so the swap in [63354] left WP_Post, WP_Term, WP_Comment, WP_Site and WP_Network::get_instance() writing to the object cache while additions are suspended.

The reason [63354] reached for set() still holds: an unusable cached value has to be replaced and add() will not overwrite. The decision here is that those two goals do not conflict. Replacing an entry that already exists is not an addition, so only a genuine miss defers to the flag, and a poisoned entry is still repaired while additions are suspended. There is a test for each half.

$unusable_value_cached is derived from the cached value rather than from the $found argument of wp_cache_get(), because a drop-in declaring only three parameters would leave $found undefined and warn on PHP 8.

Measured on a running site with the cache cleared and the flag on: post, term and comment are PRESENT on trunk, MISS with only the five class files from [63353], and MISS again on this branch. A plain wp_cache_add() control is MISS throughout. Each guard is mutation checked, reverting one file fails exactly one test. Groups post, comment, taxonomy, cache, ms-site and ms-network are green, and the single taxonomy warning is present on clean trunk too. PHPCS and PHPStan clean.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Drafting the fix and the regression tests, and running the reproduction, mutation and lint checks. Reviewed and verified by me before opening.

cc @josephscott

$unusable_value_cached is derived from the cached value rather than from the $found argument of wp_cache_get(), because a drop-in declaring only three parameters would leave $found undefined and warn on PHP 8.

Are there drop-ins that only declare 3 params? The wp_cache_get() function specifically seems to expect at least 4 as it has a func_num_args() > 4 check.

@westonruter Only one that I can find, and it is not a live case: ​Simple Cache's memcache drop-in, 1,000 installs and last updated in 2021. Everything current declares the fourth parameter, including the memcached drop-in bundled in tests/phpunit/includes/object-cache.php.

The reason was weak anyway. Initialising $found before the call removes the warning, which is what WP_AI_Client_Cache::get() does. Dropping that line from the description.

Thank you for the ping. I've been thinking through what this could look like and ultimately given the past decisions and where WordPress core is right now I'm leaning towards utilizing $found and doing a conditional delete then add flow.

Using Post as an example, it would be:

$found = false;
$_post = wp_cache_get( $post_id, 'posts', false, $found );

// A cached value that is not usable as a post is treated as a cache miss.
if ( ! ( $_post instanceof stdClass || $_post instanceof WP_Post ) || ! isset( $_post->ID ) ) {
    $_post = sanitize_post( $_post, 'raw' );

    /*
     * A cache result was found, but it was invalid.  Remove it so that the correct value
     * can be added.  This pattern is used instead of a direct wp_cache_set() because
     * wp_cache_add() will respect wp_suspend_cache_addition().
     */
    if ( $found ) {
        wp_cache_delete( $post_id, 'posts' );
    }

    wp_cache_add( (int) $_post->ID, $_post, 'posts' );
} elseif ( empty( $_post->filter ) || 'raw' !== $_post->filter ) {
    $_post = sanitize_post( $_post, 'raw' );
}

If we had something like wp_cache_get_valid() from https://core.trac.wordpress.org/ticket/66005 it could take care of deleting the invalid cache entries, allowing the Post code to skip the $found checks. If we aren't going to have that soon, then we should consider the $found, conditional delete, then add flow.

@josephscott Perhaps but don't you think incorporating a ! wp_suspend_cache_addition() check would be a more immediate fix to preserve the previous behavior whereby the object cache was expected to not get populated?

It is an approach that would work. When I looked at the usage of wp_suspend_cache_addition() in WordPress core I found it was ​only used twice ( outside of the function definition and the add check ). Those two calls were ​both in `display_callback()`, and neither of those calls were related to a wp_cache_set() condition.

Then I searched for wp_cache_add() usage, ​it is used 15 times. Based on those numbers WordPress code is depending much more on wp_cache_add() to check wp_suspend_cache_addition() than to do a separate check directly.

I still think having a wp_cache_get_valid() approach would be even better, but I'm trying to be pragmatic and look at what is currently available.

We should look into the history of wp_suspend_cache_addition(). My understanding is that it was primarily added for the sake of the WordPress Importer, to avoid filling up a persistent object cache with data unnecessarily. It was also added for the preventing previewed data from being populated in the object cache, and thus inadvertently leaking previewed changes live. This would seem to make it essential to prevent setting _or_ adding any data to the _persistent_ object cache when one is active. So wp_using_ext_object_cache() could be relevant here. But if that is the case, then I think it would be preferable to be consistent across all the wp_cache_*() functions to short-circuit if wp_suspend_cache_addition() is true. I see this is only done for WP_Object_Cache::add(). Ironically, this is for a non-persistent object cache, so here it seems the main point of it is to prevent filling up memory in the PHP process during a WordPress Import.

I think it would be preferable to be consistent across all the wp_cache_*() functions to short-circuit if wp_suspend_cache_addition() is true.

That was the first thing I had wondered about to - and even considered updating wp_cache_set() to detect and swap to a replace call. But given how long the current situation had been in place, that would potentially open a different can of worms.

Looking at redis-cache - ​https://plugins.trac.wordpress.org/browser/redis-cache/trunk/includes/object-cache.php - it also only checks on add.