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: Allow caching of different types of theme.json files at different levels of the class #42748

Closed
wants to merge 1 commit into from

Conversation

scruffian
Copy link
Contributor

What?

When a core function calls get_theme_data it saves a version of the the theme data on the parent class (WP_Theme_JSON_Resolver). Then when a Gutenberg function tries to overload theme data by adding new properties (for example fluid typography) the version of the theme.json data from core is used, which filters out these new properties.

This PR adds $theme as a static variable on the WP_Theme_JSON_Resolver_Gutenberg class, so that it has its own version of the cached data, rather than using cores.

Why?

This came up in #42454, which calls get_block_templates which is a core function that calls get_theme_data internally. That was forcing Gutenberg to use the old version of theme.json, so newer features were broken.

How?

We define the static $theme variable on the WP_Theme_JSON_Resolver_Gutenberg class.

Testing Instructions

  • Using trunk try adding fluid typography to your theme.json (settings > typography > fluid: true).
  • Notice that your typography isn't fluid
  • Switch to this branch
  • Notice that you now have fluid typography

@WordPress/block-themers

Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Tested and works. Thanks for the quick debugging and for the fix 🙇

@ramonjd
Copy link
Member

ramonjd commented Jul 28, 2022

Tested and works. Thanks for the quick debugging and for the fix 🙇

Works in manual testing*, but it looks like test_translations_are_applied and test_merges_child_theme_json_into_parent_theme_json php unit tests are failing

I think it's something to do with the call to WP_Theme_JSON_Resolver_Gutenberg::get_theme_data() and the status of null === static::$theme in each test. Maybe it's referencing the core instance (?)

*I tested by trying to apply blockGap to a block in theme.json, e.g.,

	"styles": {
		"blocks": {
			"core/social-links": {
				"spacing": {
					"padding": "100px",
					"blockGap": "100px"
				}
			},
			"core/columns": {
				"spacing": {
					"blockGap": "100px"
				}
			}
		}
	},

Before this change, blockGap did not appear at all.

@ramonjd ramonjd self-requested a review July 28, 2022 03:58
@andrewserong
Copy link
Contributor

Thanks for looking into a fix @scruffian!

I managed to get the unit tests passing for me locally if I add the following to lib/init.php:

add_action( 'switch_theme', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );

In core, switching a theme resets the resolver cache, which is required for these tests to pass (this is the line in core: https://github.com/WordPress/wordpress-develop/blob/b316c8b25fc71920d89ee37ee26609a77b2a305a/src/wp-includes/default-filters.php#L348). So, for the sub-classed version of static::$theme to be cleared, it looks like we need that additional action, too.

I'm wondering if this means we could encounter other edges cases as well, as there's also an action for start_previewing_theme to clear the cache, too (here)...

@andrewserong
Copy link
Contributor

@scruffian based on an idea @talldan came up with, we've opened up an alternative PR in #42756 that updates the cache check instead of adding in a new static variable in the sub-classed version. It appears to still fix the issue, and works around having to add in additional actions to clear the cache.

Thanks so much for working out where the issue was!

@scruffian scruffian closed this Jul 28, 2022
@scruffian scruffian deleted the fix/issue-with-early-theme-json-creation branch July 28, 2022 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants