#57329 closed defect (bug) (fixed)
Cannot save new custom template when name contains none-Latin characters
Reported by: | mburridge | Owned by: | danielbachhuber |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | 6.1.1 |
Component: | REST API | Keywords: | has-patch has-unit-tests add-to-field-guide |
Focuses: | javascript, administration, rest-api | Cc: |
Description
When you try to save a new custom template where the template name contains one or more non-latin characters you get an error. The error appears to be related to the REST endpoint.
See GitHub issue #41214.
Too reproduce:
- Select: Appearance -> Editor in the WP Admin
- Click the WordPress logo in top left, and select Templates
- Click Add New button (top right)
- Select Custom template and give it a name with at least one non-latin character
- Save the new template
- Confirm that you see the error in the editor and in the console.
Change History (25)
This ticket was mentioned in PR #3819 on WordPress/wordpress-develop by @antonyagrios.
2 years ago
#2
- Keywords has-patch has-unit-tests added
There is an issue where when a page has a slug that has nonlatin characters when a template is created, the user cannot make any changes, nor delete the template. This pr solves that problem.
Trac ticket: 57329
#3
@
2 years ago
Minor explanation of the introduced regex:
Before it was \w
: This actually matches the following: a-z
, A-Z
, 0-9
and _
Changing this to \pL0-9_
, we match the same as before, plus all the unicode characters for all the languages mentioned [here](https://www.php.net/regexp.reference.unicode).
I also kept the 0-9_
to match what we had before when we used \w
.
@antonyagrios commented on PR #3819:
2 years ago
#4
One thing I'm uncertain about is whether
\p
is safe to use on all platforms. It seems support was added in PHP 5.1, so I imagine we're _probably_ fine? It'd be good to have some confirmation of this.
This is what I thought as well. That is because it was introduced in PHP 5.1 would be safe. I also noticed that the tests we are running have 5.6 as the older target. So my guess is that we are good here. Also, we have some PHP compatibility tests in place, which, I assume as I haven't read them, there are there to check code compatibility.
danielbachhuber commented on PR #3819:
23 months ago
#6
@kozer When you have a moment, can you update the pull request description?
It also looks like you need to resolve any merge conflicts.
@Mamaduka commented on PR #3819:
23 months ago
#7
This is a great improvement. Thank you, @kozer, @danielbachhuber!
P.S. I think we also need to update slug pattern matcher in schema - https://github.com/WordPress/wordpress-develop/blob/6.1/src/wp-includes/rest-api/endpoints/class-wp-rest-templates-controller.php#L837
@antonyagrios commented on PR #3819:
23 months ago
#8
This is a great improvement. Thank you, @kozer, @danielbachhuber!
P.S. I think we also need to update slug pattern matcher in schema - https://github.com/WordPress/wordpress-develop/blob/6.1/src/wp-includes/rest-api/endpoints/class-wp-rest-templates-controller.php#L837
Done! Thanks for mentioning that.
danielbachhuber commented on PR #3819:
23 months ago
#9
@kozer Looks like there's one last test that might need to be updated here, too.
@antonyagrios commented on PR #3819:
23 months ago
#10
@kozer Looks like there's one last test that might need to be updated here, too.
@danielbachhuber , It seems there is a check that's failing in all of those checks:
Ensure version-controlled files are not modified or deleted
However wp-api-generated.js
is auto-generated when tests are running, and indeed there are changes that have occurred.
Do I need to version the file somehow? Do something else? The tests are passing and this isn't an implication locally.
danielbachhuber commented on PR #3819:
23 months ago
#11
It seems there is a check that's failing in all of those checks:
Ensure version-controlled files are not modified or deleted
Howeverwp-api-generated.js
is auto-generated when tests are running, and indeed there are changes that have occurred.
Do I need to version the file somehow? Do something else? The tests are passing and this isn't an implication locally.
@kozer The specific test that fails is git diff --exit-code
:
It fails because git is dirty when you run the entire test suite:
$ git status ## fix_template_api_non_latin M tests/qunit/fixtures/wp-api-generated.js
If you run composer test -- --filter test_build_wp_api_client_fixtures
locally, the file is regenerated and you can commit the diff.
@antonyagrios commented on PR #3819:
23 months ago
#12
It seems there is a check that's failing in all of those checks:
Ensure version-controlled files are not modified or deleted
Howeverwp-api-generated.js
is auto-generated when tests are running, and indeed there are changes that have occurred.
Do I need to version the file somehow? Do something else? The tests are passing and this isn't an implication locally.
@kozer The specific test that fails is
git diff --exit-code
:
It fails because git is dirty when you run the entire test suite:
$ git status ## fix_template_api_non_latin M tests/qunit/fixtures/wp-api-generated.jsIf you run
composer test -- --filter test_build_wp_api_client_fixtures
locally, the file is regenerated and you can commit the diff.
@danielbachhuber I run this locally, and the file is now regenerated. However, I see some changes that I don't know if they should be there.
Can you help me with that?
@danielbachhuber commented on PR #3819:
23 months ago
#13
However, I see some changes that I don't know if they should be there.
Can you help me with that?
@kozer What are the changes?
@antonyagrios commented on PR #3819:
23 months ago
#14
However, I see some changes that I don't know if they should be there.
Can you help me with that?
@kozer What are the changes?
For example this change or this
@danielbachhuber commented on PR #3819:
23 months ago
#15
I assume that only the regexes should be changed.
@kozer Yep, that's correct.
For example this change or this
What did you find when you looked around for additional context?
@antonyagrios commented on PR #3819:
23 months ago
#16
What did you find when you looked around for additional context?
@danielbachhuber I didn't look around for additional context, tbh. And this is because even with those changes, the tests are still failing.
Originally I had only the regexes committed. And then I took those changes back, tests kept failing, and then tested the theory you proposed (which introduced the extra changes)
I'm not even sure if we should commit any changes to this file, as it seems that it changes, every time the tests run.
@danielbachhuber commented on PR #3819:
23 months ago
#17
I didn't look around for additional context, tbh. And this is because even with those changes, the tests are still failing.
Originally I had only the regexes committed. And then I took those changes back, tests kept failing, and then tested the theory you proposed (which introduced the extra changes)
@kozer Thanks for looking into it! No further action needed on your part. I'll see if I can track it down sometime next week and then land it once I do.
#18
@
22 months ago
- Owner set to danielbachhuber
- Resolution set to fixed
- Status changed from new to closed
In 55294:
@danielbachhuber commented on PR #3819:
22 months ago
#19
I didn't look around for additional context, tbh. And this is because even with those changes, the tests are still failing.
Originally I had only the regexes committed. And then I took those changes back, tests kept failing, and then tested the theory you proposed (which introduced the extra changes)
@kozer As it turns out, the generated wp-api-generated.js
is slightly different if you run the full test site (vs the individual test). I updated it with 2e58f622d58fe6782cf2ab47afdc88721f2a2885
Pull request is shipped with https://core.trac.wordpress.org/changeset/55294
@antonyagrios commented on PR #3819:
22 months ago
#20
I didn't look around for additional context, tbh. And this is because even with those changes, the tests are still failing.
Originally I had only the regexes committed. And then I took those changes back, tests kept failing, and then tested the theory you proposed (which introduced the extra changes)
@kozer As it turns out, the generated
wp-api-generated.js
is slightly different if you run the full test site (vs the individual test). I updated it with 2e58f62
Thanks for sticking with it!
Pull request is shipped with https://core.trac.wordpress.org/changeset/55294
Can you provide me the exact command you run to update that file, for reference?
@Mamaduka commented on PR #3819:
22 months ago
#21
@kozer, I think it's npm run test:php
without the --fiter
or a --group
.
This ticket was mentioned in PR #4099 on WordPress/wordpress-develop by @danielbachhuber.
22 months ago
#22
Moves test files from https://github.com/WordPress/wordpress-develop/pull/3819 to Latin character filenames. Non-Latin characters can break SVN checkouts (e.g. https://github.com/wp-cli/scaffold-command/actions/runs/4202081401/jobs/7293427452)
Trac ticket: https://core.trac.wordpress.org/ticket/57329
#57328 was marked as a duplicate.