bug #20078 Fix #19943 Make sure to process each interface metadata only once (lemoinem)

This PR was merged into the 2.7 branch. Discussion ---------- Fix #19943 Make sure to process each interface metadata only once | Q | A | ------------- | --- | Branch? | 2.7+ | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19943 | License | MIT | Doc PR | N/A Here is the fix for #19943, If you have `InterfaceA <- InterfaceB <- Class` with a constraint on a method defined on `InterfaceA`, the constraint and its eventual violations are currently validated and reported twice. Copy from https://github.com/symfony/symfony/issues/19943#issuecomment-250238529: As far as I can see, the problem seems to arise in [`LazyLoadingMetadataFactory`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php#L117-L123): [`ReflectionClass::getInterfaces()`](http://php.net/manual/en/reflectionclass.getinterfaces.php) returns both interfaces implemented directly and through inheritance (either through another interface or through a parent class). In the end, the following process occurs: 1. `PriceInterface` is parsed and its `NotBlank` constraint on `value` is loaded 2. `VariablePriceInterface` is parsed and inherits `PriceInterface`'s constraints (which is OK). 3. `ProductPrice` is parsed and inherits both `PriceInterface` and `VariablePriceInterface`'s constraints, which leads to a duplicated `NotBlank` constraint, one from each Interface. The Best Way ™️ would be to be able to extract the list of interfaces implemented by a class directly only. However, the process seems a bit intricate... I will start working on it and prepare a PR to that effect. However, if any of you has a better idea, I'm all hears... TODO: - [x] Regression tests to make sure the bug doesn't reappear Commits ------- 2f9b65a Fix #19943 Make sure to process each interface metadata only once
2016-09-30 12:33:15 -07:00 · 2016-09-30 12:33:15 -07:00 · c1c42a52bc
parent d2c050ebe0 2f9b65aa5a
commit c1c42a52bc
7 changed files with 88 additions and 23 deletions
--- a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php
+++ b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php
@ -116,9 +116,27 @@ class LazyLoadingMetadataFactory implements MetadataFactoryInterface
            $metadata->mergeConstraints($this->getMetadataFor($parent->name));
        }

