-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix blank site editor when theme name contains a period #37167
Conversation
d8c1f55
to
23abb01
Compare
Will this change break the endpoint for themes inside subfolders? Did we test that use-case? |
Thanks @youknowriad that explains the regex complexity, I couldn't figure out why. With themes in a sub-directory, the template-parts API doesn't seem to load properly but the other APIs do. I'll troubleshoot some more. |
ok, I added the It considers the theme name: |
Should the theme slug include the subdir? There appears to be a conflict. When looking at the template list it includes the subdir in the slug? However, when including from template parts the subdir is not included - since a theme author likely did not create in the same subdirectory. |
There are minor differences between how Core and Gutenberg register API routes, I created a core ticket with a patch to trunk that seems to fix this problem: https://core.trac.wordpress.org/ticket/54596 The same change applied to the Gutenberg plugin still has some minor issues. My environment is a bit of a mess between multiple core versions, themes, modified templates, and other discrepancies. I'll circle back to this one likely tomorrow and test with a fresh setup. |
I've updated this PR to match the core trac ticket and now testing properly against 5.9 source, there are more errors when testing against WP 5.8.2, so we'll need more testing around backwards compatibility if the new plugin version is supposed to work with older WP installs. Right now it is working functionally as expected. There is a 404 error on the API call when browsing to the listing of templates or template parts page: However, clicking into the templates (or template parts) updating and saving works. It appears the issue on that page is the shortening of the theme name from the directory to the actual name itself, ie. from twentytwentytwo-1.0 -> "Twenty Twenty Two" |
I'm adding @adamziel as a reviewer here as he worked on similar issues in the endpoint recently. |
This also needs to take into account additional characters. |
Hi, @mkaz It looks like the fix was committed in core - https://core.trac.wordpress.org/changeset/52376. |
Can we bring the changes to Gutenberg as well (as we need to support older WP versions) |
Maybe we don't even need a separate PR, updating this one should do |
Sync the updates in core from Trac https://core.trac.wordpress.org/changeset/52376 that support theme directories with special characters. Confirm site editor loads properly.
c1cc4a4
to
b90d15b
Compare
👍 I migrated the changes made in the trac ticket 52376 and everything is now testing well. Please take a look and confirm. Same testing instructions as above but you can also try for additional special characters such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a failing unit test but otherwise looks good.
My fault, bad grepping. I saw the test in the trac ticket but searched in the wrong directory for it. There are some tests in core that aren't in the plugin. I updated the failing test with changes made to core. 👍 |
845a454
to
d182856
Compare
The core ticket added additional fixtures and tests that are not in the plugin. So those are not being copied over. Just updating the existing test and adding one for /themes endpoint.
8f75497
to
6567fb5
Compare
NOTE: Even though the label is there, this does not need to be backported to core, it is an update from core, so the code is already synced. |
* Add \. as valid character in rest route regex * Fix regex for stylesheet vs. id * Add slash to regex to support subdir themes * Include subdir in regex * Update regex to match core trac update * Sync route regex for theme directory characters Sync the updates in core from Trac https://core.trac.wordpress.org/changeset/52376 that support theme directories with special characters. Confirm site editor loads properly. * Update global styles phpunit test from trac ticket. The core ticket added additional fixtures and tests that are not in the plugin. So those are not being copied over. Just updating the existing test and adding one for /themes endpoint.
Description
The rest routes do not include a period in the regex so if the theme name includes a period the API requests will fail.
There is still an issue when the route matches and the callback
get_item()
is passed the theme name as the id. Theget_item()
method attempts look up global styles using this id but as a string it fails.@oandregal
How should that look up work? Checking for numeric seems to work but feels like it is skipping an important part.I think I figured out the lookup, it got oversimplied but I think the regex might of been from template routes, and didn't need to be as complicated.I've confirmed via test instructions below and this now seems to work with no errors.
Fixes #37156
How has this been tested?