Problem/Motivation

UserMailRequiredValidator fails on new user entities, because it calls ::id() on it and expects a valid entity ID, which at that point does not exist yet. The reason this was never caught before is probably because Drupal core does not validate entities, except in the default UIs. Our application is headless and validates all custom and some core entities before saving them, which uncovered this problem.

This bug appears to have caused a critical regression in 8.7.x - see #3045607: Can no longer migrate users

Proposed resolution

Extend the if statement to check that the user entity is not new.

Remaining tasks

Alter the if statement.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano created an issue. See original summary.

Xano’s picture

Status: Active » Needs review
FileSize
778 bytes

I'm not really sure how to test this, since we have an entire validation pipeline lying around, mostly unused and I think we should be using that instead of writing elaborate tests.

TBI’s picture

Status: Needs review » Closed (cannot reproduce)

The email validation for user registration and for user edit already works. No requirement for applying patch in the code. Is there any specific condition where we can see this issue?

Xano’s picture

Assigned: Xano » Unassigned
Status: Closed (cannot reproduce) » Needs review

Please don't just close issues like that. The issue summary clearly states there's a problem when the default UIs are not being used.

To reproduce the problem, create a user entity that has all its fields filled in. Don't save it yet, but validate it. This constraint will cause an error because it tried to get the user ID, which does not exist yet before saving.

dawehner’s picture

The fix looks alright. I'm wondering though whether there are more places in core which have that kind of problem. Do you have any idea how to maybe find more places?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pandaski’s picture

