Make Annotations/Attribute mapping drivers report fields for the clas…

…ses where they are declared This PR will make the annotations and attribute mapping drivers report mapping configuration for the classes where it is declared, instead of omitting it and reporting it for subclasses only. This is necessary to be able to catch mis-configurations in `ClassMetadataFactory`. Fixes #10417, closes #10450, closes #10454. #### ⚠️ Summary for users getting `MappingExceptions` with the new mode When you set the `$reportFieldsWhereDeclared` constructor parameters to `true` for the AnnotationDriver and/or AttributesDriver and get `MappingExceptions`, you may be doing one of the following: * Using `private` fields with the same name in different classes of an entity inheritance hierarchy (see #10450) * Redeclaring/overwriting mapped properties inherited from mapped superclasses and/or other entities (see #10454) As explained in these two PRs, the ORM cannot (or at least, was not designed to) support such configurations. Unfortunately, due to the old – now deprecated – driver behaviour, the misconfigurations could not be detected, and due to previously missing tests, this in turn was not noticed. #### Current situation The annotations mapping driver has the following condition to skip properties that are reported by the PHP reflection API: https://github.com/doctrine/orm/blob/69c7791ba256d947ddb1aafe5f2439ab31704937/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php#L345-L357 This code has been there basically unchanged since the initial 2.0 release. The same condition can be found in the attribute driver, probably it has been copied when attributes were added. I _think_ what the driver tries to do here is to deal with the fact that Reflection will also report `public`/`protected` properties inherited from parent classes. This is supported by the observation (see #5744) that e. g. YAML and XML drivers do not contain this logic. The conditions are not precise enough for edge cases. They lead to some fields and configuration values not even being reported by the driver. Only since the fields would be "discovered" again when reflecting on subclasses, they eventually end up in class metadata structures for the subclasses. In one case of inherited ID generator mappings, the `ClassMetadataFactory` would also rely on this behaviour. Two potential bugs that can result from this are demonstrated in #10450 and #10454. #### Suggested solution In order to find a more reliable way of separating properties that are merely reported again in subclasses from those that are actual re-declarations, use the information already available in `ClassMetadata`. In particular, `declared` tells us in which non-transient class a "field" was first seen. Make the mapping driver skip only those properties for which we already know that they have been declared in parent classes, and skip them only when the observed declaring class matches the expectation. For all other properties, report them to `ClassMetadataFactory` and let that deal with consistency checking/error handling. #10450 and #10454 are merged into this PR to show that they pass now. #### Soft deprecation strategy To avoid throwing new/surprising exceptions (even for misconfigurations) during a minor version upgrade, the new driver mode is opt-in. Users will have to set the `$reportFieldsWhereDeclared` constructor parameters to `true` for the `AnnotationDriver` and/or `AttributesDriver`. Unless they do so, a deprecation warning will be raised. In 3.0, the "new" mode will become the default. The constructor parameter can be deprecated (as of ORM 3.1, probably) and is a no-op. We need to follow up in other places (DoctrineBundle, ... – what else?) to make this driver parameter an easy-to-change configuration setting.
doctrine · May 8, 2023 · b52a8f8 · b52a8f8
1 parent 37c8953
commit b52a8f8
Show file tree

Hide file tree

Showing 21 changed files with 413 additions and 53 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,3 +1,18 @@
+# Upgrade to 2.16
+
+## Changing the way how reflection-based mapping drivers report fields, deprecated the "old" mode
+
+In ORM 3.0, a change will be made regarding how the `AttributeDriver` reports field mappings.
+This change is necessary to be able to detect (and reject) some invalid mapping configurations.
+
+To avoid surprises during 2.x upgrades, the new mode is opt-in. It can be activated on the 
+`AttributeDriver` and `AnnotationDriver` by setting the `$reportFieldsWhereDeclared`
+constructor parameter to `true`. It will cause `MappingException`s to be thrown when invalid
+configurations are detected.
+
+Not enabling the new mode will cause a deprecation notice to be raised. In ORM 3.0, 
+only the new mode will be available.
+
 # Upgrade to 2.15
 
 ## Deprecated configuring `JoinColumn` on the inverse side of one-to-one associations

diff --git a/lib/Doctrine/ORM/Configuration.php b/lib/Doctrine/ORM/Configuration.php
@@ -164,7 +164,7 @@ public function setMetadataDriverImpl(MappingDriver $driverImpl)
      *
      * @return AnnotationDriver
      */
-    public function newDefaultAnnotationDriver($paths = [], $useSimpleAnnotationReader = true)
+    public function newDefaultAnnotationDriver($paths = [], $useSimpleAnnotationReader = true, bool $reportFieldsWhereDeclared = false)
     {
         Deprecation::trigger(
             'doctrine/orm',
@@ -203,7 +203,8 @@ public function newDefaultAnnotationDriver($paths = [], $useSimpleAnnotationRead
 
         return new AnnotationDriver(
             $reader,
-            (array) $paths
+            (array) $paths,
+            $reportFieldsWhereDeclared
         );
     }
 

diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
@@ -110,7 +110,7 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
         if ($parent) {
             $class->setInheritanceType($parent->inheritanceType);
             $class->setDiscriminatorColumn($parent->discriminatorColumn);
-            $class->setIdGeneratorType($parent->generatorType);
+            $this->inheritIdGeneratorMapping($class, $parent);
             $this->addInheritedFields($class, $parent);
             $this->addInheritedRelations($class, $parent);
             $this->addInheritedEmbeddedClasses($class, $parent);
@@ -138,12 +138,8 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
             throw MappingException::reflectionFailure($class->getName(), $e);
         }
 
-        // If this class has a parent the id generator strategy is inherited.
-        // However this is only true if the hierarchy of parents contains the root entity,
-        // if it consists of mapped superclasses these don't necessarily include the id field.
-        if ($parent && $rootEntityFound) {
-            $this->inheritIdGeneratorMapping($class, $parent);
-        } else {
+        // Complete id generator mapping when the generator was declared/added in this class
+        if ($class->identifier && (! $parent || ! $parent->identifier)) {
             $this->completeIdGeneratorMapping($class);
         }
 

diff --git a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
@@ -34,6 +34,7 @@
 class AnnotationDriver extends CompatibilityAnnotationDriver
 {
     use ColocatedMappingDriver;
+    use ReflectionBasedDriver;
 
     /**
      * The annotation reader.
@@ -60,7 +61,7 @@ class AnnotationDriver extends CompatibilityAnnotationDriver
      * @param Reader               $reader The AnnotationReader to use
      * @param string|string[]|null $paths  One or multiple paths where mapping classes can be found.
      */
-    public function __construct($reader, $paths = null)
+    public function __construct($reader, $paths = null, bool $reportFieldsWhereDeclared = false)
     {
         Deprecation::trigger(
             'doctrine/orm',
@@ -70,6 +71,17 @@ public function __construct($reader, $paths = null)
         $this->reader = $reader;
 
         $this->addPaths((array) $paths);
+
+        if (! $reportFieldsWhereDeclared) {
+            Deprecation::trigger(
+                'doctrine/orm',
+                'https://github.com/doctrine/orm/pull/10455',
+                'In ORM 3.0, the AttributeDriver will report fields for the classes where they are declared. This may uncover invalid mapping configurations. To opt into the new mode also with the AnnotationDriver today, set the "reportFieldsWhereDeclared" constructor parameter to true.',
+                self::class
+            );
+        }
+
+        $this->reportFieldsWhereDeclared = $reportFieldsWhereDeclared;
     }
 
     /**
@@ -348,15 +360,7 @@ public function loadMetadataForClass($className, PersistenceClassMetadata $metad
 
         // Evaluate annotations on properties/fields
         foreach ($class->getProperties() as $property) {
-            if (
-                $metadata->isMappedSuperclass && ! $property->isPrivate()
-                ||
-                $metadata->isInheritedField($property->name)
-                ||
-                $metadata->isInheritedAssociation($property->name)
-                ||
-                $metadata->isInheritedEmbeddedClass($property->name)
-            ) {
+            if ($this->isRepeatedPropertyDeclaration($property, $metadata)) {
                 continue;
             }
 

diff --git a/lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php b/lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php
@@ -30,6 +30,7 @@
 class AttributeDriver extends CompatibilityAnnotationDriver
 {
     use ColocatedMappingDriver;
+    use ReflectionBasedDriver;
 
     private const ENTITY_ATTRIBUTE_CLASSES = [
         Mapping\Entity::class => 1,
@@ -53,7 +54,7 @@ class AttributeDriver extends CompatibilityAnnotationDriver
     protected $reader;
 
     /** @param array<string> $paths */
-    public function __construct(array $paths)
+    public function __construct(array $paths, bool $reportFieldsWhereDeclared = false)
     {
         if (PHP_VERSION_ID < 80000) {
             throw new LogicException(sprintf(
@@ -73,6 +74,17 @@ public function __construct(array $paths)
                 self::class
             );
         }
+
+        if (! $reportFieldsWhereDeclared) {
+            Deprecation::trigger(
+                'doctrine/orm',
+                'https://github.com/doctrine/orm/pull/10455',
+                'In ORM 3.0, the AttributeDriver will report fields for the classes where they are declared. This may uncover invalid mapping configurations. To opt into the new mode today, set the "reportFieldsWhereDeclared" constructor parameter to true.',
+                self::class
+            );
+        }
+
+        $this->reportFieldsWhereDeclared = $reportFieldsWhereDeclared;
     }
 
     /**
@@ -298,15 +310,8 @@ public function loadMetadataForClass($className, PersistenceClassMetadata $metad
 
         foreach ($reflectionClass->getProperties() as $property) {
             assert($property instanceof ReflectionProperty);
-            if (
-                $metadata->isMappedSuperclass && ! $property->isPrivate()
-                ||
-                $metadata->isInheritedField($property->name)
-                ||
-                $metadata->isInheritedAssociation($property->name)
-                ||
-                $metadata->isInheritedEmbeddedClass($property->name)
-            ) {
+
+            if ($this->isRepeatedPropertyDeclaration($property, $metadata)) {
                 continue;
             }
 

diff --git a/lib/Doctrine/ORM/Mapping/Driver/ReflectionBasedDriver.php b/lib/Doctrine/ORM/Mapping/Driver/ReflectionBasedDriver.php
@@ -0,0 +1,54 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Mapping\Driver;
+
+use Doctrine\ORM\Mapping\ClassMetadata;
+use ReflectionProperty;
+
+/** @internal */
+trait ReflectionBasedDriver
+{
+    /** @var bool */
+    private $reportFieldsWhereDeclared = false;
+
+    /**
+     * Helps to deal with the case that reflection may report properties inherited from parent classes.
+     * When we know about the fields already (inheritance has been anticipated in ClassMetadataFactory),
+     * the driver must skip them.
+     *
+     * The declaring classes may mismatch when there are private properties: The same property name may be
+     * reported multiple times, but since it is private, it is in fact multiple (different) properties in
+     * different classes. In that case, report the property as an individual field. (ClassMetadataFactory will
+     * probably fail in that case, though.)
+     */
+    private function isRepeatedPropertyDeclaration(ReflectionProperty $property, ClassMetadata $metadata): bool
+    {
+        if (! $this->reportFieldsWhereDeclared) {
+            return $metadata->isMappedSuperclass && ! $property->isPrivate()
+                || $metadata->isInheritedField($property->name)
+                || $metadata->isInheritedAssociation($property->name)
+                || $metadata->isInheritedEmbeddedClass($property->name);
+        }
+
+        $declaringClass = $property->getDeclaringClass()->getName();
+
+        if (
+            isset($metadata->fieldMappings[$property->name]['declared'])
+            && $metadata->fieldMappings[$property->name]['declared'] === $declaringClass
+        ) {
+            return true;
+        }
+
+        if (
+            isset($metadata->associationMappings[$property->name]['declared'])
+            && $metadata->associationMappings[$property->name]['declared'] === $declaringClass
+        ) {
+            return true;
+        }
+
+        return isset($metadata->embeddedClasses[$property->name]['declared'])
+            && $metadata->embeddedClasses[$property->name]['declared'] === $declaringClass;
+    }
+}
diff --git a/lib/Doctrine/ORM/ORMSetup.php b/lib/Doctrine/ORM/ORMSetup.php
@@ -63,7 +63,8 @@ public static function createAnnotationMetadataConfiguration(
      */
     public static function createDefaultAnnotationDriver(
         array $paths = [],
-        ?CacheItemPoolInterface $cache = null
+        ?CacheItemPoolInterface $cache = null,
+        bool $reportFieldsWhereDeclared = false
     ): AnnotationDriver {
         Deprecation::trigger(
             'doctrine/orm',
@@ -89,7 +90,7 @@ public static function createDefaultAnnotationDriver(
             $reader = new PsrCachedReader($reader, $cache);
         }
 
-        return new AnnotationDriver($reader, $paths);
+        return new AnnotationDriver($reader, $paths, $reportFieldsWhereDeclared);
     }
 
     /**

diff --git a/tests/Doctrine/Tests/DoctrineTestCase.php b/tests/Doctrine/Tests/DoctrineTestCase.php
@@ -21,6 +21,7 @@ abstract class DoctrineTestCase extends TestCase
         'assertMatchesRegularExpression' => 'assertRegExp', // can be removed when PHPUnit 9 is minimum
         'assertDoesNotMatchRegularExpression' => 'assertNotRegExp', // can be removed when PHPUnit 9 is minimum
         'assertFileDoesNotExist' => 'assertFileNotExists', // can be removed PHPUnit 9 is minimum
+        'expectExceptionMessageMatches' => 'expectExceptionMessageRegExp', // can be removed when PHPUnit 8 is minimum
     ];
 
     /**

diff --git a/tests/Doctrine/Tests/Models/Company/CompanyFlexContract.php b/tests/Doctrine/Tests/Models/Company/CompanyFlexContract.php
@@ -11,8 +11,6 @@
 use Doctrine\ORM\Mapping\Entity;
 use Doctrine\ORM\Mapping\EntityResult;
 use Doctrine\ORM\Mapping\FieldResult;
-use Doctrine\ORM\Mapping\GeneratedValue;
-use Doctrine\ORM\Mapping\Id;
 use Doctrine\ORM\Mapping\JoinColumn;
 use Doctrine\ORM\Mapping\JoinTable;
 use Doctrine\ORM\Mapping\ManyToMany;
@@ -67,14 +65,6 @@
 #[ORM\Entity]
 class CompanyFlexContract extends CompanyContract
 {
-    /**
-     * @Id
-     * @GeneratedValue
-     * @Column(type="integer")
-     * @var int
-     */
-    public $id;
-
     /**
      * @Column(type="integer")
      * @var int

diff --git a/tests/Doctrine/Tests/ORM/ConfigurationTest.php b/tests/Doctrine/Tests/ORM/ConfigurationTest.php
@@ -101,7 +101,7 @@ public function testNewDefaultAnnotationDriver(): void
         $paths           = [__DIR__];
         $reflectionClass = new ReflectionClass(ConfigurationTestAnnotationReaderChecker::class);
 
-        $annotationDriver = $this->configuration->newDefaultAnnotationDriver($paths, false);
+        $annotationDriver = $this->configuration->newDefaultAnnotationDriver($paths, false, true);
         $reader           = $annotationDriver->getReader();
         $annotation       = $reader->getMethodAnnotation(
             $reflectionClass->getMethod('namespacedAnnotationMethod'),

diff --git a/tests/Doctrine/Tests/ORM/Functional/EnumTest.php b/tests/Doctrine/Tests/ORM/Functional/EnumTest.php
@@ -36,7 +36,7 @@ public function setUp(): void
     {
         parent::setUp();
 
-        $this->_em         = $this->getEntityManager(null, new AttributeDriver([dirname(__DIR__, 2) . '/Models/Enums']));
+        $this->_em         = $this->getEntityManager(null, new AttributeDriver([dirname(__DIR__, 2) . '/Models/Enums'], true));
         $this->_schemaTool = new SchemaTool($this->_em);
 
         if ($this->isSecondLevelCacheEnabled) {

diff --git a/tests/Doctrine/Tests/ORM/Functional/MergeProxiesTest.php b/tests/Doctrine/Tests/ORM/Functional/MergeProxiesTest.php
@@ -242,7 +242,9 @@ private function createEntityManager(): EntityManagerInterface
 
         TestUtil::configureProxies($config);
         $config->setMetadataDriverImpl(ORMSetup::createDefaultAnnotationDriver(
-            [realpath(__DIR__ . '/../../Models/Cache')]
+            [realpath(__DIR__ . '/../../Models/Cache')],
+            null,
+            true
         ));
 
         // always runs on sqlite to prevent multi-connection race-conditions with the test suite

diff --git a/tests/Doctrine/Tests/ORM/Functional/ReadonlyPropertiesTest.php b/tests/Doctrine/Tests/ORM/Functional/ReadonlyPropertiesTest.php
@@ -26,7 +26,8 @@ protected function setUp(): void
         }
 
         $this->_em         = $this->getEntityManager(null, new AttributeDriver(
-            [dirname(__DIR__, 2) . '/Models/ReadonlyProperties']
+            [dirname(__DIR__, 2) . '/Models/ReadonlyProperties'],
+            true
         ));
         $this->_schemaTool = new SchemaTool($this->_em);
 

diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC719Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC719Test.php
@@ -31,7 +31,7 @@ public function testIsEmptySqlGeneration(): void
     {
         $q = $this->_em->createQuery('SELECT g, c FROM Doctrine\Tests\ORM\Functional\Ticket\DDC719Group g LEFT JOIN g.children c  WHERE g.parents IS EMPTY');
 
-        $referenceSQL = 'SELECT g0_.name AS name_0, g0_.description AS description_1, g0_.id AS id_2, g1_.name AS name_3, g1_.description AS description_4, g1_.id AS id_5 FROM groups g0_ LEFT JOIN groups_groups g2_ ON g0_.id = g2_.parent_id LEFT JOIN groups g1_ ON g1_.id = g2_.child_id WHERE (SELECT COUNT(*) FROM groups_groups g3_ WHERE g3_.child_id = g0_.id) = 0';
+        $referenceSQL = 'SELECT g0_.id AS id_0, g0_.name AS name_1, g0_.description AS description_2, g1_.id as id_3, g1_.name AS name_4, g1_.description AS description_5 FROM groups g0_ LEFT JOIN groups_groups g2_ ON g0_.id = g2_.parent_id LEFT JOIN groups g1_ ON g1_.id = g2_.child_id WHERE (SELECT COUNT(*) FROM groups_groups g3_ WHERE g3_.child_id = g0_.id) = 0';
 
         self::assertEquals(
             strtolower($referenceSQL),