Add validation constraint to `type: label` + `type: text`: disallow control characters

Here's a starting point!

This blocks 📌 [PP-1] Make NodeType config entities fully validatable Needs review .

I'm guessing some tests will fail here, but in general I'm satisfied with this approach and I think we can start reviewing it.

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.

I 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? 🤔

And I have one rather important question for y'all: should we disallow control characters in all strings by default? 🤔

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.

#11: sorry, #10 was not very clear. I think the default for type: string should be "no control characters allowed, except for \r and \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: string tweaking the validation constraint, and ideally defining their own type, because then it becomes a special kind of string!), I could see \t also being allowed by default, since it's A) safe, B) widely used.

That being said … maybe it's better to postpone this type: string stuff to a later time — type: label being stricter is a good step in the right direction, we can still move it up to that higher level in the inheritance chain later 😊

Okay, added explicit test coverage of emoji!

Looks great! 😄

Moving to NR to get the question on the MR answered

🏓

@lauriii pointed out that \t is also reasonable to allow. Marking for that. Updated the issue summary too.

🤩 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! 👍

Committed 3a661cf and pushed to 11.x. Thanks!

Wondering if we should create one central CR for the additional label + text constraints 🤔

lauriii → closed merge request !4567

Automatically closed - issue fixed for 2 weeks with no activity.