Bugfix: handle repeatable attributes (#8756)

* fix: handle repeatable attributes

* Restore interface compatibility

A reader is supposed to only ever return objects. By introducing
RepeatableAttributeCollection, we fulfill the interface and improve
clarity.

* refactor: accurate type declarations and returns

* refactor: remove unused use

* Ignore AttributeReader in phpstan and psalm to pass on CI running PHP 7.4.

* test: isTransient

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
Co-authored-by: Benjamin Eberlei <kontakt@beberlei.de>

composer.json

@@ -52,6 +52,7 @@
     "autoload-dev": {
         "psr-4": {
             "Doctrine\\Tests\\": "tests/Doctrine/Tests",
+            "Doctrine\\StaticAnalysis\\": "tests/Doctrine/StaticAnalysis",
             "Doctrine\\Performance\\": "tests/Doctrine/Performance"
         }
     },

docs/en/reference/attributes-reference.rst

@@ -384,7 +384,7 @@ Example:
     use Doctrine\ORM\Mapping\GeneratedValue;
     use Doctrine\ORM\Mapping\Id;
 
-    #[Id, Column(type: "integer"), GeneratedValue(strategy="IDENTITY")]
+    #[Id, Column(type: "integer"), GeneratedValue(strategy: "IDENTITY")]
     protected $id = null;
 
 .. _attrref_haslifecyclecallbacks:
@@ -485,7 +485,7 @@ Example:
     use Doctrine\ORM\Mapping\Column;
     use Doctrine\ORM\Mapping\Id;
 
-    #[Id, Column(type="integer")]
+    #[Id, Column(type: "integer")]
     protected $id = null;
 
 .. _attrref_inheritancetype:
@@ -514,7 +514,7 @@ Examples:
 
     #[Entity]
     #[InheritanceType("SINGLE_TABLE")]
-    #[DiscriminatorColumn(name="discr", type="string")]
+    #[DiscriminatorColumn(name: "discr", type: "string")]
     #[DiscriminatorMap({"person" = "Person", "employee" = "Employee"})]
     class Person
     {
@@ -523,7 +523,7 @@ Examples:
 
     #[Entity]
     #[InheritanceType("JOINED")]
-    #[DiscriminatorColumn(name="discr", type="string")]
+    #[DiscriminatorColumn(name: "discr", type: "string")]
     #[DiscriminatorMap({"person" = "Person", "employee" = "Employee"})]
     class Person
     {
@@ -1003,7 +1003,7 @@ Basic example:
     use Doctrine\ORM\Mapping\UniqueConstraint;
 
     #[Entity]
-    #[UniqueConstraint(name: "ean", columns=["ean"])]
+    #[UniqueConstraint(name: "ean", columns: ["ean"])]
     class ECommerceProduct
     {
     }

lib/Doctrine/ORM/Mapping/ClassMetadata.php

@@ -24,9 +24,21 @@ namespace Doctrine\ORM\Mapping;
  * {@inheritDoc}
  *
  * @todo remove or rename ClassMetadataInfo to ClassMetadata
- * @template T of object
+ * @template-covariant T of object
  * @template-extends ClassMetadataInfo<T>
  */
 class ClassMetadata extends ClassMetadataInfo
 {
+    /**
+     * Repeating the ClassMetadataInfo constructor to infer correctly the template with PHPStan
+     *
+     * @see https://github.com/doctrine/orm/issues/8709
+     *
+     * @param string $entityName The name of the entity class the new instance is used for.
+     * @psalm-param class-string<T> $entityName
+     */
+    public function __construct($entityName, ?NamingStrategy $namingStrategy = null)
+    {
+        parent::__construct($entityName, $namingStrategy);
+    }
 }

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

@@ -81,7 +81,7 @@ use const PHP_VERSION_ID;
  *    get the whole class name, namespace inclusive, prepended to every property in
  *    the serialized representation).
  *
- * @template T of object
+ * @template-covariant T of object
  * @template-implements ClassMetadata<T>
  */
 class ClassMetadataInfo implements ClassMetadata

lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php

@@ -18,8 +18,8 @@ use ReflectionProperty;
 use function assert;
 use function class_exists;
 use function constant;
-use function count;
 use function defined;
+use function get_class;
 
 class AttributeDriver extends AnnotationDriver
 {
@@ -38,6 +38,23 @@ class AttributeDriver extends AnnotationDriver
         parent::__construct(new AttributeReader(), $paths);
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    public function isTransient($className)
+    {
+        $classAnnotations = $this->reader->getClassAnnotations(new ReflectionClass($className));
+
+        foreach ($classAnnotations as $a) {
+            $annot = $a instanceof RepeatableAttributeCollection ? $a[0] : $a;
+            if (isset($this->entityAnnotationClasses[get_class($annot)])) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     public function loadMetadataForClass($className, ClassMetadata $metadata): void
     {
         assert($metadata instanceof ClassMetadataInfo);

lib/Doctrine/ORM/Mapping/Driver/AttributeReader.php

@@ -11,7 +11,8 @@ use ReflectionClass;
 use ReflectionMethod;
 use ReflectionProperty;
 
-use function count;
+use function assert;
+use function is_string;
 use function is_subclass_of;
 
 /**
@@ -22,63 +23,75 @@ final class AttributeReader
     /** @var array<string,bool> */
     private array $isRepeatableAttribute = [];
 
-    /** @return array<object> */
+    /** @return array<Annotation|RepeatableAttributeCollection> */
     public function getClassAnnotations(ReflectionClass $class): array
     {
         return $this->convertToAttributeInstances($class->getAttributes());
     }
 
-    /** @return array<object>|object|null */
+    /** @return Annotation|RepeatableAttributeCollection|null */
     public function getClassAnnotation(ReflectionClass $class, $annotationName)
     {
-        return $this->getClassAnnotations($class)[$annotationName] ?? ($this->isRepeatable($annotationName) ? [] : null);
+        return $this->getClassAnnotations($class)[$annotationName]
+            ?? ($this->isRepeatable($annotationName) ? new RepeatableAttributeCollection() : null);
     }
 
-    /** @return array<object> */
+    /** @return array<Annotation|RepeatableAttributeCollection> */
     public function getMethodAnnotations(ReflectionMethod $method): array
     {
         return $this->convertToAttributeInstances($method->getAttributes());
     }
 
-    /** @return array<object>|object|null */
+    /** @return Annotation|RepeatableAttributeCollection|null */
     public function getMethodAnnotation(ReflectionMethod $method, $annotationName)
     {
-        return $this->getMethodAnnotations($method)[$annotationName] ?? ($this->isRepeatable($annotationName) ? [] : null);
+        return $this->getMethodAnnotations($method)[$annotationName]
+            ?? ($this->isRepeatable($annotationName) ? new RepeatableAttributeCollection() : null);
     }
 
-    /** @return array<object> */
+    /** @return array<Annotation|RepeatableAttributeCollection> */
     public function getPropertyAnnotations(ReflectionProperty $property): array
     {
         return $this->convertToAttributeInstances($property->getAttributes());
     }
 
-    /** @return array<object>|object|null */
+    /** @return Annotation|RepeatableAttributeCollection|null */
     public function getPropertyAnnotation(ReflectionProperty $property, $annotationName)
     {
-        return $this->getPropertyAnnotations($property)[$annotationName] ?? ($this->isRepeatable($annotationName) ? [] : null);
+        return $this->getPropertyAnnotations($property)[$annotationName]
+            ?? ($this->isRepeatable($annotationName) ? new RepeatableAttributeCollection() : null);
     }
 
     /**
-     * @param array<object> $attributes
+     * @param array<ReflectionAttribute> $attributes
      *
-     * @return array<Annotation>
+     * @return array<Annotation|RepeatableAttributeCollection>
      */
     private function convertToAttributeInstances(array $attributes): array
     {
         $instances = [];
 
         foreach ($attributes as $attribute) {
+            $attributeName = $attribute->getName();
+            assert(is_string($attributeName));
             // Make sure we only get Doctrine Annotations
-            if (! is_subclass_of($attribute->getName(), Annotation::class)) {
+            if (! is_subclass_of($attributeName, Annotation::class)) {
                 continue;
             }
 
             $instance = $attribute->newInstance();
+            assert($instance instanceof Annotation);
 
-            if ($this->isRepeatable($attribute->getName())) {
-                $instances[$attribute->getName()][] = $instance;
+            if ($this->isRepeatable($attributeName)) {
+                if (! isset($instances[$attributeName])) {
+                    $instances[$attributeName] = new RepeatableAttributeCollection();
+                }
+
+                $collection = $instances[$attributeName];
+                assert($collection instanceof RepeatableAttributeCollection);
+                $collection[] = $instance;
             } else {
-                $instances[$attribute->getName()] = $instance;
+                $instances[$attributeName] = $instance;
             }
         }
 

lib/Doctrine/ORM/Mapping/Driver/RepeatableAttributeCollection.php

@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Mapping\Driver;
+
+use ArrayObject;
+use Doctrine\ORM\Mapping\Annotation;
+
+/**
+ * @template-extends ArrayObject<int,Annotation>
+ */
+final class RepeatableAttributeCollection extends ArrayObject
+{
+}

lib/Doctrine/ORM/Mapping/Embedded.php

@@ -22,6 +22,7 @@ namespace Doctrine\ORM\Mapping;
 
 use Attribute;
 use Doctrine\Common\Annotations\Annotation\NamedArgumentConstructor;
+use Doctrine\Deprecations\Deprecation;
 
 /**
  * @Annotation
@@ -39,6 +40,14 @@ final class Embedded implements Annotation
 
     public function __construct(?string $class = null, $columnPrefix = null)
     {
+        if ($class === null) {
+            Deprecation::trigger(
+                'doctrine/orm',
+                'https://github.com/doctrine/orm/issues/8753',
+                'Passing no class is deprecated.'
+            );
+        }
+
         $this->class        = $class;
         $this->columnPrefix = $columnPrefix;
     }

lib/Doctrine/ORM/Mapping/ManyToMany.php

@@ -22,6 +22,7 @@ namespace Doctrine\ORM\Mapping;
 
 use Attribute;
 use Doctrine\Common\Annotations\Annotation\NamedArgumentConstructor;
+use Doctrine\Deprecations\Deprecation;
 
 /**
  * @Annotation
@@ -31,7 +32,7 @@ use Doctrine\Common\Annotations\Annotation\NamedArgumentConstructor;
 #[Attribute(Attribute::TARGET_PROPERTY)]
 final class ManyToMany implements Annotation
 {
-    /** @var string */
+    /** @var string|null */
     public $targetEntity;
 
     /** @var string */
@@ -61,7 +62,7 @@ final class ManyToMany implements Annotation
      * @param array<string> $cascade
      */
     public function __construct(
-        string $targetEntity,
+        ?string $targetEntity = null,
         ?string $mappedBy = null,
         ?string $inversedBy = null,
         ?array $cascade = null,
@@ -69,6 +70,14 @@ final class ManyToMany implements Annotation
         bool $orphanRemoval = false,
         ?string $indexBy = null
     ) {
+        if ($targetEntity === null) {
+            Deprecation::trigger(
+                'doctrine/orm',
+                'https://github.com/doctrine/orm/issues/8753',
+                'Passing no target entity is deprecated.'
+            );
+        }
+
         $this->targetEntity  = $targetEntity;
         $this->mappedBy      = $mappedBy;
         $this->inversedBy    = $inversedBy;

lib/Doctrine/ORM/Mapping/ManyToOne.php

@@ -22,6 +22,7 @@ namespace Doctrine\ORM\Mapping;
 
 use Attribute;
 use Doctrine\Common\Annotations\Annotation\NamedArgumentConstructor;
+use Doctrine\Deprecations\Deprecation;
 
 /**
  * @Annotation
@@ -57,6 +58,14 @@ final class ManyToOne implements Annotation
         string $fetch = 'LAZY',
         ?string $inversedBy = null
     ) {
+        if ($targetEntity === null) {
+            Deprecation::trigger(
+                'doctrine/orm',
+                'https://github.com/doctrine/orm/issues/8753',
+                'Passing no target entity is deprecated.'
+            );
+        }
+
         $this->targetEntity = $targetEntity;
         $this->cascade      = $cascade;
         $this->fetch        = $fetch;

lib/Doctrine/ORM/Mapping/ReflectionEmbeddedProperty.php

@@ -22,6 +22,7 @@ namespace Doctrine\ORM\Mapping;
 
 use Doctrine\Instantiator\Instantiator;
 use ReflectionProperty;
+use ReturnTypeWillChange;
 
 /**
  * Acts as a proxy to a nested Property structure, making it look like
@@ -61,6 +62,7 @@ class ReflectionEmbeddedProperty extends ReflectionProperty
     /**
      * {@inheritDoc}
      */
+    #[ReturnTypeWillChange]
     public function getValue($object = null)
     {
         $embeddedObject = $this->parentProperty->getValue($object);
@@ -75,6 +77,7 @@ class ReflectionEmbeddedProperty extends ReflectionProperty
     /**
      * {@inheritDoc}
      */
+    #[ReturnTypeWillChange]
     public function setValue($object, $value = null)
     {
         $embeddedObject = $this->parentProperty->getValue($object);

phpcs.xml.dist

@@ -136,6 +136,11 @@
         <exclude-pattern>lib/Doctrine/ORM/Cache/DefaultQueryCache.php</exclude-pattern>
     </rule>
 
+    <rule ref="SlevomatCodingStandard.Commenting.UselessInheritDocComment.UselessInheritDocComment">
+        <!-- Workaround for https://github.com/slevomat/coding-standard/issues/1233 -->
+        <exclude-pattern>lib/Doctrine/ORM/Mapping/ReflectionEmbeddedProperty.php</exclude-pattern>
+    </rule>
+
     <rule ref="SlevomatCodingStandard.Classes.SuperfluousInterfaceNaming">
         <exclude-pattern>lib/Doctrine/ORM/EntityManagerInterface.php</exclude-pattern>
     </rule>

phpstan.neon

@@ -4,7 +4,10 @@ includes:
 parameters:
     level: 5
     paths:
-        - %currentWorkingDirectory%/lib
+        - lib
+        - tests/Doctrine/StaticAnalysis
+    excludePaths:
+        - lib/Doctrine/ORM/Mapping/Driver/AttributeReader.php
     earlyTerminatingMethodCalls:
         Doctrine\ORM\Query\Parser:
             - syntaxError

psalm-baseline.xml

@@ -1218,10 +1218,6 @@
     <DocblockTypeContradiction occurrences="1">
       <code>$class</code>
     </DocblockTypeContradiction>
-    <LessSpecificReturnStatement occurrences="1">
-      <code>$mapping</code>
-    </LessSpecificReturnStatement>
-    <MoreSpecificReturnType occurrences="1"/>
     <NoInterfaceProperties occurrences="5">
       <code>$metadata-&gt;inheritanceType</code>
       <code>$metadata-&gt;isEmbeddedClass</code>
@@ -1289,10 +1285,6 @@
       <code>$value[1]</code>
     </InvalidArrayAccess>
     <InvalidScalarArgument occurrences="1"/>
-    <LessSpecificReturnStatement occurrences="1">
-      <code>$mapping</code>
-    </LessSpecificReturnStatement>
-    <MoreSpecificReturnType occurrences="1"/>
     <NonInvariantDocblockPropertyType occurrences="1">
       <code>$entityAnnotationClasses</code>
     </NonInvariantDocblockPropertyType>
@@ -1324,12 +1316,6 @@
     <ArgumentTypeCoercion occurrences="1">
       <code>$attributeClassName</code>
     </ArgumentTypeCoercion>
-    <InvalidReturnStatement occurrences="1">
-      <code>$instances</code>
-    </InvalidReturnStatement>
-    <InvalidReturnType occurrences="1">
-      <code>array&lt;Annotation&gt;</code>
-    </InvalidReturnType>
     <MissingParamType occurrences="3">
       <code>$annotationName</code>
       <code>$annotationName</code>

psalm.xml

@@ -9,8 +9,10 @@
 >
     <projectFiles>
         <directory name="lib/Doctrine/ORM" />
+        <directory name="tests/Doctrine/StaticAnalysis" />
         <ignoreFiles>
             <directory name="vendor" />
+            <file name="lib/Doctrine/ORM/Mapping/Driver/AttributeReader.php" />
         </ignoreFiles>
     </projectFiles>
     <issueHandlers>
@@ -20,5 +22,11 @@
                 <file name="lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php"/>
             </errorLevel>
         </ParadoxicalCondition>
+        <NullArgument>
+            <errorLevel type="suppress">
+                <!-- See https://github.com/vimeo/psalm/issues/5920 -->
+                <file name="lib/Doctrine/ORM/Mapping/Driver/AttributeReader.php"/>
+            </errorLevel>
+        </NullArgument>
     </issueHandlers>
 </psalm>

tests/Doctrine/StaticAnalysis/Mapping/class-metadata-constructor.php

@@ -0,0 +1,21 @@
+<?php
+
+namespace Doctrine\StaticAnalysis\Mapping;
+
+use Doctrine\ORM\Mapping\ClassMetadata;
+
+/**
+ * @template T of object
+ */
+class MetadataGenerator
+{
+    /**
+     * @psalm-param class-string<T> $entityName
+     *
+     * @psalm-return ClassMetadata<T>
+     */
+    public function createMetadata(string $entityName): ClassMetadata
+    {
+        return new ClassMetadata($entityName);
+    }
+}

tests/Doctrine/Tests/ORM/Mapping/AttributeDriverTest.php

@@ -4,9 +4,12 @@ declare(strict_types=1);
 
 namespace Doctrine\Tests\ORM\Mapping;
 
+use Attribute;
 use Doctrine\ORM\Mapping as ORM;
+use Doctrine\ORM\Mapping\Annotation;
 use Doctrine\ORM\Mapping\Driver\AttributeDriver;
 use Doctrine\Persistence\Mapping\Driver\MappingDriver;
+use stdClass;
 
 use const PHP_VERSION_ID;
 
@@ -16,7 +19,7 @@ class AttributeDriverTest extends AbstractMappingDriverTest
     public function requiresPhp8Assertion(): void
     {
         if (PHP_VERSION_ID < 80000) {
-            $this->markTestSkipped('requies PHP 8.0');
+            $this->markTestSkipped('requires PHP 8.0');
         }
     }
 
@@ -78,6 +81,19 @@ class AttributeDriverTest extends AbstractMappingDriverTest
         );
         $this->assertEquals(['assoz_id', 'assoz_id'], $metadata->associationMappings['assoc']['joinTableColumns']);
     }
+
+    public function testIsTransient(): void
+    {
+        $driver = $this->loadDriver();
+
+        $this->assertTrue($driver->isTransient(stdClass::class));
+
+        $this->assertTrue($driver->isTransient(AttributeTransientClass::class));
+
+        $this->assertFalse($driver->isTransient(AttributeEntityWithoutOriginalParents::class));
+
+        $this->assertFalse($driver->isTransient(AttributeEntityStartingWithRepeatableAttributes::class));
+    }
 }
 
 #[ORM\Entity]
@@ -95,3 +111,20 @@ class AttributeEntityWithoutOriginalParents
     /** @var AttributeEntityWithoutOriginalParents[] */
     public $assoc;
 }
+
+#[ORM\Index(name: 'bar', columns: ['id'])]
+#[ORM\Index(name: 'baz', columns: ['id'])]
+#[ORM\Entity]
+class AttributeEntityStartingWithRepeatableAttributes
+{
+}
+
+#[Attribute(Attribute::IS_REPEATABLE | Attribute::TARGET_ALL)]
+class AttributeTransientAnnotation implements Annotation
+{
+}
+
+#[AttributeTransientAnnotation]
+class AttributeTransientClass
+{
+}