Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Remove IDNA conversion of local-part #66

Closed
wants to merge 6 commits into from

Conversation

BreyndotEchse
Copy link
Contributor

@@ -741,8 +741,8 @@ public function testUseMxCheckBasicValid()
];

if (extension_loaded('intl')) {
$emailAddresses[] = 'иван@письмо.рф';
Copy link
Member

Choose a reason for hiding this comment

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

Why modify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current validator only accepts RFC 5321 email addresses (atext = ALPHA / DIGIT / "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "/" / "=" / "?" / "^" / "_" / "`" / "{" / "|" / "}" / "~"). Because of the wrong use of IDNA for local parts, validate() returned false positives for local parts with unicode characters. When we implement RFC 6532 #3.2 the user should be able to choose if he wants to validate against RFC 5321 or RFC 6532 because SMTPUTF8 is not yet widely supported and just an extension

Copy link
Member

Choose a reason for hiding this comment

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

So this is a breaking change, as you said an option should be implemented for choose what rules should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. But this is part of another PR to make this validator RFC 6532 compatible (coming soon).

Copy link
Member

Choose a reason for hiding this comment

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

Please place those values which you consider invalid in the appropiate test (testUseMxRecordsBasicInvalid)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some tests for valid/invalid emails.

@BreyndotEchse
Copy link
Contributor Author

Depends on /pull/67. Merge that one first.

@weierophinney weierophinney added this to the 2.7.3 milestone May 12, 2016
@weierophinney weierophinney self-assigned this May 12, 2016

if (extension_loaded('intl')) {
$return['bob@موقع.إختبار'] = ['bob@موقع.إختبار'];
$return['[email protected]'] = ['[email protected]'];
Copy link
Member

Choose a reason for hiding this comment

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

I've merged #67 at this point, and just attempted to merge this PR. When running tests, both of these specific cases fail, each with:

not a valid hostname for the email address
The input appears to be a DNS hostname but cannot match TLD against known list
The input appears to be a local network name but local network names are not allowed
Failed asserting that false is true.

I'm not sure if that's expected at this point, and you have more work to do, or if there's another, more subtle issue.

@BreyndotEchse
Copy link
Contributor Author

@weierophinney I used a IANA test TLD in this test, but Hostname validator does not accept any of these. Could have tested it by myself first... sorry :/

Xerkus added a commit that referenced this pull request Mar 17, 2017
Xerkus added a commit that referenced this pull request Mar 17, 2017
@Xerkus Xerkus closed this in 4e4e836 Mar 17, 2017
@Xerkus Xerkus added this to the 2.9.0 milestone Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants