Reverts caching from r55086 #3864
Conversation
|
Committed via https://core.trac.wordpress.org/changeset/55092. |
There was a problem hiding this comment.
@hellofromtonya I think it makes sense to revert as a quick solution to fix sites running trunk, but IMO it's not a preferable outcome, and not one we need to settle for.
It was already suggested in https://core.trac.wordpress.org/ticket/56975#comment:37 to wrap the cache code in function_exists checks, I would advise the same
We should optimize performance for the frontend, where load-styles.php is typically not used and where the benefits of this caching are most important. Obviously we must not break load-styles.php, but I think the approach of function_exists checks is a reasonable tradeoff.
| @@ -575,7 +575,7 @@ function switch_to_blog( $new_blog_id, $deprecated = null ) { | |||
| ); | |||
| } | |||
|
|
|||
| wp_cache_add_non_persistent_groups( array( 'counts', 'plugins', 'theme_json' ) ); | |||
There was a problem hiding this comment.
I think this change goes beyond what we should possibly revert here. Why should we revert adding the non-persistent cache group?
There was a problem hiding this comment.
It only removed theme_json' from each of these. https://core.trac.wordpress.org/changeset/55092
There was a problem hiding this comment.
@hellofromtonya I understand, I meant the addition of theme_json here was not responsible for the fatal error, so I'm not sure we should remove it. But anyway, let's follow up on a new PR :)
| /* | ||
| * By using the 'theme_json' group, this data is marked to be non-persistent across requests. | ||
| * @see `wp_cache_add_non_persistent_groups()`. | ||
| * | ||
| * The rationale for this is to make sure derived data from theme.json | ||
| * is always fresh from the potential modifications done via hooks | ||
| * that can use dynamic data (modify the stylesheet depending on some option, | ||
| * settings depending on user permissions, etc.). | ||
| * For some of the existing hooks to modify theme.json behavior: | ||
| * @see https://make.wordpress.org/core/2022/10/10/filters-for-theme-json-data/ | ||
| * | ||
| * A different alternative considered was to invalidate the cache upon certain | ||
| * events such as options add/update/delete, user meta, etc. | ||
| * It was judged not enough, hence this approach. | ||
| * @see https://github.com/WordPress/gutenberg/pull/45372 | ||
| */ | ||
| $cache_group = 'theme_json'; | ||
| $cache_key = 'wp_theme_has_theme_json'; | ||
| $theme_has_support = wp_cache_get( $cache_key, $cache_group ); | ||
|
|
||
| /* | ||
| * $theme_has_support is stored as an int in the cache. | ||
| * | ||
| * The reason not to store it as a boolean is to avoid working | ||
| * with the $found parameter which apparently had some issues in some implementations | ||
| * @see https://developer.wordpress.org/reference/functions/wp_cache_get/ | ||
| * | ||
| * Ignore cache when `WP_DEBUG` is enabled, so it doesn't interfere with the theme | ||
| * developer's workflow. | ||
| * | ||
| * @todo Replace `WP_DEBUG` once an "in development mode" check is available in Core. | ||
| */ | ||
| if ( ! WP_DEBUG && is_int( $theme_has_support ) ) { | ||
| return (bool) $theme_has_support; | ||
| } |
There was a problem hiding this comment.
I would much prefer if we, instead of a full revert, wrapped all cache related logic here in a function_exists check.
The caching
wp_cache_*functions are not available whenwp-admin/load-styles.phpis loaded. The caching introduced in r55086 caused fatal errors and broken styles.This PR removes all of the caching code to restore functionality for sites and local development using
trunk.Trac ticket: https://core.trac.wordpress.org/ticket/56975
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.