#3377030: Add validation constraint to `type: label`: disallow multiple lines added a basic constraint to type: label: disallowing multiple lines.
But … do we really want to allow any single-line string? 🤔 For example: do we want invisible control characters (think \x08 for "backspace": https://www.asciitable.com/) to be accepted?
Same thing for type: text, which is the multi-line sibling of type: label.
N/A
\PC. See https://stackoverflow.com/a/66587087.
\n (*nix line endings, part of Windows line endings)
\r (old macOS line endings, part of Windows line endings)
\t (tabs, cannot be entered via the browser, but are harmless and could conceivably be added when modifying config outside a browser context)
TBD
None.
None.
Labels and text in config will be guaranteed to be both sensible and safe if the config has been validated: no invisible control characters present, besides \r, \n and \t.
None.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !4567
Comments
Comment #2
wim leersHere's a starting point!
Comment #3
wim leersThis blocks #3379091: Make NodeType config entities fully validatable.
Comment #6
phenaproximaI'm guessing some tests will fail here, but in general I'm satisfied with this approach and I think we can start reviewing it.
Comment #7
wim leersComment #8
phenaproximaComment #9
borisson_This is great, it a very simple improvement of the validation and it has solid test coverage. It's also great to see that not in core at least breaks this. I'm going to leave this assigned to Wim so he can give signoff as well, but at least from my side it's rtbc.
Comment #10
wim leersI requested a bit of extra test coverage, to ensure we don't disallow the ever-important emojis 😇
I agree that this looks great beyond that!
And I have one rather important question for y'all: should we disallow control characters in all strings by default? 🤔
Comment #11
borisson_I don't think so, I know that at least for Search API we have a configuration to control "whitespace characters", that contains \t\n\r by default.
Comment #12
wim leers#11: sorry, #10 was not very clear. I think the default for
type: stringshould be "no control characters allowed, except for\rand\n" (it is clearer on the GitLab merge request).It's then up to individual projects to tweak the restrictions of
type: string. Just like this MR is already doing for translatable strings.While that should be the general rule (specific uses of
type: stringtweaking the validation constraint, and ideally defining their own type, because then it becomes a special kind of string!), I could see\talso being allowed by default, since it's A) safe, B) widely used.That being said … maybe it's better to postpone this
type: stringstuff to a later time —type: labelbeing stricter is a good step in the right direction, we can still move it up to that higher level in the inheritance chain later 😊Comment #13
phenaproximaOkay, added explicit test coverage of emoji!
Comment #14
wim leersLooks great! 😄
Comment #15
lauriiiMoving to NR to get the question on the MR answered
Comment #16
wim leers🏓
Comment #17
wim leers@lauriii pointed out that
\tis also reasonable to allow. Marking for that. Updated the issue summary too.Comment #18
phenaproximaComment #19
wim leers🤩 Love how little had to change — that shows how easy it is to evolve this, or copy/paste and tweak in other subtypes of
type: string! 👍Comment #20
phenaproximaComment #22
lauriiiCommitted 3a661cf and pushed to 11.x. Thanks!
Wondering if we should create one central CR for the additional label + text constraints 🤔