Strip useless "_" at beginning and end of JS-transliterated machine names

Updates on MR:
- solve conflicts with the target branch 10.1.x
- Fix a test that expect special chars () at the end of "name" - not valid anymore
- The transliterate method for MachineNameController allow also to have a request with lowercase ... which means we need to allow also the Upper case in those cases ... updated.

Is there something else left?
Back to "Needs Review"

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Thanks a lot @vasike! Changes are looking good and tests are all green now! 🙂

Should we dare to rebase the MR on 11.x, so this annoying "needs-review-queue-bot" stops reporting an issue?

@Spokje, I think, because you originally created the issue branch, you also need to edit the current MR to target 11.x, I don't have the permissions, also not for deleting the current MR (to create a new one).

In case @Spokje isn't active in this issue anymore, I'll just add the "no-needs-review-bot" tag. If this should not EVER be done, @smustgrave can put me in my place again. 😜

haha. There's no policy when it shouldn't be done but the bot may have been doing the job correctly.

We will need a 11.x version first. No issue though opening a separate branch for 11.x.

Alright! Feel free to edit the MR to target 11.x, I don't have the rights unfortunately.

I don't either unfortunately.

What I mean is you can open a separate branch from the 10.1

Switched MR to 11.x, still needs rebase, good luck with that :evil_smile:

Grevil → closed merge request !4219

Alright, I think, this will take a bit...

Alright! Manually rebasing the changes on 11.x was basically impossible...

So instead I reset the branch on current 11.x and applied the changes done in this issue manually. Please review!

(Changes can be seen here in https://git.drupalcode.org/project/drupal/-/merge_requests/3012/diffs)

Test fail.

OK, it seems that the machine name code changed quite a bit, so simply applying the patch manually won't solve the issue anymore, as seen in the test case.

Two test values do not have the expected output on 11.x any more:

• The input 'Test value !0-9@' leads to the machine name being 'test_value_0_9_' instead of 'test_value_0_9'
• The input ', Neglect?! ' leads to the machine name being '_neglect_' instead of 'neglect'

Maybe someone could look into this?! 🙂

@Grevil that would mean, that the current MR doesn't change anything, as those cases show what this issue is supposed to fix.
Could you once more ensure all changes were applied on the 11.x MR?

I guess the next time, we should better create a second MR, leave the old one on the old branch and instead apply the changes manually / by patch + 3 way merge on the new MR to make it easier to compare changes. -.-

I think, we need to check what the current state of transliteration is. As it seems, that quite a bit has changed in 11.x, see 🐛 Machine name generation is way too slow Fixed .

From transliterate() in "core/modules/system/src/MachineNameController.php":

@trigger_error(__METHOD__ . '() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/node/3367037', E_USER_DEPRECATED);

So the question is, do the tests currently test the "new" transliteration? Or are these just legacy tests? Is there even a "new" way to transliterate already? Or is our regex just inserted at the wrong point of the js? Maybe there is further code, we need to adjust? A lot of stuff to think about, or maybe it is quite obvious. 😅

Thanks @Grevil so the issue must be in the JS code. To me it looks like the trimming was now in the wrong place, after the changes that already went into 11.x - I think, this now belonged into the transliterate() function.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The various changes made in 11.x (and deprecation added in 10.2.x) can be seen here:
🐛 Machine name generation is way too slow Fixed
or directly in the merged MR:
https://git.drupalcode.org/project/drupal/-/commit/570710ad7bcdd4fd342f9...

Can someone help me to understand the following line:
const needsTransliteration = !/^[A-Za-z0-9_\s]*$/.test(baseValue);
Why don't uppercase letters need transliteration? Isn't this a regression?

In the latest change, I removed the "_" as it wouldn't trigger transliteration, if there is a starting or trailing underscore. So I think we'd need to improve the regex, but first I'd like to understand what it's for.

Question does ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed cover this?

@smustgrave ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed only covers file name sanitization for the core "file" module, so unrelated! 🙂

@Anybody most code style fixes in https://git.drupalcode.org/project/drupal/-/merge_requests/3012/diffs?co..., could be reverted as running eslint using the core ".eslintrc.json" will now result in the following errors:

/var/www/html/web/core/misc/machine-name.js
    8:2   warning  Unexpected unnamed function                                           func-names
   48:15  error    Use object destructuring                                              prefer-destructuring
   61:15  error    Use object destructuring                                              prefer-destructuring
   62:15  error    Use object destructuring                                              prefer-destructuring
  261:22  error    Use a regular expression literal instead of the 'RegExp' constructor  prefer-regex-literals

✖ 5 problems (4 errors, 1 warning)
  3 errors and 0 warnings potentially fixable with the `--fix` option.

But I guess we can leave it as is, as there were eslint errors before in this file and Jenkins doesn't report anything.

We do not need to change the "MachineNameControllerTest" as its "transliterate()" function will be removed in 11.0.0, see #69.

Done, tests are green locally! Please review :)

