Add setting to allow users to login with email address or username

subscribing

I just ran the patch and got the following message. Any suggestions? Is there a specific point at which I should have run the patch. For what it's worth, I first installed email_registration. Didn't use it. Then I disabled the module. Then I ran the patch and got the following.

"Hunk #1 succeeded at 2 with fuzz 1.
patching file email_registration.module
Hunk #1 FAILED at 14.
Hunk #2 succeeded at 27 (offset -15 lines).
Hunk #3 succeeded at 54 (offset -15 lines).
Hunk #4 succeeded at 96 (offset -7 lines).
1 out of 4 hunks FAILED -- saving rejects to file email_registration.module.rej"

this is what is in email_registration.module.rej...
<?
***************
*** 14,51 ****
function email_registration_user($op, &$edit, &$account, $category = NULL) {
switch ($op) {
case 'insert':
- // Other modules may implement hook_email_registration_name($edit, $account)
- // to generate a username (return a string to be used as the username, NULL
- // to have email_registration generate it)
- $names = module_invoke_all('email_registration_name', $edit, $account);
- // Remove any empty entries
- $names = array_filter($names);
-
- if (empty($names)) {
- // Default implementation of name generation
- $namenew = preg_replace('/@.*$/', '', $edit['mail']);
- // if username generated from email record already exists, append underscore and number eg:(chris_123)
- if (db_result(db_query("SELECT count(*) FROM {users} WHERE uid <> %d AND LOWER(name) = LOWER('%s')", $account->uid, $namenew)) > 0) {
- // find the next number available to append to the name
- $sql = "SELECT SUBSTRING_INDEX(name,'_',-1) FROM {users} WHERE name REGEXP '%s' ORDER BY CAST(SUBSTRING_INDEX(name,'_',-1) AS UNSIGNED) DESC LIMIT 1";
- $nameidx = db_result(db_query($sql, '^'. $namenew .'_[0-9]+$'));
- $namenew .= '_'. ($nameidx + 1);
- }
- }
- else {
- // One would expect a single implementation of the hook, but if there
- // are multiples out there use the last one
- $namenew = array_pop($names);
- }

- // replace with generated username
if (db_query("UPDATE {users} SET name = '%s' WHERE uid = '%s'", $namenew, $account->uid)) {
$edit['name'] = $namenew; // update in the user array for access by other modules
}

- // if email verification is off and a new user is the one creating account, log the new user in with correct name
global $user;
- if (!variable_get('user_email_verification', 1) && $user->uid == 0) {
$user = $account;
$user->name = $namenew;
}
--- 14,30 ----
function email_registration_user($op, &$edit, &$account, $category = NULL) {
switch ($op) {
case 'insert':

+ $namenew = email_registration_generate_name($edit, $account);
+
+ // Replace with generated username
if (db_query("UPDATE {users} SET name = '%s' WHERE uid = '%s'", $namenew, $account->uid)) {
$edit['name'] = $namenew; // update in the user array for access by other modules
}

+ // If email verification is off and a new user is the one creating account, log the new user in with correct name
global $user;
+ if (!variable_get('user_email_verification', 1) && $user->uid == 0) {
$user = $account;
$user->name = $namenew;
}

?>

sorry. Just noticed this should have been posted under 'support'.

Patch re-roll. It also updates the text on the user login form to reflect the code changes.

Hello,

Thank you very much on your hard work. The patch at #4 against the latest dev resulted in the below error.
Can the patch at #4 be brought up to the latest dev? I look forward to testing this. Thanks!

patching file email_registration.module
Hunk #1 FAILED at 14.
Hunk #2 succeeded at 45 (offset 3 lines).
Hunk #3 succeeded at 63 (offset 4 lines).
Hunk #4 succeeded at 80 (offset 4 lines).
Hunk #5 succeeded at 144 (offset 34 lines).
1 out of 5 hunks FAILED -- saving rejects to file email_registration.module.rej
can't find file to patch at input line 189
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: email_registration.install
|===================================================================
|--- email_registration.install (revision 144)
|+++ email_registration.install (working copy)

Patch re-roll

Hello,
i have install Email Registration module but still its allowing me to login using the username.

Thanks,
Kruti

#6 stella, patch failed. otherwise RTBC.

may the patch at #6 be re-rolled?

@Chris - is there a good chance of this option being implemented in Email Registration module?
Thanks!

+1 on adding this to Email Registration

Attached is patch #4 rerolled against 1.3 (since .install was removed from -dev).

subscribing

Patch applied smoothly and I see the "Allow users to login with their email address OR their username" setting in User settings.
I have the setting unchecked so that users must enter their email address.

I'm seeing an issue when a user tries to log in with an unknown email address.

Without applying the patch the user gets the below warning message, which makes sense:

Sorry, unrecognized username or password. Have you forgotten your password?

After applying the patch the user gets the below warning message, which does not make sense:

The username has not been activated or is blocked.

Can this issue be looked into? I appreciate the support!

Any chance someone could look into the issue of the unrecognized email message being changed after the patch is applied? I'm available for heavy testing and would like to see this committed into the module. Thanks!

Any updates in this development?

Subscribing

Would anyone be able to kindly look into finalizing the patch?

I guess this should be 'needs work' until #13 message issue is addressed?

Great module! It would be awesome to see it further develop to add these types of very useful features.

Thanks for all the hard work!

Isn't this feature also available using LoginToboggan module? Seems that the two modules are overlapping in some parts :|

Re-rel for the latest email_registration module
The settings now allow of fine tune the "login" process: admin can choose between:
1. username
2. username OR email
3 email only

Review needed and maybe some more simple test cases.

Notes: this patch integrates #421078-6: Abstract name unique check for re-use, make it is cross-database compatible, make it more robust, us it on external names patch for for cross-database query and some code-cleanups (done with Coder module)

#21 works fine! Still is one issue to solve: after registration 'username' is exactly the same as e-mail address, proper 'username' is no longer generated as it was originally.
[email: me@myhost.com -> username: me@myhost.com but should be username: me]

I've fixed your issue (please check again) and implemented few more SimpleTests for the "login with email OR username" feature. No tests for "generate username" yet.

Please review the new patch

notes: unfortunately this patch also includes the following patches (with fixes)
#678434: Don't generate username if the account->user is already set
#421078: Abstract name unique check for re-use, make it is cross-database compatible, make it more robust, us it on external names

I can help as a co-maintainer if the module owner wants :)

There is sth wrong with that patch - it can be applied only to *.module file. Another issues:
1. during first log in two very short messages are displayed to user:
-- " array() "
-- " 'my_username' " where my_username is generated from e-mail address
2. registration confirm email (when user can log in only with e-mail address) should be override with email address instead of username (information given is wrong - user cannot log in with this username)

I tried to use this patched version (patch applied to email_registration.module) with 'login toboggan' and it seems that it is possible to use both at the same time (I'm talking about logging in with e-mails only).

@pawel_r : you're right, unfortunately I left some debug messages (using drupal_set_message() function) in the patch code :| I'm sorry

The attached patch should fix it

hi thePanz, it would be AWESOME if you were able to support this module as co-maintainer.
I am not sure about the process but maybe opening up a new post in the queue requesting is the first step?
Thank you!

Please consider tackling #738134: Conflict with LoginToboggan submit handler
It seems to be one issue caused by specific code in Email Registration which doesn't play nice with other modules.

Many thanks

#25 thx I will try it this week. It would be great if you could prepare special patch that adds ER functionality to LT. In my opinion register with 'username' not with e-mail address is just 'obsolete' idea.

@pawel_r: unfortunately I fount LT's code a bit confusing (I sent a patch with a first code-cleanup, but the owner didn't accept it, sentencing that the stable branch for Drupal6 is in "code-freeze")..

I fount, instead, that ER is a great and simple module to deal with. Some functionality should be improved or fine-tuned, but it's almost great! :)

I'd like to help as a co-maintainer.. the module owner maybe will accept my "candidature" let see! :)

Regards

thePanz - were you accepted as a co-maintainer? More development in ER would be great!

Patch to be applied after patch given at #25. fully integrated with Login Toboggan 6.x-1.10 (couldn't spot any errors). It has not been tested with disabled Login Toboggan (always enabled).

EDIT actually it is not perfectly fine

Thanks for the patch #25 it works on my site. I have installed logintoboggan with option "Set password" and "Immediate login" but so far don't really see the error. What #31 is for? Is it a patch for http://drupal.org/node/738134?

By the way, if the maximum length of username is set by Access rules at my-site.com/admin/user/rules for example to 15 it gives error on the registration like this. I just removed this access rule I previously used to use and then the error is gone so it's not a really big deal.
"email_registration_bUSefQHSMf can't be registered. Username can only contain up to 15 characters and punctuation is not allowed except for periods, hyphens, and underscores. "

I just noticed that I had to disable "Allow users to login using their e-mail address" option for logintoboggan to use "eMail only" option for email registration module. Otherwise the title and description of login form can't be override by email registration module.

So, this patch does a lot of cleanup/style issues, which is certainly well intentioned, but kind of a bummer because it makes it harder to review. At a minimum it needs the $Id to be removed. It would be nice to get a reroll without the cleanup/reorganization stuff and move those to a separate issue.

I've now focused this issue down to just the specific feature and updated the issue summary to reflect that.

Here's an updated patch for 7.x that I think does the job.

thePanz had some code to add more complex user messages if they theoretically can use their username to login. In my opinion we should keep the text to a real minimum here.

One bug: If you try to login with a valid username, you get the message "The username has not been activated or is blocked."

I've also added some tests to cover these two settings.

As I think about it more, I don't feel super strongly about this issue. If someone else can review the code, provide feedback and maybe fix the one bug I note in #35 that would be great.

Why not add an email validation handler to the form['name']['#element_validate'] and toggle there? As it currently stands now I can login with my email address or my username (which I want and I am happy with) but if you wanted to block username logins, that could be a lot easier as it will need to validate as a valid email address before any processing is done. The only thing with that is if the username is a valid email address. Then wuts it really matter?

Here is a patch that:

- fixes the bug mentioned in #35
- changes the login forms to say 'E-mail or username' when a username is also an allowed option
- fixes inconsistencies in UI strings that the prior patch had (email/e-mail, full stops, etc)

Another minor point is that our new admin setting technically should not go in the 'Registration and cancellation' fieldset, since it's about neither. But I am loath to add a new fieldset for a single checkbox!

Urgh. Will figure that out tomorrow!

Fixed. The test for logging in with just the username was passing in the $login array with the password from the first user the test created.

The last submitted patch, 657472.email_registration.allow-login-with-username.41.patch, worked for me. Only thing which I felt is missing is that if we can have toggle option available for login via username or email address on user setting page, that will make module more user friendly.

+++ b/email_registration.module
@@ -138,21 +142,61 @@ function email_registration_form_user_login_alter(&$form, &$form_state) {
+ * Implements hook_form_FORM_ID_alter().
+ */
+function email_registration_form_user_admin_settings_alter(&$form, &$form_state) {
+  $form['registration_cancellation']['email_registration_login_with_username'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Allow users login with e-mail or username.'),
+    '#description' => t('Allow users to login with their username in addition to their e-mail.'),
+    '#default_value' => variable_get('email_registration_login_with_username', FALSE),
+    );
+  $form['#submit'][] = 'email_registration_form_user_admin_settings_submit';
+}

This part of the patch puts the option on the user settings page.

@joachim - I think @nicesukhi1 was suggesting that it be an option within each user's control on a per-user basis on user/UID/edit. In my opinion that's more information than should be exposed to the individual user.

I agree conceptually with this issue though I haven't yet tested out the behavior of this patch yet.

I'm reviewing this now and have a question...which way should be the default?

Should the default be to allow both email and username (the current module behavior, but which would change user facing strings) or to only allow email (what the strings say, but NOT the behavior).

Everything works as expected, the code looks good, and it's pretty compact.

I decided that I'd rather have the UI strings change than the behavior of the site, so I flipped them all to true (and changed the tests and comments to match).

This is RTBC from me, but leaving Needs Review for a bit longer for others to review.

If I'm following this correctly:

- the basic intention of this module is to allow users to be blissfully ignorant of the concept of a 'username', and to hide that concept from them
- however, current behaviour of this module allows login with username, which is an unintended feature.
- therefore, the default for this new setting ought to be to NOT allow login with username.
- however, imposing the default as FALSE would change the behaviour of the module mid-cycle, which is bad.
- therefore, we compromise, and set the default to TRUE.

Is that a fair summary?

If so, this all seems sensible to me.

And presumably, we should file a follow-up to change the default to FALSE come D8?

Yes, #47 is exactly how I feel.

focus the issue on the one problem

Re-rolling the patch from #46.

Patch in #49 applied successfully on 7.x-1.x-dev version of this module. But it is working. I cannot find any setting where I can select username or email registration.

@subhojit777, the settings are under Account settings - /admin/config/people/accounts

'Allow users login with e-mail or username.'

The option is to allow users to either login using either username or email, or login only using email.

Apologies, I was expecting the setting in a new page. Patch in #49 is working.

EDIT:
Patch from 49 works.

Hmmm ... not sure why #49 is showing up as deleted ... re-uploading so it stops messing with testbot.

I just rolled a new release, so this seemed like a good time to add in this feature to get some testing. Then in a month or two we can make a new release of 7.x.

So, I've committed this! Thanks everyone for your work on this over the years (years!) that it took to get it in. I don't work on d6 any more so am moving this to fixed instead of backport. If someone wants to backport, please post the patch and set the status to needs review.

This commit break test, so I pushed fix, also cleaned up the test.
This needs to be ported to 8.x

EDIT probably #423920: [2.x] Prevent race condition in account generation code by adding uid to name caused the test breakage

Taking a stab at 8.x-1.x patch.

Updating default values and fixing failing config set in test.

Trying this one more time... maybe someone can suggest what could be causing failure. I've verified that the intended functionality has been achieved.

I suppose most of failures caused by absence of default config and schema for it

+++ b/email_registration.module
@@ -138,12 +138,14 @@ function email_registration_form_user_pass_alter(&$form, FormStateInterface $for
+  $config = \Drupal::config('email_registration.settings');
+  $login_with_username = $config->get('login_with_username') ? $config->get('login_with_username') : TRUE;

when you introduce config it needs defaults in module & schema

also and upgrade path for existing sites

Ok, adding a config file.

My brief reading of Defining and using your own configuration in Drupal 8 seems to indicate that a schema file if translation is required. Since this is a boolean config value, I don't believe it is applicable.

Turns out we do need a schema.yml.

--- /dev/null
+++ b/config/install/email_registration.settings.yml

@@ -0,0 +1,7 @@
+email_registration.config:

name of config in schema should be the same

+++ b/config/install/email_registration.settings.yml
@@ -0,0 +1 @@
+login_with_username: TRUE

I think default value should be FALSE to keep backward compatibility but having it TRUE make more sense from UX pov

sorry xposted

+++ b/email_registration.module
@@ -138,12 +138,14 @@ function email_registration_form_user_pass_alter(&$form, FormStateInterface $for
+  $login_with_username = $config->get('login_with_username') ? $config->get('login_with_username') : TRUE;

Please use php 5.3 ternary op

$login_with_username = $config->get('login_with_username') ?: TRUE;

Btw this condition always TRUe(

Why support 5.3 syntax if D8 doesn't support 5.3? Fixing config evaluation...

Switching default value to false.

Fixing login values.

I'm going to guess that `$form_state->setValue('name', NULL);` isn't working out well. Will need to flag an error in some other way, e.g., $form_state->setErrorByName().

Not sure why this one is failing. Manual testing appears to work fine.

Update the patch in #86:
* Fixed failing test
* Added UI to configure the newly introduced setting

+++ b/email_registration.info.yml
@@ -1,4 +1,10 @@
-core: 8.x
+# core: 8.x
+
+# Information added by Drupal.org packaging script on 2016-07-15
+version: '8.x-1.0-rc3'
+core: '8.x'
+project: 'email_registration'
+datestamp: 1468594748

I think this change should be removed.

I also did some manual testing and login works with username and e-mail, so this patch is almost ready.

Fixed the changes.

@harsha012,

Thanks for the patch, but there are a few files missing that are added in #88. Can you also add those files again?

+++ b/email_registration.info.yml
@@ -1,4 +1,3 @@
+description: 'Allows users to register with an e-mail address as their username.'
\ No newline at end of file

There should be a newline at the end of this file.

core: 8.x should also be re-added to the email_registration.info.yml file.

Added the core version

@harsha012,

Thanks for the patch, but the files added in #88 are still missing.

+++ b/email_registration.info.yml
@@ -1,4 +1,4 @@
-core: 8.x
+core: 8.x
\ No newline at end of file

Newline is still missing. Can you please add it?

Fixed it.

@harsha012,

The changes in email_registration.info.yml look great, but the following files are still missing:

• config/install/email_registration.settings.yml
• config/schema/email_registration.schema.yml
• email_registration.links.menu.yml
• email_registration.permissions.yml
• email_registration.routing.yml
• src/Form/SettingsForm.php

Can you add please add those files back to the patch? You can find more information here: https://www.drupal.org/node/707484

okay @JeroenT

Added files back to patch

@harsha012,

Great to see the files are back.

There is still one problem, the following files have no newline at the end of the file:

• config/install/email_registration.settings.yml
• config/schema/email_registration.schema.yml
• email_registration.links.menu.yml
• email_registration.permissions.yml
• email_registration.routing.yml
• src/Form/SettingsForm.php

but this can also be fixed on commit. marked as RTBC.

@harsha012 please revert the patch to previous state - I mean #98 + config schema and default config to pass tests

• config/install/email_registration.settings.yml
• config/schema/email_registration.schema.yml

Also it needs hook_update_N() for existing sites

@JeroenT separate form with one checkbox make no sense at all

1. +++ b/config/install/email_registration.settings.yml
   @@ -0,0 +1 @@
   \ No newline at end of file
   
   +++ b/config/schema/email_registration.schema.yml
   @@ -0,0 +1,7 @@
   \ No newline at end of file
   

   still needs to be fixed

2. +++ b/email_registration.module
   @@ -166,3 +171,27 @@ function email_registration_user_login_validate($form, FormStateInterface $form_
   +function email_registration_form_user_admin_settings_alter(&$form, &$form_state) {
   +  $config = \Drupal::config('email_registration.settings');
   +  $form['registration_cancellation']['login_with_username'] = array(
   +    '#type' => 'checkbox',
   
   +++ b/src/Form/SettingsForm.php
   @@ -0,0 +1,54 @@
   +  public function buildForm(array $form, FormStateInterface $form_state) {
   +    $config = $this->config('email_registration.settings');
   +
   +    $form['login_with_username'] = array(
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Allow users login with e-mail or username.'),
   +      '#default_value' => $config->get('login_with_username'),
   +      '#description' => $this->t('Allow users to login with their username in addition to their e-mail.'),
   +    );
   +
   +    return parent::buildForm($form, $form_state);
   

   There's already checkbox for that setting so I see not actual reason to implement new form with single checkbox on it.

   Moreover there's no upgrade path for existing sites in the patch...

   Also new permission makes no sense because this setting could be changed in user administration form

3. +++ b/src/Tests/EmailRegistrationTestCase.php
   @@ -23,6 +23,7 @@ class EmailRegistrationTestCase extends WebTestBase {
   +    $email_registration_config = $this->container->get('config.factory')->getEditable('email_registration.settings');
   
   @@ -49,6 +50,21 @@ class EmailRegistrationTestCase extends WebTestBase {
   +    $email_registration_config->set('login_with_username', TRUE)->save();
   

   Nitpick - this is used only once so no reason in separate variable,
   I'm fine to keep it, because tests should be polished after #2066993: Use \Drupal consistently in tests

Added patch. please review.

+++ b/email_registration.links.menu.yml
@@ -0,0 +1,5 @@
+email_registration.settings:

+++ b/email_registration.routing.yml
@@ -0,0 +1,7 @@
+  path: '/admin/config/people/email_registration'

+++ b/src/Form/SettingsForm.php
@@ -0,0 +1,54 @@
+    $form['login_with_username'] = array(

please remove form with only one checkbox and move this checkbox to account settings form

hi @andypost,

Improved the patch. please review

Overall this looks mostly ready, except update hook and nitpick with extra empty lines
Needs work for update hook clean-up

1. +++ b/email_registration.install
   @@ -23,3 +25,36 @@ function email_registration_requirements() {
   +function email_registration_update_8100() {
   +  $email_registration_settings = \Drupal::config('email_registration.settings');
   +  $config_factory = \Drupal::configFactory();
   +  $email_registration = $config_factory->getEditable('email_registration.settings');
   +  $login_with_username = $email_registration_settings->get('login_with_username');
   +  $email_registration->set('login_with_username', $login_with_username);
   +  $email_registration->save();
   +  $message = "Email Registration setting Form Added";
   +  return $message;
   +}
   +
   +function email_registration_update_8102() {
   +  $message = NULL;
   +  if(\Drupal::moduleHandler()->moduleExists('email_registration')) {
   +    $config_path = drupal_get_path('module', 'email_registration') . '/config/install/email_registration.settings.yml';
   +    $data = Yaml::parse($config_path);
   +    \Drupal::configFactory()->getEditable('email_registration.settings')->setData($data)->save(TRUE);
   +    $message = 'The email registration setting updated.';
   +  }
   + return $message;
   +}
   +
   +function email_registration_update_8103() {
   +  $message = NULL;
   +  if(\Drupal::moduleHandler()->moduleExists('email_registration')) {
   +    $config_path = drupal_get_path('module', 'email_registration') . '/config/schema/email_registration.schema.yml';
   +    $data = Yaml::parse($config_path);
   +    \Drupal::configFactory()->getEditable('email_registration.settings')->setData($data)->save(TRUE);
   +    $message = 'The email registration setting schema created.';
   +  }
   + return $message;
   +}
   

   this looks totally wrong

   1) this should be one update hook
   2) this should just set to FALSE default setting for existing installs (getEditable, set FALSE, save)

   for new installs provided config file will be installed automatically

2. +++ b/email_registration.module
   @@ -166,3 +171,30 @@ function email_registration_user_login_validate($form, FormStateInterface $form_
   +function email_registration_form_user_admin_settings_submit(array &$form, FormStateInterface $form_state) {
   +    $config = \Drupal::configFactory()->getEditable('email_registration.settings');
   +    $config->set('login_with_username', $form_state->getValue('login_with_username'))->save();
   +}
   +
   +
   +
   

   no need in extra empty lines

improved it. please check

1. +++ b/email_registration.install
   @@ -23,3 +25,12 @@ function email_registration_requirements() {
   +  $message = "Email Registration setting Form Added";
   +  return $message;
   

   no need in this message

2. +++ b/email_registration.install
   @@ -23,3 +25,12 @@ function email_registration_requirements() {
   \ No newline at end of file
   
   +++ b/email_registration.module
   @@ -166,3 +171,27 @@ function email_registration_user_login_validate($form, FormStateInterface $form_
   \ No newline at end of file
   

   needs new lines

@andypost improved the patch as per comment #116

@harsha012 thanx a lot! now it looks ready to go

Assigning to @greggles to commit/approve

I reviewed the patch and it looks really great. I found a few small nitpicks.

1. +++ b/email_registration.install
   @@ -1,5 +1,7 @@
   +use Symfony\Component\Yaml\Yaml;
   +
   

   This use statement seems to be unused.

2. +++ b/email_registration.module
   @@ -138,12 +134,14 @@ function email_registration_form_user_pass_alter(&$form, FormStateInterface $for
   +  $form['name']['#title']  = isset($login_with_username) ? t('E-mail or username') : t('E-mail');
   +  $form['name']['#description']  = isset($login_with_username) ? t('Enter your e-mail address or username.') : t('Enter your e-mail address.');;
   

   These two lines have 2 spaces before the '='.

   The last line has two semicolons at the end.

3. +++ b/email_registration.module
   @@ -154,9 +152,16 @@ function email_registration_form_user_login_form_alter(&$form, FormStateInterfac
   +      $query = isset($user_input['name']) ? array('name' => $user_input['name']) : array();
   +      $form_state->setErrorByName('name', t('Unrecognized e-mail address or password. <a href=":password">Forgot your password?</a>', array(':password' => Url::fromRoute('user.pass', [], array('query' => $query))->toString())));
   

   It is recommended to use the shorthand array syntax '[]' instead of the longhand 'array()' for D8.

4. +++ b/email_registration.module
   @@ -166,3 +171,27 @@ function email_registration_user_login_validate($form, FormStateInterface $form_
   +function email_registration_form_user_admin_settings_alter(&$form, &$form_state) {
   +    $config = \Drupal::config('email_registration.settings');
   

   It's a good practice to type hint the $form_state on FormStateInterface. Also since it is an object it is not needed to pass it by reference.

5. +++ b/email_registration.module
   @@ -166,3 +171,27 @@ function email_registration_user_login_validate($form, FormStateInterface $form_
   +        '#title' => t('Allow users login with e-mail or username.'),
   +        '#description' => t('Allow users to login with their username in addition to their e-mail.'),
   

   Maybe update these user facing strings a bit. It's 'to log in' and not 'to login'.

Fixed my nitpicks from #119. Leaving this on RTBC since this only changes coding standards and strings, not actual functionality.

+++ b/email_registration.module
@@ -138,12 +134,14 @@ function email_registration_form_user_pass_alter(&$form, FormStateInterface $for
+  $login_with_username = $config->get('login_with_username');
+  $form['name']['#title'] = isset($login_with_username) ? t('E-mail or username') : t('E-mail');
+  $form['name']['#description'] = isset($login_with_username) ? t('Enter your e-mail address or username.') : t('Enter your e-mail address.');

This part doesn't work yet. $login_with_username is always set, so the message about the 'e-mail or username' is always shown, even if the option is disabled.

This should use !empty() instead of isset().

nice catch, then test should be also improved

I'll give it a go, I have some free time now.

Added the test and fixed the bug. I also ported the test to BrowserTestBase, it was just a matter of moving the test to correct location and changing the base class.

The additional test coverage also discovered a new small bug: if the login form was cached by the page cache, the cached version was not cleared when the Email Registration settings change. The old form would still be displayed until the next time a full cache clear occurs. I fixed this too.

There are a few ways that this can still be improved. It would be great to for example make the UI texts and error messages configurable. I consider this nice-to-haves though. They can be added later in follow ups.

Marking as RTBC since remarks in #119 are fixed and the tests are improved.

I just released next RC including #2825909: Password Reset Email Field Type
so patch maybe needs reroll, I think if everything went fine to release 1.0 and include this change to 1.1

+++ b/tests/src/Functional/EmailRegistrationTestCase.php
@@ -23,6 +23,7 @@ class EmailRegistrationTestCase extends WebTestBase {
+    $email_registration_config = $this->container->get('config.factory')->getEditable('email_registration.settings');

there's suggestion to get rid of $this->container just can;t find core issue

Needs work because removes compatibility with early commited #2777701: Email registration module needs to do Profile validation prior to saving

+++ b/email_registration.module
@@ -130,8 +130,12 @@ function email_registration_form_user_pass_alter(&$form, &$form_state) {
+  $form['name']['#title'] = variable_get('email_registration_login_with_username', TRUE)

looks we need migration path from d7 in follow-up

1. +++ b/config/install/email_registration.settings.yml
   @@ -0,0 +1 @@
   +login_with_username: FALSE
   
   +++ b/email_registration.install
   @@ -23,3 +23,13 @@ function email_registration_requirements() {
   +  $email_registration = $config_factory->getEditable('email_registration.settings');
   +  $email_registration->set('login_with_username', FALSE);
   

   I think for new installs better to have this TRUE but for existing ones set to false in update hook

2. +++ b/email_registration.module
   @@ -44,12 +45,7 @@ function email_registration_user_insert(UserInterface $account) {
   -  $account->setUsername($new_name);
   -  if ($account->isValidationRequired() && !$account->validate()) {
   -    \Drupal::logger('email_registration')->error('Email registration failed setting the new name on user @id.', ['@id' => $account->id()]);
   -    return;
   -  }
   -  $account->save();
   +  $account->setUsername($new_name)->save();
   

   still not sure about removal

   on one hand that should never happens otoh that code was added for reason in previous release #2777701: Email registration module needs to do Profile validation prior to saving

that code was added for reason in previous release #2777701: Email registration module needs to do Profile validation prior to saving

OK I didn't realize this. It appears a bug was fixed in that issue but there was no test added for it, and no documentation on why this is needed.

I think for new installs better to have this TRUE but for existing ones set to false in update hook

Are you sure about this? I think it is not the most common use case for websites to allow users to log in using either a username or e-mail address. OK, there are a lot of sites out there that do this, but the vast majority allow only to log in with e-mail address, or only to log in with username.

I think it's better to default to e-mail address only, I think that better fits with what people expect who install this module. The help texts are also easier to understand. Anyway this is not so important, it can be configured easily :)

The issue summary mentions "The documentation seems to imply that this is not the intended behavior but it seems like some people might want it." -> this means we also need to update the documentation in this issue.

• #129.1. Created follow-up for the migration path. #2844622: Migration path email_registration_login_with_username to $config['email_registration.settings']['login_with_username']..
• #129.2. Reverted the code. and added an issue #2844618: Write test for #657472.

Patch attached.

What's keeping us from committing this issue?

Remarks from #129 are answered and Follow-up issues are created.

Works as expected

Now that looks totally ready for commit, gonna commit it this WE

+++ b/config/install/email_registration.settings.yml
@@ -0,0 +1 @@
+login_with_username: FALSE

to fix on commit, should be lowercase

Thanx everyone! finally commited