bug #36627 [Validator] fix lazy property usage. (bendavies)

This PR was squashed before being merged into the 3.4 branch (closes #36627). Discussion ---------- [Validator] fix lazy property usage. | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #36343 | License | MIT | Doc PR | This attempts to fix a large regression introduced in #36343, which broke recursing values returned from `getter` Constraints, because they are now wrapped in in a `LazyProperty`. The `LazyProperty` needs to be evaluated because some checks are done on the type of `$value`, i.e `is_array` etc... in `validateGenericNode`. I'm concerned that the original PR didn't really add sufficient test coverage for the introduction of `LazyProperty`, and I'm not 100% sure that I've caught all the cases where the `instanceof` check are needed in this PR. For the tests, I added the `@dataProvider getConstraintMethods` to every test that hit the problem area of code. ~~The only issue is that my fixed has broken the test introduced in #36343, `testGroupedMethodConstraintValidateInSequence`.~~ ~~I think I need @HeahDude to help me work through this. Maybe there is a more simple solution, one that doesn't require doing `instanceof LazyPropery` checks in multiple places, because this feels very brittle.~~ EDIT: fixed that test. Commits ------- 281861e [Validator] fix lazy property usage.
symfony · May 2, 2020 · aee10cd · aee10cd
2 parents d765a09 + 281861e
commit aee10cd
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 13 deletions.
diff --git a/src/Symfony/Component/Validator/Tests/Fixtures/Entity.php b/src/Symfony/Component/Validator/Tests/Fixtures/Entity.php
@@ -53,6 +53,11 @@ public function __construct($internal = null)
         $this->internal = $internal;
     }
 
+    public function getFirstName()
+    {
+        return $this->firstName;
+    }
+
     public function getInternal()
     {
         return $this->internal.' from getter';
@@ -141,4 +146,9 @@ public function setChildB($childB)
     {
         $this->childB = $childB;
     }
+
+    public function getReference()
+    {
+        return $this->reference;
+    }
 }
diff --git a/src/Symfony/Component/Validator/Tests/Validator/AbstractValidatorTest.php b/src/Symfony/Component/Validator/Tests/Validator/AbstractValidatorTest.php
@@ -32,6 +32,8 @@ abstract class AbstractValidatorTest extends TestCase
 
     const REFERENCE_CLASS = 'Symfony\Component\Validator\Tests\Fixtures\Reference';
 
+    const LAZY_PROPERTY = 'Symfony\Component\Validator\Validator\LazyProperty';
+
     /**
      * @var FakeMetadataFactory
      */
@@ -54,6 +56,7 @@ protected function setUp()
         $this->referenceMetadata = new ClassMetadata(self::REFERENCE_CLASS);
         $this->metadataFactory->addMetadata($this->metadata);
         $this->metadataFactory->addMetadata($this->referenceMetadata);
+        $this->metadataFactory->addMetadata(new ClassMetadata(self::LAZY_PROPERTY));
     }
 
     protected function tearDown()
@@ -510,7 +513,10 @@ public function testFailOnScalarReferences()
         $this->validate($entity);
     }
 
-    public function testArrayReference()
+    /**
+     * @dataProvider getConstraintMethods
+     */
+    public function testArrayReference($constraintMethod)
     {
         $entity = new Entity();
         $entity->reference = ['key' => new Reference()];
@@ -528,7 +534,7 @@ public function testArrayReference()
             $context->addViolation('Message %param%', ['%param%' => 'value']);
         };
 
