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 data in memory #5024

Closed
wants to merge 8 commits into from
Closed

Store merged data in memory #5024

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8f3fbcd
Store merged data in memory
joemcgill Aug 17, 2023
b9cd9c8
Allow cached merged data to be cleaned
joemcgill Aug 17, 2023
d1319a1
Validate block registration for merged data
joemcgill Aug 18, 2023
f985db4
Merge branch 'trunk' into update/57789-store-merged-theme-json
joemcgill Sep 18, 2023
6811e6d
Cache theme supports data for use when caching merged data
joemcgill Sep 18, 2023
b477c3d
Remove unused global var
joemcgill Sep 18, 2023
a5922fe
Don't return early in get_theme_data
joemcgill Sep 18, 2023
3727709
Apply PR feedback
joemcgill Sep 22, 2023
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
40 changes: 40 additions & 0 deletions src/wp-includes/class-wp-theme-json-resolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,19 @@ class WP_Theme_JSON_Resolver {
*/
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 @@ -576,6 +589,25 @@ public static function get_merged_data( $origin = 'custom' ) {
_deprecated_argument( __FUNCTION__, '5.9.0' );
}

if ( ! in_array( $origin, WP_Theme_JSON::VALID_ORIGINS, true ) ) {
$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's the advantage of declaring this map over just using the origin?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. The existing calls to has_same_registered_blocks do not use the origin. It's clear if they do, that'd be a little nice change before this PR (or as part of it).

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. I did this because keys used to store the $blocks_cache values don't match the $origin values used by WP_Theme_JSON. If they did, then this mapping wouldn't be necessary. Not sure if we can make that update in a backwards compatible way, but it would be nice if these were stored using the same keys so it's obvious how one relates to the other.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it's unfortunate that the names don't match, but I also think we shouldn't make a change to that at this point due to backward compatibility risk. If we want to update them, that should be in its own decoupled issue due to the additional considerations.

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

if (
null !== static::$merged[ $origin ]
&& static::has_same_registered_blocks( $cache_map[ $origin ] )
) {
return static::$merged[ $origin ];
}

$result = new WP_Theme_JSON();
$result->merge( static::get_core_data() );
if ( 'default' === $origin ) {
Expand All @@ -597,6 +629,8 @@ public static function get_merged_data( $origin = 'custom' ) {
$result->merge( static::get_user_data() );
$result->set_spacing_sizes();

static::$merged[ $origin ] = $result;
Copy link
Member

Choose a reason for hiding this comment

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

There are early returns above, so I think this needs to be added to all those clauses as well, otherwise only the custom origin will ever have cached values.

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 actually wonder if there's any value in storing anything other than the completed merged data. I'll raise that question as part of the PR to the Gutenberg repo, but this is good feedback!


return $result;
}

Expand Down Expand Up @@ -674,6 +708,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