WordPress.org

Make WordPress Core

Opened 7 weeks ago

Closed 5 weeks ago

Last modified 5 weeks ago

#54596 closed defect (bug) (fixed)

Block theme with period in name causes blank editor

Reported by: mkaz Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version: trunk
Component: REST API Keywords: has-testing-info has-patch commit
Focuses: Cc:

Description

Testing instructions:

  1. Rename a block theme to include periods, for example: twentytwentytwo-1.0
  2. Attempt to load site editor, confirm blank screen
  3. Additional testing, add theme under subdirectory with or without periods

Original issue discovered in #54544

Upstream issue in Gutenberg plugin here:
https://github.com/WordPress/gutenberg/issues/37156

The register rest routes are different between core and the Gutenberg plugin, this same change works in core but not in the plugin. So additional testing is needed to fix upstream.

Attachments (10)

54596.diff (1.8 KB) - added by mkaz 7 weeks ago.
54596-test-valid-dirname.gif (4.1 MB) - added by hellofromTonya 6 weeks ago.
Test Report: testing valid theme dirname with PR 2034 applied. Works as expected.
54596-test-invalid-dirname.gif (7.5 MB) - added by hellofromTonya 6 weeks ago.
Test Report: testing invalid theme dirname with PR 2034 applied. Fails as expected.
test-in-subdir.gif (6.6 MB) - added by hellofromTonya 6 weeks ago.
Test: wp-content/themes/subdir/twentytwentytwo v0.4.0 @[( -_-)] (with latest PR changes). Works ✅
test-invalid-theme-dirname.gif (6.0 MB) - added by hellofromTonya 6 weeks ago.
Test Report: wp-content/themes/tt2-{invalid}/ (with latest PR changes). Result => status: 404, message: "No route was found matching the URL and request method"
test-valid-theme-not-in-subdir.gif (3.7 MB) - added by hellofromTonya 6 weeks ago.
Test Report: wp-content/themes/twentytwentytwo @[0.4.0] ( -_-)/ (with latest PR changes). Works ✅
test-same-theme-slug-resolves.png (896.0 KB) - added by hellofromTonya 6 weeks ago.
Test Report: Do same theme with different directory names resolve correctly? Yes, works ✅
test-site-editor-same-theme-resolves.png (1.0 MB) - added by hellofromTonya 6 weeks ago.
Test Report: 2 themes with same theme name but different directory names - Resolve in Site Editor? Yes, works ✅
test-pr2064-macos.gif (3.7 MB) - added by hellofromTonya 5 weeks ago.
Test PR 2064 (on macOS) with themes in wp-content/themes/subdir/tt2é [v1.0.0](-_-)/ and wp-content/themes/tt2é [v1.0.0] {@!$,^~}(-_-)/ directories. Works ✅
test-before-after-pr2064.png (1.0 MB) - added by hellofromTonya 5 weeks ago.
Test (on Mac) BEFORE and AFTER PR 2064 with themes wp-content/themes/ŧĥémé-1.0.0/ and wp-content/themes/subdir/ŧĥémé-1.0.0/. Results: before fails; after works.

Change History (53)

@mkaz
7 weeks ago

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


7 weeks ago

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


7 weeks ago

#3 @jffng
7 weeks ago

I gave this patch a spin and confirm the site editor loads as expected when a theme name contains a period (e.g. twentytwentytwo-0.4.0), as well as when it appears in a subdirectory.

#4 @hellofromTonya
7 weeks ago

  • Component changed from General to REST API
  • Keywords has-patch has-testing-info added
  • Milestone changed from Awaiting Review to 5.9
  • Owner set to hellofromTonya
  • Status changed from new to accepted
  • Version set to trunk

This ticket was mentioned in PR #2034 on WordPress/wordpress-develop by hellofromtonya.


7 weeks ago

  • Keywords has-unit-tests added

Fix: block theme with period in name causes blank editor.

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

#6 follow-up: @peterwilsoncc
7 weeks ago

This looks to be a wider problem with other characters that are permitted in folder names but do not currently work. I've put some notes below.

