- 🇺🇸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.
- 🇺🇸United States dcam
I converted #31 to an MR and added an assertion for the updated machine name element description.
- 🇺🇸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 thereplace_pattern
andreplace
keys of themachine_name
element.
This will allow lowercase letters, numbers, and underscores, and the "replace" key will replace other special characters with_
.Please review.
- 🇺🇸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. - 🇺🇸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.