- 🇺🇸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 .
- Install 10.1.x with the Standard profile.
- Enable the Media and Media Library modules.
- Go to
/admin/structure/types/manage/article/fields/add-field
and select the media reference field type from the dropdown. - Enter a label and click "Save and continue".
- Set "Allowed number of values" to 2 and click "Save field settings"
- Check "Create referenced entities if they don't already exist".
- Under "Media type", check "Image".
- Click "Save settings".
- Go to
node/add/article
and enter a title. - Click the "Add media" button.
- Upload three new images in the Media Library dialog.
- Leave all three media items checked (the default) and click "Insert selected".
- 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 7:18pm 23 January 2023 - 🇺🇸United States xjm
-
+++ 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...".
-
+++ 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?
-
+++ 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.
-
+++ b/core/lib/Drupal/Core/Field/Plugin/Validation/Constraint/CardinalityConstraintValidator.php @@ -0,0 +1,46 @@ + * Validator for the Cardinality constraint.
"Validates the cardinality constraint."
-
+++ 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?
-
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
- Status changed to Needs review
about 1 year ago 7:23pm 25 July 2023 - 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.