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

This PR will make the annotations and attribute mapping drivers more consistently report mapping configuration for the classes where it is declared. This is necessary to be able to catch mis-configurations in `ClassMetadataFactory`.

Fixes doctrine#10417, closes doctrine#10450, closes doctrine#10454.

 #### 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 doctrine#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 doctrine#10450 and doctrine#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.

 doctrine#10450 and doctrine#10454 are merged into this PR to show that they pass now.

 #### Soft deprecation strategy

When users re-declare (overwrite) mapped properties inherited from mapped superclasses and/or other entities, the new behaviour may cause their mapping configuration to be rejected as invalid. This applies only to configurations that were never allowed as per the documentation.

For some cases, we missed the opportunity to reject the configuration with an exception early on. In other cases, we had the exception-throwing code in place, but due to the driver's behaviour, it was never reached.

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 should follow up in other places (DoctrineBundle, ... – what else?) to make this driver parameter an easy-to-change configuration setting.

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date:      Tue Jan 24 20:28:32 2023 +0000
#
# On branch fix-mapping-driver-load
# Your branch is up-to-date with 'mpdude/fix-mapping-driver-load'.
#
# Changes to be committed:
#	modified:   UPGRADE.md
#	modified:   lib/Doctrine/ORM/Configuration.php
#	modified:   lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
#	modified:   lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
#	modified:   lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php
#	new file:   lib/Doctrine/ORM/Mapping/Driver/ReflectionBasedDriver.php
#	modified:   lib/Doctrine/ORM/ORMSetup.php
#	modified:   tests/Doctrine/Tests/DoctrineTestCase.php
#	modified:   tests/Doctrine/Tests/Models/Company/CompanyFlexContract.php
#	modified:   tests/Doctrine/Tests/ORM/ConfigurationTest.php
#	modified:   tests/Doctrine/Tests/ORM/Functional/EnumTest.php
#	modified:   tests/Doctrine/Tests/ORM/Functional/MergeProxiesTest.php
#	modified:   tests/Doctrine/Tests/ORM/Functional/ReadonlyPropertiesTest.php
#	modified:   tests/Doctrine/Tests/ORM/Functional/Ticket/DDC719Test.php
#	new file:   tests/Doctrine/Tests/ORM/Functional/Ticket/GH10450Test.php
#	new file:   tests/Doctrine/Tests/ORM/Functional/Ticket/GH10454Test.php
#	modified:   tests/Doctrine/Tests/ORM/Functional/Ticket/GH5998Test.php
#	modified:   tests/Doctrine/Tests/ORM/Mapping/AttributeDriverTest.php
#	modified:   tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php
#	modified:   tests/Doctrine/Tests/OrmFunctionalTestCase.php
#	modified:   tests/Doctrine/Tests/OrmTestCase.php
#
# Untracked files:
#	phpunit.xml
#

Author: mpdude

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