-        $this->metadata->addPropertyConstraint('reference', new Valid());
+        $this->metadata->$constraintMethod('reference', new Valid());
         $this->referenceMetadata->addConstraint(new Callback([
             'callback' => $callback,
             'groups' => 'Group',
@@ -548,8 +554,10 @@ public function testArrayReference()
         $this->assertNull($violations[0]->getCode());
     }
 
-    // https://github.com/symfony/symfony/issues/6246
-    public function testRecursiveArrayReference()
+    /**
+     * @dataProvider getConstraintMethods
+     */
+    public function testRecursiveArrayReference($constraintMethod)
     {
         $entity = new Entity();
         $entity->reference = [2 => ['key' => new Reference()]];
@@ -567,7 +575,7 @@ public function testRecursiveArrayReference()
             $context->addViolation('Message %param%', ['%param%' => 'value']);
         };
 
-        $this->metadata->addPropertyConstraint('reference', new Valid());
+        $this->metadata->$constraintMethod('reference', new Valid());
         $this->referenceMetadata->addConstraint(new Callback([
             'callback' => $callback,
             'groups' => 'Group',
@@ -611,7 +619,10 @@ public function testOnlyCascadedArraysAreTraversed()
         $this->assertCount(0, $violations);
     }
 
-    public function testArrayTraversalCannotBeDisabled()
+    /**
+     * @dataProvider getConstraintMethods
+     */
+    public function testArrayTraversalCannotBeDisabled($constraintMethod)
     {
         $entity = new Entity();
         $entity->reference = ['key' => new Reference()];
@@ -620,7 +631,7 @@ public function testArrayTraversalCannotBeDisabled()
             $context->addViolation('Message %param%', ['%param%' => 'value']);
         };
 
-        $this->metadata->addPropertyConstraint('reference', new Valid([
+        $this->metadata->$constraintMethod('reference', new Valid([
             'traverse' => false,
         ]));
         $this->referenceMetadata->addConstraint(new Callback($callback));
@@ -631,7 +642,10 @@ public function testArrayTraversalCannotBeDisabled()
         $this->assertCount(1, $violations);
     }
 
-    public function testRecursiveArrayTraversalCannotBeDisabled()
+    /**
+     * @dataProvider getConstraintMethods
+     */
+    public function testRecursiveArrayTraversalCannotBeDisabled($constraintMethod)
     {
         $entity = new Entity();
         $entity->reference = [2 => ['key' => new Reference()]];
@@ -640,9 +654,10 @@ public function testRecursiveArrayTraversalCannotBeDisabled()
             $context->addViolation('Message %param%', ['%param%' => 'value']);
         };
 
-        $this->metadata->addPropertyConstraint('reference', new Valid([
+        $this->metadata->$constraintMethod('reference', new Valid([
             'traverse' => false,
         ]));
+
         $this->referenceMetadata->addConstraint(new Callback($callback));
 
         $violations = $this->validate($entity);
@@ -651,25 +666,31 @@ public function testRecursiveArrayTraversalCannotBeDisabled()
         $this->assertCount(1, $violations);
     }
 
-    public function testIgnoreScalarsDuringArrayTraversal()
+    /**
+     * @dataProvider getConstraintMethods
+     */
+    public function testIgnoreScalarsDuringArrayTraversal($constraintMethod)
     {
         $entity = new Entity();
         $entity->reference = ['string', 1234];
 
-        $this->metadata->addPropertyConstraint('reference', new Valid());
+        $this->metadata->$constraintMethod('reference', new Valid());
 
         $violations = $this->validate($entity);
 
         /* @var ConstraintViolationInterface[] $violations */
         $this->assertCount(0, $violations);
     }
 
-    public function testIgnoreNullDuringArrayTraversal()
+    /**
+     * @dataProvider getConstraintMethods
+     */
+    public function testIgnoreNullDuringArrayTraversal($constraintMethod)
     {
         $entity = new Entity();
         $entity->reference = [null];
 
-        $this->metadata->addPropertyConstraint('reference', new Valid());
+        $this->metadata->$constraintMethod('reference', new Valid());
 
         $violations = $this->validate($entity);
 
@@ -1218,6 +1239,14 @@ public function testReplaceDefaultGroup($sequence, array $assertViolations)
         }
     }
 
+    public function getConstraintMethods()
+    {
+        return [
+            ['addPropertyConstraint'],
+            ['addGetterConstraint'],
+        ];
+    }
+
     public function getTestReplaceDefaultGroup()
     {
         return [

diff --git a/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php b/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
@@ -677,6 +677,10 @@ private function validateGenericNode($value, $object, $cacheKey, MetadataInterfa
         // See validateClassNode()
         $cascadedGroups = null !== $cascadedGroups && \count($cascadedGroups) > 0 ? $cascadedGroups : $groups;
 
+        if ($value instanceof LazyProperty) {
+            $value = $value->getPropertyValue();
+        }
+
         if (\is_array($value)) {
             // Arrays are always traversed, independent of the specified
             // traversal strategy