Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persistently cache theme.json files via WP Cache API #56095

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

Persistently cache theme.json files via WP Cache API #56095

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f586b5
Persistently cache theme.json files via WP Cache API.
felixarntz Nov 14, 2023
1d17bda
Fix WPCS violations.
felixarntz Nov 14, 2023
d299fa4
Rely on the Gutenberg versions of relevant classes.
felixarntz Nov 14, 2023
42f8bc2
Clarify since for new parameter.
felixarntz Nov 14, 2023
4d4542c
Merge branch 'trunk' into performance/persistently-cache-theme-json-f…
felixarntz Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 96 additions & 44 deletions lib/class-wp-theme-json-resolver-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,43 +77,35 @@ class WP_Theme_JSON_Resolver_Gutenberg {
*/
protected static $user_custom_post_type_id = null;

/**
* Container to keep loaded i18n schema for `theme.json`.
*
* @since 5.8.0 As `$theme_json_i18n`.
* @since 5.9.0 Renamed from `$theme_json_i18n` to `$i18n_schema`.
* @var array
*/
protected static $i18n_schema = null;

/**
* `theme.json` file cache.
*
* @since 6.1.0
* @var array
*/
protected static $theme_json_file_cache = array();

Comment on lines -80 to -96
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to keep them if someone is extending the class and expects these to exist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that matters here as the class is marked private. It must not be overwritten for this purpose, and if anyone does so it's at their own risk (similar to e.g. using "experimental" APIs in Gutenberg).

/**
* Processes a file that adheres to the theme.json schema
* and returns an array with its contents, or a void array if none found.
*
* @since 5.8.0
* @since 6.1.0 Added caching.
* @since 6.5.0 Added persistent caching using object cache, and the `$cache_key` parameter.
*
* @param string $file_path Path to file. Empty if no file.
* @param string $cache_key Optional. Key to cache the result under. Omitting the parameter results in no caching
* being used. Default empty string.
* @return array Contents that adhere to the theme.json schema.
*/
protected static function read_json_file( $file_path ) {
protected static function read_json_file( $file_path, $cache_key = '' ) {
if ( $file_path ) {
if ( array_key_exists( $file_path, static::$theme_json_file_cache ) ) {
return static::$theme_json_file_cache[ $file_path ];
$cache_group = 'theme_json_files';
Copy link
Member Author

@felixarntz felixarntz Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joemcgill Related to https://core.trac.wordpress.org/ticket/59719#comment:18, what is your thought on this?

  • The PR currently uses theme_json_files, yet another cache group name than what we've talked about in that ticket so far.
  • Out of the 3 names we've been discussing there, this fits best into theme_files, as it's global data and the cache keys already include the theme slug. It's also exclusively about data from the filesystem (i.e. unlike the theme_json cache group), plus it needs to be persistent to be beneficial (i.e. unlike the themes cache group).
  • One caveat is that currently this cache is also used for theme.json files that are not from a theme. Hence the theme_json_files cache group name makes sense here, whereas using a broader theme_files cache group name would be misleading in that sense, as core's theme.json is not really a theme file.

I think we have 3 options here:

  1. Change this to become part of the theme_files cache group, and acknowledge that that cache group has a special case for WP core's own "fallback" theme files (specifically in this case, the theme.json file). In that case, the only other change needed here would be to adjust the cache key names so that they include a mention of theme_json, to clarify that these are cache keys for a theme.json file, rather than another file from a theme.
  2. Keep this as is. Probably best from a separation of concerns and understandability perspective, but yet another cache group related to theme data may seem excessive.
  3. Combine this with what we've so far considered under the theme_files cache group, but use another name that is broadly about any files. In that case, the cache keys would not only contain the theme slug, but also some prefix like theme_{themeSlug}, so that it could also cover files that are in core, or in theory even plugins.

if ( $cache_key ) {
$decoded_file = wp_cache_get( $cache_key, $cache_group );
if ( false !== $decoded_file ) {
return $decoded_file;
}
}

$decoded_file = wp_json_file_decode( $file_path, array( 'associative' => true ) );
if ( is_array( $decoded_file ) ) {
static::$theme_json_file_cache[ $file_path ] = $decoded_file;
return static::$theme_json_file_cache[ $file_path ];
if ( $cache_key ) {
wp_cache_set( $cache_key, $decoded_file, $cache_group );
}
return $decoded_file;
}
}

Expand Down Expand Up @@ -145,12 +137,22 @@ public static function get_fields_to_translate() {
* @return array Returns the modified $theme_json_structure.
*/
protected static function translate( $theme_json, $domain = 'default' ) {
if ( null === static::$i18n_schema ) {
$i18n_schema = wp_json_file_decode( __DIR__ . '/theme-i18n.json' );
static::$i18n_schema = null === $i18n_schema ? array() : $i18n_schema;
// Include an unmodified $wp_version.
require ABSPATH . WPINC . '/version.php';

$cache_group = 'theme_json_files';
$cache_key = "i18n_schema_{$wp_version}"; // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable

$i18n_schema = wp_cache_get( $cache_key, $cache_group );

if ( false === $i18n_schema ) {
$i18n_schema = wp_json_file_decode( __DIR__ . '/theme-i18n.json' );
$i18n_schema = null === $i18n_schema ? array() : $i18n_schema;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use $i18n_schema = ( null === $i18n_schema ) ? array() : $i18n_schema;? Wrap condition in the brackets. So it can be a good readable format.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krupal-panchal I don't think it's needed for a single condition like this? I'm all for readability, but I personally find parentheses like this only helpful if there's more than one conditions in a ternary operator.

Also worth noting this line is already in WordPress core in exactly the same format, so I'm not sure we should change it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, not an issue. LGTM also.


wp_cache_set( $cache_key, $i18n_schema, $cache_group );
}

return translate_settings_using_i18n_schema( static::$i18n_schema, $theme_json, $domain );
return translate_settings_using_i18n_schema( $i18n_schema, $theme_json, $domain );
}

/**
Expand All @@ -165,15 +167,24 @@ public static function get_core_data() {
return static::$core;
}

$config = static::read_json_file( __DIR__ . '/theme.json' );
// Only use cache when not currently developing for core.
$cache_key = '';
if ( ! wp_is_development_mode( 'core' ) ) {
// Include an unmodified $wp_version.
require ABSPATH . WPINC . '/version.php';

$cache_key = "core_{$wp_version}"; // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable
}

$config = static::read_json_file( __DIR__ . '/theme.json', $cache_key );
$config = static::translate( $config );

/**
* Filters the default data provided by WordPress for global styles & settings.
*
* @since 6.1.0
*
* @param WP_Theme_JSON_Data Class to access and update the underlying data.
* @param WP_Theme_JSON_Data $theme_json Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'wp_theme_json_data_default', new WP_Theme_JSON_Data_Gutenberg( $config, 'default' ) );
$config = $theme_json->get_data();
Expand Down Expand Up @@ -241,14 +252,10 @@ public static function get_theme_data( $deprecated = array(), $options = array()
$options = wp_parse_args( $options, array( 'with_supports' => true ) );

if ( null === static::$theme || ! static::has_same_registered_blocks( 'theme' ) ) {
$wp_theme = wp_get_theme();
$theme_json_file = $wp_theme->get_file_path( 'theme.json' );
if ( is_readable( $theme_json_file ) ) {
$theme_json_data = static::read_json_file( $theme_json_file );
$theme_json_data = static::translate( $theme_json_data, $wp_theme->get( 'TextDomain' ) );
} else {
$theme_json_data = array();
}
$wp_theme = wp_get_theme();

// Read main theme.json (which may also come from the parent theme).
$raw_theme_json_data = static::read_theme_json_data_for_theme( $wp_theme );

/**
* Filters the data provided by the theme for global styles and settings.
Expand All @@ -257,17 +264,15 @@ public static function get_theme_data( $deprecated = array(), $options = array()
*
* @param WP_Theme_JSON_Data Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'wp_theme_json_data_theme', new WP_Theme_JSON_Data_Gutenberg( $theme_json_data, 'theme' ) );
$theme_json = apply_filters( 'wp_theme_json_data_theme', new WP_Theme_JSON_Data_Gutenberg( $raw_theme_json_data, 'theme' ) );
$theme_json_data = $theme_json->get_data();
static::$theme = new WP_Theme_JSON_Gutenberg( $theme_json_data );

if ( $wp_theme->parent() ) {
// Get parent theme.json.
$parent_theme_json_file = $wp_theme->parent()->get_file_path( 'theme.json' );
if ( $theme_json_file !== $parent_theme_json_file && is_readable( $parent_theme_json_file ) ) {
$parent_theme_json_data = static::read_json_file( $parent_theme_json_file );
$parent_theme_json_data = static::translate( $parent_theme_json_data, $wp_theme->parent()->get( 'TextDomain' ) );
$parent_theme = new WP_Theme_JSON_Gutenberg( $parent_theme_json_data );
// Read parent theme.json, and only merge it if successful and different from main theme.json data.
$raw_parent_theme_json_data = static::read_theme_json_data_for_theme( $wp_theme->parent() );
if ( $raw_parent_theme_json_data && $raw_theme_json_data !== $raw_parent_theme_json_data ) {
$parent_theme = new WP_Theme_JSON_Gutenberg( $raw_parent_theme_json_data );

/*
* Merge the child theme.json into the parent theme.json.
Expand Down Expand Up @@ -402,6 +407,45 @@ public static function get_block_data() {
return static::$blocks;
}

/**
* Returns theme.json data for the given theme.
*
* If the theme has a parent theme, the data may also come from the parent theme's theme.json.
*
* Data will be cached for the theme that the theme.json file belongs to.
*
* @since 6.5.0
*
* @param WP_Theme $wp_theme Theme instance.
* @return array Raw array of data read from theme.json, or empty array if not readable.
*/
protected static function read_theme_json_data_for_theme( $wp_theme ) {
$theme_json_file = $wp_theme->get_file_path( 'theme.json' );

if ( ! is_readable( $theme_json_file ) ) {
return array();
}

// If the file found is actually from the parent theme, cache it for the parent theme instead.
if ( $wp_theme->parent() ) {
$parent_theme_json_file = $wp_theme->parent()->get_file_path( 'theme.json' );
if ( $theme_json_file === $parent_theme_json_file ) {
$wp_theme = $wp_theme->parent();
}
}

// Only use cache when not currently developing the theme.
$cache_key = '';
if ( ! wp_is_development_mode( 'theme' ) ) {
$cache_key = "theme_{$wp_theme->stylesheet}_{$wp_theme->version}";
}

$theme_json_data = static::read_json_file( $theme_json_file, $cache_key );
$theme_json_data = static::translate( $theme_json_data, $wp_theme->get( 'TextDomain' ) );

return $theme_json_data;
}

/**
* When given an array, this will remove any keys with the name `//`.
*
Expand Down Expand Up @@ -683,6 +727,8 @@ protected static function get_file_path_from_theme( $file_name, $template = fals
* and `$i18n_schema` variables to reset.
* @since 6.1.0 Added the `$blocks` and `$blocks_cache` variables
* to reset.
* @since 6.5.0 Modified resetting of i18n schema to use cache
* instead of variable.
*/
public static function clean_cached_data() {
static::$core = null;
Expand All @@ -696,7 +742,13 @@ public static function clean_cached_data() {
static::$theme = null;
static::$user = null;
static::$user_custom_post_type_id = null;
static::$i18n_schema = null;

// Include an unmodified $wp_version.
require ABSPATH . WPINC . '/version.php';

$cache_group = 'theme_json_files';
$cache_key = "i18n_schema_{$wp_version}"; // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable
wp_cache_delete( $cache_key, $cache_group );
}

/**
Expand Down
Loading