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.

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

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

Conversation

andrewserong
Copy link
Contributor

This is an attempt to resolve the issue where global styles for static blocks are not being output in the WordPress 6.1 beta. There is discussion behind why this issue occurred over in WordPress/gutenberg#44434 (comment). The short version is that the Template Part block calls get_block_templates, a public function which has a side-effect of caching the current list of registered blocks. The Template Part block calls this at registration time, so unless we clear the cache after calling it, the Theme JSON classes will not recognise subsequently registered blocks.

The explored fix here is to add a mechanism to also clear the static cache in WP_Theme_JSON and clear the cache for both this and WP_Theme_JSON_Resolver once the Template Part block has concluded getting the information it requires to generate the list of variations.

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

See also related Github issue: WordPress/gutenberg#44434

How to test

A simple way to test whether this fix works, is to add a few Button blocks to a post in TwentyTwentyTwo theme. See the below before and after and note that in the after screenshot, the theme's theme.json styles for the Button block are correctly output to the site frontend.

A note on this fix

A better fix might to come up with a more generalised solution for this, or to update how get_block_templates works to ensure data isn't cached? The goal with this PR is to try to find a pragmatic but good enough short-term fix. If this looks viable, I'm happy to have a go at adding tests, too — though, I'm not 100% sure how to reliably test for it, so would be very happy for any feedback.

Before After
image image

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
Copy link
Contributor Author

Update: it looks like the E2E test is failing because I'm attempting to update version controlled files that exist in Gutenberg (template-part.php). Given that the Gutenberg version would likely call WP_Theme_JSON_Gutenberg instead of WP_Theme_JSON, if this fix winds up being appropriate, we might need to ignore that failure, and separately update the Gutenberg version of the file?

@cbravobernal
Copy link
Contributor

Nice job @andrewserong!
It works as expected.

  • The Gutenberg plugin is not installed. No other plugins installed on the site.
  • WP Version: 6.1-beta2-54337-src
  • TT3 Theme
  • PHP 7.4.30
fix_styles_working.mov

I will take a look at the E2E test in the Gutenberg repo.

@ockham
Copy link
Contributor

ockham commented Sep 28, 2022

Update: it looks like the E2E test is failing because I'm attempting to update version controlled files that exist in Gutenberg (template-part.php).

So we will need to apply the fix to the Template Part block in GB, as dynamic blocks' code is automatically synced to Core -- so any changes here will be overridden.

Given that the Gutenberg version would likely call WP_Theme_JSON_Gutenberg instead of WP_Theme_JSON, if this fix winds up being appropriate, we might need to ignore that failure, and separately update the Gutenberg version of the file?

AFAIK, generally speaking, block code isn't supposed to call a Static_Class::member, but instead some kind of wp_wrapper_function. (We have some logic to replace the wp_ prefix by gutenberg_ during our build process in GB). We have some precedent for this, see e.g. WordPress/gutenberg#44099 and #3244.

@ockham
Copy link
Contributor

ockham commented Sep 28, 2022

I've filed an alternative fix here: #3359 😊

@andrewserong
Copy link
Contributor Author

Closing as other solutions are now in the process of being merged / and or validated. Thanks for all the feedback and collaboration!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants