[Validator] Fix regression with class metadatada on parent classes

symfony · nicolas-grekas · Jul 20, 2023 · Jun 27, 2023 · Jul 20, 2023 · e98f5f2782b3bbe849b78f5b486d35b3e57afe8e
commit e98f5f2782b3bbe849b78f5b486d35b3e57afe8e
diff --git a/src/Symfony/Bridge/Doctrine/Validator/DoctrineLoader.php b/src/Symfony/Bridge/Doctrine/Validator/DoctrineLoader.php
@@ -108,7 +108,7 @@ public function loadClassMetadata(ClassMetadata $metadata): bool
                if (isset($mapping['originalClass']) && !str_contains($mapping['declaredField'], '.')) {
                    $metadata->addPropertyConstraint($mapping['declaredField'], new Valid());
                    $loaded = true;
-                } elseif (property_exists($className, $mapping['fieldName'])) {
+                } elseif (property_exists($className, $mapping['fieldName']) && (!$doctrineMetadata->isMappedSuperclass || $metadata->getReflectionClass()->getProperty($mapping['fieldName'])->isPrivate())) {
                    $metadata->addPropertyConstraint($mapping['fieldName'], new Length(['max' => $mapping['length']]));
                    $loaded = true;
                }

diff --git a/src/Symfony/Component/Validator/Mapping/ClassMetadata.php b/src/Symfony/Component/Validator/Mapping/ClassMetadata.php
@@ -358,11 +358,11 @@ public function mergeConstraints(self $source)

                if ($member instanceof MemberMetadata && !$member->isPrivate($this->name)) {
                    $property = $member->getPropertyName();
-                    $this->members[$property] = [$member];
+                    $this->members[$property][] = $member;

-                    if ($member instanceof PropertyMetadata) {
+                    if ($member instanceof PropertyMetadata && !isset($this->properties[$property])) {
                        $this->properties[$property] = $member;
-                    } elseif ($member instanceof GetterMetadata) {
+                    } elseif ($member instanceof GetterMetadata && !isset($this->getters[$property])) {
                        $this->getters[$property] = $member;
                    }
                } else {

diff --git a/src/Symfony/Component/Validator/Tests/Fixtures/Annotation/Entity.php b/src/Symfony/Component/Validator/Tests/Fixtures/Annotation/Entity.php
@@ -55,6 +55,10 @@ class Entity extends EntityParent implements EntityInterfaceB
    private $internal;
    public $data = 'Overridden data';
    public $initialized = false;
+    /**
+     * @Assert\Type("integer")
+     */
+    protected $other;

    public function __construct($internal = null)
    {

diff --git a/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/Entity.php b/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/Entity.php
@@ -57,6 +57,8 @@ class Entity extends EntityParent implements EntityInterfaceB
    private $internal;
    public $data = 'Overridden data';
    public $initialized = false;
+    #[Assert\Type('integer')]
+    protected $other;

    public function __construct($internal = null)
    {

diff --git a/src/Symfony/Component/Validator/Tests/Fixtures/NestedAttribute/Entity.php b/src/Symfony/Component/Validator/Tests/Fixtures/NestedAttribute/Entity.php
@@ -78,6 +78,8 @@ class Entity extends EntityParent implements EntityInterfaceB
    private $internal;
    public $data = 'Overridden data';
    public $initialized = false;
+    #[Assert\Type('integer')]
+    protected $other;

    public function __construct($internal = null)
    {

diff --git a/src/Symfony/Component/Validator/Tests/Mapping/ClassMetadataTest.php b/src/Symfony/Component/Validator/Tests/Mapping/ClassMetadataTest.php
@@ -163,8 +163,8 @@ public function testMergeConstraintsMergesMemberConstraints()
        $parent->addPropertyConstraint('firstName', new ConstraintA());
        $parent->addPropertyConstraint('firstName', new ConstraintB(['groups' => 'foo']));

-        $this->metadata->mergeConstraints($parent);
        $this->metadata->addPropertyConstraint('firstName', new ConstraintA());
+        $this->metadata->mergeConstraints($parent);

        $constraintA1 = new ConstraintA(['groups' => [
            'Default',
@@ -179,35 +179,29 @@ public function testMergeConstraintsMergesMemberConstraints()
            'groups' => ['foo'],
        ]);

-        $constraints = [
-            $constraintA1,
-            $constraintB,
-            $constraintA2,
-        ];
+        $members = $this->metadata->getPropertyMetadata('firstName');

-        $constraintsByGroup = [
-            'Default' => [
-                $constraintA1,
-                $constraintA2,
-            ],
-            'EntityParent' => [
-                $constraintA1,
-            ],
-            'Entity' => [
-                $constraintA1,
-                $constraintA2,
+        $this->assertCount(2, $members);
+        $this->assertEquals(self::CLASSNAME, $members[0]->getClassName());
+        $this->assertEquals([$constraintA2], $members[0]->getConstraints());
+        $this->assertEquals(
+            [
+                'Default' => [$constraintA2],
+                'Entity' => [$constraintA2],
            ],
-            'foo' => [
-                $constraintB,
+            $members[0]->constraintsByGroup
+        );
+        $this->assertEquals(self::PARENTCLASS, $members[1]->getClassName());
+        $this->assertEquals([$constraintA1, $constraintB], $members[1]->getConstraints());
+        $this->assertEquals(
+            [
+                'Default' => [$constraintA1],
+                'Entity' => [$constraintA1],
+                'EntityParent' => [$constraintA1],
+                'foo' => [$constraintB],
            ],
-        ];
-
-        $members = $this->metadata->getPropertyMetadata('firstName');
-
-        $this->assertCount(1, $members);
-        $this->assertEquals(self::PARENTCLASS, $members[0]->getClassName());
-        $this->assertEquals($constraints, $members[0]->getConstraints());
-        $this->assertEquals($constraintsByGroup, $members[0]->constraintsByGroup);
+            $members[1]->constraintsByGroup
+        );
    }

    public function testMemberMetadatas()

diff --git a/src/Symfony/Component/Validator/Tests/Mapping/Loader/AnnotationLoaderTest.php b/src/Symfony/Component/Validator/Tests/Mapping/Loader/AnnotationLoaderTest.php
@@ -27,6 +27,7 @@
 use Symfony\Component\Validator\Constraints\Range;
 use Symfony\Component\Validator\Constraints\Required;
 use Symfony\Component\Validator\Constraints\Sequentially;
+use Symfony\Component\Validator\Constraints\Type;
 use Symfony\Component\Validator\Constraints\Valid;
 use Symfony\Component\Validator\Mapping\ClassMetadata;
 use Symfony\Component\Validator\Mapping\Loader\AnnotationLoader;
@@ -98,6 +99,7 @@ public function testLoadClassMetadata(string $namespace)
        $expected->addGetterConstraint('lastName', new NotNull());
        $expected->addGetterMethodConstraint('valid', 'isValid', new IsTrue());
        $expected->addGetterConstraint('permissions', new IsTrue());
+        $expected->addPropertyConstraint('other', new Type('integer'));

        // load reflection class so that the comparison passes
        $expected->getReflectionClass();
@@ -139,18 +141,16 @@ public function testLoadClassMetadataAndMerge(string $namespace)
        $loader->loadClassMetadata($parent_metadata);

        $metadata = new ClassMetadata($namespace.'\Entity');
+        $loader->loadClassMetadata($metadata);

        // Merge parent metaData.
        $metadata->mergeConstraints($parent_metadata);

-        $loader->loadClassMetadata($metadata);
-
        $expected_parent = new ClassMetadata($namespace.'\EntityParent');
        $expected_parent->addPropertyConstraint('other', new NotNull());
        $expected_parent->getReflectionClass();

        $expected = new ClassMetadata($namespace.'\Entity');
-        $expected->mergeConstraints($expected_parent);

        $expected->setGroupSequence(['Foo', 'Entity']);
        $expected->addConstraint(new ConstraintA());
@@ -187,11 +187,18 @@ public function testLoadClassMetadataAndMerge(string $namespace)
        $expected->addGetterConstraint('lastName', new NotNull());
        $expected->addGetterMethodConstraint('valid', 'isValid', new IsTrue());
        $expected->addGetterConstraint('permissions', new IsTrue());
+        $expected->addPropertyConstraint('other', new Type('integer'));

        // load reflection class so that the comparison passes
        $expected->getReflectionClass();
+        $expected->mergeConstraints($expected_parent);

        $this->assertEquals($expected, $metadata);
+
+        $otherMetadata = $metadata->getPropertyMetadata('other');
+        $this->assertCount(2, $otherMetadata);
+        $this->assertInstanceOf(Type::class, $otherMetadata[0]->getConstraints()[0]);
+        $this->assertInstanceOf(NotNull::class, $otherMetadata[1]->getConstraints()[0]);
    }

    /**