Problem/Motivation

in #3379102: Add validation constraint to `type: label` + `type: text`: disallow control characters, a constraint that prevents adding special characters to a label has been added to label core schema. But plural_label (which inherits the label schema) contains, by design, at least one \x03 special character.

In core, there's only views.field.numeric:format_plural_string extending plural_label. In #3379102: Add validation constraint to `type: label` + `type: text`: disallow control characters, views.field.numeric:format_plural_string schema has been defined as:

    format_plural_string:
      type: plural_label
      label: 'Plural variants'
      constraints:
        Regex:
          # Normally, labels cannot contain invisible control characters. In this particular
          # case, an invisible character (ASCII 3, 0x03) is used to encode translation
          # information, so carve out an exception for that only.
          # @see \Drupal\views\Plugin\views\field\NumericField
          pattern: '/([^\PC\x03])/u'
          match: false
          message: 'Labels are not allowed to span multiple lines or contain control characters.'

But this is not quite correct, as all schemas extending plural_label should benefit from this relaxed constraint, not only views.field.numeric:format_plural_string.

I came to this in #2765065: Allow plurals on bundle labels, as suddenly teste started to fail.

Steps to reproduce

See #2765065: Allow plurals on bundle labels

Proposed resolution

Move the constraint from views.field.numeric:format_plural_string, upstream, in plural_label.

Question: Should we enforce at least one \x03 in plural_label?

As a side note, plural_label is semantically a sequence. Why is serialized as a string by concatenating the sequence items? A more correct fix is #2829919: Either avoid or explicitly test binary encoding in default configuration but that is too complex to be handled on short term. I think we should go with this for now.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

plural_label will get the correct constraint.

Release notes snippet

Issue fork drupal-3409588

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:

About issue forks

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea opened merge request !5868

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Needs review

Ready for review

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Related issues: +#2765065: Allow plurals on bundle labels
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Move views.field.numeric:format_plural_string constraint upstream, in plural_label » Add validation constraint to type: plural_label; remove work-around from views.field.numeric:format_plural_string
Category: Bug report » Task
Status: Needs review » Needs work
Issue tags: +validation, +Needs tests

You're right. In the parent issue, we focused on type: label and just exempted this one exception (see https://git.drupalcode.org/project/drupal/-/merge_requests/4567/diffs?co...).

I think we can generalize the scope of this issue though 🤓

We should expand the test coverage in \Drupal\KernelTests\Core\Config\SimpleConfigValidationTest::testSpecialCharacters() (which was added in #3379102: Add validation constraint to `type: label` + `type: text`: disallow control characters).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +blocker

This blocks #2765065: Allow plurals on bundle labels.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue tags: -Needs tests

Tests added. Removing tag

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Tests green. Ready for review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative 
1) Drupal\KernelTests\Core\Config\SimpleConfigValidationTest::testSpecialCharacters with data set "plural label" ('config_test.with_plural_label', 'label', Binary String: 0x03, null)
Failed asserting that actual size 1 matches expected size 0.
/builds/issue/drupal-3409588/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-3409588/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-3409588/core/tests/Drupal/KernelTests/Core/Config/SimpleConfigValidationTest.php:149
/builds/issue/drupal-3409588/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 69, Assertions: 206, Failures: 1.

Shows the test coverage for the change.

The moving of the constraint makes sense to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Let's test the other side of this. That other characters result in an error. Plus it would be great if the test content looked a little more realistic that just the character.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Let's test the other side of this. That other characters result in an error.

Just that the error message could be misleading because it might refer to \x03

dxvargas’s picture

dxvargas commented
Related issues: +#3533926: Config of the type plural_label can never be valid due to the label constraint

The MR here has conflicts after https://www.drupal.org/project/drupal/issues/3533926.
Adding this related issue here.

dxvargas’s picture

dxvargas commented
Title: Add validation constraint to type: plural_label; remove work-around from views.field.numeric:format_plural_string » Test validation constraint to type: plural_label in core.data_types.schema

The changes in the first commit of the MR here are the same as the changes already done in core after the MR in #3533926.
Like that, the only changes that we can keep here are the introduction of a test.
I'll adapt the title of this issue accordingly.

The new test is failing, so this still needs work.

dxvargas’s picture

dxvargas commented
Status: Needs work » Needs review

The test is fixed. It had an unrelated violation ( contains a translatable label, so a `langcode` is required, resulting in a violation of \Drupal\Core\Config\Plugin\Validation\Constraint\LangcodeRequiredIfTranslatableValuesConstraint).
Moving this issue to Needs review.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me, this adds the needed test coverage.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Also, some credits, I think should be added in #3533926: Config of the type plural_label can never be valid due to the label constraint for the devs doing the initial code here, code that was the same as in #3533926: Config of the type plural_label can never be valid due to the label constraint (@wim leers, @smustgrave and me)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@claudiu.cristea I've creditted everyone on that issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed a22092a and pushed to 11.x. Thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • alexpott committed a22092a1 on 11.x 
    [#3409588] task: Test validation constraint to type: plural_label in...

Status: Fixed » Closed (fixed)

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