Request for feedback: Should username change on user login or save

Upgrading to major for the reason stated above.

Seems as though this may be intended functionality. Please close if so.

Yeah, I think ✨ Update username when updating user entity Needs review introduced this behavior. If it negatively affects your use case we might want to consider making it an option that people can enable/disable?

Thanks, yes I agree with @greggles, we should discuss this. Sadly @Grevil is in parental leave until January '24.

Adjusting the title to reflect the current goal. We can discuss (and get feedback) prior to January 2024 :) Also, congratulations, @Grevil!

Just ran into this, thankfully, on localhost.

I would side that this should be an option/config for site builders to choose the behaviour, as this would have caused some issues if it were deployed as is for my particular use-case.

Especially with the current config of `login_with_username` enabled, if users are used to logging in with a self-defined username rather than a trimmed version of their email, this may lead to users suddenly not being able to login how they are used to, and many password reset requests being made following this update.

This commit is where the hook was changed from email_registration_user_insert() to email_registration_user_presave()

https://git.drupalcode.org/project/email_registration/-/commit/b4aa835b6...

I'm not sure what the rationale for doing this was but this has caused all my users names to be set to the first part of their email before the @ any time an account is updated, so now I have a bunch of users with the names like info, info_1, info_2

At the very least this should be a config setting, but this is not what I expected. It prevents anyone for changing their username on the user edit form. The name is always set to the first part of the email.

I've reverted to using 8.x-1.4 which does not have this change.

Thanks @loze no doubt, that should not happen on account update (or we should have a setting for that).

This is one of the issues where the change was introduced: 🌱 [2.x] Rework username generation save logic Active
It shouldn't be reverted IMHO, but we should instead add more tests to ensure it works as expected, document the functionality and keep the existing behaviour. Still there might also be cases, where that kind of username update is expected!

I'll have a closer look at this with @Grevil next year. Until that I think we should recommend the 8.x-1.x branch.

I added a note on the module page to let users know until this is solved.

PS: Is the Email Registration Username an alternative for you? Could you please describe your username generation logic and if users are allowed to change their usernames? So we can better understand these cases.

Thanks @Anybody

I use this module on a handful of Drupal sites like this:

• Allow user to create an account with only an email and password (no username)
• On registration (user_insert) I set a default username typically something like 'user-[uid]'
• After they authenticate theor account, I allow them to change their username, this is mainly used for creating friendly user profile urls with pathauto (mydomain.com/u/myusername)
• I also allow users to log in using wither the username or the email address.

I don't think Email Registration Username would fit this. Actually I've really had no complaints with this workflow using the 1.x version. I just assumed 2.x would keep the same behavior.

Thankfully I only updated to 2.x on one site. I actually had it running in production for a few weeks before I realized why some users were reporting they couldn't login any longer with (what they thought) was their username and their profile urls had changed.

Thank you very much for your detailed description @loze that helps a lot!

So TL;DR for the username change (and I think we'll be able to fix that) is to only set the generated username on profile creation, not when re-saving / changing the username later. (Which makes sense - so perhaps it's indeed a bug. Later changes in batch can be done through the provided action already).

What we have to take care of, is an email address change. In that case we may also need to update the username, but I guess that's specific to Email Registration Username.

@chucksimply and @Wakuiy would be super nice, if you could add a short description of your workflows, so we can also consider these.

@greggles Thank you! :)
I am back now. @Anybody switched up the dates.

I don't think I'll get the time to look into it today, but I'll have a look tomorrow!

I honestly can't remember any more, why we removed the empty and starts_with check from "email_registration_user_presave", I know that we got a bit confused on how to keep the deprecated "hook_email_registration_name" logic, while also having our new hook in place, but on first look the current code just doesn't check out...

Alright, I fixed the problem! And I remember again, why we did it this way. Our main focus for the last couple updates, was the submodule "email_registration_username", which has the functionality to sync the username with the email address. But because the submodule defines an alter hook, which is called in the main modules presave() hook. They have heavy dependencies to each other.