Somewhat challenging is that not all characters allowed in a folder name are permitted in a post name, so global styles set up with the theme my-theme and my.theme may both end up with the same slug.


Manual testing with various characters in the folder name with this patch applied follows.

For the 2022 clone, success or failure is indicated by whether the editor works.

For the 2020 clone, success or failure is indicated by whether the customizer works.

Character2022 clone2020 clone
space Failed Successful
. Successful Successful
[ Failed Failed
| Failed Successful
= Failed Successful
( Failed Successful
+ Failed Successful
@ Failed Successful

#7 @mkaz
6 weeks ago

@peterwilsoncc good catch. We can switch the regex to \w\W to grab everything, but not sure if that is a good idea or not, I fear spaces and other characters might introduce additional issues.

Your point about the slug is curious if themes share the same slug the styles and templates can conflict with one another.

#8 @peterwilsoncc
6 weeks ago

Yeah, I'm not sure what the correct answer to this is but I thought it better to note it down none the less.

The one thing that did occur to me overnight was that maybe the post slugs could be a hash of the directory name. It doesn't need to be salted as it's a hash of public information anyway. However, I suspect it's way to late in the release cycle to consider such a change.

This ticket was mentioned in Slack in #core-themes by jffng. View the logs.


6 weeks ago

#10 @prbot
6 weeks ago

spacedmonkey commented on PR #2034:

@hellofromtonya I would love to see some unit test with a test data provider, with list of theme names with funny characters, like, {?: etc.

#11 in reply to: ↑ 6 @hellofromTonya
6 weeks ago

Replying to peterwilsoncc:

Character2022 clone2020 clone
space Failed Successful
. Successful Successful
[ Failed Failed
| Failed Successful
= Failed Successful
( Failed Successful
+ Failed Successful
@ Failed Successful

After testing with @costdev, | fails on Windows as it's not a valid dirname and + gets stripped out.

PR 2034 adds support for the following characters: ., [, ], (, ), @, _, and space.

It's ready for testing and review.

To test:

  1. Pull and apply the PR.
  2. Rename the twentytwentytwo theme's directory to something like twentytwentytwo -_@ [(v0.4.0)].
  3. Activate the theme.
  4. Click on the 'Customize' button.

Expected behavior: The Site Editor opens without issue.

Last edited 6 weeks ago by hellofromTonya (previous) (diff)

#12 @prbot
6 weeks ago

hellofromtonya commented on PR #2034:

I would love to see some unit test with a test data provider, with list of theme names with funny characters, like, {?: etc.

Me too, @spacedmonkey. Those data providers are there now.

@hellofromTonya
6 weeks ago

Test Report: testing valid theme dirname with PR 2034 applied. Works as expected.

@hellofromTonya
6 weeks ago

Test Report: testing invalid theme dirname with PR 2034 applied. Fails as expected.

#13 @costdev
6 weeks ago

Test Report

Env

  • WordPress 5.9-beta2-52344-src
  • Chrome 96.0.4664.93
  • Windows 10
  • Theme: Twenty Twenty-Two
  • Gutenberg Editor
  • Plugin: None activated.


Steps to test

  1. Rename a block theme directory to include [ ] ( ) - _ (space) @. For example: [twenty_twenty-two] (@ 1.0).
  2. Navigate to Appearance > Themes and activate the theme.
  3. Navigate to Appearance > Site Editor.
  4. The Site Editor should fail to load.
  5. Apply PR 2034.
  6. Repeat steps 1-3.
  7. The Site Editor should now load.

Additional testing:

  • Repeat steps 1-7 but place the theme in a sub-directory with [ ] ( ) - _ (space) @ in its directory name.

Results

  • Before the PR: The Site Editor fails to load.
  • After the PR: The Site Editor loads as expected.
  • Issue fixed ✅

#14 @hellofromTonya
6 weeks ago

Test Report

Env:

  • OS: macOS Big Sur 11.6
  • Browsers: Edge, Chrome, Safari, and Firefox
  • Theme: Copy of TT2 with directory name changed (see test info)
  • Plugins: None
  • WordPress: trunk
  • Patch: PR 2034
  • Localhost: wp-end and Local (with ngnix and apache)

Steps

  1. Copy TT2 theme:
    • Rename it's directory to twentytwentytwo @[0.4.0] ( - _ - )
    • Open style.css and change the Theme Name
  2. Go to Appearance > Themes
  3. Activate the new TT2 theme with the funky directory naming
  4. Go to the Site Editor, i.e. click on "Customize" button

Expected behavior: The Site Editor should load

  1. Navigate back to the Themes UI
  2. Activate the original TT2 theme
  3. Rename the TT2 copy to twentytwentytwo {notvalid}
  4. Activate this TT2
  5. Click on "Customize" button

Expected behavior: The Site Editor should not load as there's an error with the invalid theme directory name

Results

  • Confirmed using space, -, _, ., @, [, ], (, ) characters in dirname works ✅
  • Confirmed using { and } does not work and gives a 404 error ✅
Last edited 6 weeks ago by hellofromTonya (previous) (diff)

#15 @hellofromTonya
6 weeks ago

  • Keywords commit added

Marking PR 2034 for commit.

Note: This patch adds support for the following characters: ., [, ], (, ), _, and @. It does not add support for | (which fail on Windows), + (which gets stripped), or = (which is a separator for query string key=value pairs).

#16 @prbot
6 weeks ago

spacedmonkey commented on PR #2034:

I would really like @TimothyBJacobs to review this before this is merged.

#17 @hellofromTonya
6 weeks ago

  • Keywords commit removed

Removing the commit keyword until folks like @TimothyBlynJacobs and @mkaz review and/or test the PR.

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


6 weeks ago

@hellofromTonya
6 weeks ago

Test: wp-content/themes/subdir/twentytwentytwo v0.4.0 @[( -_-)] (with latest PR changes). Works ✅

#19 @justinahinon
6 weeks ago

Test Report

Env:

Results

Before the patch

  • Confirmed using space, -, _, ., @, [, ], (, ) characters in dirname works
  • Confirmed using { and } does not work, and output a 404 error on http://localhost:8889/index.php?rest_route=/wp/v2/templates/twentytwentytwo {notvalid}//home&context=edit&_locale=user
Last edited 6 weeks ago by justinahinon (previous) (diff)

@hellofromTonya
6 weeks ago

Test Report: wp-content/themes/tt2-{invalid}/ (with latest PR changes). Result => status: 404, message: "No route was found matching the URL and request method"

@hellofromTonya
6 weeks ago

Test Report: wp-content/themes/twentytwentytwo @[0.4.0] ( -_-)/ (with latest PR changes). Works ✅

#20 @mkaz
6 weeks ago

Test Report

Env

OS: Windows / WSL2 (Apache)

Theme: Twentytwentytwo

Plugins: None (Issue still present in Gutenberg plugin (See 37167)

WordPress: trunk

Patch: PR 2034

Results

Works as expected, tested using set of characters directory name ( - _ . [ ] ( ) @ )

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


6 weeks ago

#22 @hellofromTonya
6 weeks ago

  • Keywords commit added

Marking it again for commit. If there are follow-ups from @TimothyBlynJacobs, can catch those in a follow-up commit for Beta 3 or Beta 4.

This ticket was mentioned in Slack in #core-restapi by hellofromtonya. View the logs.


6 weeks ago

#24 follow-up: @TimothyBlynJacobs
6 weeks ago

The change looks good to me. I do worry about @peterwilsoncc's concerns that different themes will end up mapping to the same slug.

#25 @prbot
6 weeks ago

spacedmonkey commented on PR #2034:

@hellofromtonya This needs to be rebased and conflicts fixed.

#26 follow-up: @TimothyBlynJacobs
6 weeks ago

Sorry noticed something, does this not affect the themes endpoint? Is that regex already broad enough?

@hellofromTonya
6 weeks ago

Test Report: Do same theme with different directory names resolve correctly? Yes, works ✅

@hellofromTonya
6 weeks ago

Test Report: 2 themes with same theme name but different directory names - Resolve in Site Editor? Yes, works ✅

#27 in reply to: ↑ 24 @hellofromTonya
6 weeks ago

Replying to TimothyBlynJacobs:

I do worry about @peterwilsoncc's concerns that different themes will end up mapping to the same slug.

I was worried about that too. Tested it with 2 the TT2 theme with different theme directory names and changed the wp-content/themes/twentytwentytwo-0.4.0/theme.json to background color to red. See attachments test-site-editor-same-theme-resolves.png and test-same-theme-slug-resolves.png. Both themes resolve in the Site Editor, front-end, and in the Appearance > Themes.

#28 @prbot
6 weeks ago

hellofromtonya commented on PR #2034:

Merge conflict resolved.

#29 in reply to: ↑ 26 ; follow-up: @hellofromTonya
6 weeks ago

Replying to TimothyBlynJacobs:

Sorry noticed something, does this not affect the themes endpoint? Is that regex already broad enough?

Thanks for raising the question. Did further testing through Customizer, Site Editor, Block Editor, and frontend. I'm curiously not seeing an issue with the Themes endpoint.

#30 @hellofromTonya
6 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 52376:

REST API: Add block theme support for valid non-alphanumeric characters in theme's directory name.

Themes whose wp-content/themes/<dirname> include valid non-alphanumeric (cross-platform) characters work for non-block themes, but did not previously resolve for block themes. For example, a block theme in wp-content/themes/twentytwentytwo-0.4.0/ directory resulted a 404 "No route was found matching the URL and request method" response when attempting to customize it in the Site Editor.

This commit adds support for the following characters in a theme's root directory: _, ., @, [, ], (, and ). Subdirectory themes and - are already supported.

Follow-up to [51003], [52051], [52275].

Props mkaz, costdev, hellofromTonya, jffng, justinahinon, peterwilsoncc, spacedmonkey, TimothyBlynJacobs.
Fixes #54596.

#32 in reply to: ↑ 29 ; follow-up: @TimothyBlynJacobs
6 weeks ago

Replying to hellofromTonya:

Replying to TimothyBlynJacobs:

Sorry noticed something, does this not affect the themes endpoint? Is that regex already broad enough?

Thanks for raising the question. Did further testing through Customizer, Site Editor, Block Editor, and frontend. I'm curiously not seeing an issue with the Themes endpoint.

The single /wp/v2/themes/<theme> endpoint would need to be tested manually or via Unit Tests. AFAIK, no parts of the UI currently use the single theme endpoint directly, they still use the collections response.

#33 in reply to: ↑ 32 @hellofromTonya
6 weeks ago

  • Keywords needs-testing added; has-patch has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to TimothyBlynJacobs:

The single /wp/v2/themes/<theme> endpoint would need to be tested manually or via Unit Tests. AFAIK, no parts of the UI currently use the single theme endpoint directly, they still use the collections response.

Thanks @TimothyBlynJacobs and DOH on my part :facepalm:. Reopening this ticket to test this, i.e. just to make sure that endpoint is okay or it needs an adjustment too.

Hey @costdev would you have some time this week to manually test this?

#34 @bijayyadav
6 weeks ago

I installed 5.3 beta and copied twentytwentytwo theme and renamed it twentytwentytwo!. After that on the backend, I clicked on Appearance > Editor and the page was blank. There are two errors on console:

Screenshot: http://prntscr.com/2328a4g

#35 @kafleg
6 weeks ago

I also have the same issue. My theme folder is, twenty!@#$%^&()_+ ~-+`.

Also, there console error,

Uncaught (in promise) TypeError: Cannot destructure property 'id' of '(intermediate value)' as it is null.

at K (edit-site.min.js?ver=dd4604f4d44cfa19261ecfc79f1e6f06:2)
at K.next (<anonymous>)
at Y (edit-site.min.js?ver=dd4604f4d44cfa19261ecfc79f1e6f06:2)
at Y.next (<anonymous>)
at redux-routine.min.js?ver=f2016f404fbc2c831239953e37cc686c:2

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


6 weeks ago

#37 @hellofromTonya
5 weeks ago

Thanks @kafleg and @bijayyadav. Currently, the endpoint does not support these characters: !#$%^&+~.

TODO: Confirm if all of these characters are valid for directory names on cross-server OSs including Windows, Mac, Linux, etc.

This ticket was mentioned in PR #2064 on WordPress/wordpress-develop by hellofromtonya.


5 weeks ago

  • Keywords has-patch added

Adds support for localized characters (e.g. é) and other valid directory name characters.

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

#39 @hellofromTonya
5 weeks ago

[52376] was too limiting as valid directory name characters such as é, &, =, #, !, $, ,, ^, ~, %, etc.

Notice that the original regex for listing themes global styles excludes . and / characters.

'/' . $this->rest_base . '/themes/(?P<stylesheet>[^.\/]+(?:\/[^.\/]+)?)'

Aha, . was being excluded.

Then @TimothyBlynJacobs asked if WP_REST_Themes_Controller::PATTERN was also too limiting:

Sorry noticed something, does this not affect the themes endpoint? Is that regex already broad enough?

Here's it original regex:

const PATTERN = '[^.\/]+(?:\/[^.\/]+)?';

Notice again the . character is excluded.

PR 2064 reverts the regex changes made by [52376], removes the . from the exclusion lists, and adds additional invalid characters (invalid on Windows). It then adds additional tests to further validate the changes.

@costdev tested it on Windows. I tested it on Mac. Works well on both. Test report is coming.

@hellofromTonya
5 weeks ago

Test PR 2064 (on macOS) with themes in wp-content/themes/subdir/tt2é [v1.0.0](-_-)/ and wp-content/themes/tt2é [v1.0.0] {@!$,^~}(-_-)/ directories. Works ✅

#40 @hellofromTonya
5 weeks ago

  • Keywords needs-testing removed

Test Report

Env:

  • OS: macOS Big Sur 11.6
  • Browsers: Edge
  • Theme: Copies of TT2 with directory name changed (see test info)
  • Plugins: None
  • WordPress: trunk
  • Patch: PR 2064
  • Localhost: wp-env

Steps

  1. Prepare first theme:
    • Copy wp-content/themes/twentytwentytwo/ to wp-content/themes/tt2é [v1.0.0] {@!$,^~}(-_-)/
    • In copied theme, open style.css and change the Theme Name to tt2é [v1.0.0] {@!$,^~}(-_-)
    • Open the theme.json and change the background color of #000000 to #FF0000 (visual cue that this theme resolves)
  2. Prepare second theme:
    • Copy wp-content/themes/tt2é [v1.0.0] {@!$,^~}(-_-)/ to wp-content/themes/subdir/tt2é [v1.0.0](-_-)/
    • Open style.css and change the Theme Name to subdir/tt2é [v1.0.0](-_-)
    • Open the theme.json and change the background color of #FF0000 to #00FF00 (visual cue that this theme resolves)
  3. Go to Appearance > Themes
  4. Activate the subdir/tt2é [v1.0.0](-_-) theme (the one in the subdirectory)
  5. Click its "Customize" button

Expected behavior: The Site Editor should load and the header background should be a bright green color.

  1. Click on the W logo which opens the Site Editor's sidebar
  2. Navigate to Templates

Expected behavior: The Added by column should list subdir/tt2é [v1.0.0](-_-). (See the left side of test-pr2064-macos.gif.)

  1. View the site in the frontend.
  2. Repeat steps 4 through 7 with the wp-content/themes/tt2é [v1.0.0] {@!$,^~}(-_-)/ theme.

Expected behavior: The header background color should red and "Added by" column should list tt2é [v1.0.0] {@!$,^~}(-_-). (See the right side of test-pr2064-macos.gif.)

  1. Repeat steps 4 through 7 with the wp-content/themes/twentytwentytwo/ theme.

Expected behavior: The header background color should black and "Added by" column should list Twenty Twenty-Two.

Results

  • Twenty Twenty-Two theme loads properly ✅
  • wp-content/themes/tt2é [v1.0.0] {@!$,^~}(-_-)/ loads properly ✅
  • wp-content/themes/subdir/tt2é [v1.0.0](-_-)/ loads properly ✅
  • Copying TT2 wp-content/themes/sudir/tt2 loads properly ✅ (an extra test I did)

Works as expected ✅

Notes:

  • On Mac, themes named with &, =, % characters do not load in the Site Editor. No 404 (so the route is found). Uncaught (in promise) TypeError: Cannot destructure property 'id' of '(intermediate value)' as it is null. error in the console. Suspect this is due to encoding.
    • Theme directories named with these characters are likely far edge cases.
Last edited 5 weeks ago by hellofromTonya (previous) (diff)

@hellofromTonya
5 weeks ago

Test (on Mac) BEFORE and AFTER PR 2064 with themes wp-content/themes/ŧĥémé-1.0.0/ and wp-content/themes/subdir/ŧĥémé-1.0.0/. Results: before fails; after works.

#41 @hellofromTonya
5 weeks ago

  • Keywords commit added

Theme directories named with characters like {@!$,^~"} and &=% are likely far edge cases. Theme directories named with localized characters like é are not.

Test Report

Env:

  • OS: macOS Big Sur 11.6
  • Browsers: Edge
  • Theme: Copies of TT2 with directory name changed (see test info)
  • Plugins: None
  • WordPress: trunk
  • Patch: PR 2064
  • Localhost: wp-env

Steps

Using the steps as a starting point:

  1. Prepare first theme:
    • Rename wp-content/themes/tt2é [v1.0.0] {@!$,^~}(-_-)/ to wp-content/themes/ŧĥémé-1.0.0/
    • Open style.css and change the Theme Name to ŧĥémé-1.0.0
  2. Prepare second theme:
    • Rename wp-content/themes/subdir/tt2é [v1.0.0](-_-)/ to wp-content/themes/subdir/ŧĥémé-1.0.0
    • Open style.css and change the Theme Name to subdir/ŧĥémé-1.0.0

Now test:

  1. Use trunk (not the PR)
  2. Go to Appearance > Themes
  3. Activate the subdir/ŧĥémé-1.0.0 theme (the one in the subdirectory)
  4. Click its "Customize" button

Actual behavior: Site Editor does not load and has a 404 with the following error message: {"code":"rest_no_route","message":"No route was found matching the URL and request method.","data":{"status":404}} (To view the error, open browser's Network tab, refresh the page, scroll down the 404 item in the list, click on it to open a panel, and then click on the Response tab to view the JSON response.)

  1. Apply PR 2064
  2. Refresh the Site Editor.

Expected behavior: The Site Editor should load and the header background should be a bright green color.

Results

  • Yes, can reproduce the 404 error before the PR is applied ✅
  • With the PR 2064 applied, resolves the endpoint (no error) and Site Editor opens as expected ✅

Marking for commit.

#42 @hellofromTonya
5 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 52399:

REST API: Support . in theme directory names in WP_REST_Global_Styles_Controller, WP_REST_Templates_Controller, and WP_REST_Themes_Controller.

Regex changes from [52376] are reverted to restore the original regex patterns. Why? [52376] used an include characters pattern, which was too limiting. It did not account for localized characters, such as é, or other valid directory name characters.

The original theme directory regex pattern, i.e. [^.\/]+(?:\/[^.\/]+)? excluded the period . character. Removing the . character resolves the reported issue by allowing matching for themes/theme-dirname-1.0/ or themes/<subdirname>/theme-dirname-1.0/.

As the pattern used an exclude approach, all characters are valid for matching except for /. However, not all characters are cross-platform valid for directory names. For example, the characters /:<>*?"| are not valid on Windows OS. The pattern now excludes those characters.

The theme's directory (or subdirectory) name pattern matching is now used in WP_REST_Global_Styles_Controller, WP_REST_Templates_Controller, and WP_REST_Themes_Controller.

Follow-up to [51003], [52051], [52275], [52376].

Props costdev, hellofromTonya, spacedmonkey, TimothyBlynJacobs, bijayyadav, kafleg.
Fixes #54596.

Note: See TracTickets for help on using tickets.