[Validator] EmailValidator cannot extract hostname if email contains multiple @ symbols #18147
Conversation
natechicago
commented
Mar 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 |
|
@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 |
|
Please update the tests to ensure will not break in the future 👍 Status: needs work |
|
@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.
As we need to support PHP 5.3 we cannot use the short array syntax.
There was a problem hiding this comment.
Whoops, force of habit! I've updated this line to use array() instead.
|
checkdnsrr mocking implemented in #18181 |
|
@natechicago your turn |
aed6486 to
8bcfd27
Compare
|
@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 to
eabe394
Compare
|
The bridge is used to test 2.3 also so this PR is fine. See my PRs from yesterday |
|
@natechicago Can you rebase here please? |
…me if email contains multiple @ symbols
eabe394 to
e64c3bc
Compare
|
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.
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.
K, changes have been pushed.
…me if email contains multiple @ symbols
|
👍 |
1 similar comment
|
👍 |
|
Status: reviewed |
|
👍 |
|
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/a Commits ------- c551bd1 [Validator] EmailValidator cannot extract hostname if email contains multiple @ symbols