So when we only allowed username change on account creation, we needed to call all alter hook implementation inside that if case, meaning our sync functionality of the submodule didn't work any more, because the account name already existed. And then we removed the if case entirely not thinking of the main module, but focusing too much on the functionality of the submodule.

Now my fix introduces a new "$onlyChangeOnCreation" variable, so the module implementing the alter hook, can decide itself, whether it wants to always alter the username or only on user creation. This way the main module functions as before and the submodule can overwrite "$onlyChangeOnCreation" to always run the presave hook and have their own if cases for altering inside their implementation.

All tests were green locally!!

Thanks @Grevil for picking this up. Perhaps we could simplify this even more and keep the existing hook parameters (BC!) by using $account->isNew() in the hook!

It's true, if the account is newly created. So the implementing child module could decide if it needs to change the username or return early.

In the main module we could perhaps create a copy of the original name before it is passed to the alter hook and after the hook was executed, compare it to the original value.

Only if they changed, the name should be updated.

I think that might be more straightforward to understand and read and introduce no BC. But I may have overseen something, so let's implement that as separate issue fork, please.

Afterwards we should discuss, tests and review this very carefully to not introduce a new issue. Ist's jut a few lines, but they aren't that simple to understand.

@Grevil: And I think we need a test in the base module which ensures the username does not change when resaving the user profile, even if it doesn't fit the new standard.

Please implement that one in a separate MR so we can merge it separately from the taken solution. It should also not change, if 1.x was used previously.

Please check if we have tests in the submodule which ensure that the name is changed when the email address changes and it's configured to do so. This is too risky without tests and I'm sure they will help a lot :)

Perhaps we could simplify this even more and keep the existing hook parameters (BC!) by using $account->isNew() in the hook!

I don't think this would be a good idea, the presave hook doesn't only fire if the user is new. We should keep to our existing logic, using the temporary "email_registration_" name prefix to decide whether it should fire or not, like it did before.

This way we don't break anything. For example our "UpdateUserNameAction", which prefixes existing accounts with "email_registration_" flagging it as "to be updated" "$account->isNew()" wouldn't do much here. And there are probably more issues with our commerce API.

Although to be fair "$onlyChangeOnCreation" isn't probably the best variable name, we should change that up for sure. I'll start adding some tests and create a "test-only" branch to see the tests fail before the changes.

Please check if we have tests in the submodule which ensure that the name is changed when the email address changes and it's configured to do so

Yes we do! That test failed locally, that's how I originally remembered the problem!

Alright, added 4 appropriate tests!

They succeed with the patch provided and fail without the patch. Please review!

Grevil → closed merge request !35

Alright, @Anybody and I came up with another solution, which doesn't introduce a new variable for the alter hook, but instead the main module also implements the alter hook itself now. This way people wanting to use the alter hook in their own module have more freedom now and the code is a bit cleaner and understandable.

The only difficult part, might be the hook execution order → now, which will affect any implementation as the main module itself uses the alter hook. But we still think it is better than the other implementation, which would introduce a breaking change through the new hook variable.

Please review!

Thanks @Grevil! Tests (old and new) are green and this fixes the described issue.

Setting this RTBC for that reason and think we should merge this into 2.x-dev now, so that people running into this have a solution.
We should wait for some weeks if there's further feedback until we create a new tagged release and eventually remove the warning from the module page.

The hook execution order thing should be a rare edge-case and not happen with regular users, would just something developers of extending modules have to think about.

We could also then discuss creating 3.x and remove the deprecations, once 2.0.0 is out, but that's a different topic.

Grevil → closed merge request !36

Merged! @chucksimply, @Wakuiy, @loze, feel free to test dev and give further feedback here! 😊

I also ran into this issue with 2.0-rc3 today and can confirm that the latest commit in dev works well for me

Thanks @JeremySkinner so if all agree, I'd say we should tag a new release soon?

Yes, seems like time to do another RC, probably.

Thanks for the feedback, I tagged 2.0.0-rc4 now!

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