Allow administrator to edit account without email address (regression: accounts can be created without email addresses also)

Patch no longer applied against CVS/HEAD.

This is still an issue when using an external auth/account creation module such as Twitter.

Ported patch to 8.x and modified it a little bit. The email is now not required if the user making the changes has "administer users" permission and if the account didn't have an email address set.

If the account had an email address, the field remains mandatory.

+++ b/modules/user/user.module
@@ -980,7 +980,7 @@ function user_account_form(&$form, &$form_state) {
+    '#required' => (empty($account->mail) && user_access('administer users')) ? FALSE : TRUE,

can't this just be

+  '#required' => !(empty($account->mail) && user_access('administer users'))

Powered by Dreditor.

Yes it can, though I don't know if that's considered equally readable :-)

Redid patch according to the suggestion in #3.

To allow users without an email address to be deleted, I've also wrapped the call to user_validate_mail() in an if statement, so said function is not called is the email field is empty and not required.

...because with the use of signups via Twitter (and probably other contrib modules) D6 and D7 sites can have users without email addresses, I think it might be worth considering backporting this patch. Not so much as a new feature, but as a fix for the "cannot delete user without email" bug ;-)

Fixed my logic error in the validate handler. Updated patch attached.

Needs tests to make sure this remains working.

Attached patch hopefully contains two test cases that should fail if an admin cannot edit or delete users without an email address.

Updated test case to actually unset the test user's email, then admin delete the user.

Fixup tests so the created test users *actually* have no e-mail addresses set :-)

Thanks for your work on this patch! I reviewed it and have two small points of feedback:

+++ b/modules/user/user.moduleundefined
@@ -980,7 +980,7 @@ function user_account_form(&$form, &$form_state) {
+    '#required' => !(empty($account->mail) && user_access('administer users')),

I had to read this several times before I understood it. Could we add an inline comment explaining how the value is being set and why?

+++ b/modules/user/user.testundefined
@@ -796,6 +796,34 @@ class UserCancelTestCase extends DrupalWebTestCase {
+   * Create an administrative user and delete a user without an e-mail address.

This should start with "Creates..." (see http://drupal.org/node/1354#functions). We're currently cleaning up the other docblocks as a part of #1310084: [meta] API documentation cleanup sprint, so we should make sure the new docblocks we add are also consistent with the standard.

Also, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Finally, can we get a separate patch with just the tests to demonstrate that they fail without the fix?

Rerolling is easy, since the files were all moved via git mv, a simple rebase does the trick.

Attached are patch 14, which contains the original changes plus the updated doc strings and patch fail, which contains only the new tests, but not the code that should make these tests pass.

So 14 should pass, fail should fail.

[edit] Sigh. Double-posted because Chrome got stuck and didn't show me it'd already added the patches.

Yay, failpatch has failed testing :-)

+++ b/core/modules/user/user.testundefined
@@ -796,6 +796,35 @@ class UserCancelTestCase extends DrupalWebTestCase {
+   * Creates an administrative user and deletes a user without an
+   * e-mail address.

Oops, one more thing. We should put this all on one line. (It's less than 80 chars already I think.)

Other than that, this patch looks good to me, and it solves something that annoys me constantly. :)

So now I have a question, that line is 78 characters from * to . but if you include the indent spacing at the front, it's 81 characters. The latter is why I broke the line. Should I be counting from the * ?

Edit: Not changing the comment :-)

I answered cafuego's question in IRC. ;) All the characters count, including those from indentation and such. (Well.. okay, I won't get into the "slings and arrows of outrageous newlines.") Basically, though, just make the friendly gray line in dreditor happy!

Re-rolled with updated comments. Also put the edit user without e-mail test in its own function, so a test failure message is a bit clearer as to what failed.

Okay, that seems to be all loose ends tied up :-)

The user module has changed enough since November that this patch no longer applied cleanly. The attached is updated to include the logic change that cafuego had made, but does not include the change to the user_validate_mail() call, since that function no longer exists.

However, one issue remained: if the admin created a new user without providing an email address, and checks the "Notify user of new account" option, they got an error from user_register_submit(): "The e-mail address is already taken."

