Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 supportstheme.json
#3556wp_theme_has_theme_json
: add public method to know whether the theme supportstheme.json
#3556Changes from 14 commits
752ce7d
bb9d154
5b09d4d
3dfbad3
04d72ee
771a347
5d5c69f
16b5d6c
8841842
8604182
901e98a
cbcd7d2
91d6ec6
c4a2037
3a5b3a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 onWP_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
wordpress-develop/src/wp-includes/theme.php
Lines 4209 to 4218 in e50c65c
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.
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
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:
theme.json
is useful for Core.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.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 #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 inwp_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:
get_stylesheet()
does not callWP_Theme::get_stylesheet()
.WP_Theme
).We can create the
WP_Theme
method in a separate PR though, it doesn't have to be the same one.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.
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.
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.
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.
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)
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.