Make Annotations/Attribute mapping drivers report fields for the classes where they are declared

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

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
         );
     }
 

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);
         }
 

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;
             }
 

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;
             }
 

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;
+    }
+}

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);
     }
 
     /**

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
     ];
 
     /**

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

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'),

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) {

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

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);
 

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),