bug #10690 [Validator] Fix hack for nested Collection/All losing context (GromNaN)
This PR was submitted for the master branch but it was merged into the 2.4 branch instead (closes #10690).
Discussion
----------
[Validator] Fix hack for nested Collection/All losing context
The PR https://github.com/silexphp/Silex/pull/943 highlighted [a hack in `ConstraintValidatorFactory`](d4ebbfd02d (diff-3a3e44a703775a35fbdd66850a43968dR48)
).
The issue that is solved by this hack is that the context injected into the `ConstraintValidator` can change during the execution of the method `validate()` ; because the same instance of the validator is reused.
The hack was fixing this issue for the `Collection` constraint only. This patch fixes also the `All` constraint as it can be nested too (new test case found by @harrytruong).
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | https://github.com/silexphp/Silex/pull/943
| License | MIT
| Doc PR | no
Commits
-------
edfa6d5 [Validator] Fix hack for nested Collection/All losing context
This commit is contained in:
commit
a19eb9bcc6
@ -45,16 +45,7 @@ class ConstraintValidatorFactory implements ConstraintValidatorFactoryInterface
|
||||
{
|
||||
$className = $constraint->validatedBy();
|
||||
|
||||
// The second condition is a hack that is needed when CollectionValidator
|
||||
// calls itself recursively (Collection constraints can be nested).
|
||||
// Since the context of the validator is overwritten when initialize()
|
||||
// is called for the nested constraint, the outer validator is
|
||||
// acting on the wrong context when the nested validation terminates.
|
||||
//
|
||||
// A better solution - which should be approached in Symfony 3.0 - is to
|
||||
// remove the initialize() method and pass the context as last argument
|
||||
// to validate() instead.
|
||||
if (!isset($this->validators[$className]) || 'Symfony\Component\Validator\Constraints\CollectionValidator' === $className) {
|
||||
if (!isset($this->validators[$className])) {
|
||||
$this->validators[$className] = 'validator.expression' === $className
|
||||
? new ExpressionValidator($this->propertyAccessor)
|
||||
: new $className();
|
||||
|
@ -35,11 +35,12 @@ class AllValidator extends ConstraintValidator
|
||||
throw new UnexpectedTypeException($value, 'array or Traversable');
|
||||
}
|
||||
|
||||
$group = $this->context->getGroup();
|
||||
$context = $this->context;
|
||||
$group = $context->getGroup();
|
||||
|
||||
foreach ($value as $key => $element) {
|
||||
foreach ($constraint->constraints as $constr) {
|
||||
$this->context->validateValue($element, $constr, '['.$key.']', $group);
|
||||
$context->validateValue($element, $constr, '['.$key.']', $group);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
@ -35,7 +35,17 @@ class CollectionValidator extends ConstraintValidator
|
||||
throw new UnexpectedTypeException($value, 'array or Traversable and ArrayAccess');
|
||||
}
|
||||
|
||||
$group = $this->context->getGroup();
|
||||
// We need to keep the initialized context when CollectionValidator
|
||||
// calls itself recursively (Collection constraints can be nested).
|
||||
// Since the context of the validator is overwritten when initialize()
|
||||
// is called for the nested constraint, the outer validator is
|
||||
// acting on the wrong context when the nested validation terminates.
|
||||
//
|
||||
// A better solution - which should be approached in Symfony 3.0 - is to
|
||||
// remove the initialize() method and pass the context as last argument
|
||||
// to validate() instead.
|
||||
$context = $this->context;
|
||||
$group = $context->getGroup();
|
||||
|
||||
foreach ($constraint->fields as $field => $fieldConstraint) {
|
||||
if (
|
||||
@ -44,10 +54,10 @@ class CollectionValidator extends ConstraintValidator
|
||||
($value instanceof \ArrayAccess && $value->offsetExists($field))
|
||||
) {
|
||||
foreach ($fieldConstraint->constraints as $constr) {
|
||||
$this->context->validateValue($value[$field], $constr, '['.$field.']', $group);
|
||||
$context->validateValue($value[$field], $constr, '['.$field.']', $group);
|
||||
}
|
||||
} elseif (!$fieldConstraint instanceof Optional && !$constraint->allowMissingFields) {
|
||||
$this->context->addViolationAt('['.$field.']', $constraint->missingFieldsMessage, array(
|
||||
$context->addViolationAt('['.$field.']', $constraint->missingFieldsMessage, array(
|
||||
'{{ field }}' => $field
|
||||
), null);
|
||||
}
|
||||
@ -56,7 +66,7 @@ class CollectionValidator extends ConstraintValidator
|
||||
if (!$constraint->allowExtraFields) {
|
||||
foreach ($value as $field => $fieldValue) {
|
||||
if (!isset($constraint->fields[$field])) {
|
||||
$this->context->addViolationAt('['.$field.']', $constraint->extraFieldsMessage, array(
|
||||
$context->addViolationAt('['.$field.']', $constraint->extraFieldsMessage, array(
|
||||
'{{ field }}' => $field
|
||||
), $fieldValue);
|
||||
}
|
||||
|
@ -11,13 +11,14 @@
|
||||
|
||||
namespace Symfony\Component\Validator\Tests;
|
||||
|
||||
use Symfony\Component\Validator\Constraints\Collection;
|
||||
use Symfony\Component\Validator\Constraints\All;
|
||||
use Symfony\Component\Validator\ConstraintValidatorFactory;
|
||||
use Symfony\Component\Validator\ConstraintViolation;
|
||||
use Symfony\Component\Validator\ConstraintViolationList;
|
||||
use Symfony\Component\Validator\ExecutionContext;
|
||||
use Symfony\Component\Validator\Constraints\Collection;
|
||||
use Symfony\Component\Validator\Tests\Fixtures\ConstraintA;
|
||||
use Symfony\Component\Validator\ValidationVisitor;
|
||||
use Symfony\Component\Validator\ConstraintValidatorFactory;
|
||||
|
||||
class ExecutionContextTest extends \PHPUnit_Framework_TestCase
|
||||
{
|
||||
@ -277,22 +278,50 @@ class ExecutionContextTest extends \PHPUnit_Framework_TestCase
|
||||
$this->assertEquals('bam.baz', $this->context->getPropertyPath('bam.baz'));
|
||||
}
|
||||
|
||||
public function testGetPropertyPathWithNestedCollectionsMixed()
|
||||
public function testGetPropertyPathWithNestedCollectionsAndAllMixed()
|
||||
{
|
||||
$constraints = new Collection(array(
|
||||
'foo' => new Collection(array(
|
||||
'foo' => new ConstraintA(),
|
||||
'bar' => new ConstraintA(),
|
||||
)),
|
||||
'shelves' => new All(array('constraints' => array(
|
||||
new Collection(array(
|
||||
'name' => new ConstraintA(),
|
||||
'books' => new All(array('constraints' => array(
|
||||
new ConstraintA()
|
||||
)))
|
||||
))
|
||||
))),
|
||||
'name' => new ConstraintA()
|
||||
));
|
||||
$data = array(
|
||||
'shelves' => array(
|
||||
array(
|
||||
'name' => 'Research',
|
||||
'books' => array('foo', 'bar'),
|
||||
),
|
||||
array(
|
||||
'name' => 'VALID',
|
||||
'books' => array('foozy', 'VALID', 'bazzy'),
|
||||
),
|
||||
),
|
||||
'name' => 'Library',
|
||||
);
|
||||
$expectedViolationPaths = array(
|
||||
'[shelves][0][name]',
|
||||
'[shelves][0][books][0]',
|
||||
'[shelves][0][books][1]',
|
||||
'[shelves][1][books][0]',
|
||||
'[shelves][1][books][2]',
|
||||
'[name]'
|
||||
);
|
||||
|
||||
$visitor = new ValidationVisitor('Root', $this->metadataFactory, new ConstraintValidatorFactory(), $this->translator);
|
||||
$context = new ExecutionContext($visitor, $this->translator, self::TRANS_DOMAIN);
|
||||
$context->validateValue(array('foo' => array('foo' => 'VALID')), $constraints);
|
||||
$violations = $context->getViolations();
|
||||
$context->validateValue($data, $constraints);
|
||||
|
||||
$this->assertEquals('[name]', $violations[1]->getPropertyPath());
|
||||
foreach ($context->getViolations() as $violation) {
|
||||
$violationPaths[] = $violation->getPropertyPath();
|
||||
}
|
||||
|
||||
$this->assertEquals($expectedViolationPaths, $violationPaths);
|
||||
}
|
||||
}
|
||||
|
||||
|
Reference in New Issue
Block a user