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

[andrewserong]

Ensure that block data does not become stale in the Theme JSON and Theme JSON Resolver classes if they are called during block registration. Specifically, attempt to resolve the issue described in: WordPress/gutenberg#44434

This is an alternative to #3352 and #3359. The goal in this PR is to take a similar general approach to #3359, but focus on clearing the cached data when the number of registered blocks changes. This appears to have the same desired result of fresh data once all blocks have been registered, without removing the caching logic altogether.

Scope of changes:

• Add $theme_cache_key and get_new_theme_cache_key to WP_Theme_JSON_Resolver to allow cache busting when the list of registered blocks changes.
• Add $blocks_cache_key and get_new_blocks_cache_key to WP_Theme_JSON, also to allow cache busting when the list of registered blocks changes.

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

Testing this change:

1. Confirm that the issue as raised in [WP 6.1] Block-specific global styles don't seem to save gutenberg#44434 and https://core.trac.wordpress.org/ticket/56644 is resolved. You can do this by:
2. Activate theme TwentyTwentyTwo, and add a couple of buttons to a post. On the site frontend the buttons should use the theme's green colour, and have a border-radius of 0
3. In global styles, go to make changes at the block level to the Site Title block. You should be able to set background color, padding, etc.

This screenshot shows the Site Title block with custom background color and padding styling, and the Buttons block using the theme's styling:

---

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.

[ramonjd]

This is working as advertised for me, thanks @andrewserong !

It seems like a good approach, assuming that the number of blocks alone is enough to determine the validity of any cached theme data.

I guess if it turns out that it's not down the road, get_new_theme_cache_key and others are generic enough to accommodate any changes to the internal cache key logic.

I'll leave to folks with more context to determine the robustness of the approach, but it LGTM!

[ajlende]

assuming that the number of blocks alone is enough to determine the validity of any cached theme data.

I was wondering about the robustness of this too. This works fine in core without any plugins, but the way this would break is if someone unregisters a block and registers a new one after core/template-part has been registered.

I don't expect that someone would unregister and re-register a block with the same name as often, so maybe something like this could work?

hash( 'crc32', implode( array_keys( $blocks ) ) )

[ajlende]

I don't expect that someone would unregister and re-register a block with the same name as often

It looks like we're already potentially doing this in register_legacy_post_comments_block, and it happens after core/template-part is registered with a custom priority of 21.

[ajlende]

If we make sure that core/template-part (which only registers itself and does not unregister anything) is registered last, the count($blocks) heuristic will work. I opened #3384 to see what that would look like.

[ndiego]

@andrewserong do you think this fix might solve WordPress/gutenberg#44619? It seems related somehow to WordPress/gutenberg#44434.

[andrewserong]

@andrewserong do you think this fix might solve WordPress/gutenberg#44619? It seems related somehow to WordPress/gutenberg#44434.

Thanks for the ping @ndiego, yes, it should fix that issue, too, as I believe it's the same underlying problem.

[andrewserong]

If we make sure that core/template-part (which only registers itself and does not unregister anything) is registered last, the count($blocks) heuristic will work. I opened #3384 to see what that would look like.

Thanks for the feedback @ajlende! That's a good point about registration / re-registration of blocks. I think ideally we wouldn't need to depend on the Template Part block being registered in a particular order, but I'm also all in favour of pragmatic fixes to resolve the problem in the shorter-term 🙂

I'll have a play with either pulling in your changes to this PR, or updating the caching logic today and see what I can come up with!

[andrewserong]

Great points about the issues of robustness @ajlende and @ramonjd! I've had a go at opening up (yet another) alternative PR in #3392 that proposes to add a registered_block_type action and use this as the mechanism to flush the cache instead of adding an extra cache key as in this PR. I think something like that might be preferable so that we don't have to fuss about trying to determine whether or not the list of registered blocks has changed.

One concern I have with the idea in #3384 is that altering the order in which blocks are registered also seems to be a bit brittle, in that we then need to be careful about future changes to the block registration order, and it's still possible for a plugin to accidentally break things if something similar to what the Post Template block does, happens in a 3rd party block.

Happy to keep exploring options tomorrow, if anyone has any extra feedback. Thanks so much for digging in, too, @ajlende! 🙇

[oandregal]

Can this result in a case where we end up with the same number of blocks but they're different? Perhaps we could get a hash of the $blocks array instead.

[ockham]

Yeah, that seems to be a concern shared by some folks: #3373 (comment)

[ajlende]

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)

[ockham]

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.

Thank you for pointing that out! I just tried to address that at #3359 (comment).

[andrewserong]

Thanks folks! Now that #3359 has landed I'll close this out.