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:

  1. contact.form.*:message
  2. 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

  1. Create merge request here that adds RegEx validation constraint to type: label
  2. 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 be type: text.
  3. 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

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Now 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!

Wim Leers’s picture

Wow, 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 running StandardTest. Fails as expected 👍

So let's:

  1. Add explicit test coverage.
  2. Run the full test suite again.
Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
borisson_’s picture

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.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work

Oops, yes, I forgot about that 🙈 Done! Probably hundreds of test failures now 😄

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review

Well 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!

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, even better that nothing fails but we still improve strictness.

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Shouldn'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.

Wim Leers’s picture

Issue summary: View changes

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Change looks good and appears all green.

Question though is if changing to text does that mean it can be translated?

Wim Leers’s picture

Question though is if changing to text does that mean it can be translated?

Both type: label and type: text are translatable already:

# Human readable string that must be plain text and editable with a text field.
label:
  type: string
  label: 'Label'
  translatable: true

…
# Human readable string that can contain multiple lines of text or HTML.
text:
  type: string
  label: 'Text'
  translatable: true

core.data_types.schema.yml

and:

function config_translation_config_schema_info_alter(&$definitions) {
  $map = [
    'label' => '\Drupal\config_translation\FormElement\Textfield',
    'text' => '\Drupal\config_translation\FormElement\Textarea',
    'date_format' => '\Drupal\config_translation\FormElement\DateFormat',
    'text_format' => '\Drupal\config_translation\FormElement\TextFormat',
    'mapping' => '\Drupal\config_translation\FormElement\ListElement',
    'sequence' => '\Drupal\config_translation\FormElement\ListElement',
    'plural_label' => '\Drupal\config_translation\FormElement\PluralVariants',
  ];

👍😊

lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed b7ee3dc and pushed to 11.x. Thanks!

  • lauriii committed b7ee3dcf on 11.x
    Issue #3377030 by Wim Leers, borisson_, smustgrave: Add validation...

Status: Fixed » Closed (fixed)

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