This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Fix custom templates with fallback being incorrectly attributed #5447
Conversation
This file contains 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
Category and tags templates can fallback to the generic archive if, e.g., the theme provides one for the latter but not for the former. However, since Gutenberg is not aware of this fallback mechanism, it would incorrectly attribute the custom template to the user instead of the theme. Here we are explicitly setting the `has_theme_file` to make sure Gutenberg knows we do, in fact, have a theme fail (if not what it expects). Fixes #5441
sunyatasattva
added
type: bug
|
Size Change: +137 B (0%) Total Size: 819 kB
ℹ️ View Unchanged
|
opr
suggested changes
Dec 23, 2021
src/Utils/BlockTemplateUtils.php
Outdated
| * @return boolean | ||
| */ | ||
| public static function confirm_theme_file_when_fallback_is_available( $query_result, $template ) { | ||
| $template_with_fallback_idx = null; |
There was a problem hiding this comment.
What is idx? If it means index can we just use the full word instead?
src/Utils/BlockTemplateUtils.php
Outdated
| $template_with_fallback_idx = null; | ||
| $is_duplicate = false; | ||
|
|
||
| array_walk( |
There was a problem hiding this comment.
Any reason why you chose to use this over a foreach? foreach is quicker :)
There was a problem hiding this comment.
No reason but my massive PHP ignorance. I didn't know you could get the index with foreach but it looks like you can! Thanks for pointing that out.
src/Utils/BlockTemplateUtils.php
Outdated
| ); | ||
|
|
||
| if ( $is_duplicate ) { | ||
| if ( is_int( $template_with_fallback_idx ) ) { |
There was a problem hiding this comment.
Could we add a comment here explaining why we need to check is_int? This will stop someone "refactoring" it in future 🤓
src/Utils/BlockTemplateUtils.php
Outdated
| * | ||
| * @return boolean | ||
| */ | ||
| public static function confirm_theme_file_when_fallback_is_available( $query_result, $template ) { |
There was a problem hiding this comment.
Do you think something like set_has_theme_file_if_fallback_is_available would be a slightly better name? At a glance it is hard to tell what this function does imo.
src/Utils/BlockTemplateUtils.php
Outdated
Comment on lines
297
to
305
| if ( | ||
| in_array( $template_slug, $eligible_for_fallbacks, true ) | ||
| && ! self::theme_has_template( $template_slug ) | ||
| && self::theme_has_template( 'archive-product' ) | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Could we just return the result of the evaluation instead of using an if here?
|
@opr Thanks for the feedback, especially for the |
opr
approved these changes
Dec 23, 2021
src/Utils/BlockTemplateUtils.php
Outdated
| * @return boolean | ||
| */ | ||
| public static function set_has_theme_file_if_fallback_is_available( $query_result, $template ) { | ||
| foreach ( $query_result as $i => &$query_result_template ) { |
There was a problem hiding this comment.
Looks much better but $i is not used
Suggested change
| foreach ( $query_result as $i => &$query_result_template ) { | |
| foreach ( $query_result as $query_result_template ) { |
There was a problem hiding this comment.
🤦♀️ I had used the index, but then I realized I didn't need that. I think I still need to pass the query_result_template as reference because I edit it, right? Again, my lackluster PHP knowledge her, but I thought the & was required here.
There was a problem hiding this comment.
Oh right, yeah you do sorry I missed that! I didn't notice the & there.
There was a problem hiding this comment.
At least that's what I understand from the docs:
In order to be able to directly modify array elements within the loop precede
$valuewith&
sunyatasattva
deleted the
fix/5441-wrongly-assigned-template-fallbacks
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Category and tags templates can fallback to the generic archive if, e.g., the theme provides one for the latter but not for the former (#5380). However, since Gutenberg is not aware of this fallback mechanism, it would incorrectly attribute the custom template
to the user instead of the theme.
Here we are explicitly setting the
has_theme_fileto make sure Gutenberg knows we do, in fact, have a theme fail (if not what it expects).Fixes #5441
Testing
Manual Testing
How to test the changes in this Pull Request:
Gutenbergplugin.TT1 Blockstheme.archive-product.html. This is because we are using the themes archive-product.html file for the category and tag templates as they're typically the same.Product Archive,Product CategoryandProduct Tagare added by the theme.Product Categorytemplate and save it.Regression testing
Note: The logic of all these steps is a bit convoluted, so doing some regression tests is advised for the following scenarios:
product-archive.htmlon both the frontend and in the site editor. Please also check these are customizable and render as expected.User Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).
Changelog