This patch adds two things in addition to what cafuego already wrote:

• Checks that the email address was provided before sending a welcome message. If email address is empty, it skips the mail send and displays a different message to the admin. Is there other documentation, translations, etc, that needs to be updated for the addition of a message like this?
• Don't check if the email address is already in use if none is provided. Otherwise, the admin gets an error the second time a user is created without an email address: "The e-mail address is already taken."

I think this looks great overall, and both the changes from #24 are good ones. Tests look good as well. Few stylistic cleanups:

1. It would be better to remove the t() from the assertion messages that are only displayed to the admin or bot when the test is run. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

2. +++ b/core/modules/user/user.moduleundefined
   @@ -943,11 +943,14 @@ function user_account_form(&$form, &$form_state) {
   +  // The mail field is NOT required if account originally had no mail set
   +  // and the user performing the edit has 'administer users' permission.
   +  // This allows users without e-mail address to be edited and deleted.
   

   This comment seems to wrap a bit before 80 chars. Edit: Either that, or it was moved up from above the #required maybe? I think it might actually be more helpful down there.

3. +++ b/core/modules/user/user.moduleundefined
   @@ -1129,7 +1132,7 @@ function user_account_form_validate($form, &$form_state) {
   +  if (!empty($mail) && (bool) db_select('users')->fields('users', array('uid'))->condition('uid', $account->uid, '<>')->condition('mail', db_like($mail), 'LIKE')->range(0, 1)->execute()->fetchField()) {
   

   Oy. I realize this predates the patch, but can we move the query up a line and set a variable so that the condition is legible?

4. +++ b/core/modules/user/user.testundefined
   @@ -785,6 +785,34 @@ class UserCancelTestCase extends DrupalWebTestCase {
   +    // Create administrative user.
   ...
   +    // Delete regular user without e-mail address.
   

   It would be good to add articles to these comments ("Create an administrative user", "delete the regular user", etc.)

Once that is fixed, let's have a couple people testing this manually and checking that everything behaves as expected and is intuitive. If a couple people can try adding/editing a few accounts and confirm that it behaves, I think it's RTBC.

Rerolled patch with adjusted comment, variablized giant SQL query and articled test comments :-)

... forgot to remove superfluous calls to t(), which is done as per http://drupal.org/simpletest-tutorial-drupal7#t (I think) in this patch.

Manually tested the latest patch in #28 and it looks good. I think we should have at least one more person test it before RTBC.

This might be a question for a different issue, but should we allow admins to remove an email address from a user? With this patch, an admin can create a user with no email, and then edit that user without having to provide an email. But, if the admin does set an email address, the email is a required field forevermore on that user. Is this ideal?

should we allow admins to remove an email address from a user

I had a think about that as well when I first worked on the patch and I decided against it. A fair bit of Drupal functionality depends on a user having an email address (password reset, notifications, etc) and usually users only end up without an email address when a user is created programmatically or via some third party integration thing (twitter, facebook).

I think if we're going to allow users without an email address maybe that should be a setting on the user settings page, which can contain some text to explain caveats, so it's a decision admins need to expressly make. I'm hesitant to just throw it out the window as a required field.

Manual testing passed.

1. Created a user:
2. Removed the email address via sql:
3. Edited the user:

Thanks for the testing @drupkick! I think this is RTBC. :)

Looks good to me too, committed/pushed to 8.x.

Thanks catch! Now let's see if we can backport this into D7 as a fix, because not being able to delete users just because they don't happen to have an email address is a bug.

So the things to be aware of for backporting this are:

• Introduces a new string.
• Eensy teensy form change in that the field is no longer always required.

I think this is safe. We probably could even get around the string addition if need be.

First roll of this patch for D7.

In the commit on Apr 13th an unrelated file core/modules/search/lib/Drupal/search/SearchQuery.php was committed and is still there.

core/modules/search/lib/Drupal/search/SearchQuery.php is part of #1513970: Convert SearchQuery to PSR-0. So the file should be there... it just should have been part of the other commit instead. :)

This is good to go.

I tested (manually) that:

• When I delete an existing user's email from the database - I can still edit the user from the UI.
• I can create a new user with no email address.
• I can create two users with no email address - so the potential problem of looking up in the database to see if the email "" is taken is not an issue.
• The message The new user Derp was created without an email address, so no welcome message was sent. is displayed when I check the "Notify user of new account" checkbox and the user has no email address.

There is a new string that should be translated: The new user %name was created without an email address, so no welcome message was sent.

The simpletests look fine too.

I can create a new user with no email address.

I can create two users with no email address - so the potential problem of looking up in the database to see if the email "" is taken is not an issue.

Hmm, can you create them as administrator or when signing up as visitor? If the former, that's OK (I think) but if the latter then that's a feature this patch isn't supposed to provide ;-)

Sorry. That was unclear.
I did all the tests above as a privileged user.

I really love the idea of this patch, however, David has requested help on resolving release blockers, so I don't feel comfortable committing feature patches until that happens.

Er, whoa... Why are we allowing administrators to create accounts without an email address? The Drupal core UI should not be allowing that, not even in Drupal 8 and especially not in Drupal 7.

Also, I'm having trouble understanding most of the code in this patch. Contrib modules can easily use hook_form_alter() to set #required to FALSE if they want to make the field not required. And they can do so on their own terms.... So why should we make assumptions in core about the exact conditions under which it might be made un-required?

It seems like instead we need to focus this issue on the original goal, which would be removing only those things that are actual barriers to a contrib module which wants to make the field non-required (like the changes to user_account_form_validate(), etc).

Also, if the other goal of the patch is to fix the bug where an account without a valid email can't be deleted, that seems good (and the test that was added for that looks very nice) but this should be done by setting #limit_validation_errors on the delete button, rather than modifying the #required property of one particular field. (That would solve the problem for other fields too; there are many reasons an account could be invalid but none of them should necessarily prevent the account from being deleted.) At least, it's definitely the way it should be fixed in Drupal 8.

Not including this in D7 makes sense. Should it be rolled back out of D8? And if not, can this issue be closed?

The patch in this issue *should* not allow administrators to create accounts without email addresses, that is not what this issue is about... the #required check is:
code>!(empty($account->mail) && user_access('administer users'))
So that should be FALSE if account->mail is empty and the viewing user has administer users access and TRUE in all other cases.

Perhaps there should also be a test to see that account->uid is not empty, so the email field remains required when an admin creates a new user.

What we're trying to address is the editing and deleting of accounts that lack an email address by administrators. Such accounts can be created by third party authorization plugins such as twitter or Facebook, which pass back a username and authentication result, but not an email address.

As it stands, administrators cannot edit or delete such accounts because the mail field is a required field on the account edit form. The patch makes it be *not* required when an administrator edits an account that has no email set. It should not affect the form otherwise and should not allow users to remove their email address or indeed save the form without first adding an email address if it was not originally present.

I will be happy to read up on the limit validation errors and perhaps rewrite the patch, but backing the fix out and marking this issue as closed leaves a big problem for many site admins, so do not.

Just recognized this on the user add form. Bumping to critical, since this is a major regression. An e-mail address should be required in core.

The fastest way to get this critical out of the queue would be to revert the committed patch.

Aside from that, I also question the original idea of allowing contrib to store no e-mail address at all for a user account. To my knowledge, that's an extremely bad practice, since it means that a user is not able to regain access to the own account in case the originating service or account at that service ceases to exist. If someone really wants to destroy that option, then it is trivial to inject a fake e-mail address into each created user account which does not exist — same effect, no core changes required.

Patch above does not revert cleanly, so we'll need a patch to do that.

The entire code that was touched by #28 moved to new locations, so reverting this patch wasn't exactly trivial.

Furthermore, the commit mistakenly added a new Drupal\search\SearchQuery class, which of course should not be reverted either.

Changed the test in core/modules/user/lib/Drupal/user/Tests/UserEditTest.php to assert that the e-mail should always be required - I doubted between removing, but I feel this might be a better option.

Hope this passes, all my local tests were failing and those html5 things are annoying sometimes as well to test.

Changed the comment in that test as well.

Since this issue deals with deleting entities with empty required fields I want to point out a related issue, #1342444: Do not validate fields if entity is in the process of being deleted. This isn't only an issue with users.

#58 As being pointed out earlier, in those cases a fake email could be use. An account registered via Facebook could be assigned an email FacebookUID@facebook.com that is unique (and many other social platform, why do not reveal user email, can expose an address to route emails to user's real email address).

For now, it would be a good idea to get this critical regression out of the way. This does not imply any kind of "absolute" decision that e-mail addresses cannot or must not be optional.

However, re-introducing this capability should be covered by much more serious considerations with regard to:

1. How a potentially missing/empty e-mail address will affect other (core and contributed) modules.
2. Double-checking where and how e-mail addresses are used throughout Drupal core.
3. How this change affects modules using drupal_mail() to send mails to users.
4. Security aspects, such as recovering user account access/information. (Also: What if uid 1 has no e-mail address?)
5. Preventing the user/password reset form from attempting to send an e-mail to a user account without e-mail address. Requires special error handling.
6. What about contrib modules that only make any sense with non-empty user account e-mail addresses? (Wanted to mention comment_notify, but that's an invalid example. Anyway, I'm sure there are some.)
7. Various stuff like: #85494: Use email verification when changing user email addresses
8. etc.pp.

We also need much more sophisticated tests - at minimum for an entire user registration + edit + password-reset + cancellation flow.

I fail to see the data loss that makes this critical. Also, since when it is acceptable to fix something by nuking its test? If commit this as it stands, I think, unless I am missing something here, you won't be able to cancel an account without an email address. At least you need a #limit_validation_errors on the Cancel Account button so that cancel goes through without validation.

Attached patch contains a subset of swentels patch from #56. It leaves in place the tests, and sets #limit_validation_errors to array('uid') on the cancel action.

This still means an account without email address cannot be edited and saved without adding in a (possibly fake) email address. That's not ideal, but at least such accounts can be deleted from the database now.

I'm not sure how to sensibly use #limit_validation_errors for the save action without iterating through the whole form array and adding all elements except mail. Is there a property we can set that lists the elements to be ignored by the validation handler rather than the ones that should NOT be ignored? Like an inverse #limit_validation_errors?

Reverted the assignment of #required on the profile form submit button, then added a test on $account->uid.

This now means that the email field is not required if:

- The account is being edited, not created.
- The account had no email when the form was loaded.
- The current user has administer users permission.
- The account is being canceled.

When a new account is being created, the email field is required.

Also added in a few comments to explain what and why is happening.

I've also left the logic that allows an administrator to edit an account if there are multiples without an email address (by not checking if an empty email address is already in use).

Finally, the logic that previously allowed a new user with an empty email address to be created and react accordingly in the RegisterFormController is now gone, as that case should never happen (and was also outside the scope of the original patch!)

Tagged for manual testing.

I'm not sure what the needs manual testing tag is for in this case. Don't we want to add some tests to make sure this behavior stays?

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -47,12 +47,12 @@ public function form(array $form, array &$form_state, EntityInterface $account)
+      '#required' => !(!empty($account->uid) && empty($account->mail) && user_access('administer users')),

Minor nitpick, I think the form !(!A && B && C) is a little clearer as: A || !B || !C

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Unless I'm missing something, this was fixed in #189544: Allow administrator to edit account without email address (regression: accounts can be created without email addresses also).

Unless I'm missing something, this was fixed in #189544: Allow administrator to edit account without email address (regression: accounts can be created without email addresses also).

@jhedstrom, you a referrering to this issue as duplicating itself?

Set back to Active as this is still valid.

We have a Drupal site where there shouldn't be emails used at all. The users are migrated to the system without email adresses and that works fine. Unfortunately they cannot be edited (for example to change the password or deleted) because the email is a mandatory field. This is a bit contradictory since you can also create a user without an email through UI.

Finally found more recent issue for this one: https://www.drupal.org/project/drupal/issues/2600158
Set back as duplicate and sorry for the noise.