Username regenerated improperly after rc6 release

Sorry to hear about this bug. Thanks for identifying where it was introduced. I've referenced this issue from there and also added this as a release blocker on #3021912: Plan for creating email registration 8.x-1.0 final stable.

One thought:

> This can make it very confusing for admins as user's usernames will keep changing.

My sense is that the best way to use this module is to just consider emails, and not usernames. There's also the idea of using a "real name" profile field that is shown to admins and hiding the username.

Totally makes sense, I just meant for admins it's ideal to use emails (if they are the kind of admin that sees emails, which I guess I'm assuming but might not be the case).

I just want to emphasize that this is critical. I was using email_registration together with masquarade. Finding the correct user with masquared
is impossible with "email_registration_***".

This newly introduced behaviour makes the Allow log in with email address or username functionality completely useless.
It will also require every site using this module to change the /admin/people overview and have a solution for some of the emails that are sent from the website (those using the [user:display-name] token).

Hi @greggles (and everyone), for now I resolved the issue changing the function email_registration_form_user_form_alter like rc5 version (until a new version it's release):

In email_registration.module file:

1.- The function name email_registration_form_user_form_alter was modified to email_registration_form_user_register_form_alter.
2.- Then added function email_registration_form_user_form_alter again with the code:

function email_registration_form_user_form_alter(&$form, FormStateInterface $form_state) {
  $form['account']['name']['#title'] = t('Display name');
}

I didn't analize all the funtionality involve in this reverse to rc5 version, so please @greggles tell us if it's ok to make this temporaly change, or if it's possible that conflicts occur (I did several tests and for now everything works correctly, but I may be ignoring something).

Thanks in advanced.

---Spanish

Hola @greggles (y a todos), por ahora resolví el error cambiando la función email_registration_form_user_form_alter tal como estaba en la versión rc5 (hasta que se publique una nueva versión):

En el archivo email_registration.module:

1.- El nombre de la función email_registration_form_user_form_alter fue modificado a email_registration_form_user_register_form_alter.
2.- Luego se agregó nuevamente la función email_registration_form_user_form_alter con el código:

function email_registration_form_user_form_alter(&$form, FormStateInterface $form_state) {
  $form['account']['name']['#title'] = t('Display name');
}

No analicé toda la funcionalidad involucrada en éste reverso a la versión rc5, así que, por favor @greggles, indicar si está bien hacer éste cambio temporal, o si es posible que ocurran conflictos (hice varias pruebas y por ahora todo funciona correctamente, pero puedo estar obviando algo).

Gracias de antemano.

This is just caused me some headaches on a few Drupal sites we've included this module on.

Put @andreyna 's fix into a patch.

I believe that patch in #10 basically just reverts the changes in #2919733: Alter AccountForms instead of just registration form when using this module. so I think that's not an ideal solution. Perhaps since this causes a lot of problems for people it's worth committing a revert of that change and then reopening that issue.

Because it does not check the default value for name field exist or not. And assigning "email_registration_'. user_password()"
value in email_registration_form_user_form_alter username is changing on save. And as password is encrypted it is getting strange value e.g. email_registration_Aghs1dsh. This patch will fix that issue.

The first part of the patch in #10 reverts https://cgit.drupalcode.org/email_registration/commit/?id=23596fe The second part of the patch was not in #2919733: Alter AccountForms instead of just registration form when using this module..

Created a patch which only reverts the code from the issue @greggles mentioned in his previous comment.

#12 looks like a great solution! Can folks please test that?

#12 will probably work, but to check that default user name value will be saved, fix can be improved to:

<?php
  $form['account']['name']['#value'] = $form['account']['name']['#default_value'] ? $form['account']['name']['#default_value'] : 'email_registration_' . user_password();
?>

Value input in form doesn't have default_value parameter so will be not used: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

btw User in d8 has some set of entity forms - registration & default http://cgit.drupalcode.org/drupal/tree/core/modules/user/src/Entity/User...

I bet on keep 2 form hooks separate, so like #10 + #15

surely it needs test coverage

I merge #10 + #15, do not add 'Display name' label for value type doesn't make sense. Should pass tests now.

This may come in handy to fix accounts that have been changed:

use Drupal\user\Entity\User;

/**
 * Fixes broken users
 */
function MODULE_NAME_update_8001() {
  $ids = Drupal::entityQuery('user')
    ->condition('name', 'email_registration_%', 'LIKE')
    ->execute();
  $users = User::loadMultiple($ids);
  foreach ($users as $account) {
      $new_name = preg_replace('/@.*$/', '', $account->getEmail());
      // Clean up the username.
      $new_name = email_registration_cleanup_username($new_name);
      $new_name = email_registration_unique_username($new_name, $account->id());
      $account->setUsername($new_name);
      $account->save();
  }
}

RTBC . Works fine

The patch looks good and applies successfully. +1 for RTBC.

As per comment #18, this needs tests to make sure it doesn't happen again in a future release.

When will the fix be integrated to the module?

This is partly a security concern as the encrypted password is visible in the pages. Kindly apply the fix to the module with an update as early as possible.

Thank you.

@Senthil Kumar Kumaran it is generated a random alphanumeric password so security is not a problem, function name is not clear but you can read it here https://api.drupal.org/api/drupal/core%21modules%21user%21user.module/fu...

@ptmkenny what kind of tests you propose? Probably we need some functional test here base on BrowserTestBase?

@henk Thank you for the response. However, i'm still waiting for update to get the fix applied in module.

I guess as per #18, it's waiting for tests to be added to ensure that the issue is indeed fixed and also can't be re-introduced in a future release as a regression.

The most recent patch in #19 includes no tests for this change.

This problem appear when user was edited and saved, e.g. activated. Current test have only "Test email_registration_unique_username()." register user case. Not for editing user. I will try to update tests and add scenario for user edition, should fix problem.

Patch #19 works fine. Thanks @henk

I am believing that one consideration is not in the mix here. Perhaps I am missing something in this message thread but I am adding one caveat to this issue. To most users of a website their username is something that gives them an identity on the the site. It goes next to their content their comments and may be how the other users at the site address them in their personal contact forms. The module needs a different generation of a user name and an option to allow a user (based on the original permissions of core) to change their user name. To a user their username is not a generated blob of letters.

With patch #19 and then adding the patch located here https://www.drupal.org/project/email_registration/issues/3034828 we have a working email_registration module. Thanks.

@Chris Pergantis Your comment in #32 is not really related to the issue at hand; please open a new issue if you want to start a discussion on a different topic.

To give users the ability to have a name to represent themselves on the site, you can use this module in combination with the Real Name module.

Patch attached, contains a test to check if the username stays the same when editing the user.

+++ b/tests/src/Functional/EmailRegistrationTestCase.php
@@ -3,6 +3,7 @@
+use Drupal\user\Entity\User;

Unused use statement. I guess this can be fixed on commit.

Tested & works as expected.

1. +++ b/email_registration.module
   @@ -123,6 +123,15 @@ function email_registration_cleanup_username($name) {
    function email_registration_form_user_form_alter(&$form, FormStateInterface $form_state) {
   ...
   +/**
   + * Implements hook_form_BASE_FORM_ID_alter().
   + */
   +function email_registration_form_user_register_form_alter(&$form, FormStateInterface $form_state) {
   

   wrong comment, this is hook_form_FORM_ID_alter() as it was before #2919733: Alter AccountForms instead of just registration form when using this module., right?

   Why do we have 2 form alters now? Does this fix work with the changes in email_registration_form_user_form_alter() only?

2. +++ b/email_registration.module
   @@ -123,6 +123,15 @@ function email_registration_cleanup_username($name) {
   +  $form['account']['name']['#value'] = $form['account']['name']['#default_value'] ? $form['account']['name']['#default_value'] : 'email_registration_' . user_password();
   

   we should have a comment here that this form alter applies to the user registration form AND the user edit form. That is why we have to check for existing user names in #default_value.

3. +++ b/tests/src/Functional/EmailRegistrationTestCase.php
   @@ -3,6 +3,7 @@
   +use Drupal\user\Entity\User;
   

   unused use statement

4. +++ b/tests/src/Functional/EmailRegistrationTestCase.php
   @@ -106,6 +107,16 @@ class EmailRegistrationTestCase extends BrowserTestBase {
   +    $name = $this->randomMachineName(32);
   

   never use random data in tests as this can lead to random test fails. Better hard-code a fixed test user name here.

5. +++ b/tests/src/Functional/EmailRegistrationTestCase.php
   @@ -106,6 +107,16 @@ class EmailRegistrationTestCase extends BrowserTestBase {
   +    $this->drupalPostForm('/user/' . $user->id() . '/edit', [], t('Save'));
   

   never use t() in tests when you are only testing the English version of the site anyway.

   I know, the other test code probably violates this as well but all new test code should be written correctly.

Patch in #10 works for me, patch in #35 does not. Using form_mode_manager and account_field_split. (which complicates things ;))

Please also consider the the $form['account']['name'] might not even be present if it was disabled. We are doing this all the time using different form modes with form_mode_manager module.

And please also consider that a module like account_field_split renders the username field in $form['name'] instead of $form['account']['name'].

So before making changes to the username in the form array we should always check if it exists first.

The patch still needs some work. Setting the #value to 'email_registration_' . user_password() or to the #default_value will will hijack the username field and not allow any other modules to update the username.

For my use case, admin users need to be able to set the user name to a specific username for our REST endpoint users. In my custom module I have a form alter where I check that the admin has permission to administer users and then set the field back to type textfield. On save, because we're copying the #default_value to the #value field, the new value does not get saved.

Simply checking if #default_value is empty and then setting the 'email_registration_' . user_password() will work better.

Made changes as suggested by Klausi in #41.

In hook_form_alter, the type of the field is changed to "value". The only valid way to set a value for type value is using '#value' :

• https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...
• https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

Thanks, looks good!

Patch #44 fixes the issue for me when saving a user form. However, if the user had already been saved the botched username persists. This is likely expected behavior as the module shouldn't update the username on form submission. But I cannot be certain? A fix might be the aforementioned #3.

@brandonratz, have you tried the suggestion in #21?

@JeroenT Thanks! test do not check account edition and after edit problem appears not after login (in my case). So bug is not covered by the tests still. Following steps:

1. Create account
2. Log in
3. Edit account
4. Logout
5. Log in using same user name

Some thoughts on this fun issue:

#42 touches a topic that hasn't been addressed so far.
#49 Claims that the new tests are not specific to the editing of an account

Additionally, and after reading through this issue as well as the source of this problem (#2919733: Alter AccountForms instead of just registration form when using this module., I found another way to tackle this.
Simply put: The form_alter logic should only kick in if the form has to do with a registration, i.e. new user creation.

I am attaching updates to #44 where the tests are more specific (following #49) and the form_alter uses an alternative condition before applying changes. Also checking if the 'name' element is even present (see #42)

P.S.: Point 4 from #41 hasn't been addressed yet, calling createUser() basically calls randomMachineName() under the hood. In this case there is nothing wrong with this though, since the generated machine name is never going to mess with the logic we are testing.

Taking this back as it didn't fix the problem, the better implementation would be like this:

function email_registration_form_user_form_alter(&$form, FormStateInterface $form_state) {
  // Modify the form only if this involves a new user, i.e. is a registration.
  if ($form_state->getFormObject()->getEntity()->isNew() && isset($form['account']['name'])) {

However I think I just fully realized what @henk meant at #49, the tests were green, even though the code was failing. Will be looking at the tests soon.

Tests are doing the job, "Needs review" is correct.

The problem with the patch from #52 is that it now always shows the user name form field when editing a user.

So you are introducing a behavior change with that. Previously the user name form field was hidden, not editable by an admin or the user. With your patch the user name field can now be edited. Is that desired behavior? Looks like we have no test coverage for that, should probably be decided in another issue.

I did not fully understand comment #49. Should we just add a logout and then login check after editing in the test?

So let's expand the test and then go back to the approach in #44 to avoid the form field behavior change.

#49 that's exactly what I tried to test:

// Create user
$user = $this->createUser();
// Log in with the newly created user.
$this->drupalLogin($user);
// Edit the current user. Username is changed here without the patch.
$this->drupalPostForm('/user/' . $user->id() . '/edit', [], 'Save');
// Log out.
$this->drupalLogout();
// Try to log in using the username. Without the patch, the username is no longer the same and log in will fail.
$this->drupalLogin($user);

#55: I don't know why this is introducing a behaviour change, maybe I am missing sth. The only change #52 introduces is the conditional wrapping to make sure the form alterations apply only to forms that create new user objects (i.e. registrations) and don't edit existing ones. This is quite close to the state of things before the change from user_register_form to user_form was introduced that caused the current regression. My intention is to revert that change in a way that still facilitates the request from #2919733: Alter AccountForms instead of just registration form when using this module.

#49: I would also like to know that, I assumed that a test failure when calling drupalLogin() is not a specific as checking if the user name was modified which is what I tried to check in my modified version of the tests.

Thanks for the feedbacks in any case, much appreciated!

#55: The check if the user is already created or not is added. If you are on a user edit page, the user is already created and the field type of name is not changed to "value". So on that form, the username will be visible. This is a change because in previous versions of the email_registration module you were not allowed to change the username.

#49: the drupalLogin() method uses the username and password to log in. When the username is changed, the user will nog longer be able to log in using the username he expects and this will throw an error. So both tests will work.

#58: Gotcha, thanks! Then, if I may insist some, the changeset is even smaller, i.e.:

function email_registration_form_user_form_alter(&$form, FormStateInterface $form_state) {
  $form['account']['name']['#type'] = 'value';
-  $form['account']['name']['#value'] = 'email_registration_' . user_password();
  $form['account']['mail']['#title'] = t('Email');
+  // Modify the form only if this involves a new user, i.e. is a registration.
+  if ($form_state->getFormObject()->getEntity()->isNew() && isset($form['account']['name'])) {
+    $form['account']['name']['#value'] = 'email_registration_' . user_password();
+  }
}

@stefanos.petrakis,

That will probably work, but imo the check if the user is new or check if a name is already set, will lead to the same behaviour.

Fair enough. So, it comes down to these two versions:

(from #44)

-  $form['account']['name']['#value'] = 'email_registration_' . user_password();
+  // Check for existing values because this hook alters the user registration
+  // and user edit form.
+  $form['account']['name']['#value'] = $form['account']['name']['#default_value'] ? $form['account']['name']['#default_value'] : 'email_registration_' . user_password();

or, (from #59):

-  $form['account']['name']['#value'] = 'email_registration_' . user_password();
+  // Modify the form only if this involves a new user, i.e. is a registration.
+  if ($form_state->getFormObject()->getEntity()->isNew()) {
+    $form['account']['name']['#value'] = 'email_registration_' . user_password();
+  }
}

The two versions are very similar and both solve the problem.
I would mildly argue that the code I propose is easier to understand because it makes explicit the assumption that this is a new user entity with an uninitialized #value for name.
I also find that not performing value settings or other alterations when not dealing with a registration is more consistent with the responsibility zone of the module.

Up to the reviewers, I am fine with both options since they solve the problem. :-)

As far as the tests are concerned, maybe we could merge the extra assertion I added, roughly like this:

+    // Check if custom username stays the same when user is edited.
+    $user = $this->createUser();
+    $name = $user->label();
+    $this->drupalLogin($user);
+    $this->drupalPostForm('/user/' . $user->id() . '/edit', [], 'Save');
+    $this->assertEqual($name, User::load($user->id())->label(), 'Username should not change after empty edit.');
+    $this->drupalLogin($user);

Changing the "Version" for this issue as it should be against dev I believe.

N.B.: I think the concern expressed in #42 still deserves some consideration, for the time being, I removed it from the snippet in this comment for clarity.

@stefanos.petrakis,

Can you make a patch of those changes?

Sure, here goes.

Looks good to me.

Just one point:

1. <li>
   <code>
   +++ b/tests/src/Functional/EmailRegistrationTestCase.php
   @@ -106,6 +107,16 @@ class EmailRegistrationTestCase extends BrowserTestBase {
   +    $this->assertEqual($name, User::load($user->id())->label(), 'Username should not change after empty edit.');
   +    $this->drupalLogout($user);
   +    $this->drupalLogin($user);
   

   Why do we need to log out and back in again after we've made the assertion?

Thanks for the fix, I will test it and give feedback here!

Now committed. Thanks, everyone for your work on this. I'll likely make a new release shortly to be sure this gets included. Please comment on #3021912: Plan for creating email registration 8.x-1.0 final stable with anything else you'd like to see fixed soon.

And for anyone who needs to cleanup, from @darrenwh in comment #21:

use Drupal\user\Entity\User;

/**
 * Fixes broken users
 */
function MODULE_NAME_update_8001() {
  $ids = Drupal::entityQuery('user')
    ->condition('name', 'email_registration_%', 'LIKE')
    ->execute();
  $users = User::loadMultiple($ids);
  foreach ($users as $account) {
      $new_name = preg_replace('/@.*$/', '', $account->getEmail());
      // Clean up the username.
      $new_name = email_registration_cleanup_username($new_name);
      $new_name = email_registration_unique_username($new_name, $account->id());
      $account->setUsername($new_name);
      $account->save();
  }
}

Thanks for the update code @darrenwh! My version didn't check for uniqueness and so crashed my server :(

However, the code in #68 doesn't scale for sites with large numbers of users.

Here's my version that runs a batch, with @darrenwh's code for setting the user name added in:

/**
 * Fix usernames that have been mangled by a bug in email_registration.
 */
function MODULE_NAME_update_8001(&$sandbox) {
  // This fixes users whose names have been changed during an edit operation,
  // due to a bug in the email_registration module.
  // See https://www.drupal.org/project/email_registration/issues/3024558.
  if (!isset($sandbox['progress'])) {
    // This must be the first run. Initialize the sandbox.
    $sandbox['progress'] = 0;
    $sandbox['current_uid'] = 0;
    $sandbox['max'] = \Drupal::database()->query("SELECT COUNT(uid) FROM {users_field_data} WHERE `name` LIKE 'email_registration\_%'")->fetchField();
  }

  // Update in chunks of 20.
  $uids = \Drupal::database()->query("SELECT uid FROM {users_field_data} WHERE `name` LIKE 'email_registration\_%' ORDER BY uid limit 20")->fetchCol();
  $users = \Drupal::entityTypeManager()->getStorage('user')->loadMultiple($uids);

  foreach ($users as $user) {
    $new_name = preg_replace('/@.*$/', '', $user->getEmail());
    // Clean up the username.
    $new_name = email_registration_cleanup_username($new_name);
    $new_name = email_registration_unique_username($new_name, $user->id());
    $user->setUsername($new_name);
    $user->save();

    $sandbox['progress']++;
    $sandbox['current_uid'] = $user->id();
  }

  $sandbox['#finished'] = empty($sandbox['max']) ? 1 : ($sandbox['progress'] / $sandbox['max']);

  return t('@count users out of @total have been updated to have their usernames fixed.', [
    '@count' => $sandbox['progress'],
    '@total' => $sandbox['max'],
  ]);
}

When will the fix be updated in the module?

8.x-1.0-rc7 was released in May https://www.drupal.org/project/email_registration/releases/8.x-1.0-rc7 with this fix in it. Did you upgrade to that? Do you still experience a bug? If so, please open a new issue with the details of that bug.

@greggles,

Thanks for the update. I'm using the latest version https://www.drupal.org/project/email_registration/releases/8.x-1.0-rc7

However, the issue is still not fixed. I had seen the .install file and couldn't find any code related to this.

Updating the DB with patch provided in this issue will resolve my problem. But i want a fix from the module update that will automatically fix and i can make sure my DB is safe.

Please help as this is a long pending issue. Thank you in advance.

Updating the DB with patch provided in this issue will resolve my problem.

Do you mean the code from comment #69?

But i want a fix from the module update that will automatically fix and i can make sure my DB is safe.

Patches welcome. I think ideally it would be an action that could be used with views bulk operations as it seems somewhat generally useful.

Filed follow-up #3081817: Add action to regenerate username /cc @greggles