-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
wp_theme_has_theme_json
: add public method to know whether the theme supports theme.json
#3556
Conversation
* | ||
* @return boolean | ||
*/ | ||
function wp_theme_has_theme_json( $clear_cache = false ) { |
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.
The fact you have to have a clear cache param to function is bad. This is why the use of static variables should be avoided. Why not just use a global instead?
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 the choices we have are polluting the global namespace vs making cache part of the APIs, none is good in my book.
I'm inclined to avoid global as much as possible for scalability (creating a global for every method that needs a run-time cache is too heavy-handed). Additionally, this pattern is in use in WordPress core (example) and plugins (example). It does not matter that the code uses static or an object cache, that's an implementation detail.
Does this clarify my thinking and unblock this conversation?
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.
No. Boolean based flags are a bad pattern. See PHPMD . The example you shared does not link to an example of where a static variable was used. Here is an example of use of static variable made it a real problem to write unit tests. There are other examples of theme based globals like themes_allowedtags
ortheme_field_defaults
and $_wp_theme_features
. I don't see how this would be different.
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.
BTW, I am also aware that global in general, it is how WordPress works I am not here to fix that.
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.
Jonny, going back to this other thread about using object cache and wp_cache_add_non_persistent_groups
.
We could update these methods to:
wp_theme_has_theme_json()
: no cache parameter. It usesWP_Object_Cache
within andwp_cache_add_non_persistent_groups
to signal data should not be cached across requests.wp_theme_clean_theme_json_cached_data()
. It clears the object cache instead of callingwp_theme_has_theme_json
.
Would that be a good compromise?
I only have one concern with this approach: whether object cache plugins have good support for wp_cache_add_non_persistent_groups
. The docs say they may not, although major plugins (memcached, redis, w3 total cache, litespeed) seem to implement it.
The scene of object cache plugins is out of my expertise. So, as long as others confirm this is a good approach that won't be problematic (data won't be cached across requests), I'm fine trying it.
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.
@oandregal What is the reasoning for not caching this data across requests? Whether the theme (or parent theme) has theme.json
rarely changes, so I'd like to understand why we need to detect this in every request. Based on my understanding, it would make more sense to:
- Use
wp_cache_get()
andwp_cache_set()
here, using a persistent cache group. - Use
wp_cache_delete()
to wipe this cached data whenever the active theme changes or when a theme has been updated.
Long story short, I would agree with your suggestion to use WP_Object_Cache
, wondering though why this should not be persistently cached.
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 agree with @felixarntz. Object caching is the way forward. I put together a POC of using object caching. See this commit. e99fd77
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.
What is the reasoning for not caching this data across requests?
I've seen many issues with caches, so my confidence in how we've done caching so far is low. Concrete examples are: transients for the global styles stylesheet have gone through a few iterations, the last one is that people are asking to disable them because it's not working well. Another example is the cache for user data: it had to be reverted during the WordPress 6.1 RC cycle. And so on.
I want us to succeed. I feel part of it is to not repeat the same mistakes. My suggestion is to work in small steps that focus on solving one thing. By making isolated changes, they can be better tested and are easily reverted without affecting unrelated things that are good on their own (such as migrating a private method to a public one). We gain clarity.
Thinking on how to unblock this conversation. I'm happy to hold on merging this PR and introducing a persistent cache in a Gutenberg PR, test it there, and re-evaluate (either backport it here if it works well or go with a runtime cache if it does not).
Does this sound sensible as a path forward?
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.
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.
@oandregal Big +1 to working in small steps, I reviewed WordPress/gutenberg#45543 and it looks solid to me.
The WP core functions around caching are very reliable and established for more than a decade, so I would question why we couldn't rely on them. Needless to say, cache invalidation is hard so potentially there were previous issues with it, but that doesn't mean we should give up on using caching. One thing we need to also account for here is that caching around code paths like we're doing here is different from the usual WordPress caching around database requests, so I'd recommend wrapping those cache lookups in ! WP_DEBUG
in order to not be a pain to someone who is developing a theme for example and wants changes to be reflected immediately.
Overall I think we're working in the right direction. 👍
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.
The changes in here look good to me, other than the caching approach. Specifically, see my comment in https://github.com/WordPress/wordpress-develop/pull/3556/files#r1013265755.
WordPress/gutenberg#45543 is updating a bit the code, I need to port those changes here once they land in Gutenberg. |
I think we should look at and focus on my PR here - #3604 |
wp_theme_has_theme_json
public methodwp_theme_has_theme_json
: add public method to know whether the theme supports theme.json
I've ported the last changes done in Gutenberg to this PR, so it's up to date and ready to review. Marked the old conversations as done as they no longer reflected the state of this code (nor the conversations over at Gutenberg). Because its relationship with this other PR (both use the cc @felixarntz @spacedmonkey @hellofromtonya @azaozz @anton-vlasenko @costdev |
src/wp-includes/default-filters.php
Outdated
@@ -346,8 +346,8 @@ | |||
add_action( 'init', '_register_core_block_patterns_and_categories' ); | |||
add_action( 'init', 'check_theme_switched', 99 ); | |||
add_action( 'init', array( 'WP_Block_Supports', 'init' ), 22 ); | |||
add_action( 'switch_theme', array( 'WP_Theme_JSON_Resolver', 'clean_cached_data' ) ); | |||
add_action( 'start_previewing_theme', array( 'WP_Theme_JSON_Resolver', 'clean_cached_data' ) ); | |||
add_action( 'switch_theme', '_wp_clean_theme_json_caches' ); |
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 the wp_theme_has_theme_json()
function, which uses that cache.
* @access private | ||
*/ | ||
function _wp_clean_theme_json_caches() { | ||
wp_cache_delete( 'wp_theme_has_theme_json', '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, this line needs to be added to the function.
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.
Same question here #3556 (comment)
@@ -753,7 +753,7 @@ function wp_start_object_cache() { | |||
) | |||
); | |||
|
|||
wp_cache_add_non_persistent_groups( array( 'counts', 'plugins' ) ); | |||
wp_cache_add_non_persistent_groups( array( 'counts', 'plugins', '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, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here #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.
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.
See above, IMO this is good to keep.
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 for handling this PR @oandregal! I've left some thoughts below.
$theme_has_support = is_readable( get_template_directory() . '/theme.json' ); | ||
} | ||
|
||
$theme_has_support = $theme_has_support ? 1 : 0; |
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.
$theme_has_support = $theme_has_support ? 1 : 0; | |
$theme_has_support = (int) $theme_has_support; |
Might as well cast to int
here right?
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 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.
@costdev Thanks for the suggestion to use a data provider in the tests, they're much clearer now! All your feedback was addressed. |
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 for the updates @oandregal! LGTM 👍
* | ||
* @return bool | ||
*/ | ||
function wp_theme_has_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.
@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 WP_Theme
. For that method, performance is not as much of a concern, so I think it's perfectly fine not having caching in there (as other methods on WP_Theme
don't have such caching either, plus the common use-case we're optimizing for is the current theme, which this function is for).
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this logic live in a method in the WP_Theme
class? See is_block_theme as a example.
wordpress-develop/src/wp-includes/class-wp-theme.php
Lines 1486 to 1506 in e27c5a3
/** | |
* Returns whether this theme is a block-based theme or not. | |
* | |
* @since 5.9.0 | |
* | |
* @return bool | |
*/ | |
public function is_block_theme() { | |
$paths_to_index_block_template = array( | |
$this->get_file_path( '/block-templates/index.html' ), | |
$this->get_file_path( '/templates/index.html' ), | |
); | |
foreach ( $paths_to_index_block_template as $path_to_index_block_template ) { | |
if ( is_file( $path_to_index_block_template ) && is_readable( $path_to_index_block_template ) ) { | |
return true; | |
} | |
} | |
return false; | |
} |
wordpress-develop/src/wp-includes/theme.php
Lines 4209 to 4218 in e50c65c
/** | |
* Returns whether the active theme is a block-based theme or not. | |
* | |
* @since 5.9.0 | |
* | |
* @return boolean Whether the active theme is a block-based theme or not. | |
*/ | |
function wp_is_block_theme() { | |
return wp_get_theme()->is_block_theme(); | |
} |
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.
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.
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.
Should this logic live in a method in the WP_Theme class? See is_block_theme as a example.
Note that there is a wp_is_block_theme
as well, which is the main API for consumers, like this functions aims to be.
Whether we also have a WP_Theme->has_theme_json
is something to talk about. This was brought up in the conversations we had in Gutenberg already. There was no clarity about value, so the decision was to ship the pieces that are ready and keep iterating on the parts that needed more thought.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- Initiative 1: Add functionality to provide a global way to identify if a theme supports
theme.json
is useful for Core. - Initiative 2: Improve its performance, if needed.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
WP_Theme::has_theme_json()
makes sense.- and can be added in a separate PR and commit, ie for smaller scopes of work.
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.
WP_Theme::has_theme_json() makes sense.
See #3604 where I did this
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.
@spacedmonkey The implementation from #3604 uses the WP_Theme
method in wp_theme_has_theme_json()
function though, which is different from what @hellofromtonya and I were referring to further above though.
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:
- For example,
get_stylesheet()
does not callWP_Theme::get_stylesheet()
. - Instead, it implements the behavior in a simpler and less expensive way (without relying on
WP_Theme
). - That pattern I think we should follow here as well.
We can create the WP_Theme
method in a separate PR though, it doesn't have to be the same one.
Rebased from latest |
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.
LGTM!
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.
Blocking this commit until we resolve WordPress/gutenberg#45955.
This PR changes the cache invalidation.
One PR is a backport of a function introduced and validated in Gutenberg. The other is a potential improvement still in debate. Both rely on |
// 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' ); | ||
} |
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.
Consider WordPress/gutenberg#45831
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are have months to go until the 6.2 release, there is no need to merge this change until...
...
I am in no hurry to merge this...
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't say this could not be merged. Just that is should not be backported to 6.1.2.
I'm confused why this backport PR includes caching changes as these seem out-of-scope for this initiative. Adding Does adding |
That's a great question. The more important thing for me is to have this method available. Not making performance worse is also something I tried to keep in mind, that's why I thought it should have some cache, as the older one also had. However! I also want to provide a more nuanced answer based on data. I ran some numbers and this is what I found (see raw data extracted from the cachegrind files linked below):
When looking at the numbers, you can say that using static variables is faster, and the However, that's a partial picture. This is the other part of the picture:
All things considered, even the fact that we're talking 1ms here while there are PRs such as #3789 that reduce +350ms, I think the current approach is the more future-proof. With what I've learned, I wouldn't mind removing the cache altogether if that is preferable for core folks and provided it helps to ship this faster: it is no big impact either way. I wouldn't recommend going back to caching using static variables in the method, as it compromises the API. |
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.
@oandregal Overall the PR looks good to me. We can open a follow up PR to bring a similar method to WP_Theme
, for parity and to allow for e.g. REST endpoints to use it to cover also other themes than the current one.
I have a few small things though that I think need to be adjusted, see comments below.
* | ||
* @return bool | ||
*/ | ||
function wp_theme_has_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.
@spacedmonkey The implementation from #3604 uses the WP_Theme
method in wp_theme_has_theme_json()
function though, which is different from what @hellofromtonya and I were referring to further above though.
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:
- For example,
get_stylesheet()
does not callWP_Theme::get_stylesheet()
. - Instead, it implements the behavior in a simpler and less expensive way (without relying on
WP_Theme
). - That pattern I think we should follow here as well.
We can create the WP_Theme
method in a separate PR though, it doesn't have to be the same one.
src/wp-includes/default-filters.php
Outdated
@@ -346,8 +346,8 @@ | |||
add_action( 'init', '_register_core_block_patterns_and_categories' ); | |||
add_action( 'init', 'check_theme_switched', 99 ); | |||
add_action( 'init', array( 'WP_Block_Supports', 'init' ), 22 ); | |||
add_action( 'switch_theme', array( 'WP_Theme_JSON_Resolver', 'clean_cached_data' ) ); | |||
add_action( 'start_previewing_theme', array( 'WP_Theme_JSON_Resolver', 'clean_cached_data' ) ); | |||
add_action( 'switch_theme', '_wp_clean_theme_json_caches' ); |
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 the wp_theme_has_theme_json()
function, which uses that cache.
@@ -753,7 +753,7 @@ function wp_start_object_cache() { | |||
) | |||
); | |||
|
|||
wp_cache_add_non_persistent_groups( array( 'counts', 'plugins' ) ); | |||
wp_cache_add_non_persistent_groups( array( 'counts', 'plugins', '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.
See above, IMO this is good to keep.
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.
LGTM 👍
Everyone reviewing this PR should consider this alternative #3604. This has number of key benefits.
|
Which Gutenberg PR(s) is/are covered by this? |
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.
@hellofromtonya Approved from my end too. My only nit-pick would be in https://github.com/WordPress/wordpress-develop/pull/3556/files#r1073175993, I think the cache cleanup function should use the suffix _cache
for its name instead of _caches
, for parity with other core function names.
Thank you everyone. I'm preparing the commit. |
Committed via changeset https://core.trac.wordpress.org/changeset/55086. |
Trac ticket https://core.trac.wordpress.org/ticket/56975
Related Gutenberg PRs:
wp_theme_has_theme_json
as a public API to know whether a theme has atheme.json
gutenberg#45168WP_Theme_JSON_Resolver:theme_has_support()
gutenberg#45380theme.json
object caches non persistent gutenberg#46150wp_initial_constants()
are defined gutenberg#45979Lnking to Gutenberg's 6.2 backport tracking ticket.
What
Adds a new public method
wp_theme_has_theme_json
that checks whether a theme has atheme.json
file.Why
Part of WordPress/gutenberg#45171
This offers consumers a public API so they don't have to rely on private code. Additionally, it uses the object cache under the
theme_json
group to maintain this data, consolidating with the changes done for settings (core PR) and settings (core PR soon, see Gutenberg PR instead).How
Creates the new public method, substitutes usage, and deprecates the old one.
Test