-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 Resolver: Update cache check to also check that the object is an instance of the Gutenberg version #42756
Theme JSON Resolver: Update cache check to also check that the object is an instance of the Gutenberg version #42756
Conversation
… is an instance of the Gutenberg version
@@ -33,7 +33,7 @@ public static function get_theme_data( $deprecated = array(), $settings = array( | |||
_deprecated_argument( __METHOD__, '5.9' ); | |||
} | |||
|
|||
if ( null === static::$theme ) { | |||
if ( null === static::$theme || ! static::$theme instanceof WP_Theme_JSON_Gutenberg ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to add a comment with some instructions to remove the || ! static::$theme instanceof WP_Theme_JSON_Gutenberg
when backporting to core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here: 3673bc8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this by adding block styles to my theme.json (tested in block and classic themes).
E.g.,
"styles": {
"blocks": {
"core/social-links": {
"spacing": {
"padding": "100px",
"blockGap": "100px"
}
},
"core/columns": {
"spacing": {
"blockGap": "100px"
}
}
}
},
The blockGap and other styles are being merged into the theme_json object and output in the CSS as expected.
Smoke tested other theme.json API features such as fluid typography and block supports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff, glad it was a relatively easy fix 😄 .
Maybe in the long run there's a better way to extend the theme json resolver, but this works for now!
Gave it a quick test on BlockBase theme and worked as expected. |
Thanks for the reviews, all! I'll merge this in now, and we can always follow up if we need to tweak the logic further. @Mamaduka this is another PR that would be great for us to get into 13.8 since it's a pretty visible bug. Do you mind cherry picking this one, too? We were also wondering if it'd be worth putting out an updated RC to include it. |
I will cherry-pick both changes and then can release RC2 👍 |
Perfect, thanks so much @Mamaduka! Much appreciated 🙇 |
… is an instance of the Gutenberg version (#42756) * Theme JSON Resolver: Update cache check to also check that the object is an instance of the Gutenberg version * Add comment for when the resolver is backported to core
Cherry-picked the fixes for 13.8. |
@andrewserong, the RC2 is now available https://github.com/WordPress/gutenberg/releases/tag/v13.8.0-rc.2. |
Excellent, thanks again @Mamaduka! |
Same code as core. There's a check for detecting whether the class is an instance of WP_Theme_JSON_Gutenberg that was not ported. This check was introduced to make sure the cache of the core class didn't interfere with the cache of the Gutenberg class, so it's no longer necessary. See #42756
… inheriting per WordPress version (#46750) * Backport WP_Theme_JSON_Resolver from core as it is * Substitute WP_Theme_JSON by WP_Theme_JSON_Gutenberg * Substitute WP_Theme_JSON_Data by WP_Theme_JSON_Data_Gutenberg * Rename WP_Theme_JSON_Resolver to WP_Theme_JSON_Resolver_Base The goal is to use it as base to inherit from, until we are able to remove all the children. * Make WP_Theme_JSON_Resolver_6_1 inherit from WP_Theme_JSON_Resolver_Base * 6.1: remove field already defined in base class * 6.1: remove translate, it is equal to base method * 6.1: remove get_core_data, it does not have base changes * 6.1: remove get_user_data Missed core changes and does not do anything differently from core. * 6.1: remove get_user_data_from_wp_global_styles It misses core changes and does not do anything differently. * 6.2: inherit from WP_Theme_JSON_Resolver_Base instead of 6.1 This makes the WP_Theme_JSON_Resover_6_1 class unused. * 6.1: remove class no longer in use * 6.2: deprecate theme_has_support #45380 * 6.2: substitute WP_Theme_JSON_Resolver::theme_has_support by wp_theme_has_theme_json() #45168 * 6.2: port changes to get_user_data_from_wp_global_styles #46043 * 6.2: update get_merged_data #45969 * 6.2: remove get_user_data Same code as core. There's a check for detecting whether the class is an instance of WP_Theme_JSON_Gutenberg that was not ported. This check was introduced to make sure the cache of the core class didn't interfere with the cache of the Gutenberg class, so it's no longer necessary. See #42756 * experimental: make it inherit from WP_Theme_JSON_Resolver_Base * 6.2: remove class no longer in use * experimental: delete remove_json_comments It's already part of the base class. * experimental: remove get_block_data It's already part of the base class and it didn't have the changes from core. * experimental: appearanceTools Port changes to the base class that were meant to be part of 6.1. See #43337 * experimental: port webfonts (experimental API) see #37140 * experimental: use gutenberg_get_legacy_theme_supports_for_theme_json #46112 * experimental: get_theme_data, all code is in the base class * experimental: remove empty class * Rename WP_Theme_JSON_Resolver_Base to WP_Theme_JSON_Resolver_Gutenberg * Fix lint issue: rename class-wp-theme-json-resolver.php to class-wp-theme-json-resolver-gutenberg.php * Move theme.json to unversioned lib/ * Move theme-i18n.json to unversioned lib/
… inheriting per WordPress version (#46750) * Backport WP_Theme_JSON_Resolver from core as it is * Substitute WP_Theme_JSON by WP_Theme_JSON_Gutenberg * Substitute WP_Theme_JSON_Data by WP_Theme_JSON_Data_Gutenberg * Rename WP_Theme_JSON_Resolver to WP_Theme_JSON_Resolver_Base The goal is to use it as base to inherit from, until we are able to remove all the children. * Make WP_Theme_JSON_Resolver_6_1 inherit from WP_Theme_JSON_Resolver_Base * 6.1: remove field already defined in base class * 6.1: remove translate, it is equal to base method * 6.1: remove get_core_data, it does not have base changes * 6.1: remove get_user_data Missed core changes and does not do anything differently from core. * 6.1: remove get_user_data_from_wp_global_styles It misses core changes and does not do anything differently. * 6.2: inherit from WP_Theme_JSON_Resolver_Base instead of 6.1 This makes the WP_Theme_JSON_Resover_6_1 class unused. * 6.1: remove class no longer in use * 6.2: deprecate theme_has_support #45380 * 6.2: substitute WP_Theme_JSON_Resolver::theme_has_support by wp_theme_has_theme_json() #45168 * 6.2: port changes to get_user_data_from_wp_global_styles #46043 * 6.2: update get_merged_data #45969 * 6.2: remove get_user_data Same code as core. There's a check for detecting whether the class is an instance of WP_Theme_JSON_Gutenberg that was not ported. This check was introduced to make sure the cache of the core class didn't interfere with the cache of the Gutenberg class, so it's no longer necessary. See #42756 * experimental: make it inherit from WP_Theme_JSON_Resolver_Base * 6.2: remove class no longer in use * experimental: delete remove_json_comments It's already part of the base class. * experimental: remove get_block_data It's already part of the base class and it didn't have the changes from core. * experimental: appearanceTools Port changes to the base class that were meant to be part of 6.1. See #43337 * experimental: port webfonts (experimental API) see #37140 * experimental: use gutenberg_get_legacy_theme_supports_for_theme_json #46112 * experimental: get_theme_data, all code is in the base class * experimental: remove empty class * Rename WP_Theme_JSON_Resolver_Base to WP_Theme_JSON_Resolver_Gutenberg * Fix lint issue: rename class-wp-theme-json-resolver.php to class-wp-theme-json-resolver-gutenberg.php * Move theme.json to unversioned lib/ * Move theme-i18n.json to unversioned lib/
What?
Alternative to #42748 to fix the issue of the Theme JSON data being cached from the core version of the resolver class. This issue prevents features that have not yet landed in core, such as fluid typography and blockGap at the theme.json level, from functioning.
Why?
As in #42748 the issue appears to be that by the time the sub-classed Gutenberg version of the Resolver class runs, the core version of the Resolver has already cached the theme data.
How?
Where #42748 attempted to deal with the problem by adding another static
$theme
variable, this PR updates the cache check so that if doesn't just check whether the$theme
variable isnull
, but it also checks whether or not it's an instance of theWP_Theme_JSON_Gutenberg
class, which should help ensure that the cache is invalidated when we expect it to be. Kudos @talldan for the idea, and @scruffian for identifying the original issue.Testing Instructions
In a blocks-based theme, test that experimental features that have not yet landed in core, work as expected. For example fluid typography, and blockGap at the individual block level in
theme.json
. For an example of the latter, update yourtheme.json
file to include the following (ensuring that you're running a theme like TwentyTwentyTwo that setssettings.spacing.blockGap
totrue
or opts-in toappearanceTools
):Add some blocks to a post or page, and publish it. On the front-end of your site, you should see the spacing takes effect.
Screenshots or screencast