Add a plural-aware cardinality constraint to replace the custom message in FieldItemList cardinality validation

Created on 15 May 2019, over 5 years ago
Updated 25 July 2023, about 1 year ago

Problem/Motivation

FieldItemList uses the following code:

$constraints[] = $this->getTypedDataManager()
        ->getValidationConstraintManager()
        ->create('Count', [
          'max' => $cardinality,
          'maxMessage' => t('%name: this field cannot hold more than @count values.', ['%name' => $this->getFieldDefinition()->getLabel(), '@count' => $cardinality]),
        ]);

The uses the CountConstraint, but overrides the message to be more descriptive for field cardinality validation. The overridden message is a translateableMarkup object containing two parameters, name and count. Symfony violation messages are supposed to be strings, and in Symfony 4 they will be typecast to strings, so we need to make the parameters part of the constraint so that we will eventually be able to construct the correct markup later. Additionally, it does not properly handle the case of the count being 1, returning <Field Name>: this field cannot hold more than 1 values

Proposed resolution

Create a first class constraint for cardinality, with a proper pluralized template, that can eventually be refactored to work with Symfony 4

Remaining tasks

Do it

User interface changes

The user may now properly translate plurals for the cardinality error message.

Before

After

API changes

Create a new 'CardinalityConstraint'

Data model changes

none

Release notes snippet

📌 Task
Status

Needs review

Version

11.0 🔥

Component
Field 

Last updated 1 day ago

Created by

🇺🇸United States mikelutz Michigan, USA

Live updates comments and jobs are added and updated live.
  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States xjm
  • 🇺🇸United States xjm

    Thanks for working on this issue!

    As an API addition, this should have a change record. It also hasn't had a test run in nearly a year, so I queued that.

    @joaopauloc.dev, it's always helpful to provide the exact steps you took when manually testing something. If manually testing requires custom code, then it's helpful to provide that code as an embedded code snippet or file attachment.

    The way I know of to trigger this through the UI in core is with the Media Library module, because of 🐛 It is possible to overflow the number of items allowed in Media Library Fixed .

    1. Install 10.1.x with the Standard profile.
    2. Enable the Media and Media Library modules.
    3. Go to /admin/structure/types/manage/article/fields/add-field and select the media reference field type from the dropdown.
    4. Enter a label and click "Save and continue".
    5. Set "Allowed number of values" to 2 and click "Save field settings"
    6. Check "Create referenced entities if they don't already exist".
    7. Under "Media type", check "Image".
    8. Click "Save settings".
    9. Go to node/add/article and enter a title.
    10. Click the "Add media" button.
    11. Upload three new images in the Media Library dialog.
    12. Leave all three media items checked (the default) and click "Insert selected".
    13. Click "Save". You will get the error: "media: this field cannot hold more than 2 values."

    Since this deals with plurals, my main question is: Would it respect pluralization rules for languages where "plural" is more complicated than "one" versus "many"?

    The language I'm familiar with that has more complex plural grammar is Russian, so I tested HEAD in Russian. However, in the case of this sentence, the strings are the same. Apparently in the "more than N" construction, Russian always uses the genitive plural (otherwise the construction for "5 or more") rather than the genitive singular (the construction for "2, 3, or 4"). Because "more than" is an unspecified plural amount, it uses the same plural as "many" or any generic plural you don't know the exact count of. So the strings are the same. Currently:

    Сообщение об ошибке
    media: поле не может содержать больше чем 2 значений.

    which aside from field name and the number itself is identical to:

    Сообщение об ошибке
    test2: поле не может содержать больше чем 5 значений.

    The other language I know of that's more complicated is Hebrew, which has a dual, but I don't actually speak Hebrew and haven't tried to read the script since I was a child. I looked it up and apparently modern Hebrew only uses it for things that normally come in pairs, like eyes and ears. Then I thought, well, let's try another Semitic language... and hey, Arabic does have a dual. Tested in Arabic too... the strings are the same there as well as far as I can tell:

    لا يمكن لهذا الحقل أن يحتوي أكثر من 2 قيمة

    لا يمكن لهذا الحقل أن يحتوي أكثر من 5 قيمة

    That's the end of my knowledge on the subject (except the case of zero of something in French, which is not applicable since you can't set cardinality to zero).

    I then manually tested with the patch applied. Maybe should have done this in the first place. The new string is of course not translated yet, but it does include support for different plural forms:

    Before

    After

    Based on that, I'm confident in the change from a linguistic standpoint, at least! Nice work.

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States xjm
    1. +++ b/core/lib/Drupal/Core/Field/Plugin/Validation/Constraint/CardinalityConstraint.php
      @@ -0,0 +1,29 @@
      + * Cardinality constraint.
      

      Should be "Defines a constraint for field cardinality" or something according to the class API documentation standard :

      Use a third person verb to start the summary of a class, interface, or method. For example: "Represents a ..." or "Provides...".

    2. +++ b/core/lib/Drupal/Core/Field/Plugin/Validation/Constraint/CardinalityConstraint.php
      @@ -0,0 +1,29 @@
      +class CardinalityConstraint extends Constraint {
      

      What about extending Symfony's CountValidator?

    3. +++ b/core/lib/Drupal/Core/Field/Plugin/Validation/Constraint/CardinalityConstraint.php
      @@ -0,0 +1,29 @@
      +  public $message = '%name: this field cannot hold more than @limit value.|%name: this field cannot hold more than @limit values.';
      +  public $limit;
      +  public $name;
      

      These need property documentation, whether it's {@inheritdoc} or something else. (I realize that some other constraint properties in core are missing docblocks, but other constraints do have the property docs, so we shouldn't perpetuate the incorrect pattern.)

      Also, I think the word "This" should be capitalized here. Other constraints have capitalized sentences.

    4. +++ b/core/lib/Drupal/Core/Field/Plugin/Validation/Constraint/CardinalityConstraintValidator.php
      @@ -0,0 +1,46 @@
      + * Validator for the Cardinality constraint.
      

      "Validates the cardinality constraint."

    5. +++ b/core/lib/Drupal/Core/Field/Plugin/Validation/Constraint/CardinalityConstraintValidator.php
      @@ -0,0 +1,46 @@
      +class CardinalityConstraintValidator extends ConstraintValidator {
      

      What about extending Symfony's Count constraint?

    6. Finally, we should probably add unit test coverage for the new constraint and validator.

    Thanks!

  • 🇧🇷Brazil joaopauloc.dev

    Ok @xjm, sorry about that, the next time will provide more details about how I tested the issue.
    Thanks for the tips.

  • Issue was unassigned.
  • 🇺🇸United States dww

    @xjm linked me to #17 while we're trying to figure out the correct Plural-Forms value for Hawaiian. Fascinating, love it!

    Per #18.5, this also needs tests, so tagging for that, too.

    @mikelutz's last comment was 2019, so no need to call this "Assigned". 😉

  • Status changed to Needs review about 1 year ago
  • last update about 1 year ago
    29,612 pass, 45 fail
  • 🇺🇸United States mikelutz Michigan, USA

    Well, this issue is a blast from the past. :-). Addressed #18. Regarding 18.2, The reason we don't extend count (which the system is currently using) is because we want to add the field name to the list of arguments used to build the error message inside the validator. Extending count or range wouldn't help us because we would also have to extend the validator for either and override the entire validate method in order to build our own violation with the additional argument.

Production build 0.71.5 2024