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

Add object caching for WP_Theme_JSON::compute_style_properties #5567

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

Add object caching for WP_Theme_JSON::compute_style_properties #5567

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
85e9170
Add object caching for `WP_Theme_JSON::compute_style_properties`
pereirinha Oct 24, 2023
7450d26
The values of the args are only useful for the cache key
pereirinha Oct 24, 2023
c9b36ea
Deal with cache before updating `$properties`
pereirinha Oct 24, 2023
dd8c83e
Only use cache if not in dev mode for theme. Ensure we do not process…
pereirinha Nov 3, 2023
7d97433
Address PHPCS warning. It really does not matter if the value of `$pr…
pereirinha Nov 3, 2023
bd6f08c
Merge branch 'WordPress:trunk' into add/object-cache-compute-style-pr…
pereirinha Jan 4, 2024
2377348
Remove unecessary dev mode check for cache
pereirinha Jan 4, 2024
d707a79
Add the global settings to the cache key recipe, so that when it chan…
pereirinha 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
11 changes: 11 additions & 0 deletions src/wp-includes/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1934,6 +1934,15 @@ protected static function compute_style_properties( $styles, $settings = array()
return $declarations;
}

// The global settings can include dynamic data related to typography. We need evaluate it so that the cache is invalidated when it changes.
$args = array( func_get_args(), wp_get_global_settings() ); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed
Copy link
Member

@oandregal oandregal Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The way the theme.json code is organized is as follows:

  1. WP_Theme_JSON: private class that deals with a theme.json structure. It doesn't depend on anything else.
  2. WP_Theme_JSON_Resolver: private class that pulls data (theme.json files, theme.json from database) and merges it together appropriately (get_merged_data). It depends on the WP_Theme_JSON class.
  3. Public API: functions such as wp_get_global_settings, wp_get_global_styles, etc. They use and depend on the two previous private classes.

By using a public function such as wp_get_global_settings in the WP_Theme_JSON class, this code is creating a circular dependency. This code is saying: when processing any theme.json structure, read all existing theme.json (core, blocks, theme, user) first.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the feedback @oandregal. This makes perfect sense. I think we would either need to add the caching to the public function that relies on WP_Theme_JSON::compute_style_properties or find a different approach to invalidation within WP_Theme_JSON.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback @oandregal. I'll have a closer look at this

$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );
Comment on lines +1938 to +1939
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion/nitpick: rename $args to $cach_args for readability.

Suggested change
$args = array( func_get_args(), wp_get_global_settings() ); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed
$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $args ) );
$cache_args = array( func_get_args(), wp_get_global_settings() ); // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed
$cache_key = 'compute_style_properties_' . md5( wp_json_encode( $cache_args ) );

$cache = wp_cache_get( $cache_key, 'wp-styles' );
Copy link
Member

Choose a reason for hiding this comment

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

Rather than introducing a new cache group, I think this should be a part of the existing theme_json group. Would need to change it below where this cache value is set, as well.

Suggested change
$cache = wp_cache_get( $cache_key, 'wp-styles' );
$cache = wp_cache_get( $cache_key, 'theme_json' );


if ( $cache ) {
return $cache;
}

$root_variable_duplicates = array();

foreach ( $properties as $css_property => $value_path ) {
Expand Down Expand Up @@ -2000,6 +2009,8 @@ protected static function compute_style_properties( $styles, $settings = array()
}
}

wp_cache_set( $cache_key, $declarations, 'wp-styles', DAY_IN_SECONDS );
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure about the best expiration time here. If there is a side effect that we've not accounted for in the cache key value, showing stale styles for a full day without flushing the cache is pretty long. Maybe limiting this to once an hour for now would be safer?

Suggested change
wp_cache_set( $cache_key, $declarations, 'wp-styles', DAY_IN_SECONDS );
wp_cache_set( $cache_key, $declarations, 'theme_json', HOUR_IN_SECONDS );

Copy link
Member

Choose a reason for hiding this comment

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

Theme json is a none presient cache, so there is no need for an expiry here.


return $declarations;
}

Expand Down
Loading