Tested out MR 3012 by creating a content type with label Hello (World) and the machine name was test_world

LGTM !

Thanks @smustgrave you mean hello_world? :) (not test_world)
Would be nice to get this fixed soon, so it doesn't need a lot of rerolls again. Took long enough :D

Looks like there are few comments on the MR 😇

I can not resolve the threads for some reason, so yea... *RESOLVED*.

I also can't set them resolved. Can @smustgrave or @lauriii help perhaps?

Setting this back to RTBC as of #78 and the comments. If there's need for more discussions about the Regex, feel free to do so, but I don't think it will be a huge benefit, but instead complicate code and prevent this issue from being finished for little benefit. (Just my 2 cents ;))

this is odd ... as the author of threads ... i can't resolve them.

Is it as it should be?

I agree that the problem related to the Regex isn't huge. I posted one idea how we might be able to get rid of the hardcoded Regex. If that doesn't sound like a good idea, I'd be fine if we decide to move forward with the current patch.

Moving to explore the trim() idea from the MR comments.

Moving back to "Needs Review", for reviewing newly added gitlab comments.

i commented on MR ... but i can't solve the thread there.

could we move forward with this?

Super nice work @lauriii and one more time funny to see the benefits of PHP's trim() implementation (with trim character optional parameter) vs. JS trim() only handling whitespace.

Would be nice to have the PHP implementation in JS natively.

So thanks for implementing this. Tests pass, implementation was improved, I'd say this is RTBC!

@lauriii could you set the comments resolved? I can't.

oh ... sorry ... i just committed an update ... i didn't notice it's RTBC

please let me know if i should revert it / remove it ...

@vasike let's keep it for review. May @lauriii decide! :)

We could have moved the refactoring to a follow-up issue to help this issue land faster but instead of reverting the refactoring, I went ahead and pushed few commits to further clean up the code 😅

Thanks @lauriii, looks really great and clean now! Once the tests pass, I'll RTBC this. Added just one comment for the comment ;)
Then I think we're done here! Yay!

Addressed feedback on the MR 😊

Great! :) All tests pass, all comments resolved. RTBC, nice work everyone!

Went over the code and did some additional manual testing. This looks to be in good shape and nice to see how much simpler it is now that machine name generation is client side. Committed to 11.x, thanks all!

bnjmnm → closed merge request !3012

This broke the Field UI "add field" flow. It says "field_undefined" and then as you enter "a" it returns "field_undefinedaundefined", and then "ab" results in "field_undefinedaundefinedbundefined" etc

I tracked it down to

diff --git a/core/misc/machine-name.js b/core/misc/machine-name.js
index ea6d531dbd..7cf8bdd9f1 100644
--- a/core/misc/machine-name.js
+++ b/core/misc/machine-name.js
@@ -106,7 +106,7 @@ function clickEditHandler(e) {
 
       function machineNameHandler(e) {
         const data = e.data;
-        const options = data;
+        const options = data.options;
         const baseValue = e.target.value;
 
         const needsTransliteration = !/^[A-Za-z0-9_\s]*$/.test(baseValue);

This broke HEAD in 11.x, reverted. Sample failure below - many similar failures in FunctionalJavascript tests:

PHPUnit 9.6.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\ckeditor5\FunctionalJavascript\StyleTest
F.                                                                  2 / 2 (100%)

Time: 00:33.157, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\StyleTest::testStyleSettingsForm
Failed asserting that a boolean is not empty.

/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/drupal/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5TestBase.php:61
/var/www/html/drupal/core/modules/ckeditor5/tests/src/FunctionalJavascript/StyleTest.php:31
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

FAILURES!
Tests: 2, Assertions: 63, Failures: 1.

lauriii → reopened merge request !3012

Was just about to push a commit to fix this but looks like you beat me to it with the revert 😅 Pushed the fix to the MR.

@lauriii's fix matches what I proposed in #100. RTBC+1

Strange, seems the change happened 2 commits before the merge: https://git.drupalcode.org/project/drupal/-/merge_requests/3012/diffs?di...

Wondering how that can happen, was it the rebase? Thanks for the fix all!

Committed and pushed 2fcc756449 to 11.x. Thanks!

Spokje → closed merge request !3012

@Anybody, possible! The rebase was quite painfully, thanks for finding and fixing it!

Automatically closed - issue fixed for 2 weeks with no activity.