wp_theme_has_theme_json: add public method to know whether the theme supports theme.json
#3556
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -102,7 +102,7 @@ function wp_get_global_stylesheet( $types = array() ) { | |||||
|
|
||||||
| $tree = WP_Theme_JSON_Resolver::get_merged_data(); | ||||||
|
|
||||||
| $supports_theme_json = wp_theme_has_theme_json(); | ||||||
| if ( empty( $types ) && ! $supports_theme_json ) { | ||||||
| $types = array( 'variables', 'presets', 'base-layout-styles' ); | ||||||
| } elseif ( empty( $types ) ) { | ||||||
|
|
@@ -184,7 +184,7 @@ function wp_get_global_styles_svg_filters() { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| $supports_theme_json = wp_theme_has_theme_json(); | ||||||
|
|
||||||
| $origins = array( 'default', 'theme', 'custom' ); | ||||||
| if ( ! $supports_theme_json ) { | ||||||
|
|
@@ -255,3 +255,70 @@ function ( $item ) { | |||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Whether a theme or its parent have a theme.json file. | ||||||
| * | ||||||
| * @since 6.2.0 | ||||||
| * | ||||||
| * @return bool | ||||||
| */ | ||||||
| function wp_theme_has_theme_json() { | ||||||
|
There was a problem hiding this comment. @oandregal See my feedback in WordPress/gutenberg#45955 (review). I think this function is good like this, however we should also add a method fulfilling the same purpose to LMKWYT. We can also do that in a follow up PR, but would like to hear your thoughts. There was a problem hiding this comment. I have that PR in my queue to re-review, but I've been AFK for a week for personal reasons and have a long backlog :) My TLDR is: I understand having the ability to check for any theme, not just the active one, is something we need. I'm not sure adding that parameter to this function is the way to go. Need to look at that. In any case, that conversation should not block this PR, as it's a potential follow-up we can do if it comes to implement it that way. There was a problem hiding this comment. As noted in the PR WordPress/gutenberg#45955. We need to know on EVERY request if the current AND parent theme support theme json. For that reason, this function is not fit for the requirements. There was a problem hiding this comment. Should this logic live in a method in the wordpress-develop/src/wp-includes/class-wp-theme.php Lines 1486 to 1506 in e27c5a3 wordpress-develop/src/wp-includes/theme.php Lines 4209 to 4218 in e50c65c There was a problem hiding this comment.
Different people have different views on this. @felixarntz mentioned it does not necessarily requires modifying this function. I also shared the implementation can be different. There's agreement on querying themes other than the active theme is needed, though the implementation is not clear yet. Even if that PR was the way to go: it's a future modification that does not affect the API. We should aim to ship work in small pieces to make progress.
Note that there is a Whether we also have a There was a problem hiding this comment. We didn't really discuss this as part of the gutenberg PR, as that is not a gutenberg change, it is a core change. Now we are in core, we need to talk about it again. There was a problem hiding this comment. I disagree @spacedmonkey. Smaller iterations of work can be committed into Core. What do I mean exactly? For this particular PR's scope of work, it can be iteratively committed as separate initiatives such as:
Rather than blocking a backport, let the first get committed with a follow-up to improve its performance. There was a problem hiding this comment. Hmm that's interesting thought #3556 (comment) @felixarntz. I agree. It's common for Core to have a global convenience function that wraps around a public method. I agree:
There was a problem hiding this comment.
See #3604 where I did this There was a problem hiding this comment. @spacedmonkey The implementation from #3604 uses the Normally I would say it makes sense and we don't want to duplicate code, but what I am outlining in #3556 (comment) is a common core pattern already:
We can create the |
||||||
| /* | ||||||
| * By using the 'theme_json' group, this data is marked to be non-persistent across requests. | ||||||
| * See `wp_cache_add_non_persistent_groups` in src/wp-includes/load.php and other places. | ||||||
| * | ||||||
| * The rationale for this is to make sure derived data from theme.json | ||||||
| * is always fresh from the potential modifications done via hooks | ||||||
| * that can use dynamic data (modify the stylesheet depending on some option, | ||||||
| * settings depending on user permissions, etc.). | ||||||
| * See some of the existing hooks to modify theme.json behaviour: | ||||||
| * https://make.wordpress.org/core/2022/10/10/filters-for-theme-json-data/ | ||||||
| * | ||||||
| * A different alternative considered was to invalidate the cache upon certain | ||||||
| * events such as options add/update/delete, user meta, etc. | ||||||
| * It was judged not enough, hence this approach. | ||||||
| * See https://github.com/WordPress/gutenberg/pull/45372 | ||||||
| */ | ||||||
| $cache_group = 'theme_json'; | ||||||
| $cache_key = 'wp_theme_has_theme_json'; | ||||||
| $theme_has_support = wp_cache_get( $cache_key, $cache_group ); | ||||||
|
|
||||||
| /* | ||||||
| * $theme_has_support is stored as an int in the cache. | ||||||
| * | ||||||
| * The reason not to store it as a boolean is to avoid working | ||||||
| * with the $found parameter which apparently had some issues in some implementations | ||||||
| * https://developer.wordpress.org/reference/functions/wp_cache_get/ | ||||||
| * | ||||||
| * Ignore cache when `WP_DEBUG` is enabled, so it doesn't interfere with the theme developers workflow. | ||||||
| */ | ||||||
| if ( ! WP_DEBUG && is_int( $theme_has_support ) ) { | ||||||
felixarntz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| return (bool) $theme_has_support; | ||||||
| } | ||||||
|
|
||||||
| // Does the theme have its own theme.json? | ||||||
| $theme_has_support = is_readable( get_stylesheet_directory() . '/theme.json' ); | ||||||
|
|
||||||
| // Look up the parent if the child does not have a theme.json. | ||||||
| if ( ! $theme_has_support ) { | ||||||
| $theme_has_support = is_readable( get_template_directory() . '/theme.json' ); | ||||||
| } | ||||||
|
Comment on lines
+303
to
+309
There was a problem hiding this comment. Consider WordPress/gutenberg#45831 There was a problem hiding this comment. That could be a future change, indeed. We need to measure the performance impact as well. If/when we do that, the corresponding backport should be filled. How is that a blocker for this PR? There was a problem hiding this comment. I am saying, we are have months to go until the 6.2 release, there is no need to merge this change until ALL the PRs on the gutenberg are resolved. I am in no hurry to merge this, let's get it right instead of hurshing changes into core. There was a problem hiding this comment.
I'm actually trying to do exactly the opposite, the "merge early, merge often" strategy. :) We are merging to WP trunk (6.2-alpha) so no point in delaying it. On the opposite, if there are any bugs the chances to discover them increase if they are also in core trunk, not just released as plugin. Then if something is fixed/patched "upstream" in the plugin, it can be added to trunk too, no pressure. Merging early and having more chances to discover bugs seems a lot better than merging in the last minute, and eventually having to fix/patch bugs during RC :) In that terms hoping to see all changes from the current Gutenberg plugin (14.7.3) in core trunk before the end of the year. There was a problem hiding this comment. I didn't say this could not be merged. Just that is should not be backported to 6.1.2. |
||||||
|
|
||||||
| $theme_has_support = $theme_has_support ? 1 : 0; | ||||||
|
There was a problem hiding this comment.
Suggested change
Might as well cast to There was a problem hiding this comment. I guess it works, though I strongly prefer the backport PR to be exactly the same as the upstream Gutenberg code, so I rather do whatever is in Gutenberg. |
||||||
|
|
||||||
| wp_cache_set( $cache_key, $theme_has_support, $cache_group ); | ||||||
|
|
||||||
| return (bool) $theme_has_support; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Private function to clean the caches under the theme_json group. | ||||||
| * | ||||||
| * @since 6.1.2 | ||||||
felixarntz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| * @access private | ||||||
| */ | ||||||
| function _wp_clean_theme_json_caches() { | ||||||
felixarntz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| wp_cache_delete( 'wp_theme_has_theme_json', 'theme_json' ); | ||||||
|
There was a problem hiding this comment. If #3712 lands first, this line needs to be added to the function. There was a problem hiding this comment. Same question here #3556 (comment) |
||||||
| WP_Theme_JSON_Resolver::clean_cached_data(); | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -753,7 +753,7 @@ function wp_start_object_cache() { | |
| ) | ||
| ); | ||
|
|
||
| wp_cache_add_non_persistent_groups( array( 'counts', 'plugins', 'theme_json' ) ); | ||
|
There was a problem hiding this comment. If #3712 lands first, this would have already been ported over there (same for the other places where the group was added). There was a problem hiding this comment. Same question here #3556 (comment). There was a problem hiding this comment. responded at #3556 (comment) There was a problem hiding this comment. See above, IMO this is good to keep. |
||
| } | ||
|
|
||
| $first_init = false; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| /* | ||
| Theme Name: Block Theme Child with no theme.json | ||
| Theme URI: https://wordpress.org/ | ||
| Description: For testing purposes only. | ||
| Template: block-theme | ||
| Version: 1.0.0 | ||
| Text Domain: block-theme-child-no-theme-json | ||
| */ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| /* | ||
| Theme Name: Default Child Theme with no theme.json | ||
| Theme URI: https://wordpress.org/ | ||
| Description: For testing purposes only. | ||
| Template: default | ||
| Version: 1.0.0 | ||
| Text Domain: default-child-no-theme-json | ||
| */ |
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.
If #3712 lands first, these can be ignored, as they'd have already been ported over there.
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.
Hmm, this change seems out-of-scope for the purpose of this PR. What is the impact of committing this PR without this change and before PR #3712 is committed?
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.
responded at #3556 (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.
IMO keeping this in here is okay. It's not as critical to clean caches for now, given that the cache is non-persistent, but it's still a good practice which we will be able to rely on more in the future. I also think having the
_wp_clean_theme_json_caches()function itself makes sense, since we also introduce thewp_theme_has_theme_json()function, which uses that cache.