Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #26423 -- Match email validation with html. #8081

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@harisibrahimkv
Copy link
Contributor

commented Feb 18, 2017

References #7742.

Adds documentation describing what the Validation logic is based on.

Ticket: https://code.djangoproject.com/ticket/26423

@timgraham
Copy link
Member

left a comment

Release notes in docs/releases/2.0.txt that explain the ramifications of this backwards incompatible change are also needed.


def __init__(self, message=None, code=None, whitelist=None):
if message is not None:
self.message = message
if code is not None:
self.code = code
if whitelist is not None:
self.domain_whitelist = whitelist
pass # deprecated

This comment has been minimized.

Copy link
@timgraham

timgraham Feb 18, 2017

Member

Not sure about the decision here -- documentation/explanation is needed.

This comment has been minimized.

Copy link
@harisibrahimkv

harisibrahimkv Feb 19, 2017

Author Contributor

According to https://html.spec.whatwg.org/multipage/forms.html#valid-e-mail-address, the text after @ can be a letter, digit or hyphen optionally followed by a . and a trail of letters, digits or hyphens.

The domain whitelist was there to allow domains that don't have a . in them to be treated as a special case (and be allowed). However, since the validation now considers a . as optional already, we don't have to specify any whitelists manually.

This comment has been minimized.

Copy link
@pope1ni

pope1ni Mar 8, 2017

Contributor

If this is the case, there needs to be a proper deprecation warning issued here.

This comment has been minimized.

Copy link
@harisibrahimkv

harisibrahimkv Mar 8, 2017

Author Contributor

@pope1ni Should I add the deprecation warning within validators.py itself? If yes, is there a specific format that I should do it in? I've added it to validators.txt already.

This comment has been minimized.

Copy link
@pope1ni

pope1ni Mar 8, 2017

Contributor

Follow the guide in the documentation on deprecating a feature.

This comment has been minimized.

Copy link
@collinanderson

collinanderson Apr 22, 2017

Contributor

My thinking was:

  • Originally django didn't allow any dotless domain names.
  • People wanted to use "localhost", so domain_whitelist was added to allow more user-specified domains. https://code.djangoproject.com/ticket/4833
  • We're changing the behavior of email validation to make it allow a lot more email addresses including all dotless domains, so you don't need to specify specific domains to allow.
  • So before this change, we allow all domains in domain_whitelist, and after this change we allow all domains in domain_whitelist (plus more)

This comment has been minimized.

Copy link
@collinanderson

collinanderson Apr 22, 2017

Contributor

(though I don't know, maybe it still makes sense to deny dotless domain names. I think we denied them because the spec said we should (see ticket 4883). html's validation allows them.)

This comment has been minimized.

Copy link
@timgraham

timgraham May 30, 2017

Member

My question was about the fact that you marked domain_whitelist as deprecated in a comment but didn't implement a deprecation. Did you intend to do some more implementation? Would it be better to remove domain_whitelist so that developers are alerted to the fact that this parameter no longer has any effect? That seems more friendly than raising a deprecation (which might be silent) about it. I doubt that parameter is used in third-party apps where multiple Django version support might be important.

This comment has been minimized.

Copy link
@collinanderson

collinanderson May 31, 2017

Contributor

Hmm... I personally really don't like backward incompatibilities, but removing it seems outright fine to me. Though I'm thinking it might be helpful to propose it on django-developers first. Should I do that?

This comment has been minimized.

Copy link
@timgraham
The :class:`EmailValidator` works in accordance with the HTML5 Email
validation standard as described `here`_.

.. _here: https://html.spec.whatwg.org/multipage/forms.html#valid-e-mail-address

This comment has been minimized.

Copy link
@timgraham

timgraham Feb 18, 2017

Member

link "HTML5 Email validation standard" and chop "as described here".
Add a versionchanged annotation as described at https://docs.djangoproject.com/en/dev/internals/contributing/writing-documentation/#documenting-new-features.


In older version, Email validation was done with Django's own
logic. It uses the HTML5 Email validation standard from 2.0
onwards.

This comment has been minimized.

Copy link
@ChillarAnand

ChillarAnand Feb 19, 2017

Contributor

Reflow this line?

