ShortcutSetForm ID element description is wrong and misleads users

Changing issue status to "Need Review" since the patch is added

Fixed the CS issues in patch #2.

 144 | WARNING | Translatable strings must not begin or end with white spaces, use placeholders with t() for variables
 144 | ERROR   | Concatenating translatable strings is not allowed, use placeholders instead and only one string literal

Hi @Abhijith S ,
By apply this patch Machine Name element description updated with hypens.
Thanks

Reviewed whether the old patch still applies to the latest code and passes automated tests.

Steps taken to review:

Downloaded the latest patch file.

Used Git to clone local Git repository of 9.2.x and 10.0.x of the development version that the issue pertains to.
The patch #7 applied successfully.
The automated tests for the project were re-run after I clicked the "retest" link in the patch file, the automated test passed successfully.

Result:

Verified the patch applied successfully, and the automated test passed successfully.

The words "hyphens" and "underscores" are not translatable in this patch. It might be simpler to have two variants of the full help text instead of trying to insert the correct word.

I changed the code to have two variants of the entire sentence.

Hey, I can review it. =)

I applied the path #13 and it follows what is requested in the problem description, no errors on coding standard and also has the two variants of the help text, as requested. In my view, it's done.

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -129,10 +129,20 @@ public static function processMachineName(&$element, FormStateInterface $form_st
+      !empty($element['#machine_name']['replace']) &&
+      $element['#machine_name']['replace'] == '-') {
+      $description = 'A unique machine-readable name. Can only contain lowercase letters, numbers, and hyphens.';
+    }
+    else {
+      $description = 'A unique machine-readable name. Can only contain lowercase letters, numbers, and underscores.';
+    }
+
     // Apply default form element properties.
     $element += [
       '#title' => t('Machine-readable name'),
-      '#description' => t('A unique machine-readable name. Can only contain lowercase letters, numbers, and underscores.'),
+      '#description' => t($description),
       '#machine_name' => [],

The translation parser isn't able to pick up t($description). What you can do is call do $description = t('A unique...'); and then '#description' => $description.

Also can the '==' be '=== ' here?

Addressed the comment #16
Attached the patch and interdiff

Applied patch #17 successfully and it changes the description to hyphens. Adding screenshots for reference

verified this patch on Drupal 10.0.x.
https://www.drupal.org/files/issues/2022-07-13/3052479-17.patch

Testing Steps:
* created new instance of Drupal 10.0.x.
* go to /admin/config/user-interface/shortcut/add-set
* created shortcut set.
* view the machine name. (By default machine name convert white space to hyphen)
For example :
Name: test 1
Machine name: test-1

Test Result:
Machine name description line change from underscore to hyphen.

Refer SS
Before:

After:

Test result pass
Can be move to RTBC

I think we're doing this in the wrong place. The allowed characters are specified by the [#machine_name][replace_pattern] setting. What blocks does is this:

    $form['id'] = [
      '#type' => 'machine_name',
      '#maxlength' => 64,
      '#description' => $this->t('A unique name for this block instance. Must be alpha-numeric and underscore separated.'),
      '#default_value' => !$entity->isNew() ? $entity->id() : $this->getUniqueMachineName($entity),
      '#machine_name' => [
        'exists' => '\Drupal\block\Entity\Block::load',
        'replace_pattern' => '[^a-z0-9_.]+',
        'source' => ['settings', 'label'],
      ],
      '#required' => TRUE,
      '#disabled' => !$entity->isNew(),
    ];

Imo \Drupal\shortcut\ShortcutSetForm::form should set an appropriate description and be done. We should document on \Drupal\Core\Render\Element\MachineName that if you customise the replace_pattern you should override the #description.

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -129,10 +129,20 @@ public static function processMachineName(&$element, FormStateInterface $form_st
+    if (array_key_exists('#machine_name', $element) &&
+      !empty($element['#machine_name']['replace']) &&
+      $element['#machine_name']['replace'] === '-') {

Or to put the previous comment another way the $element['#machine_name']['replace'] in no way determines the allowed characters... that's down to replace_pattern and I don't think we want to build a regex parser that turns a regex into a human readable message of what characters are allowed.

I've done the changes requested in #22 and created a new patch. The machine name description now mentions hyphens instead of underscores. If there is anything that needs to be changed, please feel free to give me any kind of feedback!

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -37,7 +37,8 @@
+ *     '[^a-z0-9_]+'.When using a different character, 'replace_pattern'
+ *     needs to be set accordingly.

We need a space after the fullstop and it should say that when this is changed the description should be updated accordingly. So When using a different pattern, the machine name element's '#description' should be updated accordingly.

Made changes as per comment #25.

The changes seem to be in order with #25.
+1 to RTBC.

Tested patch from #26 on drupal 10.0.x and everything seems to be working as intended.

I was able to verify that the description has changed to "A unique machine-readable name. Can only contain lowercase letters, numbers, and hyphens." and the machine name follows this pattern.

Moving this issue to RTBC since the request from #25 was adressed.

I'm unsure of what to do with the screenshot credit for #9, #19 and #20. #20 is a good screenshot comment and the other screenshot comments whilst could have had more comment but the screenshots all came at a useful time. So I'm going to leave the credit.

+++ b/core/modules/shortcut/src/ShortcutSetForm.php
@@ -28,6 +28,7 @@ public function form(array $form, FormStateInterface $form_state) {
+      '#description' => 'A unique machine-readable name. Can only contain lowercase letters, numbers, and hyphens.',

This should be a translatable string. We should use $this->t().

Made changes as per #30, Thanks

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. 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 request as a guide.

As a bug it will need a test case.

I converted #31 to an MR and added an assertion for the updated machine name element description.

Won’t this require updating machine_name schema now too?

Just reading the proposed solution “check replacement key…” not sure that’s been implemented.

Title made me think that hyphens are valid machine names and is that true? What will that break

For this we only need to replace -with _in the replace_patternand replacekeys of the machine_nameelement of the core/modules/shortcut/src/ShortcutSetForm.php.
This will allow lowercase letters, numbers, and underscores, and the "replace" key will replace other special characters with _.

Please review.

Should be a test around that change. Probably can drop the test for the description

Also I’m not sure hyphen is a valid machine character

Will try to add test coverage, if hyphen(-) is used in the machine name, it will get replaced by underscore(_).

The issue summary explicitly says "Shortcut machine names require hyphens." If this is being undone, then justification for that needs to be given and the IS should be updated accordingly. Please ensure that proper research into this change has been performed.

Just reading the proposed solution “check replacement key…” not sure that’s been implemented.

The IS needs to be updated. I forgot to do it. In #22 @alexpott outlined the solution: any element that overrides the replacement pattern should also override the description to describe the change. In #23 he clarified that statement to say that we don't want to get into the business of trying to build a regex parser to do that work automatically.

Thus, the solution to this issue is to have the Shortcut's machine name element override the description and also update the machine name Element's docs to add text saying "if you override the replacement pattern, then you also need to override the description."

Title made me think that hyphens are valid machine names and is that true?

All I know is that the IS says: "Shortcut machine names require hyphens." And I didn't look into why, but currently the element does replace disallowed characters with hyphens. I'm guessing there's no test coverage around it since the last change made by @prashant.c didn't break anything.

I updated the IS and issue title.

For the record, there's no restriction on using hyphens in machine names, other than what's defined in the element's replace_pattern. This is easy enough to test. Just make a Shortcut Set at /admin/config/user-interface/shortcut/add-set that contains spaces or other disallowed characters. The disallowed characters get replaced with hyphens with no problem. Save it and then view it in the config export page. There's no problem when you export the config to files either. It's saved with hyphens no problem.

I reverted @prashant.c's changes because they were out-of-scope. Then I added more assertions that to verify that hyphenated machine names are allowed and valid, but underscores are not.

Thank you, @dcam, maybe I misunderstood the issue, so now hyphens are allowed whereas underscores are not.

I also misunderstood this ticket so apologize @prashant.c and @dcam.

So this is actually pretty straight forward and may not need the test coverage. Lets see what the committers think.

I'd classify this as normal.

Making the title a bit stronger.

Removing credits for redundant manual testing.

Queued fresh pipelines to run the test-only job, since I learned today it expires if the pipeline is more than a week old. (Contributors, you can do this from the "pipelines" local task on the MR page. The link for it is above the diff UI, not the one in the sidebar.)

Test-only results:

There was 1 failure:
1) Drupal\Tests\shortcut\Functional\ShortcutSetsTest::testShortcutSetAdd
Behat\Mink\Exception\ResponseTextException: The text "A unique machine-readable name. Can only contain lowercase letters, numbers, and hyphens." was not found anywhere in the text of the current page.
/builds/issue/drupal-3052479/vendor/behat/mink/src/WebAssert.php:907
/builds/issue/drupal-3052479/vendor/behat/mink/src/WebAssert.php:293
/builds/issue/drupal-3052479/core/tests/Drupal/Tests/WebAssert.php:981
/builds/issue/drupal-3052479/core/modules/shortcut/tests/src/Functional/ShortcutSetsTest.php:58
FAILURES!
Tests: 12, Assertions: 163, Failures: 1.

That's good, but should we also be asserting that the shortcut set was not saved? Generally asserting error messages by itself isn't robust enough. So I just think we need to add one more assertion above this one about the entity being unchanged. Might have to clone it or whatnot.

There is also my above suggestion on the MR.

Thanks!

Removed leading space from title.
Proofread all strings and added one extra comment to the MR.

That's good, but should we also be asserting that the shortcut set was not saved? Generally asserting error messages by itself isn't robust enough.

@xjm that assertion isn't testing for an error message. It's checking for the unique machine name element description. Since the focus of this issue is specifically about that description being incorrect I wrote the assertion to check for the corrected version. But since you asked, I also included assertions to check for whether a hyphenated machine name and another attempt to save an underscored machine were correctly saved. This was because a misunderstanding starting at #37 revealed that there was no test coverage for the hyphenated naming scheme.

I addressed all feedback. Since the only changes were to code comments, one of which was written and suggested by a release manager, I'm going to be bad and self-RTBC this.

edit: sorry, I feel really stupid now. There is an assertion for the error message.

Sorry, I'm clueless. I missed something important there.

Ok, so I wasn't entirely stupid. The failing test that @xjm referenced is for the machine name element description. But there is a different assertion further down that checks for an error message. The description and error message have nearly identical text, resulting in the confusion we both had. @xjm was correct to request that an additional assertion be added. However...

So I just think we need to add one more assertion above this one about the entity being unchanged.

There's no entity being changed here. It's an attempt to create a new shortcut set with underscores in the ID. It should be sufficient to check that an attempt to load the set returns NULL, which is what I added.

There's no entity being changed here. It's an attempt to create a new shortcut set with underscores in the ID. It should be sufficient to check that an attempt to load the set returns NULL, which is what I added.

Right, sorry, I was writing as if the shortcut set already existed, but what I should have said (what I was trying to say) is that no changes should be saved for the shortcut set since it was failing validation. And in the end that's what you tested. Sorry for misleading review comments and glad you were able to translate what I meant in the end!

Hiding patches as fix is in MR.
Applied @xjm suggestion for a comment, also rebased the MR as it was 500+ commits back

Tests are still green
Test only ran

1) Drupal\Tests\shortcut\Functional\ShortcutSetsTest::testShortcutSetAdd
Behat\Mink\Exception\ResponseTextException: The text "A unique machine-readable name. Can only contain lowercase letters, numbers, and hyphens." was not found anywhere in the text of the current page.
/builds/issue/drupal-3052479/vendor/behat/mink/src/WebAssert.php:907
/builds/issue/drupal-3052479/vendor/behat/mink/src/WebAssert.php:293
/builds/issue/drupal-3052479/core/tests/Drupal/Tests/WebAssert.php:981
/builds/issue/drupal-3052479/core/modules/shortcut/tests/src/Functional/ShortcutSetsTest.php:58
FAILURES!
Tests: 12, Assertions: 163, Failures: 1.
Exiting with EXIT_CODE=1

Feedback appears to be addressed on this one.

Committed and pushed 34fa591813d to 11.x and febc710f53b to 11.2.x. Thanks!

Backported to 11.2.x because this is a bugfix.