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 #18119 -- Added a DomainNameValidator validator. #18037
base: main
Are you sure you want to change the base?
Fixed #18119 -- Added a DomainNameValidator validator. #18037
Conversation
Thank you for this Nina ⭐
All new features need some docs and a release note, so this is roughly what I am expecting:
|
fc3bd96
to
f77f5f3
Compare
Thanks Sarah - docs now updated. |
Thanks @felixxm - docs now updated following comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @nmenezes0 ⭐ I have a few more comments.
Please also squash this to a single commit 👍
151845f
to
dd9816e
Compare
Thanks @sarahboyce - I've made those changes and squashed to a single commit. |
I'm not sure why some of the checks are failing. The error is "commit changed" - potentially due to me pushing the squashed commit? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates @nmenezes0, we're really close ⭐
I'm not sure why some of the checks are failing. The error is "commit changed" - potentially due to me pushing the squashed commit?
Don't worry about this, sounds unrelated to your changes
The ticket in Trac suggests that the regex could be reused elsewhere in the codebase. I have reused it in the URLValidator, but I couldn't see how to use it in django/core/mail/message.py or django/utils/html.py.
Read through the original comment made by @claudep 12 years ago.
- It's still valid that we could chose to use this validation in other areas of the codebase.
- I don't think refactoring to include this is in scope of this ticket, however we should make sure it's defined in a good place.
- I'm not sure which
__init__
is being referred to in the comment and I don't get circular imports if I was to importDomainNameValidator
to the mentioned files.
@claudep can you clarify where you think DomainNameValidator
should be defined or whether it is correct to be in django/core/validators.py
?
I think it is ok to be defined here but I might be missing something 👍
Thanks @sarahboyce - have commited those docs changes - I assume I should squash into one commit again (will do that later). |
Co-authored-by: Mariusz Felisiak <[email protected]> Co-authored-by: Sarah Boyce <[email protected]> Co-authored-by: Nina Menezes <[email protected]>
4a05b3f
to
b7d5494
Compare
I think the suggested location is fine. |
name. Values longer than 255 characters are always considered invalid. | ||
|
||
In addition to the optional arguments of its parent :class:`RegexValidator` | ||
class, ``DomainNameValidator`` accepts an extra optional attribute: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
schemes
is not an accepted parameter. regex
shouldn't accepted either, as it is rewritten in all cases in __init__
.
BTW, you still have an extra new line just below.
|
||
This determines whether or not to accept internationalized domain name, | ||
that is, domain names that contain non-ASCII characters. Default value | ||
is true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true => True (enclosed in double backticks)
(validate_domain_name, "python..org", ValidationError), | ||
(validate_domain_name, "python-.org", ValidationError), | ||
(validate_domain_name, "too-long-name." * 20 + "com", ValidationError), | ||
(validate_domain_name, "stupid-name试", ValidationError), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also add lines with IP (v4/v6) addresses to demonstrate by tests that they are excluded by validation. And that should also be clarified in docs.
Trac ticket number
ticket-18119
Branch description
Provide a concise overview of the issue or rationale behind the proposed changes.
This ticket adds a DomainNameValidator to validate International domain names. This validator takes a flag
is_idna
(defaults toTrue
) - to determine whether or not we should use non-ASCII international domain names.This builds on the existing PR with some changes: not using IP addresses (don't think they are part of a domain name), removing changes to docs as out of date (not sure what updates are needed here now?), changes to the tests and how they were written, reusing the regex in URLValidator.
The ticket in Trac suggests that the regex could be reused elsewhere in the codebase. I have reused it in the URLValidator, but I couldn't see how to use it in
django/core/mail/message.py
ordjango/utils/html.py
.Checklist
main
branch.I wasn't sure what changes I should make to the documentation.