theme.json: Fix theme_has_support()#30479
Closed
ockham wants to merge 2 commits into
Closed
Conversation
ockham
added
[Feature] Themes
ockham
requested review from
a team,
Addison-Stavlo,
Copons,
creativecoder,
david-szabo97,
jeyip,
mattwiebe,
oandregal and
vindl
|
Size Change: 0 B Total Size: 1.42 MB ℹ️ View Unchanged
|
Member
david-szabo97
left a comment
david-szabo97
left a comment
There was a problem hiding this comment.
This seems a simple change. But it's a pity that we need it though 😄
Maybe we should leave a comment why this is needed?
| */ | ||
| function _gutenberg_add_template_part_area_info( $template_info ) { | ||
| if ( WP_Theme_JSON_Resolver::theme_has_support() ) { | ||
| if ( WP_Theme_JSON_Resolver::theme_has_support( $template_info['slug'] ) ) { |
Member
There was a problem hiding this comment.
The function doesn't accept any arguments.
Contributor
Author
There was a problem hiding this comment.
Thanks! Leftover from a previous iteration. I'll remove it.
ockham
changed the title
Contributor
Author
|
Heads-up, some new findings being discussed over at the issue. tl;dr -- this problem might be specific to unit tests. |
Contributor
Author
Member
|
Actually, I prefer this PR over #30686 The #30686 PR doesn't really solve the problem, it's a workaround to make it work. I think changing the theme programmatically (during runtime) is a legit use case, and to make sure this part of the code returns the correct value we need to make sure it works with that use case. So +1 for this over #30686 |
Contributor
Author
I'm not totally sure that's true. 🤔
That's true, but I think that the values that #30686 sets don't really preclude that; it's still possible for individual unit tests to change the theme (and a number of them actually do). IIUC, the (That said, it's true that #30686 causes a few other unit tests to fail; we'll need to figure out if that's legit or not.) |
Member
|
Have an alternative at #30830 See rationale #30478 (comment) |
Contributor
Author
|
Closing in favor of #30830, which seems like a better solution (and doesn't cause existing unit tests to fail 😬 ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
Fixes #30478. See there for details.
How has this been tested?
Read the explanation in #30478. Then, have a look at #30045, and verify that its unit tests are currently failing.
Check out #30045's branch (
update/wp-env-tt1-blocks-0-4-4), and runIt should fail.
Now cherry-pick the commits from this branch. Restart
wp-env(in order to trigger the upgrade to TT1 Blocks v0.4.5), and run the test again.This time, it should pass.
Maybe @Addison-Stavlo @david-szabo97 et al can come up with a simpler way to demonstrate this -- I'm not sure where (e.g. in the UI) the template part area is currently exposed (and broken by the underlying bug).
TODO
Note that if you run all PHP unit tests (
npm run test-unit-php) after cherry-picking, there's another test failure:I believe that is probably a bug that is exposed by this fix (and that also needs to be addressed).
Types of changes
Bug fix.