Make WordPress Core

Opened 21 months ago

Last modified 17 months ago

#56908 new defect (bug)

The result of locate_block_template function might be wrong

Reported by: arthur791004's profile arthur791004 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.8
Component: Themes Keywords: has-patch needs-testing-info
Focuses: Cc:

Description

Referring to locate_block_template, if the $template found by locate_template is given, this function will call array_slice to shorten the list of candidate templates. However, the array_search might not find a located template from candidate templates, and it leads to the return value, $index, becoming false. Therefore, when we run the following codes, we will always get the incorrect result.

$templates = array_slice( $templates, 0, $index + 1 );

Why we might not be able to find the located template from candidate templates?

The reason is the locate_template function tries to use STYLESHEETPATH and TEMPLATEPATH to make the path and those two are defined by wp_templating_constants but the locate_block_template function uses get_stylesheet_directory and get_template_directory to make the relative template path. As we're able to hook the returned value of both get_stylesheet_directory and get_template_directory, the result might be different from STYLESHEETPATH and TEMPLATEPATH respectively if the developer adds the hook after the wp_templating_constants calls.

Possible Solutions

One way is to check the $index before doing array_slice as followed to ensure the array_slice works expected.

+ if ( false !== $index ) {
    $templates = array_slice( $templates, 0, $index + 1 );
+ }

The other way is to avoid using different variables. We have to use "STYLESHEETPATH and TEMPLATEPATH" or "get_stylesheet_directory and get_template_directory" in both places

For example

function locate_template( $template_names, $load = false, $require_once = true, $args = array() ) {
  ...

- if ( file_exists( STYLESHEETPATH . '/' . $template_name ) ) {
+ if ( file_exists( get_stylesheet_directory() . '/' . $template_name ) ) {
  ...
}

What do you think?

---

BTW, there is something weird in locate_block_template. At the end of the function, it says "This file will be included instead of the theme's template file.". However, it will return the template file immediately on L96 if we have a located template. Is it the correct behavior?

Change History (8)

#1 @spacedmonkey
21 months ago

  • Component changed from General to Themes
  • Keywords needs-patch added
  • Version set to 5.8

This ticket was mentioned in PR #3531 on WordPress/wordpress-develop by @arthur791004.


21 months ago
#2

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core-test by ironprogrammer. View the logs.


21 months ago

#4 @ironprogrammer
21 months ago

  • Keywords needs-testing-info added

Thank you for the report, @arthur791004!

It would be helpful to have detailed testing steps that contributors can use to reproduce this bug. I've added the needs-testing-info keyword to indicate this.

#5 follow-up: @arthur791004
21 months ago

@ironprogrammer Thanks for the reply! But the reproduction steps are only on WP.com. Is it okay?

#6 in reply to: ↑ 5 @ironprogrammer
21 months ago

Replying to arthur791004:

@ironprogrammer Thanks for the reply! But the reproduction steps are only on WP.com. Is it okay?

Yep, based on what you've shared so far, it appears it would be related to the shared "core" codebase 👍🏻

#7 @arthur791004
21 months ago

Okay, here is the testing info, and it's related to https://github.com/Automattic/wp-calypso/issues/69687.

Testing Instructions

These steps define how to reproduce the issue, and indicate the expected behavior.

Steps to Reproduce

  1. Go to your WP.com site, e.g: https://wordpress.com/home/<your_site_domain>
  2. Go to Appearance > Themes
  3. Activate a theme whose homepage template is home.html, e.g.: Pendant/Cultivate/Archeo/Yuga. Remember to select the "Replace my homepage content with the XXX homepage." option.
  4. Go to Appearance > Themes again, and select a theme whose homepage template is index.html, e.g.: Appleton/Dorna/Russell/Pixl.
  5. You will see the preview for the "Switch theme, preserving my homepage content." option is always loading

Expected Results

When testing a patch to validate it works as expected:

  • ✅ The preview for the "Switch theme, preserving my homepage content." option will show

When reproducing a bug:

  • ❌ the preview for the "Switch theme, preserving my homepage content." option is always loading

Supplemental Artifacts

https://imgur.com/6sWzKp3

Thanks 🙂

Last edited 21 months ago by arthur791004 (previous) (diff)

This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.


17 months ago

Note: See TracTickets for help on using tickets.