-        // Include constraints from all implemented interfaces that have not been processed via parent class yet
-        foreach ($metadata->getReflectionClass()->getInterfaces() as $interface) {
-            if ('Symfony\Component\Validator\GroupSequenceProviderInterface' === $interface->name || ($parent && $parent->implementsInterface($interface->name))) {
+        $interfaces = $metadata->getReflectionClass()->getInterfaces();
+
+        $interfaces = array_filter($interfaces, function ($interface) use ($parent, $interfaces) {
+                $interfaceName = $interface->getName();
+
+                if ($parent && $parent->implementsInterface($interfaceName)) {
+                    return false;
+                }
+
+                foreach ($interfaces as $i) {
+                    if ($i !== $interface && $i->implementsInterface($interfaceName)) {
+                        return false;
+                    }
+                }
+
+                return true;
+            });
+
+        // Include constraints from all directly implemented interfaces
+        foreach ($interfaces as $interface) {
+            if ('Symfony\Component\Validator\GroupSequenceProviderInterface' === $interface->name) {
                continue;
            }
            $metadata->mergeConstraints($this->getMetadataFor($interface->name));
--- a/src/Symfony/Component/Validator/Tests/Fixtures/Entity.php
+++ b/src/Symfony/Component/Validator/Tests/Fixtures/Entity.php
@ -19,7 +19,7 @@ use Symfony\Component\Validator\ExecutionContextInterface;
 * @Assert\GroupSequence({"Foo", "Entity"})
 * @Assert\Callback({"Symfony\Component\Validator\Tests\Fixtures\CallbackClass", "callback"})
 */
-class Entity extends EntityParent
+class Entity extends EntityParent implements EntityInterfaceB
 {
    /**
     * @Assert\NotNull
--- a/src/Symfony/Component/Validator/Tests/Fixtures/EntityInterfaceA.php
+++ b/src/Symfony/Component/Validator/Tests/Fixtures/EntityInterfaceA.php
@ -11,6 +11,6 @@

 namespace Symfony\Component\Validator\Tests\Fixtures;

-interface EntityInterface
+interface EntityInterfaceA
 {
 }
--- a/src/Symfony/Component/Validator/Tests/Fixtures/EntityInterfaceB.php
+++ b/src/Symfony/Component/Validator/Tests/Fixtures/EntityInterfaceB.php
@ -0,0 +1,16 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Validator\Tests\Fixtures;
+
+interface EntityInterfaceB extends EntityParentInterface
+{
+}
--- a/src/Symfony/Component/Validator/Tests/Fixtures/EntityParent.php
+++ b/src/Symfony/Component/Validator/Tests/Fixtures/EntityParent.php
@ -13,7 +13,7 @@ namespace Symfony\Component\Validator\Tests\Fixtures;

 use Symfony\Component\Validator\Constraints\NotNull;

-class EntityParent implements EntityInterface
+class EntityParent implements EntityInterfaceA
 {
    protected $firstName;
    private $internal;
--- a/src/Symfony/Component/Validator/Tests/Fixtures/EntityParentInterface.php
+++ b/src/Symfony/Component/Validator/Tests/Fixtures/EntityParentInterface.php
@ -0,0 +1,16 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Validator\Tests\Fixtures;
+
+interface EntityParentInterface
+{
+}
--- a/src/Symfony/Component/Validator/Tests/Mapping/Factory/LazyLoadingMetadataFactoryTest.php
+++ b/src/Symfony/Component/Validator/Tests/Mapping/Factory/LazyLoadingMetadataFactoryTest.php
@ -18,17 +18,19 @@ use Symfony\Component\Validator\Tests\Fixtures\ConstraintA;

 class LazyLoadingMetadataFactoryTest extends \PHPUnit_Framework_TestCase
 {
-    const CLASSNAME = 'Symfony\Component\Validator\Tests\Fixtures\Entity';
-    const PARENTCLASS = 'Symfony\Component\Validator\Tests\Fixtures\EntityParent';
-    const INTERFACECLASS = 'Symfony\Component\Validator\Tests\Fixtures\EntityInterface';
+    const CLASS_NAME = 'Symfony\Component\Validator\Tests\Fixtures\Entity';
+    const PARENT_CLASS = 'Symfony\Component\Validator\Tests\Fixtures\EntityParent';
+    const INTERFACE_A_CLASS = 'Symfony\Component\Validator\Tests\Fixtures\EntityInterfaceA';
+    const INTERFACE_B_CLASS = 'Symfony\Component\Validator\Tests\Fixtures\EntityInterfaceB';
+    const PARENT_INTERFACE_CLASS = 'Symfony\Component\Validator\Tests\Fixtures\EntityParentInterface';

    public function testLoadClassMetadataWithInterface()
    {
        $factory = new LazyLoadingMetadataFactory(new TestLoader());
-        $metadata = $factory->getMetadataFor(self::PARENTCLASS);
+        $metadata = $factory->getMetadataFor(self::PARENT_CLASS);

        $constraints = array(
-            new ConstraintA(array('groups' => array('Default', 'EntityInterface', 'EntityParent'))),
+            new ConstraintA(array('groups' => array('Default', 'EntityInterfaceA', 'EntityParent'))),
            new ConstraintA(array('groups' => array('Default', 'EntityParent'))),
        );

@ -38,12 +40,12 @@ class LazyLoadingMetadataFactoryTest extends \PHPUnit_Framework_TestCase
    public function testMergeParentConstraints()
    {
        $factory = new LazyLoadingMetadataFactory(new TestLoader());
-        $metadata = $factory->getMetadataFor(self::CLASSNAME);
+        $metadata = $factory->getMetadataFor(self::CLASS_NAME);

        $constraints = array(
            new ConstraintA(array('groups' => array(
                'Default',
-                'EntityInterface',
+                'EntityInterfaceA',
                'EntityParent',
                'Entity',
            ))),
@ -52,6 +54,17 @@ class LazyLoadingMetadataFactoryTest extends \PHPUnit_Framework_TestCase
                'EntityParent',
                'Entity',
            ))),
+            new ConstraintA(array('groups' => array(
+                'Default',
+                'EntityParentInterface',
+                'EntityInterfaceB',
+                'Entity',
+            ))),
+            new ConstraintA(array('groups' => array(
+                'Default',
+                'EntityInterfaceB',
+                'Entity',
+            ))),
            new ConstraintA(array('groups' => array(
                'Default',
                'Entity',
@ -67,34 +80,36 @@ class LazyLoadingMetadataFactoryTest extends \PHPUnit_Framework_TestCase
        $factory = new LazyLoadingMetadataFactory(new TestLoader(), $cache);

        $parentClassConstraints = array(
-            new ConstraintA(array('groups' => array('Default', 'EntityInterface', 'EntityParent'))),
+            new ConstraintA(array('groups' => array('Default', 'EntityInterfaceA', 'EntityParent'))),
            new ConstraintA(array('groups' => array('Default', 'EntityParent'))),
        );
-        $interfaceConstraints = array(new ConstraintA(array('groups' => array('Default', 'EntityInterface'))));
+        $interfaceAConstraints = array(
+            new ConstraintA(array('groups' => array('Default', 'EntityInterfaceA'))),
+        );

        $cache->expects($this->never())
              ->method('has');
        $cache->expects($this->exactly(2))
              ->method('read')
              ->withConsecutive(
-                  array($this->equalTo(self::PARENTCLASS)),
-                  array($this->equalTo(self::INTERFACECLASS))
+                  array($this->equalTo(self::PARENT_CLASS)),
+                  array($this->equalTo(self::INTERFACE_A_CLASS))
              )
              ->will($this->returnValue(false));
        $cache->expects($this->exactly(2))
              ->method('write')
              ->withConsecutive(
-                  $this->callback(function ($metadata) use ($interfaceConstraints) {
-                      return $interfaceConstraints == $metadata->getConstraints();
+                  $this->callback(function ($metadata) use ($interfaceAConstraints) {
+                      return $interfaceAConstraints == $metadata->getConstraints();
                  }),
                  $this->callback(function ($metadata) use ($parentClassConstraints) {
                      return $parentClassConstraints == $metadata->getConstraints();
                  })
              );

-        $metadata = $factory->getMetadataFor(self::PARENTCLASS);
+        $metadata = $factory->getMetadataFor(self::PARENT_CLASS);

-        $this->assertEquals(self::PARENTCLASS, $metadata->getClassName());
+        $this->assertEquals(self::PARENT_CLASS, $metadata->getClassName());
        $this->assertEquals($parentClassConstraints, $metadata->getConstraints());
    }

@ -104,7 +119,7 @@ class LazyLoadingMetadataFactoryTest extends \PHPUnit_Framework_TestCase
        $cache = $this->getMock('Symfony\Component\Validator\Mapping\Cache\CacheInterface');
        $factory = new LazyLoadingMetadataFactory($loader, $cache);

-        $metadata = new ClassMetadata(self::PARENTCLASS);
+        $metadata = new ClassMetadata(self::PARENT_CLASS);
        $metadata->addConstraint(new ConstraintA());

        $loader->expects($this->never())
@ -116,7 +131,7 @@ class LazyLoadingMetadataFactoryTest extends \PHPUnit_Framework_TestCase
              ->method('read')
              ->will($this->returnValue($metadata));

-        $this->assertEquals($metadata, $factory->getMetadataFor(self::PARENTCLASS));
+        $this->assertEquals($metadata, $factory->getMetadataFor(self::PARENT_CLASS));
    }
 }