Skip to content
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

10.5 breaks templating logic and use index.php instead of dedicated template. #31399

Closed
chdenat opened this issue May 1, 2021 · 22 comments · Fixed by #31478
Closed

10.5 breaks templating logic and use index.php instead of dedicated template. #31399

chdenat opened this issue May 1, 2021 · 22 comments · Fixed by #31478
Assignees
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@chdenat
Copy link

chdenat commented May 1, 2021

Description

Using 10.5.3 on some site breaks page rendering.
My home page is binded to a specific page template but with Gutenberg 10.5 WordPres uses index.php template instead.
I'm using understrap child theme. The index.php was really simple, just to display title/excerpt, but of course I never use it.
Installing Gutenberg breaks the look by using index.php.

Step-by-step reproduction instructions

add a customised template to a page : WordPress render it using index.php template but not the required template.
change it to default template : WordPress uses page.php template

Expected behaviour

DIsplay as defined and rendered without gutenberg plugin.
expected

Actual behaviour

Wrong look due to wrong template
actual

WordPress information

  • WordPress version: 5.7.1
  • Gutenberg version: 10.5.3
  • Are all plugins except Gutenberg deactivated? yes
  • Are you using a default theme (e.g. Twenty Twenty-One)? no, child of understrap

Device information

  • Device: Desktop
  • Operating system: W10
  • Browser: Edge
@chdenat chdenat changed the title 10.5 breaks templating loginc and use index.php instead of dedicated template. 10.5 breaks templating logic and use index.php instead of dedicated template. May 1, 2021
@carolinan
Copy link
Contributor

carolinan commented May 3, 2021

Hi
I am not able to reproduce this with the Twenty Twenty theme which has two page templates.
When I assign a template to a page and set that page as the Homepage in the reading settings, it uses the correct templates.

Update 4/5, This may be incorrect, I will do more testing.

What is the name of the template and what is the file slug?
Is it using the parent themes index.php, or did you add an index.php file to https://github.com/understrap/understrap-child ?

@chdenat
Copy link
Author

chdenat commented May 3, 2021

Hi

It used the parent index.php because I did not have any in my child theme (normal behaviour).
Then, as a 1st workaround I put my own index.php (in child theme) to have a rendering similar to my theme and WordPress used it.
But if I use default template, WP uses page.php ...

I did it in DEBUG_MODE on , in order to check any error, but I saw nothing.

PS : I'm not alone ...
https://wordpress.org/support/topic/10-5-2-causes-some-sites-to-break/

@carolinan
Copy link
Contributor

What is the name of the template and what is the file slug?

@carolinan
Copy link
Contributor

I am able to reproduce it with Argent, a theme that was mentioned in the support topic. https://wordpress.org/themes/argent/
In Argent, the custom template I tested is called front-page.php. But this is a reserved name in the template hierarchy, maybe that is why. It needs further testing.

@carolinan carolinan added [Type] Bug An existing feature does not function as intended Needs Testing Needs further testing to be confirmed. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. labels May 3, 2021
@dreamwhisper
Copy link

We are also seeing pages use index.php with our Genesis themes with template names like templates/full-width.php, templates/landing.php & templates/blocks.php.

@carolinan carolinan added the [Type] Regression Related to a regression in the latest release label May 4, 2021
@MaggieCabrera
Copy link
Contributor

MaggieCabrera commented May 4, 2021

Does this still happen for you after this was merged? #31336

@ockham
Copy link
Contributor

ockham commented May 4, 2021

I've found something that's part of the problem: https://github.com/WordPress/gutenberg/blame/trunk/lib/init.php#L181

This code seems to be for testing; for forcing theme support. Testing should have been done via actually using a theme, and registering theme support, but whatever. It's obvious this bit should be removed at least.

I think this was a deliberate addition; there's some discussion about it here, plus some more background provided here.

@ockham
Copy link
Contributor

ockham commented May 4, 2021

I am able to reproduce it with Argent, a theme that was mentioned in the support topic. https://wordpress.org/themes/argent/
In Argent, the custom template I tested is called front-page.php. But this is a reserved name in the template hierarchy, maybe that is why. It needs further testing.

I've been able to repro the issue using these instructions with GB 10.5.3. I'll now try to see if #31123+#31336 fixes it.

Edit: Spoke too soon -- I was looking at the wrong page, i.e. I didn't haven't repro'd with Argent yet.

