Machine Name element description should allow for cases where machine name uses hyphens

Created on 3 May 2019, about 6 years ago
Updated 18 February 2023, over 2 years ago

Problem/Motivation

When adding a new shortcut set, the description on the machine name field uses the machine name element default ("A unique machine-readable name. Can only contain lowercase letters, numbers, and underscores."), but Shortcut machine names require hyphens. The machine name element validator checks the 'replace' key in the form element for '-' vs '_' and changes the validation text accordingly, but the description doesn't change accordingly (see attached screenshot). To reproduce, go to /admin/config/user-interface/shortcut/add-set and click the edit link by machine name after entering a Set name.

Before

After

Proposed resolution

Check for replace key in element in the processMachineName method of the renderer, update description text accordingly.

🐛 Bug report
Status

Needs work

Version

10.0

Component
Render 

Last updated about 8 hours ago

Created by

🇺🇸United States hart0554

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States smustgrave

    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.

  • First commit to issue fork.
  • Merge request !11948Converted #31 to an MR and added a test → (Open) created by dcam
  • 🇺🇸United States dcam

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

  • Pipeline finished with Success
    2 months ago
    Total: 1017s
    #482744
  • 🇺🇸United States smustgrave

    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

  • 🇮🇳India prashant.c Dharamshala

    prashant.c made their first commit to this issue’s fork.

  • 🇮🇳India prashant.c Dharamshala

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

    Please review.

  • Pipeline finished with Canceled
    2 months ago
    #483648
  • Pipeline finished with Success
    2 months ago
    #483652
  • 🇺🇸United States smustgrave

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

  • 🇺🇸United States smustgrave

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

  • 🇮🇳India prashant.c Dharamshala

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

  • 🇺🇸United States dcam

    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.

  • 🇺🇸United States dcam

    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.

  • 🇺🇸United States dcam

    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.

  • Pipeline finished with Success
    2 months ago
    Total: 463s
    #484920
  • 🇺🇸United States dcam

    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.

  • 🇮🇳India prashant.c Dharamshala

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

  • 🇺🇸United States smustgrave

    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.

  • Status changed to RTBC 3 days ago
  • 🇺🇸United States xjm

    I'd classify this as normal.

  • 🇺🇸United States xjm

    Making the title a bit stronger.

    Removing credits for redundant manual testing.

  • 🇺🇸United States xjm

    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.)

  • Pipeline finished with Success
    3 days ago
    Total: 805s
    #533948
  • Pipeline finished with Success
    3 days ago
    Total: 1153s
    #533949
  • 🇺🇸United States xjm

    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.

    Thanks!

  • 🇧🇪Belgium BramDriesen Belgium 🇧🇪

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

  • Pipeline finished with Canceled
    1 day ago
    Total: 464s
    #535321
  • Pipeline finished with Success
    1 day ago
    Total: 554s
    #535327
  • 🇺🇸United States dcam

    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.

  • 🇺🇸United States dcam

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

  • 🇺🇸United States dcam

    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.

  • Pipeline finished with Success
    1 day ago
    Total: 744s
    #535345
  • 🇺🇸United States xjm

    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 did. Sorry for misleading review comments and glad you were able to translate what I meant in the end!

Production build 0.71.5 2024