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][Email] - Strict validation and soft dependency #9140

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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 28 additions & 0 deletions UPGRADE-2.5.md
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
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
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
Expand Up @@ -655,6 +655,7 @@ private function registerValidationConfiguration(array $config, ContainerBuilder
$container->setParameter('validator.translation_domain', $config['translation_domain']);
$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));
$container->setParameter('validator.strict_email', $config['strict_email']);
Copy link
Member

Choose a reason for hiding this comment

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

We try to avoid creating new parameters as much as possible. Instead, can you inject the value into the service? Something like this (not tested):

$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');
Expand Down
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>%validator.strict_email%</argument>
Copy link
Member

Choose a reason for hiding this comment

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

This should be replaced with an empty value (see above.)

<tag name="validator.constraint_validator" alias="validator.email" />
</service>
</services>
</container>
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
1 change: 1 addition & 0 deletions src/Symfony/Component/Validator/CHANGELOG.md
Expand Up @@ -6,6 +6,7 @@ CHANGELOG

* deprecated `ApcCache` in favor of `DoctrineCache`
* added `DoctrineCache` to adapt any Doctrine cache
* [BC BREAK] `EmailValidator` now has a lighter validation by default, uses `Egulias\EmailValidator` for strict validation
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this is a BC break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was because some emails will pass with the non-strict validation. I'll remove this then.


2.4.0
-----
Expand Down
9 changes: 9 additions & 0 deletions src/Symfony/Component/Validator/Constraints/Email.php
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
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)
Copy link
Member

Choose a reason for hiding this comment

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

missing phpdoc. and it should set a default as well to not break BC.
and I wonder why this option is needed? why have a strict option on the constraint and on the validator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the following:

  • You can pass the strictnes to the EmailConstraint when writing the validation rules, e.g @Email(strict=true) on a per field basis
  • You can set in the config.yml system wide strict = true. This setting can only be injected into the EmailValidator (as far as I know)

{
$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 ($constraint->strict === null) {
Copy link
Member

Choose a reason for hiding this comment

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

null === $constraint->strict

$constraint->strict = $this->isStrict;
}

if (true === $constraint->strict && class_exists('\Egulias\EmailValidator\EmailValidator')) {
Copy link
Member

Choose a reason for hiding this comment

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

if ($constraint->strict ... is enough

$strictValidator = new StrictEmailValidator();
$valid = $strictValidator->isValid($value, $constraint->checkMX);
Copy link
Member

Choose a reason for hiding this comment

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

You should probably disable MX checks here as it's going to be redone below anyway.

} 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.

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
Member

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