WP_Theme_JSON: Invalidate blocks metadata cache when needed #3359

ockham · 2022-09-28T18:42:39Z

Tentative fix for 56644. Alternative to #3352.

Props @oandregal @andrewserong @talldan for tracking this down in WordPress/gutenberg#44434.

The essential conclusion from WordPress/gutenberg#44434 is that blocks metadata is cached upon reading. However, since it's read during registration of the Template Part block, metadata is never updated after that block is registered, and thus absent for a whole slew of blocks. OTOH, Global Styles uses a sanitization mechanism that, among other things, verifies that the block that a style is applied to is actually registered. This means that styles are stripped from all blocks that happen to be registered after Template Part.

The solution I'm proposing is to make WP_Theme_JSON::get_blocks_metadata()'s caching a bit smarter, i.e. by checking if there are any blocks that have been registered for which no blocks metadata exists in cache, and to add that missing metadata.

Testing Instructions

Step-by-step reproduction instructions

(based on WordPress/gutenberg#44434, originally from https://core.trac.wordpress.org/ticket/56644).

Go to the site editor.
Insert a button.
Open the global styles sidebar.
Open the blocks section.
Navigate to the button block.
Apply a background color.
Save the global styles.
Verify that the background color persist. On trunk, it would be discarded on Save.

Note that this PR doesn't fix WordPress/gutenberg#44619, which, while closely related, isn't the same issue. (Some of the comments on this PR refer to that issue; I've opted to tackle it separately, see WordPress/gutenberg#44434 (comment).)

Trac ticket: https://core.trac.wordpress.org/ticket/56644

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.

andrewserong

Thanks for working on this one @ockham, this feels much closer to an ideal fix! It largely resolves being able to save global styles in the site editor, but I'm still encountering the issue of the Button block styling not working in TwentyTwentyTwo — so I'm wondering if there's another level of cache clearing that we need to implement to get that working, too. With this PR applied, note that the custom background colour on the Site Title block is working, however the Button blocks only have their default element styling, not the green / zero border radius styling for the button in the theme's theme.json file:

In #3352, I found I needed to clear the resolver cache, too, so I wonder if we need to update the resolver's caching logic in a similar way as you've done here in the Theme JSON class?

ockham · 2022-09-29T12:12:04Z

Ah, good spot, @andrewserong! Turns out I only tested for user modifications (setting the background color of the button block in the Global Styles menu) but forgot to test for theme-supplied styling 😅

Anyway, this part might be harder to fix 😕 I think that the problem actually also goes back to WP_Theme_JSON (via WP_Theme_JSON_Resolver::get_theme_data). In WP_Theme_JSON's constructor, we set $this->theme_json after sanitizing the theme.json contents based on the list of known -- i.e. currently registered -- blocks:

wordpress-develop/src/wp-includes/class-wp-theme-json.php

Lines 512 to 515 in 92a8bd8

    
           $valid_block_names   = array_keys( static::get_blocks_metadata() ); 
        
           $valid_element_names = array_keys( static::ELEMENTS ); 
        
           $theme_json          = static::sanitize( $this->theme_json, $valid_block_names, $valid_element_names ); 
        
           $this->theme_json    = static::maybe_opt_in_into_settings( $theme_json );

$this->theme_json is then read frequently across that file; and it's never updated to take into account that the list of registered blocks might've changed.

ockham · 2022-09-29T13:13:27Z

Inside WP_Theme_JSON_Resolver::get_theme_data, we could probably do something around here:

wordpress-develop/src/wp-includes/class-wp-theme-json-resolver.php

Line 181 in 92a8bd8

if ( null === static::$theme ) {

Akin to the changes I made to WP_Theme_JSON, we'd need to check if more blocks have been registered. Unfortunately, that's probably harder here, since AFAICS, we don't have access to the list of blocks that were registered at the time that the WP_Theme_JSON object was created (ironically, this is partly a consequence of the fix to WP_Theme_JSON::get_blocks_metadata, which would otherwise give us that 😅 ).

ockham · 2022-09-29T20:01:59Z

Yeah, so I haven't found a good way to smartly update the resolver's caching logic.

So I've tentatively pushed b746d24 which I believe fixes the issue -- by removing the caching altogether for the theme-supplied styling and settings 😬 I'm not sure how big the performance impact would be.

I'll be AFK on Friday and Monday. @andrewserong would you mind taking the lead on this one (so we'll have a fix ready for WP 6.1 Beta 3 on Tuesday)? I guess we basically need to decide between this solution and #3352 (which would require some more action on the Gutenberg side, see #3352 (comment)).

Maybe folks familiar with Global Styles can weigh in on both solutions (and this PR's performance impact). cc/ @oandregal @talldan @jorgefilipecosta 😊

andrewserong · 2022-09-29T23:32:05Z

Thanks for all the digging here, @ockham!

I'll be AFK on Friday and Monday. @andrewserong would you mind taking the lead on this one (so we'll have a fix ready for WP 6.1 Beta 3 on Tuesday)?

Yes, I'm happy to dig into this one today and on Monday and try to figure out what would be the best trade-off. It'd be great to get a fix in for Beta 3, whichever direction we do go in. I'll give this PR a test and do some more exploration 👍

andrewserong · 2022-09-30T05:03:18Z

Update: I've looked into this a bit further, and I think we might need to add in a variable to keep track of the number of blocks that have been registered to fix the resolver — or to put it differently, I think we'll achieve more reliable results for this particular caching if we generate a cache key and store it in a static variable.

I've opened up #3373 as an alternative — basically, taking the same general idea as this PR, but attempting to largely preserve the caching if the number of blocks hasn't changed. This appears to be necessary because when testing this PR, it seems that by removing the resolver caching, read_json_file winds up being called 136 times on a single load of a test site running TwentyTwentyTwo theme.

Because we need to keep track of the number of blocks registered, I thought it might make for a good parallel between the two classes for them to share a similar approach to cache busting. It also gives us a function where we can keep the cache key generation logic explicit, to hopefully aid future debugging if need be. There's definitely more we could do to improve the caching behaviour, but so far #3373 seems to resolve the issue while largely preserving the caching as far as I can tell.

Happy for any feedback if folks have time to take a look at it!

src/wp-includes/class-wp-theme-json.php

oandregal · 2022-10-03T11:44:19Z

src/wp-includes/class-wp-theme-json-resolver.php

-				$parent_theme->merge( static::$theme );
-				static::$theme = $parent_theme;
-			}
+		$theme_json_data = static::read_json_file( static::get_file_path_from_theme( 'theme.json' ) );


What if we only cached the result of reading the data from the filesystem/database for every source. That's perhaps the most impactful thing we do here in terms of performance.

The other sources may suffer the same issue (be loaded before blocks are registered).

ockham · 2022-10-04T08:26:54Z

src/wp-includes/class-wp-theme-json.php

+
+		if ( null === static::$blocks_metadata ) {
+			static::$blocks_metadata = array();
+		} else {
+			// Do we have metadata for all currently registered blocks?
+			$blocks = array_diff_key( $blocks, static::$blocks_metadata );
+			if ( count( $blocks ) === 0 ) {
+				return static::$blocks_metadata;
+			}
+		}
+


Adding an explanation I posted over at WordPress/gutenberg#44658 (comment) to clarify how this works:

This mostly leverages PHP's array_diff_key: It looks at the list of currently registered $blocks and compares its keys (which are block names, e.g. core/heading) to those of the list of blocks metadata -- which are also keyed by block name. It then returns the subset of registered $blocks whose keys aren't in static::$blocks_metadata. If that subset is empty, we return the cached blocks metadata, as we can assume that we have metadata for all registered blocks already.

Otherwise, we add metadata -- for that subset of registered blocks only that doesn't have them yet.

Importantly, this means that cache invalidation is not based on the number of blocks, but indeed on which blocks have been registered.

In #3373, @ajlende noted that

Unfortunately we can't rely on only the registered block names because there is at least one instance where we unregister an old version of a block and re-register a new one with the same name within the same function.

#3373 (comment)

tl;dr: I think that in practice, the list of registered block names is a good enough indicator for caching.

The example that's present in Core, register_legacy_post_comments_block, is basically used to replace occurrences of the legacy Post Comments block with its successor (the Comments block), in "legacy" (compatibility) mode.

However, while this logic was relevant to have in the Gutenberg plugin while Core still included the legacy Post Comments block, the latter is no longer the case -- it has been scrapped from Core per #3154. So the unregistering logic will never kick in.

And even if it did, the metadata we're using to (re-)register it are the same that the Post Comments block was using.

This brings us to the bigger question: Can this situation -- a block is registered, then unregistered, and then re-registered under the same name but with different metadata -- happen in principle? I think the chance is fairly low. While we've seen one precedent of block re-registration in Core (where the functionality of one block was absorbed into another, but the original block hat to be retained for back-compat), this is something that isn't going to happen very often; and if it is, we're probably going to use the same metadata to register it.

I think the same would apply to plugins. I don't think there are a lot of plugins that will register a block only to un-register and then re-register it; what's more conceivable is that a different plugin does the un-registration and re-registration bit. (Generally, I'd expect that other plugin to be from the same author -- e.g. something like WooCommerce Blocks which can be used "on top of" WooCommerce. Otherwise, it's debatable if a plugin should really touch another plugin's block namespace that way at all.)

Re-registering blocks from a different author or even Core seems so risky that I don't think we need to provision for that case: That existing block's attributes (and other metadata) basically define the block's "interface", so any replacement would have to keep that intact. This doesn't seem to leave a lot of conceivable use cases.

ockham · 2022-10-04T10:33:03Z

Based on WordPress/gutenberg#44434 (comment) (which is in turn based on the concerns expressed in #3359 (comment)), I'll revert the commit that disables caching for WP_Theme_JSON_Resolver::get_theme_data.

This reverts commit b746d24.

@oandregal

props @oandregal

cbravobernal

It works as expected! LGTM

src/wp-includes/class-wp-theme-json.php

Co-authored-by: Tonya Mork <tonya.mork@automattic.com>

ockham · 2022-10-04T13:00:11Z

Thank you @hellofromtonya! I've applied your suggestions 😊

src/wp-includes/class-wp-theme-json.php

dream-encode

This looks OK. One suggestion.

Co-authored-by: David B <dream-encode@users.noreply.github.com>

dream-encode · 2022-10-04T15:49:09Z

Merged into core in https://core.trac.wordpress.org/changeset/54385.

WP_Theme_JSON: Invalidate blocks metadata cache when needed

514f23e

ockham self-assigned this Sep 28, 2022

ockham added 4 commits September 28, 2022 20:43

Add back block_metadata init

2072c0d

Change variable name and add comment

163b21c

Oops

e9897ff

That's more like it

d7dcc6a

ockham marked this pull request as ready for review September 28, 2022 18:59

This was referenced Sep 28, 2022

[WP 6.1] Block-specific global styles don't seem to save WordPress/gutenberg#44434

Closed

Theme JSON classes: Ensure cache is cleared after Template Part block variations are registered #3352

Closed

andrewserong reviewed Sep 28, 2022

View reviewed changes

cbravobernal mentioned this pull request Sep 29, 2022

Global Style Variations: Changes to JSON file are not applied immediately WordPress/gutenberg#43395

Closed

Caching is overrated :yolo:

b746d24

andrewserong mentioned this pull request Sep 30, 2022

Theme JSON and Resolver: Update block caching logic to clear based on number of registered blocks #3373

Closed

ajlende mentioned this pull request Sep 30, 2022

Run template-part registration after all other blocks #3384

Closed

andrewserong mentioned this pull request Oct 3, 2022

Add registered_block_type action, register clearing Theme JSON cache to fix global styles issue #3392

Closed

oandregal reviewed Oct 3, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved

oandregal reviewed Oct 3, 2022

View reviewed changes

ajlende mentioned this pull request Oct 3, 2022

Avoid calling get_blocks_metadata in WP_Theme_JSON constructor WordPress/gutenberg#44658

Merged

ockham commented Oct 4, 2022

View reviewed changes

ockham added 2 commits October 4, 2022 12:33

Revert "Caching is overrated :yolo:"

8d46f0a

This reverts commit b746d24.

Initialize blocks_metadata as empty array

103bc47

props @oandregal

cbravobernal approved these changes Oct 4, 2022

View reviewed changes

hellofromtonya reviewed Oct 4, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved

hellofromtonya reviewed Oct 4, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved

ockham and others added 2 commits October 4, 2022 14:59

Use empty() instead of count(...) === 0

bdc0923

Co-authored-by: Tonya Mork <tonya.mork@automattic.com>

Avoid "we" in comment

a9787c0

Co-authored-by: Tonya Mork <tonya.mork@automattic.com>

dream-encode reviewed Oct 4, 2022

View reviewed changes

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved

dream-encode approved these changes Oct 4, 2022

View reviewed changes

Fix PHPDoc comment

25b560a

Co-authored-by: David B <dream-encode@users.noreply.github.com>

dream-encode closed this Oct 4, 2022

ockham deleted the fix/invalidate-theme-json-blocks-metadata-cache branch October 4, 2022 15:53

ockham mentioned this pull request Oct 5, 2022

Make Editor and Frontend reflect Block Styles in theme.json #3408

Closed

This was referenced Oct 7, 2022

Performance regression in WP 6.1 for theme.json processing WordPress/gutenberg#44772

Closed

Improve caching for theme.json processing #3418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WP_Theme_JSON: Invalidate blocks metadata cache when needed #3359

WP_Theme_JSON: Invalidate blocks metadata cache when needed #3359

ockham commented Sep 28, 2022 •

edited

Loading

andrewserong left a comment •

edited

Loading

ockham commented Sep 29, 2022

ockham commented Sep 29, 2022

ockham commented Sep 29, 2022 •

edited

Loading

andrewserong commented Sep 29, 2022

andrewserong commented Sep 30, 2022

oandregal Oct 3, 2022

oandregal Oct 3, 2022

ockham Oct 4, 2022

ockham Oct 4, 2022

ockham Oct 4, 2022

ockham commented Oct 4, 2022

cbravobernal left a comment

ockham commented Oct 4, 2022

dream-encode left a comment

dream-encode commented Oct 4, 2022

WP_Theme_JSON: Invalidate blocks metadata cache when needed #3359

WP_Theme_JSON: Invalidate blocks metadata cache when needed #3359

Conversation

ockham commented Sep 28, 2022 • edited Loading

Testing Instructions

Step-by-step reproduction instructions

andrewserong left a comment • edited Loading

Choose a reason for hiding this comment

ockham commented Sep 29, 2022

ockham commented Sep 29, 2022

ockham commented Sep 29, 2022 • edited Loading

andrewserong commented Sep 29, 2022

andrewserong commented Sep 30, 2022

oandregal Oct 3, 2022

Choose a reason for hiding this comment

oandregal Oct 3, 2022

Choose a reason for hiding this comment

ockham Oct 4, 2022

Choose a reason for hiding this comment

ockham Oct 4, 2022

Choose a reason for hiding this comment

ockham Oct 4, 2022

Choose a reason for hiding this comment

ockham commented Oct 4, 2022

cbravobernal left a comment

Choose a reason for hiding this comment

ockham commented Oct 4, 2022

dream-encode left a comment

Choose a reason for hiding this comment

dream-encode commented Oct 4, 2022

ockham commented Sep 28, 2022 •

edited

Loading

andrewserong left a comment •

edited

Loading

ockham commented Sep 29, 2022 •

edited

Loading