This makes sense since the user ID can be 0 for anonymous which will lead an error

  public function validate($items, Constraint $constraint) {
    /** @var \Drupal\Core\Field\FieldItemListInterface $items */
    /** @var \Drupal\user\UserInterface $account */
    $account = $items->getEntity();
    $existing_value = NULL;
    if ($account->id()) {
      $account_unchanged = \Drupal::entityManager()
        ->getStorage('user')
        ->loadUnchanged($account->id());
      $existing_value = $account_unchanged->getEmail();
    }

Based on patch #2, #10 adding the following. You can also check the attached interdiff file

1. Return ASAP if account variable is not set.

    $account = $items->getEntity();
    if (!isset($account)) {
      return;
    }

2. Replace '\Drupal::entityManager()' to '\Drupal::entityTypeManager()'

plach’s picture

Issue tags: +8.7.0 beta testing

This seems to fix the regression reported at #3045607: Can no longer migrate users.

plach’s picture

Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks for the patch, setting to NW as this needs some test coverage capturing the bug.

Berdir’s picture

I don't understand this. $this->id() on a new should simply return nothing, yes, it's semantically strange and it's better to use isNew(), but that would make this a task/code improvement, not a major bugfix.

Core definitely validates new entities, so there must be something else going on?

samuel.mortenson’s picture

This is effecting Tome as well, just started seeing it now because Layout Builder implements hook_entity_presave() and does something with contexts that invokes typed data validation, which eventually gets to this error.

samuel.mortenson’s picture

I'm working on tests for this.

samuel.mortenson’s picture

Back to review - can this get in before 8.7.0 releases?

The last submitted patch, 16: 2891754-16-test-only.patch, failed testing. View results

vijaycs85’s picture

  1. +++ b/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequiredValidator.php
    @@ -12,24 +16,66 @@
    +  public function __construct(UserStorageInterface $user_storage, AccountProxyInterface $current_user) {
    

    Doesn't it mean there is an API change? since the class is not @internal, we are introducing a breaking change? IMO we should keep the change minimal here.

  2. +++ b/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequiredValidator.php
    @@ -12,24 +16,66 @@
    -    if ($account->id()) {
    -      $account_unchanged = \Drupal::entityManager()
    -        ->getStorage('user')
    ...
    +    if (!$account->isNew() && $account->id()) {
    

    Not sure if this change is required for the issue described in the IS. if $user->id() returns NULL, shouldn't cause any issue here?

samuel.mortenson’s picture

I'll let a core maintainer respond to #18.1 - the code in #18.2, was introduced in the first patch, #2, and I believe it is needed since a new user account might have an ID already.

samuel.mortenson’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Per Slack we should not introduce dependency injection in this issue. So the path forward would be to go back and add tests to #10 then put the issue into needs review. I'm not doing this right now but want to find time this week if no one else gets to it. Will comment if I start work on it again.

alexpott’s picture

Priority: Major » Critical
Status: Needs review » Needs work

This is a critical fix for 8.7.x as this is a regression in that things used to work and now don't. Also let's not combine fixing with refactor - the proper dependency inject should be done in a follow-up.

phenaproxima’s picture

Title: UserMailRequiredValidator fails on new user entities » [regression] UserMailRequiredValidator fails on new user entities
Issue tags: +Regression

Tagging as a regression.

alexpott’s picture

Here's a patch that removes the constructor injection from #16 and changes the test to use prophecy.

vijaycs85’s picture

+1 to RTBC.

+++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/UserMailRequiredValidatorTest.php
@@ -0,0 +1,152 @@
+    $cases['Existing users without an email should be ignored if the current user is an administrator.'] = [$items->reveal(), FALSE, TRUE];

nitpick: no . at the end of array index on other cases.

The last submitted patch, 23: 2891754-22.test-only.patch, failed testing. View results

Berdir’s picture

  1. +++ b/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequiredValidator.php
    @@ -19,11 +19,17 @@ class UserMailRequiredValidator extends ConstraintValidator {
         /** @var \Drupal\Core\Field\FieldItemListInterface $items */
    -    /** @var \Drupal\user\UserInterface $account */
    +    /* @var \Drupal\user\UserInterface $account */
         $account = $items->getEntity();
    +    if (!isset($account)) {
    +      return;
    +    }
    +
         $existing_value = NULL;
    -    if ($account->id()) {
    -      $account_unchanged = \Drupal::entityManager()
    +
    +    // Only validate for existing user.
    +    if (!$account->isNew() && $account->id()) {
    +      $account_unchanged = \Drupal::entityTypeManager()
             ->getStorage('user')
    

    I think this could be combined into a single condition:

    if ($account && !$account->isNew()) {
    ...
    

    we've seen that it is possible to have an ID but still be new but the opposite is not true. In fact, isNew() includes !$this->id()".

    That should also simplify the mock.

  2. +++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/UserMailRequiredValidatorTest.php
    @@ -0,0 +1,152 @@
    +
    +    $user_storage = $this->prophesize(UserStorageInterface::class);
    +    $user_storage->loadUnchanged('current-user')->willReturn($unchanged_account->reveal());
    +
    +    $entity_type_manager = $this->prophesize(EntityTypeManagerInterface::class);
    +    $entity_type_manager->getStorage('user')->willReturn($user_storage->reveal());
    +
    +    $current_user = $this->prophesize(AccountInterface::class);
    +    $current_user->id()->willReturn('current-user');
    +    $current_user->hasPermission("administer users")->willReturn($is_admin);
    +    $container = new ContainerBuilder();
    

    current-user for the identifier seems strange, we know that it has to be an integer for users, so I'd use 1 or an arbitrary number.

  3. +++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/UserMailRequiredValidatorTest.php
    @@ -0,0 +1,152 @@
    +
    +    if ($expected_violation) {
    +      $context->addViolation('@name field is required.', ["@name" => "label"])->shouldBeCalledTimes(1);
    +    }
    

    various places with double quote, doesn't seem to have a reason?

  4. +++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/UserMailRequiredValidatorTest.php
    @@ -0,0 +1,152 @@
    +    else {
    +      $context->addViolation()->shouldNotBeCalled(1);
    +    }
    

    shouldNotBeCalled() doesn't have an argument?

  5. +++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/UserMailRequiredValidatorTest.php
    @@ -0,0 +1,152 @@
    +    $field_definition->getLabel()->willReturn('label');
    

    similar here, we know it's the mail field, so the label could be E-Mail or so, IMHO it helps to have more realistic values when you get test fails.

  6. +++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/UserMailRequiredValidatorTest.php
    @@ -0,0 +1,152 @@
    +    $cases['New users with an email should be ignored'] = [$items->reveal(), FALSE];
    

    ignored in this context seems a bit strange, what about "New user with an e-mail is valid" or so?

Berdir’s picture

Status: Needs review » Needs work

For the record, I'm still confused about the regression part, nothing changed recently other than it being called in a way that it wasn't called before, that doesn't make it a regression in my book. Also, something being a regression doesn't automatically make it a critical issue, only the severity of the bug on its own does.

That said, whether or not something is critical doesn't really matter these days anyway, as it we have no limit on criticals like we used to and they don't block a release (not automatically anyway), so lets just fix it and move on ;)

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.31 KB
8.35 KB

Well there seem to be reports of code that was working now not working so that for me makes it a critical regression. If that's not the case then I'm wrong. See #3045607: Can no longer migrate users

Here's a patch based on @Berdir's review in #26 (thanks for that).

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks fine to me now.

  • catch committed e593d72 on 8.8.x
    Issue #2891754 by alexpott, samuel.mortenson, Joseph Zhao, Xano, Berdir...
catch’s picture

Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.8.x and 8.7.x, thanks!

  • catch committed 7454f12 on 8.7.x
    Issue #2891754 by alexpott, samuel.mortenson, Joseph Zhao, Xano, Berdir...
Wim Leers’s picture

Issue tags: +API-First Initiative

I don't know why this is suddenly a regression if it was reported nearly two years ago. It'd be great to document what caused this to become a critical regression.

That said, YAY for this, since it also helps Drupal be more API-First :) Retroactively tagging for that.

alexpott’s picture

Issue summary: View changes

@Wim Leers see https://www.drupal.org/project/drupal/issues/3045607 - I've updated the issue summary instead of this being buried in comments.

xjm’s picture

Adding credit for Christopher Riley who reported a duplicate of this issue.

Status: Fixed » Closed (fixed)

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