Skip to content
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

[Validator] EmailValidator cannot extract hostname if email contains multiple @ symbols #18147

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
8 participants
@natechicago
Copy link
Contributor

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
@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Mar 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 # so GitHub turns it into a link. Thanks!

@sstok

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2016

Please update the tests to ensure will not break in the future 👍

Status: needs work

@natechicago

This comment has been minimized.

Copy link
Contributor Author

commented Mar 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]])

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 13, 2016

Member

As we need to support PHP 5.3 we cannot use the short array syntax.

This comment has been minimized.

Copy link
@natechicago

natechicago Mar 13, 2016

Author Contributor

Whoops, force of habit! I've updated this line to use array() instead.

@@ -37,7 +37,7 @@ public function validate($value, Constraint $constraint)
$valid = filter_var($value, FILTER_VALIDATE_EMAIL);
if ($valid) {
$host = substr($value, strpos($value, '@') + 1);
$host = $this->extractHostname($value);

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 13, 2016

Member

Since extractHostname() is used only once its body should be inlined here (there is no need for an additional method call).

This comment has been minimized.

Copy link
@natechicago

natechicago Mar 13, 2016

Author Contributor

This code was extracted into its own method only so we can unit test it in isolation.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 15, 2016

Member

We prefer to not test private methods. In this case, I agree with @xabbuh for inlining, and the test case should use the public interface only.

This comment has been minimized.

Copy link
@natechicago

natechicago Mar 15, 2016

Author Contributor

@xabbuh @nicolas-grekas thanks for the feedback! Can you see my comment in the EmailValidatorTest class, and perhaps give a pointer or two on exactly how this change can be accurately tested via the public API?

public function testHostnameIsProperlyParsed($hostnameAndEmail)
{
$method = new ReflectionMethod($this->validator, 'extractHostname');
$method->setAccessible(true);

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 13, 2016

Member

This test shouldn't use reflection. Just use the public API and create a test case that would fail without this change.

This comment has been minimized.

Copy link
@natechicago

natechicago Mar 13, 2016

Author Contributor

I can't see any realistic way of testing this code if it remains in the validate method, because the test would then unnecessarily depend on network connectivity and the checkdnsrr function.

This comment has been minimized.

Copy link
@patrick-mcdougle

patrick-mcdougle Mar 14, 2016

Contributor

Can't you just add your test case to getValidEmails()?

Edit: NVM, I see what you're saying.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 15, 2016

Member

@natechicago You can mock the checkdnsrr() function (see #16194 for how we did that for the time-related functions).

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 15, 2016

checkdnsrr mocking implemented in #18181
I propose to rebase on top of it once it's merged

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

@natechicago your turn

@natechicago natechicago force-pushed the natechicago:18146-email-validator branch 2 times, most recently from aed6486 to 8bcfd27 Mar 17, 2016

@natechicago

This comment has been minimized.

Copy link
Contributor Author

commented Mar 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?

http://symfony.com/blog/new-in-symfony-2-7-phpunit-bridge

@natechicago natechicago force-pushed the natechicago:18146-email-validator branch from 8bcfd27 to eabe394 Mar 17, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

The bridge is used to test 2.3 also so this PR is fine. See my PRs from yesterday

@xabbuh

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

@natechicago Can you rebase here please?

[Validator] - Bugfix for 18146 - EmailValidator cannot extract hostna…
…me if email contains multiple @ symbols

@natechicago natechicago force-pushed the natechicago:18146-email-validator branch from eabe394 to e64c3bc Mar 17, 2016

@natechicago

This comment has been minimized.

Copy link
Contributor Author

commented Mar 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)),

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 17, 2016

Member

you don't need this line: the list of moked hosts is exclusive and a look up for this last host would fail anyway

This comment has been minimized.

Copy link
@natechicago

natechicago Mar 17, 2016

Author Contributor

K, changes have been pushed.

@@ -37,7 +37,7 @@ public function validate($value, Constraint $constraint)
$valid = filter_var($value, FILTER_VALIDATE_EMAIL);
if ($valid) {
$host = substr($value, strpos($value, '@') + 1);
$host = substr(strrchr($value, '@'), 1);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 17, 2016

Member

substr($value, strrpos($value, '@') + 1); would be faster (less string copies)

[Validator] - Bugfix for 18146 - EmailValidator cannot extract hostna…
…me if email contains multiple @ symbols
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

👍

1 similar comment
@stof

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

👍

@stof

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

Status: reviewed

@xabbuh

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

👍

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

Thank you @natechicago.

nicolas-grekas added a commit that referenced this pull request Mar 17, 2016

bug #18147 [Validator] EmailValidator cannot extract hostname if emai…
…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

@fabpot fabpot referenced this pull request Apr 29, 2016

Merged

Release v2.3.40 #18669

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.