Fix broken validation (core's email validator does not check alternative emails)

Ok, I made some progress debugging this.

To core's UniqueFieldValueValidator query,

    // Check if any item values for this field already exist in other entities.
    $query = $this->entityTypeManager
      ->getStorage($entity_type_id)
      ->getAggregateQuery()
      ->accessCheck(FALSE)
      ->condition($field_name, $item_values, 'IN')
      ->groupBy("$field_name.$property_name");
    if (!$is_new) {
      $entity_id = $entity->id();
      $query->condition($id_key, $entity_id, '<>');
    }

I manually added the query alter:

    if ($entity_type_id === 'user' && $field_name === 'mail') {
      alternative_user_emails_entity_query_user_alter($query);
    }

This means that the query returns the correct result, in this case the primary email address of the other user account that has the alternate email address that you are attempting to register.

However, the validator still fails because of the results evaluation logic:

      // The results array is a single-column multidimensional array. The
      // column key includes the field name but may or may not include the
      // property name. Pop the column key from the first result to be sure.
      $column_key = key(reset($results));
      $other_entity_values = array_column($results, $column_key);

      // If our entity duplicates field values in any other entity, the query
      // will return all field values that belong to those entities. Narrow
      // down to only the specific duplicate values.
      $duplicate_values = array_intersect($item_values, $other_entity_values);

What happens here is that core compares whether the email address to be registered (which is an alternate email address) is a duplicate value of the primary email address of the account that has the alternate email address registered. This check fails, but it should pass.

So perhaps the best approach is to override UserMailUnique with a custom validator?

You're right, we've got smacked by 📌 UniqueFieldValueValidator works only with single value fields Fixed .

So perhaps the best approach is to override UserMailUnique with a custom validator?

That sounds like a sane and maintainable solution. It was kind of neat before that just altering the entity query took care of validation magically, but a custom validator is fine.

Ok, it turns out we can use hook_validation_constraint_alter() to replace the UserMailUnique constraint with the AlternativeUserEmailsUnique constraint already provided by this module, and the tests now pass.

I did a little refactor to break the validation loop as soon as a bad (duplicate) email is found.

@jonathanshaw What do you think of this solution?

It's a good solution.

However AltenativeUserEmailsValidator does this:

if ($entity->getEntityTypeId() !== 'user' || $items->getFieldDefinition()->getName() !== 'alternative_user_emails') {
return;
}

so the tests shouldn't be passing, which is a little concerning.

I don't see why we can't just remove that whole early return block. What's the harm if people want to use this validator somewhere else, that's up to them.

Oh, the reason the tests are passing is because I refactored that block into a method:

  /**
   * The fields on which this validator can operate.
   */
  private const VALID_FIELDS = ['alternative_user_emails', 'mail'];

  /**
   * Checks whether this validator can process a field.
   *
   * @param string $field_name
   *   The name of the field to check.
   *
   * @return bool
   *   TRUE if the field can be validated by this validator.
   */
  private function canFieldBeProcessed(string $field_name): bool {
    return in_array($field_name, self::VALID_FIELDS, TRUE);
  }

We can remove it, though. As you said people should be free to use the validator as they like.

This message: 'The alternative email address %value is already associated with another user account.' is no longer quite right if this validator is being used on the core 'mail' field as well.

Ok, I removed "alternative" from the string!

That was definitely an oversight.

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