-
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
Update wp_theme_has_theme_json
to use WP_Object_Cache
#45543
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
function wp_theme_has_theme_json() { | ||
$cache_key = 'wp_theme_has_theme_json'; | ||
$cache_found = false; | ||
$theme_has_support = wp_cache_get( $cache_key, '', false, $cache_found ); |
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.
How well supported is the $found
parameter? I've seen some reports of it not working well and causing issues in hosts.
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 had to check the code reference to see what $found
does. Unfortunately, it's not apparent that the parameter is passed by reference.
I've not seen it used in the core much, but this might have changed.
My personal preference would be to avoid using 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.
It is used in core here.
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 reference, @spacedmonkey.
Do you think we need to use the $found
argument in this 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.
Yeah we do. The default return of wp_cache_get
is false if not found. We are storing a boolean value. So we need to use.
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.
Why don't we just store 0
/ 1
instead? I'm also a bit wary of using the $found
parameter since, while it's part of core, is not at all commonly used to the degree that I'm afraid some cache implementations don't implement it correctly. I think not storing a boolean here and fixing it that way would be a safer call IMO.
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 = wp_cache_get( $cache_key, '', false, $cache_found ); | |
$theme_has_support = wp_cache_get( $cache_key, 'themes' ); |
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 group and remove cache found.
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'm going to store ints. I considered using true/false as strings, though I see how it'll be tempting for our future selves to just convert them to boolean and ignore the rationale for this. I also added a comment to make sure the rationale is documented.
@@ -32,6 +30,8 @@ function wp_theme_has_theme_json( $clear_cache = false ) { | |||
$theme_has_support = is_readable( get_template_directory() . '/theme.json' ); | |||
} | |||
|
|||
wp_cache_set( $cache_key, $theme_has_support ); |
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'm considering setting an expiration period. While cache invalidation should be well managed, this should serve as a last resort in case something goes hairy. The issue is: which would be a good expiration?
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.
Please do not add an expiration here. Let's trust that cache invalidation works. There is no need for a expiration.
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.
+1, we should rely on cache invalidation, not expiration for information like this, which WordPress is in control of tracking.
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_cache_set( $cache_key, $theme_has_support ); | |
wp_cache_set( $cache_key, (int) $theme_has_support, 'themes' ); |
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.
ok, let's try without an expiration.
@@ -41,7 +41,7 @@ function wp_theme_has_theme_json( $clear_cache = false ) { | |||
* Clean theme.json related cached data. | |||
*/ | |||
function wp_theme_clean_theme_json_cached_data() { |
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_clean_theme_json_cached_data
is now called in the following filters: switch_theme
, start_previewing_theme
. Do we need any other?
What if a theme is updated and removes or adds a theme.json
? How can we flush the cache at that point?
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.
Cleaning the cache on delete_theme sounds like a good idea.
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 +1, we should definitely delete the cache when the theme is updated.
If you're referring to adding/modifying theme.json
during development of a theme for example, I think for that we can't properly handle cache, so we may want to wrap the cache usage here in a condition to only use cache if ! WP_DEBUG
.
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.
to only use cache if ! WP_DEBUG
Nice, I'm going to implement this.
I've seen something similar in wp_get_global_stylesheet
(see), though it also includes SCRIPT_DEBUG
and other stuff. I'm not really sure why it was added. It sounds more sensible to just use WP_DEBUG
.
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.
Cleaned the cache upon upgrading the theme at 13510c7
Did not added the same for the deleted_theme
action because users cannot delete the active theme (manually or via the wp-cli).
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 This looks like it's on a solid path to me, I left a few comments with suggestions below.
function wp_theme_has_theme_json() { | ||
$cache_key = 'wp_theme_has_theme_json'; | ||
$cache_found = false; | ||
$theme_has_support = wp_cache_get( $cache_key, '', false, $cache_found ); |
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.
Why don't we just store 0
/ 1
instead? I'm also a bit wary of using the $found
parameter since, while it's part of core, is not at all commonly used to the degree that I'm afraid some cache implementations don't implement it correctly. I think not storing a boolean here and fixing it that way would be a safer call IMO.
@@ -41,7 +41,7 @@ function wp_theme_has_theme_json( $clear_cache = false ) { | |||
* Clean theme.json related cached data. | |||
*/ | |||
function wp_theme_clean_theme_json_cached_data() { |
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 +1, we should definitely delete the cache when the theme is updated.
If you're referring to adding/modifying theme.json
during development of a theme for example, I think for that we can't properly handle cache, so we may want to wrap the cache usage here in a condition to only use cache if ! WP_DEBUG
.
@oandregal Now we are making this change, should we also remove or change the cache from |
835743e
to
de5d387
Compare
if ( ( ! defined( 'WP_DEBUG' ) || ! WP_DEBUG ) && | ||
( 0 === $theme_has_support || 1 === $theme_has_support ) ) { | ||
return (bool) $theme_has_support; | ||
} |
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.
Changing behavour based on debug flags is not a great idea. It can make debugging issues really hard.
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 are different views on how/when to use WP_DEBUG
. I'm going to remove it from this PR to unblock it. It can be revisited later in a separate PR should we need.
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 point of WP_DEBUG
here is that we shouldn't use a cache for data controlled by code when in "development" mode. If we don't add WP_DEBUG
here, this is going to be extremely painful to work with e.g. as a theme developer who is working on their theme.json
and then will never see their changes reflected.
Usually I would agree with what you're saying here about WP_DEBUG
, but there is a difference in how we need to handle caching data from the DB vs caching information that is purely controlled by code.
@oandregal If there's disagreement here, I'm fine to leave this off for now to unblock the PR, but IMO we should re-evaluate this as without any extra flag here this will inevitably lead to problems during development.
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'm fine to remove it from here to unblock this from landing and re-evaluate in a follow-up PR.
In development, it's less likely that a theme changes whether it has a theme.json
. For other public methods (stylesheet, settings) I feel a lot more strongly that we need that check from the beginning.
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.
#45882 brings back this conversation, for anyone interested in following up.
* | ||
* @return boolean | ||
*/ | ||
function wp_theme_has_theme_json( $clear_cache = false ) { | ||
static $theme_has_support = null; | ||
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.
This function should accept a theme as a param. So we can check if none active themes have theme json. Example.
https://github.com/WordPress/wordpress-develop/blob/551c3d6619b7e88742911ca2dd3aed77a71cb0dc/src/wp-includes/rest-api/endpoints/class-wp-rest-themes-controller.php#L368-L374
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 useful, although I'm reluctant to do it in this PR. All the current use cases we have don't need it, so it adds complexity (manage invalidation of stylesheet per theme) without real needs. When/If we need that, we should implement it. It's a good follow-up 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 agree with both of you here, but for now I agree more with @oandregal that this can be added in a follow up 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.
@felixarntz Making this change in the plugin does not make any sense. This code has lived in WordPress since 5.8. Doing this in the plugin is weird and not possible, as it is too hooked into the core code. |
@spacedmonkey The change here is touching code that's already in Gutenberg though, so that's not really the point here. Once it's merged, it'll also be merged into core afterwards, and then we can iterate on it. We shouldn't block this work here just because we can work on it in core as well. The current Gutenberg implementation is flawed as it doesn't even use caching, and that's what this PR is addressing. |
Hey there 👋 just wanted to reference this comment about an issue we have with the mobile endpoint and this PR fixes the problem. |
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
} | ||
|
||
// Has the own theme a theme.json? | ||
$theme_has_support = is_readable( get_stylesheet_directory() . '/theme.json' ); | ||
$theme_has_support = is_readable( get_stylesheet_directory() . '/theme.json' ) ? 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.
Related #45831
Co-authored-by: Miguel Torres <miguel.torres@automattic.com>
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 @oandregal for your work on this (it made much easier my changes in #45679!).
They look good and test well for me. I think it makes sense to add them first to Gutenberg and backport them later to Core since, as @felixarntz noted above, Gutenberg is currently overriding the Core behavior and changes here specifically target those Gutenberg overrides.
In the interest of making progress, I'm going to merge this PR. All the feedback has been addressed: either implemented or punted for potential follow-up PRs. I don't think I've ever merged a PR that was blocked by a reviewer. Life is full of first-times. It's with a heavy heart that I make this decision, and I want to offer some context as to why: I was requested to add object cache to this method, and I did (what this PR does). It took two weeks to get this minimal change ready. It was heavily reviewed and everything was addressed. If anything, this PR is a small first step that help the potential follow-ups to be implemented faster and does not block them in any way. To be really candid, in these two weeks I could have focused on shipping higher impact changes (a reduction of 200ms, a 33% improvement) I shared a month ago. Instead, I decided in good faith to address the feedback first, because that's the right to do. That's the kind of trade-off we need to be mindful of: we need to focus on the right things first. Given there is no any actionable basis for the blocker, the PR has two approvals, and we need to move past this, I'm merging. I also want to share a general note that my DMs are open, and I'll happily join whatever forum people think is best to talk about what how we work and what issues should be our focus, I feel we have some room for improvement. My view is that, to move faster, we need to work together, not against each other and be laser-focused on fixing these high-impact issues, first. |
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com> Co-authored-by: Miguel Torres <miguel.torres@automattic.com> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
add_action( 'start_previewing_theme', 'wp_theme_clean_theme_json_cached_data' ); | ||
add_action( 'switch_theme', 'wp_theme_has_theme_json_clean_cache' ); | ||
add_action( 'start_previewing_theme', 'wp_theme_has_theme_json_clean_cache' ); | ||
add_action( 'upgrader_process_complete', '_wp_theme_has_theme_json_clean_cache_upon_upgrading_active_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.
Follow-up #45881
This PR breaks my site: WordPress version 6.1.1 Error DetailsAn error of type E_ERROR was caused in line 67 of the file /home/customer/www/mysiteURL.org/public_html/wp-content/plugins/gutenberg/lib/compat/wordpress-6.2/get-global-styles-and-settings.php. Error message: Uncaught ArgumentCountError: Too few arguments to function _wp_theme_has_theme_json_clean_cache_upon_upgrading_active_theme(), 1 passed in /home/customer/www/mysiteURL.org/public_html/wp-includes/class-wp-hook.php on line 310 and exactly 2 expected in /home/customer/www/mysiteURL.org/public_html/wp-content/plugins/gutenberg/lib/compat/wordpress-6.2/get-global-styles-and-settings.php:67 The issue is when using 6.1.0+, but disappears if I downgrade to 6.0.3. It's also only an issue when I run this snippet, which has worked for years:
The purpose of this snippet is to publish future-dated posts. Suggestion? Thanks in advance. |
This issue I mentioned above appears to be resolved by #45881. |
This broke dotorg. See discussion on Slack: https://wordpress.slack.com/archives/C02QB8GMM/p1668957208385369 |
I'm unable to reproduce the bug locally, see https://wordpress.slack.com/archives/C02QB8GMM/p1669021968917899 My best guess at the moment is that what wordpress.org experienced is related to this other issue (that may have been made more visible due to the changes here) and that is being fixed by this core patch (it updates how |
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com> Co-authored-by: Miguel Torres <miguel.torres@automattic.com> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
Part of #45171
Follow-up to #45168
What?
This PR updates
wp_theme_has_theme_json
, so the runtime cache isWP_Object_Cache
and not a static variable.Why?
See this conversation.
There are two advantages to using
WP_Object_Cache
:wp_theme_has_theme_json
method)How?
Substitutes the use of the internal static variable by the
WP_Object_Cache
via thewp_cache_*
methods.The cache is cleaned upon switching themes, upgrade active theme, or previewing the theme.
Testing Instructions
Front-end:
theme.json
(e.g.: TwentyTwentyThree). Go to the front-end and verify that the block markup is similar to (no inner div between group & paragraph):theme.json
(e.g.: TwentyTwenty). Go to the front-end and verify that the block markup is similar to (has inner div between group & paragraph):