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.
Comment | File | Size | Author |
---|---|---|---|
#28 | 2891754-28.patch | 8.35 KB | alexpott |
#28 | 23-28-interdiff.txt | 6.31 KB | alexpott |
#23 | 2891754-22.patch | 8.45 KB | alexpott |
#23 | 2891754-22.test-only.patch | 7.29 KB | alexpott |
#23 | 16-22-interdiff.txt | 15.09 KB | alexpott |
Comments
Comment #2
XanoI'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.
Comment #3
TBI CreditAttribution: TBI commentedThe 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?
Comment #4
XanoPlease 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.
Comment #5
dawehnerThe 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?
Comment #10
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedThis makes sense since the user ID can be 0 for anonymous which will lead an error
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.
2. Replace '\Drupal::entityManager()' to '\Drupal::entityTypeManager()'
Comment #11
plachThis seems to fix the regression reported at #3045607: Can no longer migrate users.
Comment #12
plachThanks for the patch, setting to NW as this needs some test coverage capturing the bug.
Comment #13
BerdirI 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?
Comment #14
samuel.mortensonThis 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.
Comment #15
samuel.mortensonI'm working on tests for this.
Comment #16
samuel.mortensonBack to review - can this get in before 8.7.0 releases?
Comment #18
vijaycs85Doesn'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.
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?
Comment #19
samuel.mortensonI'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.
Comment #20
samuel.mortensonPer 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.
Comment #21
alexpottThis 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.
Comment #22
phenaproximaTagging as a regression.
Comment #23
alexpottHere's a patch that removes the constructor injection from #16 and changes the test to use prophecy.
Comment #24
vijaycs85+1 to RTBC.
nitpick: no
.
at the end of array index on other cases.Comment #26
BerdirI think this could be combined into a single condition:
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.
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.
various places with double quote, doesn't seem to have a reason?
shouldNotBeCalled() doesn't have an argument?
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.
ignored in this context seems a bit strange, what about "New user with an e-mail is valid" or so?
Comment #27
BerdirFor 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 ;)
Comment #28
alexpottWell 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).
Comment #29
BerdirThanks, looks fine to me now.
Comment #31
catchCommitted/pushed to 8.8.x and 8.7.x, thanks!
Comment #33
Wim LeersI 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.
Comment #34
alexpott@Wim Leers see https://www.drupal.org/project/drupal/issues/3045607 - I've updated the issue summary instead of this being buried in comments.
Comment #36
xjmAdding credit for Christopher Riley who reported a duplicate of this issue.