Move BaseUserManager.normalize_email() to AbstractUser

[Huynh Thanh Tam]

In the current code, normalize_username is placed in AbstractBaseUser, but normalize_email currently stay in BaseUserManager for backward-combatibility.
I suggest we move normalize_email logic to AbstractBaseUser for consistency with normalize_username, and raise warning for the original one.

[Tim Graham]

If the user has overridden BaseUserManager.normalize_email(), we need to use that implementation during the deprecation period. The idea is similar to the deprecation of ModelAdmin.declared_fieldsets in ebb3e50243448545c7314a1932a9067ddca5960b.

It seems to me the normalize_email() should be moved to AbstractUser (which has the email field) rather than AbstractBaseUser.

[Bang Dao]

currently BaseUserManager.normalize_email() is a classmethod, from cls we can't get model attribute like manager_instance.model. Thus when we move normalize_email to AbstractUser, I don't know how to make a extendable call to AbstractUser. normalize_email() from classmethod BaseUserManager.normalize_email() for backward compatibility.

[Berker Peksag]

It seems to me the normalize_email() should be moved to AbstractUser (which has the email field) rather than AbstractBaseUser.

Note that the username field is also defined in AbstractUser but normalize_username() is implemented in AbstractBaseUser.

[Tim Graham]

Yes, but AbstractBaseUser.normalize_username() is called on the result of AbstractBaseUser.get_username(). Are you proposing a different organization?

[Jacob Walls]

​PR

[Jacob Walls]

There are four reasons to move the method to AbstractBaseUser:

• Adam replied on the mailing list that EMAIL_FIELD is a property on AbstractBaseUser in any event.
• The reporter was in favor of it for consistency with normalize_username().
• To pass the suite, as I ​explained on the mailing list, I needed to support subclasses of AbstractBaseUser that relied on UserManager rather than BaseUserManager to manage objects during the deprecation period. I would have needed to check if normalize_email() was on the manager, and if not, use the implementation on AbstractUser. This seems too involved.
• This gets us one step closer to deprecating clean() on AbstractUser in favor of AbstractBaseUser.clean(), although I haven't thought through the backward-compatibility ramifications of that ATM.

[Jacob Walls]

A fifth reason, filed under my-patch-would-be-cleaner, is that I can have the old implementation merely call the new implementation. If the new implementation is on AbstractUser, I would encounter a circular import.

[Carlton Gibson]

OK, so Mariusz and I have each come back the PR and the discussion here a number of times over the last few months.

Having conferred we're of the opinion that the proposed change is not worth the disruption: there's clearly confusion is the code as to the levels the attributes and the methods that call them live at but, ultimately the situation isn't much improved after the patch is applied.

There isn't consensus as to which level we should move the methods to (if at all): AbstractBaseUser, AbstractUser, or leave them on BaseUserManager.

(On this last — it's in create() that you want normalize_email() so we go from self.<...> to AbstractBaseUser.<...> in the ​MyUserManager example in the diff — and I, for one, start wondering if we've really made an improvement... — this is just illustrative — let's not get distracted by it please.)

The various comments comment:2 comment:4 comment:5 comment:9, plus then Adam's reply on the mailing list point to different possible ways we could cut this cake. If I had to choose I'd probably go with Tim's suggestion to move the logic to AbstractUser — but I just don't think it's worth the disruption — and I'm equally drawn to "These methods belong on Manager"…

There's a cluster of code which is more coupled than we'd like. OK. I'm all for clean code, but I'm not sure we have a plan to make it better. (Again, having conferred with Mariusz and having looked at it for some time.) At that point we think the API stability policy dominates. We don't think we should change it without a better analysis. (The current situation is not the end of the world...)

So... I'm going to close this as needsinfo. If someone wants to puzzle this out such that we have an API that's clearly cleaner, plus a nice deprecation path for folks that have implemented on the current situation, and we can get a consensus together on the DevelopersMailingList then, we're very happy to reopen and push forwards. Otherwise, for the moment at least, I think there are probably higher ROI areas we can work on.

I hope that makes sense to all.