Problem/Motivation
#3341682: New config schema data type: `required_label` tried to add validation to type: label
. But it did so in a very broad way: disallow empty strings. That turned out to not be viable. So instead, it pivoted to introducing a new configuration schema data type: required_label
.
(So it is essentially an alias of the label
type with a NotBlank
constraint sprinkled on top.)
While wrapping up that issue, I realized that there actually is sensible validation we could add to type: label
too: disallowing multiple lines — i.e. disallow \n
and \r
. That validation constraint would then be inherited by type: required_label
too.
As a side benefit, this should surface (in combination with #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate which uses of type: label
should really be type: text
.
Steps to reproduce
Observe that:
contact.form.*:message
field.widget.settings.text_textarea_with_summary:placeholder
all use type: label
but really allow multiple lines of text — none of these are labels, they're just translatable multi-line strings.
Proposed resolution
Add RegEx
validation constraint to type: label
.
Remaining tasks
- Create merge request here that adds
RegEx
validation constraint totype: label
- Convert that MR into a patch that is applied on top of #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate (or better yet, hopefully that will land soon…) to allow detecting where all the incorrect uses of
type: label
are that should really betype: text
. - At a minimum, we'd expect failures for contact forms, menus and vocabularies (see "Steps to Reproduce")
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
None.
Issue fork drupal-3377030
Show commands
Start 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:
- 3377030-label-multiline-invalid changes, plain diff MR !4491
Comments
Comment #3
Wim LeersNow that #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate is in, we'd be able to instantly see how much effect this would have. Let's find out!
Comment #4
Wim LeersWow, zero violations, really?! Maybe the violations only occur in functional tests? 🤔
Verified again locally that this indeed works correctly by changing
core/profiles/standard/config/install/taxonomy.vocabulary.tags.yml
to have a multi-line description and runningStandardTest
. Fails as expected 👍So let's:
Comment #5
Wim LeersComment #6
borisson_This needs another rebase it seems. Very suprised that only a few things in core were giving errors on this. Not sure if all the tests have ran on the last time, there is a change in drupalci.yml that needs to be undone.
Comment #7
Wim LeersOops, yes, I forgot about that 🙈 Done! Probably hundreds of test failures now 😄
Comment #8
Wim LeersWell well … I guess almost nothing in Drupal core uses a multi-line label!
I really thought at least some contact form would trigger this, or a vocabulary description, or a menu description … but no! 😮
And sure enough, e.g.
core/profiles/demo_umami/config/install/contact.form.feedback.yml
uses a single line!Comment #9
borisson_This looks great, even better that nothing fails but we still improve strictness.
Comment #10
lauriiiShouldn't we remove the incorrect uses of
type: label
mentioned in the issue summary (or at least have a follow-up to do that)? The UIs for these certainly would enable creating invalid config at the moment.Comment #11
Wim Leers#10: works for me!
Turns out that
taxonomy.vocabulary.*:description
's UI actually does not allow multiple lines. So I'm wrong about that one. 🙈Same for
system.menu.*:description
! 🙈Both of these are apparently used in listing UIs and that's why they only allow a single line.
I am right about
contact.form.*:message
though. And I managed to find one more:field.widget.settings.text_textarea_with_summary:placeholder
.Since there are so few examples: let's fix them here? First pushed failing test coverage, then the fix (i.e. tweaking config schema).
Comment #12
smustgrave CreditAttribution: smustgrave at Mobomo commentedChange looks good and appears all green.
Question though is if changing to text does that mean it can be translated?
Comment #13
Wim LeersBoth
type: label
andtype: text
are translatable already:—
core.data_types.schema.yml
and:
👍😊
Comment #14
lauriiiCommitted b7ee3dc and pushed to 11.x. Thanks!