Bug: `wp_cache_themes_persistently` only used for setting, not getting

[mattwiebe]

The WP_Theme class sets its own $persisently_cache static property on __construct, via the wp_cache_themes_persistently filter. The class checks this property throughout before setting cache values, but cache values are fetched via the cache_get method without ever first checking self::$persisently_cache, leading to a condition where, once caching is active, it is difficult to test changes to a theme that involves cached pieces.

For instance, on WordPress.com, we return 150 on the wp_cache_themes_persistently filter to leverage our memcached backend for caching theme data. We recently wanted to remove the full-site-editing theme tags from a few themes because they were causing problems with theme activation under WP 5.8's validate_theme_requirements function, which won't activate a theme with that tag and the Gutenberg plugin inactive. (This was from an earlier, pre-Gutenberg FSE project we ran internally on wpcom.)

Nor could we simply call the cache_delete method for the affected themes, since our development environment shares the memcache pool with production servers, which would lead to cache pollution. Nor was simply deploying the fix and hoping for resolution after cache expiry a valid approach in this case.

The simple fix would be to check for self::$persistently_cache in the cache_get method, always returning false in that case. I can try to make a patch for that tomorrow, as I don't currently have a working core environment to make a patch against and need to log off.

[SergeyBiryukov]

Thanks for the ticket!

The class checks this property throughout before setting cache values

At a glance, this doesn't seem quite correct. As far as I can tell, the $persistently_cache property is used for three things in WP_Theme:

1. To decide whether to create a persistent or non-persistent cache group​ in WP_Theme::__construct().
2. To set the `$cache_expiration` property​, if the $persistently_cache value is integer.
3. To sanitize and cache all the headers​ in WP_Theme::get() when using persistent cache.

Other than that, the property is not checked before any other of the 11 calls to WP_Theme::cache_add() in the class, they appear to be the same for both persistent and non-persistent cache. So it looks like the property is not quite used for setting cache values in the way the ticket title suggests, except for the headers in WP_Theme::get().

The simple fix would be to check for self::$persistently_cache in the cache_get method, always returning false in that case.

Since the property is not checked in WP_Theme::cache_add() or WP_Theme::cache_delete(), it seems like checking it in WP_Theme::cache_get() would introduce an inconsistency. Wouldn't that essentially break persistent caching for themes? Perhaps a more targeted cache invalidation might help?

I might be misunderstanding though, would appreciate any clarification :)

[mattwiebe]

Thanks for the speedy feedback @SergeyBiryukov ! I was close to EOD and just wanted to get this up after a frustrating day of debugging the underlying issue. I should have waited a little longer and filed this with more calm investigation.

I've discovered that this is actually a bug in our memcached backend, which does not respect the call to wp_cache_add_non_persistent_groups( 'themes' ) as it should. (It just stubs out the function and returns silently). So all the cache_get and cache_add calls still wind up in the persisted group rather than simply being persisted in-memory as expected from the filter value.

Thanks again!