This comment has been minimized.

Copy link
@harisibrahimkv

harisibrahimkv Feb 19, 2017

Author Contributor

What would sound better?

Since 2.0, Django uses the standard HTML5 Email validation logic.

This comment has been minimized.

Copy link
@ChillarAnand

ChillarAnand Feb 19, 2017

Contributor

In the above paragraph, onwards. is just hanging in the last line. It is better if that can be reflowed to previous line.

This comment has been minimized.

Copy link
@harisibrahimkv

harisibrahimkv Feb 20, 2017

Author Contributor

Okay. Does Since 2.0, Django uses the standard HTML5 Email validation logic. sound better than

In older version, Email validation was done with Django's own logic. It uses the HTML5 Email validation standard from 2.0 onwards.

?

This comment has been minimized.

Copy link
@ChillarAnand

ChillarAnand Feb 21, 2017

Contributor

First version is fine. Just rearrange it to

       In older version, Email validation was done with Django's own
       logic. It uses the HTML5 Email validation standard from 2.0 onwards.

Also squash everything to a single commit. There shouldn't be any merge commits. Thanks.

This comment has been minimized.

Copy link
@pope1ni

pope1ni Mar 8, 2017

Contributor

Also change "Email" to "email" in both sentences - don't capitalise.

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch 11 times, most recently from 8a5069b to 0fb3453 Feb 22, 2017

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch 7 times, most recently from 6c55d60 to 8b9f1a5 Mar 1, 2017

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch 4 times, most recently from 493d938 to c484025 Mar 8, 2017

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch from c484025 to b133353 Mar 15, 2017

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch 2 times, most recently from 041df1e to 75f16e7 Mar 18, 2017


def __init__(self, message=None, code=None, whitelist=None):
if message is not None:
self.message = message
if code is not None:
self.code = code
if whitelist is not None:
self.domain_whitelist = whitelist
pass # deprecated

This comment has been minimized.

Copy link
@timgraham

timgraham Mar 24, 2017

Member

What's implemented now doesn't look like a deprecation since the domain_whitelist attribute is gone and if a custom whitelist is passed, it's ignored. Anyway, a warning should be raised about the deprecation indicating that whitelist is ignored and will be removed in a future version if that's the intention.

logic. Since version 2.0, it uses the HTML5 email validation
standard.

The ``whitelist`` attribute is deprecated since 2.0. It was

This comment has been minimized.

Copy link
@timgraham

timgraham Mar 24, 2017

Member

Should be documented under .. attribute:: whitelist -- although it's removed so maybe it should be deleted instead.

@@ -206,7 +206,7 @@ URLs
Validators
~~~~~~~~~~

* ...
* The :class:`django.core.validators.EmailValidator` now uses the HTML5 email validation standard.

This comment has been minimized.

Copy link
@timgraham

timgraham Mar 24, 2017

Member

Wrap documentation at 79 characters -- not sure I'd consider this a "minor feature" though.

@@ -145,6 +145,23 @@ to, or in lieu of custom ``field.clean()`` methods.
``['localhost']``. Other domains that don't contain a dot won't pass
validation, so you'd need to whitelist them as necessary.

The :class:`EmailValidator` works in accordance with the `HTML5 email

This comment has been minimized.

Copy link
@timgraham

timgraham Mar 24, 2017

Member

It would be appropriate to explain this design decision.

This comment has been minimized.

Copy link
@harisibrahimkv

harisibrahimkv Apr 6, 2017

Author Contributor

I've just added the explanation to the versionchanged annotation.

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch from 75f16e7 to 5617431 Mar 26, 2017

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch 8 times, most recently from 74e7f84 to e5da3d2 Apr 2, 2017

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch 4 times, most recently from 6249bb7 to 46a04aa Apr 11, 2017

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch from 46a04aa to f26b1f6 Apr 19, 2017

@harisibrahimkv harisibrahimkv force-pushed the harisibrahimkv:ticket_26423 branch from f26b1f6 to 50e41fd Apr 22, 2017

@timgraham

This comment has been minimized.

Copy link
Member

commented Aug 18, 2018

Closing due to inactivity.

@timgraham timgraham closed this Aug 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.