Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Validator] EmailValidator cannot extract hostname if email contains multiple @ symbols#18147
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
natechicago commentedMar 13, 2016
| Q | A |
|---|---|
| Branch | 2.3 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #18146 |
| License | MIT |
| Doc PR | n/a |
javiereguiluz commentedMar 13, 2016
@natechicago thanks for contributing this fix. As a very minor comment, next time is enough to include the fixed ticket number in the table and prefix it with |
sstok commentedMar 13, 2016
Please update the tests to ensure will not break in the future 👍 Status: needs work |
natechicago commentedMar 13, 2016
@javiereguiluz thanks for the tip!@sstok I've added an appropriate unit test to make sure this problem won't recur in the future. |
| $this->assertEquals( | ||
| $hostnameAndEmail[0], | ||
| $method->invokeArgs($this->validator, [$hostnameAndEmail[1]]) |
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.
As we need to support PHP 5.3 we cannot use the short array syntax.
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.
Whoops, force of habit! I've updated this line to usearray() instead.
nicolas-grekas commentedMar 15, 2016
checkdnsrr mocking implemented in#18181 |
nicolas-grekas commentedMar 16, 2016
@natechicago your turn |
aed6486 to8bcfd27Comparenatechicago commentedMar 17, 2016
@nicolas-grekas Since the new unit test depends on PHPUnit Bridge, should this PR (targed against 2.3) be closed and a new one opened against 2.7? |
8bcfd27 toeabe394Comparenicolas-grekas commentedMar 17, 2016
The bridge is used to test 2.3 also so this PR is fine. See my PRs from yesterday |
xabbuh commentedMar 17, 2016
@natechicago Can you rebase here please? |
…me if email contains multiple @ symbols
eabe394 toe64c3bcComparenatechicago commentedMar 17, 2016
Thanks guys, this should now be ready to merge. |
| { | ||
| DnsMock::withMockedHosts(array( | ||
| 'baz.com' =>array(array('type' =>'MX')), | ||
| '@bar"@baz.com' =>array(array('type' =>false)), |
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.
you don't need this line: the list of moked hosts is exclusive and a look up for this last host would fail anyway
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.
K, changes have been pushed.
…me if email contains multiple @ symbols
nicolas-grekas commentedMar 17, 2016
👍 |
1 similar comment
stof commentedMar 17, 2016
👍 |
stof commentedMar 17, 2016
Status: reviewed |
xabbuh commentedMar 17, 2016
👍 |
nicolas-grekas commentedMar 17, 2016
Thank you@natechicago. |
…l contains multiple @ symbols (natechicago)This PR was squashed before being merged into the 2.3 branch (closes#18147).Discussion----------[Validator] EmailValidator cannot extract hostname if email contains multiple @ symbols| Q | A| ------------- | ---| Branch | 2.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18146| License | MIT| Doc PR | n/aCommits-------c551bd1 [Validator] EmailValidator cannot extract hostname if email contains multiple @ symbols