Unique field constraint does not validated field values on delta greater than 0

Add #2478663: UniqueFieldValueValidator works only with single value fields as related issue.
It may seems to be a duplicate nop ?

I see that #2478663: UniqueFieldValueValidator works only with single value fields doesn't have a working patch, what should we do ?

Hi Guys
the latest 2999225-9.patch is failed to apply

1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
   @@ -32,8 +32,13 @@ public function validate($items, Constraint $constraint) {
   +    $values = [];
   +    foreach ($items as $item) {
   +      $values[] = $item->value;
   +    }
   +
        $value_taken = (bool) $query
   -      ->condition($field_name, $item->value)
   +      ->condition($field_name, $values, 'IN')
          ->range(0, 1)
   

   Technically hardcoding ->value is wrong as you could for example use this on an entity reference field. But that was already wrong before and is a separate problem.

2. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/UniqueFieldValueValidatorTest.php
   @@ -0,0 +1,250 @@
   +
   +  /**
   +   * Data provider.
   +   */
   +  public function validationDataProvider() {
   +    $dateTime = new \DateTime();
   

   not a huge fan of data providers on kernel tests. While they are much faster than functional tests, there's still a bit of overhead. Could also be done with just a regular loop over the data returned by this, not sure.

Thanks, this seems to be working for me!

Technically hardcoding ->value is wrong as you could for example use this on an entity reference field. But that was already wrong before and is a separate problem.

I don't want to divert the issue but was wondering if it makes sense to add a method for grabbing the value just in anticipation of solving the problem for different field types, eg. ER, elsewhere - please see attached.

(Exactly the same patch as #18 rolled for D8.7.)

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
@@ -32,19 +33,37 @@ public function validate($items, Constraint $constraint) {
+  protected function getItemValue(FieldItemInterface $item) {
+    return $item->value;
+  }

> if it makes sense to add a method for grabbing the value just in anticipation of solving the problem for different field types, eg. ER, elsewhere - please see attached.

I don't think the approach in the patch is the right one. The patch is adding a method to the validator. But we're not going to subclass the validator for different field types are we? That would be a pain.

Isn't mainPropertyName() what we should use here?

> mainPropertyName()

Yes. But as I wrote, I'd be fine to improve that in a follow-up issue, it's existing problem with the current code.

Sorry all, I think I have diverted the issue: I'll hide the patches I added. @joachim IIUC you're suggesting I do something along the lines of this?

protected function getItemValue(FieldItemInterface $item) {
  $property_name = $item->getDataDefinition()->getMainPropertyName();
  return $item->{$property_name};
}

Putting back to needs review for #13.

Thanks!

> @joachim IIUC you're suggesting I do something along the lines of this?

Yes, though I agree with @Berdir that it's best done in a follow-up, to keep this issue about just one problem.

(Sorry, I should have said that in my earlier comment!)

it's best done in a follow-up, to keep this issue about just one problem.

Thanks, understood, I kinda knew I was breakin the rules but foolishly thought my change would be small and correct! My specific use case is a multi-value ER field, so I've added a little comment to #2973455: Unique field constraint does not work with any field main property name that is not "value".

Thanks!

At the moment the validator's still passing $item->value as part of the violation message.

Also at least the following constraints' messages are worded for a single value, don't know if that's something to be addressed as part of this issue?

• FeedTitleConstraint
• FeedUrlConstraint
• FileUriUnique
• UniqueFieldConstraint
• UserMailUnique
• UserNameUnique

I guess any error message we give for a multi-value field will be (annoyingly?) vague or require more queries or entity loading?

Thanks

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
@@ -32,19 +33,37 @@ public function validate($items, Constraint $constraint) {
-      ->condition($field_name, $item->value)
+      ->condition($field_name, $values, 'IN')

In my opinion #2478663: UniqueFieldValueValidator works only with single value fields provides a more comprehensive fix considering #2478663-56... The tests from this issue would be very useful.

This issue should now be resolved by #2478663: UniqueFieldValueValidator works only with single value fields