[Django] #26790: Consistency in normalize_email and normalize_username
Django
--------------------------------------+-----------------------------
Reporter: tam0407 | Owner: tam0407
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: 1.10
Severity: Normal | Keywords: normalize_email
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+-----------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26790>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: tam0407 | Owner: tam0407
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: normalize_email | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tam0407):
* status: new => assigned
* needs_docs: => 0
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26790#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: tam0407 | Owner: tam0407
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: normalize_email | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: 1.10 => master
* stage: Unreviewed => Accepted
Comment:
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26790#comment:2>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: normalize_email | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by squallcs12):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26790#comment:3>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: normalize_email | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* cc: berker.peksag@… (added)
Comment:
> 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26790#comment:4>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: normalize_email | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Yes, but `AbstractBaseUser.normalize_username()` is called on the result
of `AbstractBaseUser.get_username()`. Are you proposing a different
organization?
--
Ticket URL: <https://code.djangoproject.com/ticket/26790#comment:5>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: normalize_email | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* owner: Huynh Thanh Tam => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/26790#comment:6>
Django
Reporter: Huynh Thanh Tam | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: normalize_email | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: (none) => Jacob Walls
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26790#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Huynh Thanh Tam | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: normalize_email | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26790#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Huynh Thanh Tam | Owner: Jacob
Type: | Walls
Component: contrib.auth | Version: dev
Severity: Normal | Resolution: needsinfo
Keywords: normalize_email | Triage Stage:
| Unreviewed
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: assigned => closed
* resolution: => needsinfo
* stage: Accepted => Unreviewed
Comment:
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
[https://github.com/django/django/pull/13527/files#diff-
b37e77411714dcbb9a65b45e4aa5e64167560461808805328f40deb86f92407cR1042
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26790#comment:11>