Make WordPress Core

Opened 8 weeks ago

Last modified 2 weeks ago

#58132 new defect (bug)

Slashes used in block templates slug is a problem on Windows

Reported by: gaft's profile gaft Owned by:
Milestone: 6.2.3 Priority: normal
Severity: normal Version: 5.9
Component: Editor Keywords: needs-testing-info reporter-feedback needs-unit-tests has-patch
Focuses: Cc:

Description

Generally, they are all stored flat in the theme templates or block-templates folder; we have no problem here. But when they are stored deep under some folder/s, the slug checks won't pass on Windows.

This is because finding the template paths https://developer.wordpress.org/reference/functions/_get_block_templates_paths return backslashes \ and https://core.trac.wordpress.org/browser/tags/6.2/src/wp-includes/block-template-utils.php#L313 currently only does substr extracts to come up with a slug that is passed to https://developer.wordpress.org/reference/functions/_build_block_template_result_from_file/, which eventually fails line https://core.trac.wordpress.org/browser/tags/6.2/src/wp-includes/block-template-utils.php#L981 as the slugs in the query are using forward slashes \.

Attachments (5)

58132-1.diff (765 bytes) - added by gaft 8 weeks ago.
directly to the actual check line
58132-2.diff (525 bytes) - added by gaft 8 weeks ago.
up in the unified template object
Screenshot 2023-04-14 220707.2.png (327.3 KB) - added by gaft 8 weeks ago.
screenshot of debug
58132-3.diff (520 bytes) - added by gaft 7 weeks ago.
using wp_normalize_path to fix
Screenshot 2023-04-15 000501.png (323.1 KB) - added by gaft 7 weeks ago.
screenshot with wp_normalize_path

Download all attachments as: .zip

Change History (17)

@gaft
8 weeks ago

directly to the actual check line

@gaft
8 weeks ago

up in the unified template object

#1 @gaft
8 weeks ago

The fix here is just an easy str_replace. Can be either at:

  1. directly to the actual check line
  2. up in the unified template object

#2 @poena
8 weeks ago

Hi!
Can you help by providing step-by-step testing instructions for reproducing the issues?

A similar problem did occur a few years ago, but it was supposedly fixed, so if it has re-appeared it is a regression.
Context: https://github.com/WordPress/gutenberg/issues/20375#issuecomment-686152426

#3 @costdev
8 weeks ago

  • Keywords needs-testing-info needs-testing reporter-feedback added
  • Milestone changed from Awaiting Review to 6.2.1

Hi @gaft, welcome to Trac and thanks for opening this ticket and adding patches for consideration!

In addition to reproduction steps, we also need to verify whether this behaviour can be observed in WordPress 6.1, or if a regression was introduced in WordPress 6.2.

Milestoning for 6.2.1 for visibility.

Last edited 8 weeks ago by costdev (previous) (diff)

#4 @gaft
8 weeks ago

Hello, I am testing it now in version 6.2. Here are the quick steps to take:

  1. Create a page and assign to a custom template
wp_insert_post([
    'post_status' => 'publish',
    'post_type' => 'page',
    'post_title' => 'Custom Template Page',
    'meta_input' => [
        '_wp_page_template' => 'deep/folder/test',
    ],
]);
  1. Add a template file named test.html in the theme under templates/deep/folder
custom-theme/
├── parts/
├── templates/
│   ├── deep/
│   │   └── folder/
│   │       └── test.html
│   ├── 404.html
│   ├── index.html
│   └── page.html
├── screenshot.png
├── styles.css
└── theme.json
  1. View the page at /custom-template-page/

Currently, it only renders the templates/page.html because https://core.trac.wordpress.org/browser/tags/6.2/src/wp-includes/block-template-utils.php#L981 evaluates to false

@gaft
8 weeks ago

screenshot of debug

#5 @davidbaumwald
8 weeks ago

  • Component changed from General to Editor
  • Keywords needs-testing removed
  • Type changed from enhancement to defect (bug)
  • Version changed from 6.2 to 5.9

I haven't spent too much time tracing this, but I think wp_normalize_path would probably be a better fix than strpos. I am exactly not sure where the fix would be, but I feel like it's higher up the chain than the current patch places it. I'm thinking perhaps the paths should be normalized in _get_block_templates_files, but not 100% sure.

This looks like it might be [52062] that introduced the _get_block_templates_files code, while [52247] abstracted the base paths into get_block_theme_folders.

#6 @gaft
7 weeks ago

I totally agree that wp_normalize_path is a more robust function to use here. Also, I think it is best handled in _get_block_templates_files where it can be useful elsewhere that expects slug to be in forward slashes.

@gaft
7 weeks ago

using wp_normalize_path to fix

@gaft
7 weeks ago

screenshot with wp_normalize_path

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


5 weeks ago

#8 @audrasjb
5 weeks ago

  • Keywords needs-unit-tests has-patch added

Patch 58132-3.diff looks good, but we need to add some unit tests for this case.

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


4 weeks ago

#10 @jeremyfelt
4 weeks ago

Partially related: https://github.com/WordPress/gutenberg/issues/49100

Or—once this ticket is resolved, there may still be issues when trying to use / in template IDs due to _sanitize_template_id().

#11 @audrasjb
4 weeks ago

  • Milestone changed from 6.2.1 to 6.2.2

Since WordPress 6.2.1 Release candidate 1 is planned today, let's move this ticket to 6.2.2 to give it more time to add unit test cases and proper testing.

#12 @desrosj
2 weeks ago

  • Milestone changed from 6.2.2 to 6.2.3

Moving this to 6.2.3.

Note: See TracTickets for help on using tickets.