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

Simple patch, works flawlessly for me.

Thanks for the patch. But don't we want to do this on the server side too / instead?

Hrm. trim(). What if I convert Yahoo! to a machine name? Do we want yahoo or yahoo_?

All good here

It will be no EDIT link for CJK inputs

Yeah. My issue able to fix this problem. But seems no one interested my idea there.

So if this issue get committed, it will broken the field UI :(

Re #5:

Hrm. trim(). What if I convert Yahoo! to a machine name? Do we want yahoo or yahoo_?

I'd say in this case we want 'yahoo' only. The trailing underscore doesn't add any valuable information, yet ugliness. This is not what the user wants in more than 80% of use cases.
If the underscore really makes a difference, it can still be added manually to the machine name.

Also, re #10:
Why would this break anything that's not yet broken?
Currently, if all characters are substituted, the resulting machine name will be empty, unless manually set, therefore failing validation. After the patch this remains true.
So while this issue does fix it for human-readable names that contain at least one valid character, the other one would fix the case of no character being empty. Doesn't seem to break things.

Funny, just ran into this and yes, this would still be useful! Shouldn't be too hard, so I hope, me or someone else will find the time... First this needs a reroll and MR against latest Drupal Core.

Used the issue template and improved the IS a bit.

Switching to MR workflow.

Great work and nice improvement. Just reviewed the MR and it LGTM!

We could of course add further tests for other cases like underscores at start, dots, etc. - should not be hard, but the core maintainers should decide, if it's good enough.

\Drupal\system\MachineNameController::transliterate() should produce the same output - with this change it's not.

Also I'm pretty sure that we can simplify the regex and by stripping the characters after the and using ^([^a-z]+)|([^a-z]+)$

I.e. once the MachineNameController has the same behaviour do something like:

        if (baseValue.toLowerCase() !== expected) {
          timeout = setTimeout(function () {
            xhr = self.transliterate(baseValue, options).done(function (machine) {
              self.showMachineName(machine.substr(0, options.maxlength), data);
            });
          }, 300);
        } else {
          // Remove punctuations and whitespaces from beginning and end of string.
          const trashRE = new RegExp('^([^a-z]+)|([^a-z]+)$', 'g',);
          expected = expected.replace(trashRE, '');
          self.showMachineName(expected, data);
        }

Oh and we have \Drupal\Core\Block\BlockPluginTrait::getMachineNameSuggestion() too.

Implemented the suggestion by @alexpott! Furthermore, numbers are allowed inside a machine name, meaning they could be present at the beginning and end of a string as well! So we should use '^([^a-z0-9]+)|([^a-z0-9]+)$' instead of '^([^a-z]+)|([^a-z]+)$'!

Done, please review!

Thanks @Grevil for picking this up! I'd really love to see this fixed.

Changes look fine, but regarding #45 shouldn't that be:
^([^a-z]+)|([^a-z0-9]+)$

The machine name must start with a letter, I think? But may end in a number.

I'll change that and have a look if we should have further tests.

@alexpott, what would you say?

@Anybody, e.g. when creating a content type, the label generated machine name can also contain numbers at the start "3nduro Content Type" will lead to the machine name "3nduro_content_type" and for these edge cases I suggest keeping it like already implemented.

@Grevil: Thanks, that means, I was wrong. Then I'll leave this as-is and just add some further tests for special cases.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Appears there are some failures in the MR.

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:

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

Edit: Note, that the diff provided shows the original changes done, since @vasike implemented his changes towards commit id "f2abd5b9" on branch 10.1.x

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 #2662330: Machine name generation is way too slow.

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() JS function that was created as a replacement of the former PHP function. It should also do this job consistently.

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:
#2662330: Machine name generation is way too slow
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 #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) cover this?

@smustgrave #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) 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!

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 this potential fix

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.

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!

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