Edit 2: I've been able to repro with Argent now. Some notes (in case others are also struggling to repro):

  • This is about pages, not posts 😬
  • Open any page (e.g. WP's "Sample Page") in the editor, and set the its page template to "Front Page".

Without GB:

image

With GB 10.5.3 active:

image

@ockham
Copy link
Contributor

ockham commented May 4, 2021

I'm replicating this without a Child Theme, and without Reserved Words, btw, tho the patches seem to be focusing on that.

Thanks! Yeah, I seem to be able to confirm.

Furthermore, it seems like the bug is still present on trunk...

@vdwijngaert
Copy link
Member

I'll try to replicate and see if I can get it fixed, or at least point in the right direction!

@ockham
Copy link
Contributor

ockham commented May 4, 2021

I'm looking into a fix as we speak. I'm pretty sure the issue is in

function gutenberg_override_query_template( $template, $type, array $templates = array() ) {

We need to get the fallback priority of templates that are returned right (which is a bit finicky -- we really need some unit test coverage 😅 ).

@skorasaurus skorasaurus removed the Needs Testing Needs further testing to be confirmed. label May 4, 2021
@vdwijngaert
Copy link
Member

vdwijngaert commented May 4, 2021

Specifically,

When we have

$template = '/var/www/html/wp-content/themes/test/templates/full-width.php'; 
$type = 'page';
$templates = [
    'templates/full-width.php',
    'page-slug.php',
    'page-1.php',
    'page.php',
];

$current_template_slug = basename( $template, '.php' );

Only gives us full-width.php, which is not templates/full-width.php and fails.

This hack seems to fix it:

$current_template_slug = str_replace( [ trailingslashit( get_template_directory() ), '.php' ], [ '', '' ], $template );

Here:

$current_template_slug = basename( $template, '.php' );

Edit: not sure whether to use get_template_directory or get_stylesheet_directory, or both to make it work for child themes as well, need to test that. I'll make a WIP PR, if that's okay.

@ockham
Copy link
Contributor

ockham commented May 4, 2021

Thanks @vdwijngaert and @WraithKenny -- I think we're on the right track here.

I agree that the template resolution algorithm has unfortunately become a bit hard to reason about. The crucial point is probably indeed the way that some functions attempt to strip e.g. .php suffixes and directories (but don't take template subfolders into account).

If we get those right, we can probably strip the child-theme specific code:

// if the theme is a child theme we want to check if a php template exists.
if ( is_child_theme() ) {
$has_php_template = file_exists( get_stylesheet_directory() . '/' . $current_template_slug . '.php' );
$block_template = _gutenberg_get_template_file( 'wp_template', $current_block_template_slug );
$has_block_template = false;
if ( null !== $block_template && wp_get_theme()->get_stylesheet() === $block_template['theme'] ) {
$has_block_template = true;
}
// and that a corresponding block template from the theme and not the parent doesn't exist.
if ( $has_php_template && ! $has_block_template ) {
return $template;
}
}

which I believe is basically duplicating this logic:

if (
! $is_custom_template &&
$current_template_slug !== $current_block_template_slug &&
$current_template_slug === $template_item_slug
) {
return $template;
}

However, let's make sure we don't add any more obscure string parsing and the like to the algorithm 😬 -- ideally, we should have a good understanding of what we're doing (as there's always the chance that our fix for custom templates might affect other scenarios, e.g. pure block themes, or child themes, etc).

I'll follow up with a few more concrete notes in a moment...

@ockham
Copy link
Contributor

ockham commented May 4, 2021

Let's try to state what we know:

If a custom template is assigned to a page, it's always the first entry in the $templates arg (as @WraithKenny mentioned here):

$template = '/var/www/html/wp-content/themes/test/templates/full-width.php'; 
$type = 'page';
$templates = [
    'templates/full-width.php',
    'page-slug.php',
    'page-1.php',
    'page.php',
];

So if we can determine that $templates[ 0 ] is a custom page template (and maybe additionally that the $template that was found indeed matches it), we might be able to exit early (via return $template). I believe that I've seen some functions in WP that should help determine that.

As stated above, we need to make sure if that has any implications for the other scenarios; and specifically, we need to find out the role of this check:

// Is this a custom template?
// This check should be removed when merged in core.
// Instead, wp_templates should be considered valid in locate_template.
$is_custom_template = 0 === strpos( $current_block_template_slug, 'wp-custom-template-' );


I've seen a number of scenarios described in the descriptions of PRs that modified the algorithm, e.g. #29026, #30599, #31123, or #31336. We'd need to make sure that we're not breaking any of these by reading them, and recreating them in our local testing. This is where unit test coverage would come in immensely helpful -- it could do that for us automatically, and would protect us from regressions, as we further refine the algorithm.

@ockham
Copy link
Contributor

ockham commented May 4, 2021

Yes, in trying to reason through the purpose of this code, the best I can grep about it is that the $templates loop is intended as a "source of truth" about the priority, allowing overrides.

The thing that's blocking me is that we already do a lot of the logic of child/parent, subdirectory templates inside of locate_template and related code. I think we are just duplicating too much effort.

Exactly my sentiment 👍

We should instead, in the $templates loop, keep the existing simple break for a block-template match, redo the $is_custom_template to a continue if true, and then simply check if $template_item is present at the end of the $template string, and delete all the other stuff.

Can I get a sanity check on my assumptions

Sounds pretty good to me! @WraithKenny It'd be great if you could file a PR to that effect -- it's getting pretty late here, unfortunately 😅

from someone more familiar with the history?

I did a major refactor to this stuff about a year ago but haven't worked with it recently, so I'm not that familiar with the latest changes. Even back then, it was kinda hard to get it right -- the template hierarchy resolution algorithm is complex as it is, and we're extending it here for a whole new breed of themes. Edge cases tend to fall through the cracks easily. My biggest regret is not having added unit test coverage back then. This issue is renewed motivation to finally do that.

@ockham
Copy link
Contributor

ockham commented May 4, 2021

I've done a simple edit via the Github website UI, because I don't have a Gutenberg repo set up. (I tested via editing a local copy of the downloaded plugin.)

But at least folks can look at the code, and see if what I wrote makes any sense!

Awesome, I'll give it a spin!

@vdwijngaert
Copy link
Member

I was going for a fix for the issue that crept in 10.5. If the code responsible for the issue can be removed completely, as suggested, that's even better!

It's getting late here, too, so I'm not able to look into the full history to make sure nothing breaks, but it might be a good idea to ask @aristath to take a look as well.

@ockham
Copy link
Contributor

ockham commented May 4, 2021

Here's a very basic unit test:

<?php
/**
 * Template_Loader_Test class
 *
 * @package WordPress
 */

/**
 * Tests for the block template loading algorithm.
 */
class Template_Loader_Test extends WP_UnitTestCase {
	private static $post;
	private static $template_part_post;

	public static function wpSetUpBeforeClass() {
		switch_theme( 'tt1-blocks' );
		gutenberg_register_template_post_type();
		gutenberg_register_template_part_post_type();
		gutenberg_register_wp_theme_taxonomy();
		gutenberg_register_wp_template_part_area_taxonomy();

		// Set up template post.
		$args       = array(
			'post_type'    => 'wp_template',
			'post_name'    => 'page',
			'post_title'   => 'Page',
			'post_content' => 'Content',
			'post_excerpt' => 'Description of my template',
			'tax_input'    => array(
				'wp_theme' => array(
					get_stylesheet(),
				),
			),
		);
		self::$post = self::factory()->post->create_and_get( $args );
		wp_set_post_terms( self::$post->ID, get_stylesheet(), 'wp_theme' );
	}

	public static function wpTearDownAfterClass() {
		wp_delete_post( self::$post->ID );
	}

	function test_gutenberg_page_template() {
		$type = 'page';
		$templates = array(
			'page-slug.php',
			'page-1.php',
			'page.php',
		);
		$resolved_template_path = gutenberg_override_query_template( $custom_page_template_path, $type, $templates );
		$this->assertEquals( plugin_dir_path( __DIR__ ) . 'lib/template-canvas.php', $resolved_template_path );
	}

	function test_gutenberg_custom_page_template() {
		$custom_page_template = 'templates/full-width.php';
		$custom_page_template_path = get_stylesheet_directory() . '/' . $custom_page_template; 
		$type = 'page';
		$templates = array(
			$custom_page_template,
			'page-slug.php',
			'page-1.php',
			'page.php',
		);
		$resolved_template_path = gutenberg_override_query_template( $custom_page_template_path, $type, $templates );
		$this->assertEquals( $custom_page_template_path, $resolved_template_path );
	}
}

This fails on current trunk, but passes both for https://github.com/WraithKenny/gutenberg/commit/8a70acd76f2c23fece9658166dc37e04ae71a05e and #31478.

@ockham
Copy link
Contributor

ockham commented May 4, 2021

Let's do the following:

Once that short-term fix is out:

Since it's very late here now, I won't do the release tonight. I'll continue work tomorrow, unless someone from @WordPress/gutenberg-core beats me to it.

@vdwijngaert
Copy link
Member

Let's do the following:

Once that short-term fix is out:

Since it's very late here now, I won't do the release tonight. I'll continue work tomorrow, unless someone from @WordPress/gutenberg-core beats me to it.

Thanks @ockham! Could you create these follow-up issue(s)?

@ockham
Copy link
Contributor

ockham commented May 5, 2021

Thanks @ockham! Could you create these follow-up issue(s)?

I've gone ahead and created a PR draft for the unit tests: #31498.

@chdenat
Copy link
Author

chdenat commented May 5, 2021

Thanks to all of you !!!

ockham added a commit that referenced this issue May 12, 2021

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
The block template resolution algorithm is quite complex, as it has to cover quite a lot of different possible cases resulting from various combinations of
- WP's "old" template hierarchy (including custom page templates, and child themes)
- block templates.

Things become especially complex when there are e.g. "oldschool" PHP templates with higher specificity than the available block templates; or when a child theme has a PHP template with equal specificity as the parent theme's corresponding block template.

For more discussion on this, see #31399. Examples of previous iterations that sought to refine the algorithms' behavior include #29026, #30599, #31123, and #31336. This PR's goal is to eventually cover all of those cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants