UniqueFieldValueValidator works only with single value fields

I agree. It doesn't make any sense if it can only check the first item in the list..

A quick patch attached :-)

There was an error in one line.

One typo error fixed.

Great! One comment:

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
@@ -29,10 +29,14 @@ public function validate($items, Constraint $constraint) {
+    foreach ($items->getIterator() as $item) {

You should be able to do only:

forach ($items as $item)

And it would be great to add test coverage.

Removed get->Iterator(), still working on tests.

Still working on this patch.

I need some guidelines regarding drupalPostForm(), when I add first values for testing there is an exception thrown, and I can not figure out what the problem is exactly.

1. +++ b/core/modules/system/src/Tests/Validation/UniqueValuesConstraintValidatorTest.php
   @@ -0,0 +1,55 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp() {
   +    parent::setUp();
   +  }
   +
   

   No need to implement setUp() with an empty body. Simply remove it here and parent's implementation will be used.

2. +++ b/core/modules/system/src/Tests/Validation/UniqueValuesConstraintValidatorTest.php
   @@ -0,0 +1,55 @@
   +    $entity = EntityTestUniqueConstraint::create(['type' => 'foo']);
   

   "foo" type doesn't exist for EntityTestUniqueConstraint entity. This is one of the reasons for fatals you are seeing.

   The nicest solution would be to remove "type" from base fields definition entirely and pass empty value here.

   You can also set field_test_text here if needed.

3. +++ b/core/modules/system/src/Tests/Validation/UniqueValuesConstraintValidatorTest.php
   @@ -0,0 +1,55 @@
   +    $entity = EntityTestUniqueConstraint::create(['type' => 'foo']);
   
   +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestUniqueConstraint.php
   @@ -0,0 +1,69 @@
   + *     "view_builder" = "Drupal\entity_test\EntityTestViewBuilder",
   + *     "access" = "Drupal\entity_test\EntityTestAccessControlHandler",
   + *     "form" = {
   + *       "default" = "Drupal\entity_test\EntityTestForm"
   + *     },
   

   This classes don't exists. You can most likely use base classes provided by core instead.

4. +++ b/core/modules/system/src/Tests/Validation/UniqueValuesConstraintValidatorTest.php
   @@ -0,0 +1,55 @@
   +    $this->drupalPostForm('entity_test_unique_constraint/manage/' . $entity->id() . '/edit', $edit, t('Save'));
   

   Another fatal here. You are trying to access a route that is not defined. You basically have two solutions here:
   - define route for this URL or
   - test constraint programatically (update fields and save instead of post to form).

   Constraints don't require forms to work. Even if you manipulate entities programatically they make sure your data is correct.

#15.1 - fixed.
#15.2 and #15.4 - still working.
#15.3 - I found those classes with the same namespaces I use in my code, so now I'm not sure do I need to change them or not?

Finished all the issues and tested locally, needs review.

Test with fix patch.

Test only patch.

This should fix the last strange test fail.

This is identical to #25.

And RTBC one more time.

1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
   @@ -29,10 +29,14 @@ public function validate($items, Constraint $constraint) {
   +    $values = array();
   +    foreach ($items as $item) {
   +      $values[] = $item->value;
   +    }
   

   Hardcoding ->value is problematic but this issue doesn't make it worse than it already is.

   For example, this won't work on an entity reference field, since the main property is target_id there, not value. If you want to do it correctly, call getMainPropertyName() from the field storage definition.

   Not sure if we want to fix that here but when we touch the code anyway, we might as well do it correctly? Would have to change the test to use an entity_reference field type to make sure this works.

2. +++ b/core/modules/system/src/Tests/Validation/UniqueValuesConstraintValidatorTest.php
   @@ -0,0 +1,62 @@
   +    // Add field with two values.
   +    $definition = ['field_test_text' => ['text1', 'text2']];
   +    $entity = EntityTestUniqueConstraint::create($definition);
   ...
   +
   +    // Add another field with invalid values.
   +    $definition = ['field_test_text' => ['text2', 'text1']];
   +    $entity = EntityTestUniqueConstraint::create($definition);
   

   Does this really fail?

   If we use the first value only, then we use text2, but that will match the existing value in the database?

   I would have expected the test to use test5, test1 so that we are sure that the non-first value is really used?

3. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestUniqueConstraint.php
   @@ -0,0 +1,61 @@
   +/**
   + * Defines a test entity class for unique constraint.
   + *
   + * @ContentEntityType(
   + *   id = "entity_test_unique_constraint",
   + *   label = @Translation("unique field entity"),
   ...
   + *   base_table = "entity_test_unique_constraint",
   + *   data_table = "entity_test_unique_constraint_data",
   

   I'm worried about the amount of entity types we have in entity_test. Every time we enable that module, at least in web test, we have to create dozens of tables for every single one of them. So adding more makes our tests slower.

   Don't have a good alternative though, other than putting it in a separate test module.

   Do we really need a data table for this, though?

Setting back to needs work so that #38 can be addressed.

Added #3027745: UniqueFieldConstraint doesn't work for entities with string IDs as related issue.

Add #2999225: Unique field constraint does not validated field values on delta greater than 0 as related issue.
It may seems to be a duplicate nop ?

I see that #2999225: Unique field constraint does not validated field values on delta greater than 0 has a working patch, what should we do ?

1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
   @@ -17,27 +17,54 @@ class UniqueFieldValueValidator extends ConstraintValidator {
   +        $this->context->addViolation($constraint->message, [
   +          '%value' => $item->value,
   +          '@entity_type' => $entity->getEntityType()->getLowercaseLabel(),
   ...
   +        ]);
   

   I think we should limit this to the specific field delta that does not pass validation instead of setting a validation error on a field. It becomes UX problematic if you use a multi-value field.

   This snippet works fine for me:

           $this->context->buildViolation($constraint->message, [
             '%value' => $item->value,
             '@entity_type' => $entity->getEntityType()->getLowercaseLabel(),
             '@field_name' => mb_strtolower($items->getFieldDefinition()->getLabel()),
           ])->atPath($delta)->addViolation();
   

2. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
   @@ -17,27 +17,54 @@ class UniqueFieldValueValidator extends ConstraintValidator {
   +        break;
   

   Also, I think the validation error becomes more useful if we run it on each delta value so a user can fix all errors at once. Thus, we should remove break.

Rerolled patched for 9.0.x, also checked to ensure it works against 8.9.x

Also removed the break line.

Rewrite patches 59 && 52 and rewrite the test for this patch.

Rewrote patch 60 according to Drupal coding standards.

Also, retrieve the break. Since if there are two identical values in entities, there is no point in further validating the type.

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestUniqueConstraint.php
@@ -0,0 +1,60 @@
+    $fields['field_test_reference'] = BaseFieldDefinition::create('entity_reference')
+      ->setLabel(t('unique_reference_test'))
+      ->setCardinality(2)
+      ->addConstraint('UniqueField')
+      ->setSettings([
+        'target_type' => 'node',
+        'handler' => 'default:node',
+        'handler_settings' => [
+          'target_bundles' => [
+            'article',
+          ],

this is breaking a ton of tests. this shouldn't use node as target but some other test entity type.

Patch 63 hoses the ability to edit existing user accounts. On a vanilla sandbox site any attempt to do so results in an error like this one:

    The username admin is already taken.
    The email address admin@example.com is already taken.

New patch. It could probably be improved in terms of coding standards.

I have not tested it against entity reference fields. I'm also completely unfamiliar with writing tests so I haven't touched them.

Coding standards...

Fix undefined variable notice.

I think string assertion errors like this one are due to the line ->atPath($delta) which is only necessary on multivalue fields. Updating patch to account for that. Hopefully this run will narrow down the errors to only those about the missing node test entity type.

Edit: That got rid of most of them, except for this one, which comes from tests added by the patch itself. Since we're now passing the delta so that the error can point to a specific item that's problematic rather than highlighting every item in the field, it should be fine to modify the test to assert against the new value.

I think the only other error which is not related to the missing node:article entity is this which seems to indicate that this patch causes string IDs to no longer validate.

I locally ran a bunch of the tests that failed to hammer this out. Big interdiff incoming.

Wrong interdiff.

I haven't been able to get functional tests running locally, so not sure how to debug PrepareUninstallTest -- although it might be fixed if I update the dependencies to remove drupal:node. Also, the extra error on 9.2.x might be fixed by adding drupal:user as a dependency. The jsonapi error has me stumped.

Adding a @todo referencing 3029782 since that's marked RTBC and this patch will need to be rerolled to incorporate it if that gets in first.

Note that this patch addresses 2973455 and https://www.drupal.org/project/drupal/issues/2999225

I'm including an interdiff for #63 as well.

Updating the patch to remove a few references to node that I missed.

A few questions for any reviewer that comes along who happens to know their stuff:

1. Would it be possible to get this backported to 9.2.x, and if so, how do I go about fixing this strange database failure? (assuming this updated patch doesn't fix it)

2. Is there a "more correct," futureproof, Drupal-y way to perform this check for ->getCardinality()?:

    // Determine the main property name of the field what we will check.
    $field_storage_definitions = \Drupal::service('entity_field.manager')->getFieldStorageDefinitions($entity_type_id);
    $property_name = $field_storage_definitions[$field_name]->getMainPropertyName();
    if (get_class($field_storage_definitions[$field_name]) == 'Drupal\Core\Field\BaseFieldDefinition') {
      $cardinality = $field_storage_definitions[$field_name]->getFieldStorageDefinition()->getCardinality();
    }
    else {
      $cardinality = $field_storage_definitions[$field_name]->getCardinality();
    }

The reason this if/else is needed is because some fields, for example username and email on a User entity, will have the class Drupal\Core\Field\BaseFieldDefinition, while other fields -- such as a field added to an entity through the UI -- have the class Drupal\field\Entity\FieldStorageConfig. This is touched on a bit in docs for getFieldStorageDefinitions which notes that the former is a "base field" while the latter is a "bundle field."

Simplify cardinality/multivalue check (addresses question 2 above).

9.2.x is still failing for some reason but the patch should be good to apply to 9.2 sites if it's needed.

Previous patches did not prevent entities from having multiple identical values within a field upon initial creation. Checking the current entity's field values does not require a database query; it should only require checking the $items that have been passed. Also added tests for this scenario.

edit: Interdiff filename should be 63-78 not 63-77

Previous patches stopped validating after encountering a single violation. The UI will probably stop multiple violations from occurring when entering data into a single multivalue field, but it's definitely possible when working programmatically. It's probably a good idea to report all violations. If that's not the case and we only want to report one violation at a time then patch 78 is fine.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rerolling...

Addressing issues that came up in tests.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I'm not sure how to get the bot to not test the 9.5.x patch against 10.1.x, so not gonna fight with it.

If someone tries to review this and finds that any of the patches really do not apply, then poke me.

I've converted the 10.1.x patch to an MR. Hopefully the NR bot checks that instead of the 9.5 and 10.0 patch files.

Verified the issue by using the snippet in the issue summary.
Added it to the node.module for testing
Used the Article content type from the standard install
Added a simple test filed, set to unlimited
Created an Article and added Test to both fields
Saved fine

Applied patch
Edited node
Tried saving again
Validation error was thrown, as expected.

Looking good, left some comments on the MR, thanks!

I addressed all MR threads to the best of my ability.

Appears all of @larowlans threads have been resolved.

Hate to send my own work back to needs review, but I mulled over one of larowlans suggestions and decided to properly implement it to reduce redundant looping during the "own duplicate value" check. See commit e71fcaca

Update looks good to me. Lets move to committers queue.

Looking good, left some comments on the MR about coding standards (minor) and one curly one about the update path here for sites with invalid data.

I'll ask the release managers what their thoughts are with respect to that.

I've asked release managers which of these options is the best

a) Do nothing, users will need to fix invalid duplicates next time they save content
b) Some sort of report for users - this will have performance issues - to generate it we'd probably need a background process
c) Some sort of update hook - this could cause data loss issues so feels risky

Discussed with @lauriii and he agreed a) felt best, but we'd also need a change record and release note snippet (as we'd want to highlight this in release notes).

Tagging as such

I'll wait for a release manager to confirm a) is the best approach and report back either way

Updated the MR for coding standards.

My personal opinion regarding this change and its implications for site content editors is that the original functionality for this constraint was not intended to work with multi-valued fields. The only way for this constraint to apply to multi-valued fields would be for a developer to write a module that adds the constraint to the field. I don't believe this constraint is used in core in any way on multi-valued fields, and I can't imagine someone hacking the core user email field to be multi-valued and then using this constraint -- but if they have, they've probably already found it to be broken and applied one of the above patches.

Describing the issue in the change record and noting the situations in which content editing would be adversely affected necessarily requires pointing out that this constraint is being used in a way it wasn't intended, resulting in either not achieving the desired result in the first place or supporting a very specific use case. For the latter, we should instruct developers to write and implement their own constraint that mimics the old functionality.

Edit: It might be better to say that the method "was not supported" rather then "not intended." The original code was simply written too narrowly, supporting only single-valued fields with value as the main property.

I discussed with @catch who also agreed that a) is the best approach

I added a draft change record and release note snippet

This is ready for review again

Change record also needs to mention that the validator will now also work with additional field types, such as Entity Reference fields.

This sentence is also not 100% accurate:

Prior to 10.2.0 the validator only supported single-value fields and would not detect duplicates for fields with more than one value.

It would actually check for duplicates, but only in the first entry (delta) for each field. If someone really wanted to enter duplicate values they could "work around" the constraint by simply making sure the first value is different. It's feasible this unsupported behavior could have been used to hack together functionality where the first value in a field would be a "key." So the CR should mention that if the original behavior was deliberately used in this way then a dev would need to write a new constraint themselves.

This is how I think it should read:

The UniqueFieldValueValidator provides a constraint that can be added to a field to ensure that all values in the field are unique to that entity.

Drupal core uses this constraint for the User's name and mail fields.

Prior to 10.2.0 the validator only supported single-value fields and would not detect all duplicates for fields with more than one value. The first entry in the field would be validated against the first entry in all other entities, but subsequent entries would not be validated for uniqueness, whether against other entities or within the entity itself. Additionally, only simple field types such as plaintext fields would be validated, while entity reference and composite fields would not.

From 10.2.0 the validator will mark entities with duplicates in multi-value fields as invalid, regardless of whether the duplicates occur in other entities or within the validating entity. Additionally, the field's main property will be used for validation, thus providing validation for e.g. Link fields whose main property is the Link URL.

Future edits of content implementing UniqueFieldValueValidator will require removing duplicate values.

If the original behavior of the validator was used deliberately, e.g. for designating only the first value in a multivalue field as a "key," then the constraint will need to be removed from the field and a new custom constraint implemented. See Defining Constraints for more information.

@caesius thanks - can you make those edits?

Didn't know I could do that -- done!

Thanks 🙌

Remarking as CR seems to be updated.

Hiding patches as there's an MR

Updating credits

Committed 3981c8a and pushed to 11.x. Thanks!

Fixed on commit too, ran the tests locally

--- a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
@@ -14,31 +14,15 @@
  */
 class UniqueFieldValueValidator extends ConstraintValidator implements ContainerInjectionInterface {
 
-  /**
-   * The entity field manager.
-   *
-   * @var \Drupal\Core\Entity\EntityFieldManagerInterface
-   */
-  protected $entityFieldManager;
-
-  /**
-   * The entity type manager.
-   *
-   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
-   */
-  protected $entityTypeManager;
-
   /**
    * Creates a UniqueFieldValueValidator object.
    *
-   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager
+   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entityFieldManager
    *   The entity type manager.
-   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
    *   The entity type manager.
    */
-  public function __construct(EntityFieldManagerInterface $entity_field_manager, EntityTypeManagerInterface $entity_type_manager) {
-    $this->entityFieldManager = $entity_field_manager;
-    $this->entityTypeManager = $entity_type_manager;
+  public function __construct(protected EntityFieldManagerInterface $entityFieldManager, protected EntityTypeManagerInterface $entityTypeManager) {
   }
 
   /**

Tests output

phpunit -c app/core app/core/tests/Drupal/KernelTests/Core/Validation/UniqueValuesConstraintValidatorTest.php
⏳️ Bootstrapped tests in 0 seconds.
🐘 PHP Version 8.1.16.
💧 Drupal Version 11.0-dev.
🗄️  Database engine mysql.
PHPUnit 9.6.10 by Sebastian Bergmann and contributors.

Testing Drupal\KernelTests\Core\Validation\UniqueValuesConstraintValidatorTest
....                                                                4 / 4 (100%)

Time: 00:03.177, Memory: 10.00 MB

OK (4 tests, 74 assertions)

I believe this fully addresses all related/referenced issues as well (some of which are functionally duplicates of each other)

I think the only thing left to do may be to write additional tests to more explicitly test e.g. Link fields and fields with custom properties. #3291483 looks like the best fit for that if we feel more tests are needed to prevent regressions.

Moving over credit from #2973455: Unique field constraint does not work with any field main property name that is not "value"

This may have caused regressions.

Also #3419816: Add support for case sensitive validation to UniqueFieldValuesConstraint.

I think this might have caused another (very similar) regression: https://www.drupal.org/project/drupal/issues/3456964