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

Store merged Theme JSON data in memory. #54742

Closed
wants to merge 1 commit into from
Closed

Store merged Theme JSON data in memory. #54742

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
81 changes: 78 additions & 3 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 @@ -60,6 +60,14 @@ class WP_Theme_JSON_Resolver_Gutenberg {
*/
protected static $theme = null;

/**
* Container to cache theme support data.
*
* @since n.e.x.t
* @var array
*/
protected static $theme_support_data = null;

/**
* Container for data coming from the user.
*
Expand All @@ -68,6 +76,19 @@ class WP_Theme_JSON_Resolver_Gutenberg {
*/
protected static $user = null;

/**
* Container for merged data.
*
* @since n.e.x.t
* @var WP_Theme_JSON
*/
protected static $merged = array(
'default' => null,
'blocks' => null,
'theme' => null,
'custom' => null,
);

/**
* Stores the ID of the custom post type
* that holds the user data.
Expand Down Expand Up @@ -290,13 +311,30 @@ public static function get_theme_data( $deprecated = array(), $options = array()
return static::$theme;
}

// Save theme supports data for future use.
static::$theme_support_data = self::get_theme_supports_data();

$with_theme_supports = new WP_Theme_JSON_Gutenberg( static::$theme_support_data );
$with_theme_supports->merge( static::$theme );
return $with_theme_supports;
}

/**
* Get merged theme supports data
*
* @since n.e.x.t
*
* @return array Config that adheres to the theme.json schema.
*/
private static function get_theme_supports_data() {
/*
* We want the presets and settings declared in theme.json
* to override the ones declared via theme supports.
* So we take theme supports, transform it to theme.json shape
* and merge the static::$theme upon that.
*/
$theme_support_data = WP_Theme_JSON_Gutenberg::get_from_editor_settings( get_classic_theme_supports_block_editor_settings() );

if ( ! wp_theme_has_theme_json() ) {
if ( ! isset( $theme_support_data['settings']['color'] ) ) {
$theme_support_data['settings']['color'] = array();
Expand Down Expand Up @@ -355,9 +393,8 @@ public static function get_theme_data( $deprecated = array(), $options = array()
}
// END EXPERIMENTAL.
}
$with_theme_supports = new WP_Theme_JSON_Gutenberg( $theme_support_data );
$with_theme_supports->merge( static::$theme );
return $with_theme_supports;

return $theme_support_data;
}

/**
Expand Down Expand Up @@ -597,26 +634,58 @@ public static function get_merged_data( $origin = 'custom' ) {
_deprecated_argument( __FUNCTION__, '5.9.0' );
}

if ( ! in_array( $origin, WP_Theme_JSON_Gutenberg::VALID_ORIGINS, true ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? Doesn't it work the same as it works today?

Copy link
Member

Choose a reason for hiding this comment

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

Technically, I agree it's not needed now more than it was needed before, but it's a good practice to have. We should make sure developers passing invalid values to the function either explicitly fails or "forces" a useful result. Having this here ensures the latter.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a bit of defensive coding to make the rest of the logic more predictable if someone passes an invalid origin value. I added it as a best practice to avoid potential bugs and edge cases.

$origin = 'custom';
}

// Map origins to block cache values.
$cache_map = array(
Copy link
Member

Choose a reason for hiding this comment

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

What if we update the $blocks_cache to have the proper keys (default instead of core, and custom instead of user) so we don't need this?

Copy link
Member

Choose a reason for hiding this comment

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

@oandregal While I agree this would be ideal, I think this should be subject to another separate issue/PR. There may be backward compatibility implications from changing the cache names - probably not, but that would be a fix not really related to what this PR is aiming to achieve.

Copy link
Member Author

Choose a reason for hiding this comment

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

If there's a path to making these consistent, I'm all for it, but was out of scope of what I was trying to show here, so I just worked with what existed.

'default' => 'core',
'blocks' => 'blocks',
'theme' => 'theme',
'custom' => 'user',
);

if (
null !== static::$merged[ $origin ]
&& static::has_same_registered_blocks( $cache_map[ $origin ] )
// Ensure theme supports data is fresh before returning cached data for theme and custom origins.
&& ( ! in_array( $origin, array( 'theme', 'custom' ), true ) || static::get_theme_supports_data() === static::$theme_support_data )
Copy link
Member

Choose a reason for hiding this comment

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

I presume the logic above to convert unknown origins to custom was necessary for this line. Thinking we can do this instead:

// theme and custom origins require access to theme supports,
// so the cached merged data will only be used for those origins if the current supports are equal to the cached ones.
( in_array( $origin, array( 'default', 'blocks' ), true ) || static::get_theme_supports_data() === static::$theme_support_data )

Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of the places where an invalid $origin value would be problematic, but there could be others. I considered writing this as you suggested, but decided it was likely better to explicitly check for the origin values where the theme supports data check is required, rather than the inverse. Both get the same result though.

) {
return static::$merged[ $origin ];
}

$result = new WP_Theme_JSON_Gutenberg();
$result->merge( static::get_core_data() );
if ( 'default' === $origin ) {
$result->set_spacing_sizes();

static::$merged[ $origin ] = $result;

return $result;
}

$result->merge( static::get_block_data() );
if ( 'blocks' === $origin ) {
static::$merged[ $origin ] = $result;

return $result;
}

$result->merge( static::get_theme_data() );
if ( 'theme' === $origin ) {
$result->set_spacing_sizes();

static::$merged[ $origin ] = $result;

return $result;
}

$result->merge( static::get_user_data() );
$result->set_spacing_sizes();

static::$merged[ $origin ] = $result;

return $result;
}

Expand Down Expand Up @@ -696,6 +765,12 @@ public static function clean_cached_data() {
'theme' => array(),
'user' => array(),
);
static::$merged = array(
'default' => null,
'blocks' => null,
'theme' => null,
'custom' => null,
);
static::$theme = null;
static::$user = null;
static::$user_custom_post_type_id = null;
Expand Down