Switch to hook_user_presave() for setting username

+1 for this. We have some other code that runs in hook_user_insert and hook_user_update, which is being thrown off-kilter by the $account->save(); in this module's insert.

As an interim, it looks as though you can just comment out the save at the end of the insert hook.

Here's an initial patch that removes the $account->save() and restores the username if it hasn't worked.

This seems to still create users with the email_registration prefix.

There seems to be some inconsistent behaviour/expectations between what the username should be and what the username i and it's affecting the patch/testing.

Previously I've noticed that users get created with usernames like email_registration_* but then sometimes it is something different (the first part of their email address). The code in the user_insert suggests that it's going to be email_registration_*, but the test looks for the first part of their email address.

Some consistency is required here and I don't know who's best to make that decision. I can modify the test (next week) to look for the right email address, but I don't feel like patching it about like that is the best way to solve this.

Good idea to explore hook change but patch does something different

Simple patch attached. My first tests indicate this is working for new registrations as well as hook_email_registration_name.
Editing a user account will also trigger hook_email_registration_name, which is an added benefit of this approach.
I didn't test commerce checkout pane yet.
We need to notify upgraders about possible backwards compatibility issues with new user creation and the non existent account->id()...

Fixed comments, doc and decoration. Not sure test fails because of this...

The problem with presave hook is that there's no some tokens (id for example is not generated yet)

@bohemier looks you're affected by related issue

Correct... I had to revert back because of this for my use case. Thanks @andypost for the reference...

I believe that the `email_registration_unique_username` function should not be changed, it's the caller that should be making sure to provide the arguments expected by the function. That can be simply done by `intval`.

Patch #18 solves the issue for me, was having issues with other modules causing duplication.

The attached patch provides an alternative solution. Rather than changing the hook to a presave (which has other implications), this patch moves the logic into a separate function which is called by the existing email_registration_user_insert method. The new function takes an optional $save parameter which indicates whether the user should be saved. This defaults to TRUE for compatibility.

Consumers can then optionally remove the email_registration_user_insert through a module_implements_alter and call the new function from a presave (or from any other place) instead.

The advantage of this approach is it doesn't alter the existing behaviour for the majority of users, but allows flexibility to customize the behaviour if it's causing issues, which would solve @andrewbelcher's original issue.

Difficult call, I'm all for maintaining compatibility. However, this should be considered a bug that needed to be fixed anyway. The following quote from the hook documentation as posted in the issue description makes it clear that the implementation was wrong in the first place. It may break some sites and you shouldn't need a developer to get the module to work.

Note that hook implementations may not alter the stored entity data.

I think this needs further maintainer feedback to decide, how to proceed here? Or from someone very very experienced in Drupal development? Any active maintainers?

Or could someone raise this in slack perhaps for help?

Created MR!10 from #20 by @JeremySkinner - please note for credits.

So we can proceed and review here more easily.

@Grevil could you please have a look, now, as you're deep into this area?
So we can probably finish this! What do you think? We might create a 2.x branch for switching to semver and have this possibly dangerous change separated for now?

We might create a 2.x branch for switching to semver and have this possibly dangerous change separated for now?

The current MR's changes are the NON-dangerous compatibility changes. We might as well merge them as a current workaround and create a follow-up issue / leave this issue open.

Although I agree with @krystalcode here. The hook implementations wasn't properly implemented in the first place. And I do not see a case, where we do not have the $account variable in presave?

So I'd say we should rather orientate our code on patch #18 by @krystalcode rather than #20. And adjust our submodule logic accordingly.

#20 would be no danger, while I'd say we should be careful with #18 as it changes functionality and in contrast to the current implementation means, the username will change, when the user is being resaved! That might be unexpected for existing sites.

I'm fine with a MR here and I'm fine to plan this for a 2.x release, as it's kind of breaking change. But we shouldn't rush here?

PS: Implementing #18 will take things closer to the logic of the new submodule. I think the logic from there (when to change an existing username) is good and should be uplifted then @Grevil?
See a similar request in #3153809: [2.x] Update username when updating user entity!

Would you like to prepare a MR from all of that, once the other issues are fixed?
Targeting 2.x then (in the MR name?)

Yea, let's do that!

the username will change, when the user is being resaved!

Yea, we need to circumvent that, similar to how we did it in the submodule.

Gave credit in #3392840: [2.x] Rework username generation save logic.

Done through #3392840: [2.x] Rework username generation save logic