Make WordPress Core

Opened 18 months ago

Last modified 4 months 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.7 Priority: normal
Severity: normal Version: 5.9
Component: Editor Keywords: has-patch has-unit-tests 2nd-opinion has-testing-info dev-feedback
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 (7)

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

Change History (42)

@gaft
18 months ago

directly to the actual check line

@gaft
18 months ago

up in the unified template object

#1 @gaft
18 months 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
18 months 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
18 months ago

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

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.

Version 0, edited 18 months ago by costdev (next)

#4 @gaft
18 months 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
18 months ago

screenshot of debug

#5 @davidbaumwald
18 months 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
18 months 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
18 months ago

using wp_normalize_path to fix

@gaft
18 months ago

screenshot with wp_normalize_path

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


18 months ago

#8 @audrasjb
18 months 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.


17 months ago

#10 @jeremyfelt
17 months 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
17 months 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
17 months ago

  • Milestone changed from 6.2.2 to 6.2.3

Moving this to 6.2.3.

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


16 months ago
#13

  • Keywords has-unit-tests added; needs-unit-tests removed

This PR adds the changes suggested by @gaft + a single PHP Unit test

Trac ticket: https://core.trac.wordpress.org/ticket/58132

#14 @soulseekah
15 months ago

  • Keywords 2nd-opinion added

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

I believe that this patch would actually break things on Windows and should not be committed.

When picking a template through the UI on Windows the _wp_page_template meta is saved with backslashes. The query works fine. If we suddenly rewrite all slugs from backslashes to forward ones the query will stop working. In order for this to work, you will need to further change the _wp_page_template meta value throughout the database as well!

The only way the reported bug can be reproduced is when saving the template to a page on Windows, then moving the whole site onto Linux (or the other way around). That's when slugs start mismatching.

Your suggested steps to reproduce the issue further follow the above case. You are setting the meta value to a Linux-style slug and hoping it to work on Windows. Your slug would have been correct if you saved it via the UI.

The best course in addressing this edge case where we've migrated from Windows to Linux or the other way around would be to actually check BOTH versions of the slug slashes (using a preg_match maybe?). This way, no matter what direction the slashes are looking in _wp_page_template it would always match.

#15 @SergeyBiryukov
14 months ago

  • Milestone changed from 6.2.3 to 6.4

Moving to 6.4 for now, as there are no plans for 6.2.3 at this time, and this does not appear to be a regression in 6.3.

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


13 months ago

#17 @nicolefurlan
13 months ago

@gaft could you read through the latest updates in this ticket and provide feedback so that we can keep this ticket moving forward?

We also need some updated testing instructions if anyone is able to provide them.

@SergeyBiryukov are you able to test this in a Windows environment? We need more Windows specific contributors to reproduce, test the patch, and weigh into the discussion.

Thank you!

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


12 months ago

#19 @oglekler
12 months ago

  • Keywords has-testing-info needs-testing added; needs-testing-info removed

This ticket was discussed during bug scrub.

This patch needs testing. Testing info: #comment:4

Add props: @mukesh27

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


12 months ago

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


12 months ago

#22 @hellofromTonya
12 months ago

  • Milestone changed from 6.4 to 6.5

This ticket was discussed today in pre-RC1 bug scrub.

In reviewing, I had some thoughts and questions.

  • On the proposed patch / PR:

Rather than normalizing after the skip checks, likely need to normalize the slug for both of the skip checks, i.e. normalize just after the $template_slug is built.

  • What is the impact elsewhere?

How will normalizing in this function impact where the template slug is used elsewhere? Are there impacts or side effects elsewhere?

  • Next steps towards resolution.

Maybe moving it to Gutenberg could help move it forward? Or folks working on the templates could take a look here to share insights to the above questions.

Given the questions combined with 6.4 RC1 happening in 6 days, I don't think there's enough time left in the 6.4 cycle.

If the questions get answered and there's confidence from maintainers / leads / committers the fix will not cause impacts elsewhere, then please move it back into the milestone for consideration.

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


8 months ago

This ticket was mentioned in Slack in #core-editor by mike. View the logs.


8 months ago

#25 @chaion07
8 months ago

Hi @gaft for reporting this. We reviewed the ticket during a recent bug-scrub session. Both @mikeschroder and @chaion07 liked the idea of finding out if folks working on working on Templates / Editor component would prefer this ticket be a Gutenberg issue. Aditionally, @mikeschroder would mention this over in the core-editor channel to ask for feedback.

Cheers!

Add props to @mikeschroder

#26 @kirasong
8 months ago

I [chatted with some folks https://wordpress.slack.com/archives/C02QB2JS7/p1707197711043599] in the #core-editor channel about this.

@Mamaduka suggested that it be handled in Core.

Both he and @talldanwp provided links to a couple related Gutenberg issues:

Thanks + props to @talldanwp and @Mamaduka.

#27 @gaft
8 months ago

Sorry guys. I stopped forcing my way here as we all know that subdirectory is not fully supported at the moment.

While my provided patches worked, another issue arises when the users try to edit them in FSE.

Because the identifier being used in wp_template_part is still the post_name column which is passed to sanitize_title on save, this results to a creation of a completely new template. Thus, users will have to manually update the post/page to point to it in order to see the wanted changes.

#28 @gaft
8 months ago

Please note that @soulseekah's comment is not entirely true. On an actual Windows environment, it is already broken to begin with. Saving in the UI completely changes the page, because the saved template meta is stripped of all slashes, which eventually makes the page fall back to the Default template.

This does not happen on Mac as of testing. Both were tested in a clean WP install with the deep > folder > test.html template added to the fresh twentytwentyfour theme.

@gaft
8 months ago

@gaft
8 months ago

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


8 months ago

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


8 months ago

#31 @rajinsharwar
8 months ago

As this is still being discussed, let's keep this in the milestone for some more days in the hope of having some activity. Or else, we need to bump to 6.6.

#32 @swissspidy
7 months ago

  • Milestone changed from 6.5 to 6.6

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


4 months ago

#34 @khoipro
4 months ago

  • Keywords reporter-feedback removed

I suggest keeping 1 more week to discuss from #core chat.

#35 @oglekler
4 months ago

  • Keywords dev-feedback added; needs-testing removed
  • Milestone changed from 6.6 to 6.7

This issue has no chances to be solved in this milestone and there is no clear solution at far. For everyone who want to work on a patch: Please check existing patch to start with and tack a look at comment #26 that presented additional issue description in the Gutenberg repo and check #14 for some concerns.

Note: See TracTickets for help on using tickets.