Templates: Search for old template names in the parent theme too #36910

scruffian · 2021-11-26T12:18:01Z

Description

In #36647 we added a fallback for themes that don't have templates in the new directory structure. However the checks only looked in the child theme, not the parent. This adds the same check to the parent theme, so that templates in child themes continue to load as before

How has this been tested?

This is quite hard to test as most child themes specify their own index.html.
This PR is an example of theme that doesn't: Automattic/themes#5094

Screenshots

Before:

After:

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

youknowriad · 2021-11-26T12:27:33Z

lib/compat/wordpress-5.9/block-template-utils.php

+		$parent_root_dir   = get_theme_root( $parent_theme_name );
+		$parent_theme_dir  = "$parent_root_dir/$parent_theme_name";
+
+		if ( is_readable( $theme_dir . '/block-templates/index.html' ) || is_readable( $parent_theme_dir . '/block-templates/index.html' ) ) {


The function here is supposed to be called with a theme stylesheet, so there shouldn't be a need for this added check. Maybe there's an issue elsewhere where we call this and we're not calling it with the parent theme as well?

Forget the first comment, I understand the issue better now and I have separate thoughts :)

What happens if the child theme has been migrated but not the parent, it seems this PR will break that behavior.

Maybe the fix is to actually check the presence of the folder and not the index file.

Yeah that would work I think, although I guess its possible that a child theme could not have the folder at all and just supply a theme.json.

If the child has been migrated before the parent then I would expect things to break to be honest!

Yeah that would work I think, although I guess its possible that a child theme could not have the folder at all and just supply a theme.json.

In that case, it doesn't matter since this function is about returning the folders.

Good point :)

youknowriad · 2021-12-09T08:40:39Z

lib/compat/wordpress-5.9/block-template-utils.php

+		$parent_root_dir   = get_theme_root( $parent_theme_name );
+		$parent_theme_dir  = "$parent_root_dir/$parent_theme_name";
+
+		if ( is_readable( $theme_dir . '/block-templates/' ) || is_readable( $parent_theme_dir . '/block-templates/' ) ) {


Can't we just remove || is_readable( $parent_theme_dir . '/block-templates/' ), since we're calling the function for both themes (parent and child), this should work right?

I don't think so. If we did that then we'd be back to the changes that are already in trunk, since the other lines don't actually do anything except assign variables.

I thought the issue was about themes that are child themes, override some templates but not "index.html" so it's different than trunk? Are there any other use-cases here?

They might not override any templates at all.

They might not override any templates at all.

In that case that function is useless and doesn't matter what it returns.

I don't think so. Child themes can simply provide a different theme.json file and no other changes.

In that case it doesn't matter what this function returns.

If I create a child theme with no templates or template parts I see this message:

Indeed, I think we had an issue with the template part block's code where the child theme was not handled properly and it should be fixed in 004cba5

The root issue is that get_theme_file_path function tries to fallback to the parent theme automatically but it doesn't account with the fact that the path could change slightly between parent and child themes (folder names)

Feel free to ship this if the fix in that commit is good enough for you.

Looks good thanks

) * Templates: Search for old template names in the parent theme too * check for the directory rather than the file * Fix template part file lookup Co-authored-by: Riad Benguella <benguella@gmail.com>

scruffian requested a review from youknowriad November 26, 2021 12:18

scruffian requested review from spacedmonkey and TimothyBJacobs as code owners November 26, 2021 12:18

scruffian self-assigned this Nov 26, 2021

youknowriad reviewed Nov 26, 2021

View reviewed changes

scruffian mentioned this pull request Dec 8, 2021

TT2: Add color variations Automattic/themes#5163

Merged

scruffian force-pushed the fix/templates branch from f5cf1d2 to 91bff04 Compare December 8, 2021 16:30

youknowriad reviewed Dec 9, 2021

View reviewed changes

scruffian force-pushed the fix/templates branch from 71852ba to 84ef2a2 Compare December 10, 2021 14:22

scruffian and others added 3 commits December 13, 2021 13:45

Templates: Search for old template names in the parent theme too

5d3869f

check for the directory rather than the file

12b017b

Fix template part file lookup

004cba5

youknowriad force-pushed the fix/templates branch from 84ef2a2 to 004cba5 Compare December 13, 2021 12:46

scruffian requested a review from ajitbohra as a code owner December 13, 2021 12:46

youknowriad approved these changes Dec 13, 2021

View reviewed changes

youknowriad added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Dec 13, 2021

scruffian merged commit 7e121d9 into trunk Dec 13, 2021

scruffian deleted the fix/templates branch December 13, 2021 13:54

github-actions bot added this to the Gutenberg 12.2 milestone Dec 13, 2021

noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 14, 2021

noisysocks added a commit to noisysocks/wordpress-develop that referenced this pull request Dec 14, 2021

Changes from WordPress/gutenberg#36910

35971eb

noisysocks mentioned this pull request Dec 14, 2021

Update @wordpress packages WordPress/wordpress-develop#2048

Closed

MaggieCabrera mentioned this pull request Dec 14, 2021

Template part has been deleted or is unavailable: header WordPress/create-block-theme#27

Closed

Mamaduka mentioned this pull request Jun 19, 2023

Improve logic in render_block_core_template_part. #50636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Templates: Search for old template names in the parent theme too #36910

Templates: Search for old template names in the parent theme too #36910

scruffian commented Nov 26, 2021

youknowriad Nov 26, 2021 •

edited

Loading

youknowriad Nov 26, 2021

youknowriad Nov 26, 2021

scruffian Nov 26, 2021

youknowriad Nov 26, 2021

scruffian Nov 26, 2021

youknowriad Dec 9, 2021

scruffian Dec 10, 2021

youknowriad Dec 10, 2021

pbking Dec 10, 2021

youknowriad Dec 10, 2021

youknowriad Dec 13, 2021

scruffian Dec 13, 2021

youknowriad Dec 13, 2021

youknowriad Dec 13, 2021

scruffian Dec 13, 2021

Templates: Search for old template names in the parent theme too #36910

Templates: Search for old template names in the parent theme too #36910

Conversation

scruffian commented Nov 26, 2021

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

youknowriad Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Nov 26, 2021 •

edited

Loading