Skip to content

[Validator][Email] - Strict validation and soft dependency #9140

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

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions UPGRADE-2.5.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,31 @@ Routing
-------

* Added a new optional parameter `$requiredSchemes` to `Symfony\Component\Routing\Generator\UrlGenerator::doGenerate()`

Copy link
Contributor

Choose a reason for hiding this comment

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

i also don't see an entry on the changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See L9

Copy link
Contributor

Choose a reason for hiding this comment

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

changelog not upgrade i mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no changelog file in master yet, how should I proceed?

Copy link
Member

Choose a reason for hiding this comment

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

Validator
---------

* EmailValidator has changed to allow `non-strict` and `strict` email validation

Before:

Email validation was done with php's `filter_var()`

After:

Default email validation is now done via a simple regex which may cause invalid emails (not RFC compilant) to be
valid. This is the default behaviour.

Strict email validation has to be explicitly activated in the configuration file by adding
```
framework_bundle:
//...
validation:
strict_email: true
//...

```
Also you have to add to your composer.json:
```
"egulias/email-validator": "1.1.*"
```
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@
"monolog/monolog": "~1.3",
"propel/propel1": "1.6.*",
"ircmaxell/password-compat": "1.0.*",
"ocramius/proxy-manager": ">=0.3.1,<0.6-dev"
"ocramius/proxy-manager": ">=0.3.1,<0.6-dev",
"egulias/email-validator": "1.1.0"
},
"autoload": {
"psr-0": { "Symfony\\": "src/" },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ private function addValidationSection(ArrayNodeDefinition $rootNode)
->scalarNode('cache')->end()
->booleanNode('enable_annotations')->defaultFalse()->end()
->scalarNode('translation_domain')->defaultValue('validators')->end()
->booleanNode('strict_email')->defaultFalse()->end()
->end()
->end()
->end()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,9 @@ private function registerValidationConfiguration(array $config, ContainerBuilder
$container->setParameter('validator.mapping.loader.xml_files_loader.mapping_files', $this->getValidatorXmlMappingFiles($container));
$container->setParameter('validator.mapping.loader.yaml_files_loader.mapping_files', $this->getValidatorYamlMappingFiles($container));

$definition = $container->findDefinition('validator.email');
$definition->replaceArgument(0, $config['strict_email']);

if (array_key_exists('enable_annotations', $config) && $config['enable_annotations']) {
$loaderChain = $container->getDefinition('validator.mapping.loader.loader_chain');
$arguments = $loaderChain->getArguments();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
<parameter key="validator.mapping.loader.xml_files_loader.mapping_files" type="collection" />
<parameter key="validator.mapping.loader.yaml_files_loader.mapping_files" type="collection" />
<parameter key="validator.expression.class">Symfony\Component\Validator\Constraints\ExpressionValidator</parameter>
<parameter key="validator.email.class">Symfony\Component\Validator\Constraints\EmailValidator</parameter>
</parameters>

<services>
Expand Down Expand Up @@ -69,5 +70,10 @@
<argument type="service" id="property_accessor" />
<tag name="validator.constraint_validator" alias="validator.expression" />
</service>

<service id="validator.email" class="%validator.email.class%">
<argument></argument>
<tag name="validator.constraint_validator" alias="validator.email" />
</service>
</services>
</container>
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ protected static function getBundleDefaultConfig()
'enabled' => false,
'enable_annotations' => false,
'translation_domain' => 'validators',
'strict_email' => false,
),
'annotations' => array(
'cache' => 'file',
Expand Down
9 changes: 9 additions & 0 deletions src/Symfony/Component/Validator/Constraints/Email.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,13 @@ class Email extends Constraint
public $message = 'This value is not a valid email address.';
public $checkMX = false;
public $checkHost = false;
public $strict = null;

/**
* {@inheritDoc}
*/
public function validatedBy()
{
return 'validator.email';
}
}
28 changes: 26 additions & 2 deletions src/Symfony/Component/Validator/Constraints/EmailValidator.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\ConstraintValidator;
use Symfony\Component\Validator\Exception\UnexpectedTypeException;
use Egulias\EmailValidator\EmailValidator as StrictEmailValidator;

/**
* @author Bernhard Schussek <bschussek@gmail.com>
Expand All @@ -22,6 +23,18 @@
*/
class EmailValidator extends ConstraintValidator
{
/**
* isStrict
*
* @var Boolean
*/
private $isStrict;

public function __construct($strict = false)
{
$this->isStrict = $strict;
}

/**
* {@inheritDoc}
*/
Expand All @@ -36,12 +49,23 @@ public function validate($value, Constraint $constraint)
}

$value = (string) $value;
$valid = filter_var($value, FILTER_VALIDATE_EMAIL);
if (null === $constraint->strict) {
$constraint->strict = $this->isStrict;
}

if ($constraint->strict && class_exists('\Egulias\EmailValidator\EmailValidator')) {
$strictValidator = new StrictEmailValidator();
$valid = $strictValidator->isValid($value, false);
} elseif ($constraint->strict === true) {
throw new \RuntimeException('Strict email validation requires egulias/email-validator');
} else {
$valid = preg_match('/.+\@.+\..+/', $value);
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails with an IPv6 address example@[IPv6:::1]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are talking about the else part, which is intended to simply allow an "@" and a "."
Any other is responsibility of the StrictEmailValidator. Please see discussion in #1581

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is filter_var() replaced by preg_match()? Doesn't it change the existing behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does, that's why I was saying this is still a BC break. But I did it this way because of the conversation on #1581 where, if I didn't got it wrong, filter_var()was to be replaced by the strict validator and the non-strict validation will be done by a simple regex (see #1581 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean BC break. BC means the opposite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes BC break, sorry.
El 04/01/2014 19:59, "Tobias Schultze" notifications@github.com escribió:

In src/Symfony/Component/Validator/Constraints/EmailValidator.php:

@@ -36,12 +50,23 @@ public function validate($value, Constraint $constraint)
}

     $value = (string) $value;
  •    $valid = filter_var($value, FILTER_VALIDATE_EMAIL);
    
  •    if ($constraint->strict === null) {
    
  •        $constraint->strict = $this->isStrict;
    
  •    }
    
  •    if ($constraint->strict === true && class_exists('\Egulias\EmailValidator\EmailValidator')) {
    
  •        $strictValidator = new StrictEmailValidator();
    
  •        $valid = $strictValidator->isValid($value, $constraint->checkMX);
    
  •    } elseif ($constraint->strict === true) {
    
  •        throw new \RuntimeException('Strict email validation requires egulias/email-validator');
    
  •    } else {
    
  •        $valid = preg_match('/.+\@.+..+/', $value);
    

you mean BC break. BC means the opposite


Reply to this email directly or view it on GitHubhttps://github.com//pull/9140/files#r8652639
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakzal it's ok? Before I fix the other changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with me if it was agreed before :)

}

if ($valid) {
$host = substr($value, strpos($value, '@') + 1);

// Check for host DNS resource records

if ($valid && $constraint->checkMX) {
$valid = $this->checkMX($host);
} elseif ($valid && $constraint->checkHost) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class EmailValidatorTest extends \PHPUnit_Framework_TestCase
protected function setUp()
{
$this->context = $this->getMock('Symfony\Component\Validator\ExecutionContext', array(), array(), '', false);
$this->validator = new EmailValidator();
$this->validator = new EmailValidator(false);
$this->validator->initialize($this->context);
}

Expand Down Expand Up @@ -100,7 +100,14 @@ public function getInvalidEmails()
array('example'),
array('example@'),
array('example@localhost'),
array('example@example.com@example.com'),
);
}

public function testStrict()
{
$this->context->expects($this->never())
->method('addViolation');

$this->validator->validate('example@localhost', new Email(array('strict' => true)));
}
}
6 changes: 4 additions & 2 deletions src/Symfony/Component/Validator/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,17 @@
"symfony/yaml": "~2.0",
"symfony/config": "~2.2",
"doctrine/annotations": "~1.0",
"doctrine/cache": "~1.0"
"doctrine/cache": "~1.0",
"egulias/email-validator": "~1.0"
},
"suggest": {
"doctrine/annotations": "For using the annotation mapping. You will also need doctrine/cache.",
"doctrine/cache": "For using the default cached annotation reader and metadata cache.",
"symfony/http-foundation": "",
"symfony/intl": "",
"symfony/yaml": "",
"symfony/config": ""
"symfony/config": "",
"egulias/email-validator": "Strict (RFC compliant) email validation"
},
"autoload": {
"psr-0": { "Symfony\\Component\\Validator\\": "" }
Expand Down