Skip to content

Conversation

berkerpeksag
Copy link
Contributor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will overwrite an explicitly given message if you use

validator = DomainNameValidator(accept_idna=True, message='Only IDNA domain allowed')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Good catch, thanks.

@dbrgn
Copy link
Contributor

dbrgn commented Nov 15, 2014

This probably clashes with #2873.

@timgraham
Copy link
Member

This is ready for a rebase and update now that the related PR has been merged.

@timgraham
Copy link
Member

Please send a new PR if someone can update this.

@timgraham timgraham closed this Apr 16, 2015
@JocelynDelalande
Copy link

@timgraham by update you mean rebase ?

@timgraham
Copy link
Member

Correct

@berkerpeksag
Copy link
Contributor Author

Unfortunately, rebasing this is not enough:

  • There are no tests for IPv4 and IPv6 and I'm not sure whether we should cover them since I haven't fully read the RFC
  • The regex in 2e65d56 needs to be modified to conform the RFC (if I remember correctly, we should reject any domain names longer than 255 chars)
  • We need more tests (e.g. TLDs longer than 3 chars)
  • I'd probably update the default value of accept_idna to False. I don't see much IDNs to make it default behavior and it would be better to avoid mostly unnecessary encoding step by default.

@dbrgn
Copy link
Contributor

dbrgn commented Apr 5, 2016

Any updates on this?

By the way, I disagree with this:

I'd probably update the default value of accept_idna to False.

An IDN is also a valid domain name.

@berkerpeksag
Copy link
Contributor Author

Yes, it is, but I'm not sure it's common enough to make it a default behavior. After the discussion on the django-developers list, I'd even prefer to remove it in order to have a simpler implementation.

I will address my comments in #3477 (comment) and send a pull request.

Thanks for the ping!

@jpic
Copy link
Contributor

jpic commented Jul 4, 2016

Is there any follow up on this ?

@berkerpeksag
Copy link
Contributor Author

Thanks for the ping. I thought I already have opened a PR for this :/ Will open a PR this week.

@berkerpeksag berkerpeksag deleted the 18119-domainnamevalidator branch September 27, 2016 01:59
@berkerpeksag
Copy link
Contributor Author

I've opened #7300 as a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants