Merge pull request #2324 from franmomu/make_persist_collection_generic

Make `PersistentCollectionInterface` and `PersistentCollection` generic

Author: malarzm

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

@@ -85,7 +85,8 @@
  *      version?: bool,
  *      lock?: bool,
  *      inherited?: string,
- *      declared?: class-string
+ *      declared?: class-string,
+ *      prime?: list<string>
  * }
  * @psalm-type FieldMapping = array{
  *      type: string,
@@ -99,7 +100,6 @@
  *      isOwningSide: bool,
  *      isInverseSide: bool,
  *      strategy?: string,
- *      notSaved?: bool,
  *      association?: int,
  *      id?: bool,
  *      collectionClass?: class-string,
@@ -124,7 +124,8 @@
  *      lock?: bool,
  *      notSaved?: bool,
  *      inherited?: string,
- *      declared?: class-string
+ *      declared?: class-string,
+ *      prime?: list<string>
  * }
  * @psalm-type AssociationFieldMapping = array{
  *      type: string,
@@ -162,7 +163,8 @@
  *      lock?: bool,
  *      notSaved?: bool,
  *      inherited?: string,
- *      declared?: class-string
+ *      declared?: class-string,
+ *      prime?: list<string>
  * }
  * @psalm-type IndexKeys = array<string, mixed>
  * @psalm-type IndexOptions = array<string, mixed>

lib/Doctrine/ODM/MongoDB/PersistentCollection.php

@@ -10,13 +10,18 @@
 
 /**
  * A PersistentCollection represents a collection of elements that have persistent state.
+ *
+ * @template TKey of array-key
+ * @template T of object
+ * @template-implements PersistentCollectionInterface<TKey,T>
  */
 final class PersistentCollection implements PersistentCollectionInterface
 {
+    /** @use PersistentCollectionTrait<TKey, T> */
     use PersistentCollectionTrait;
 
     /**
-     * @param BaseCollection $coll
+     * @param BaseCollection<TKey, T> $coll
      */
     public function __construct(BaseCollection $coll, DocumentManager $dm, UnitOfWork $uow)
     {

lib/Doctrine/ODM/MongoDB/PersistentCollection/PersistentCollectionInterface.php

@@ -13,18 +13,28 @@
  * Interface for persistent collection classes.
  *
  * @internal
+ *
+ * @psalm-import-type FieldMapping from \Doctrine\ODM\MongoDB\Mapping\ClassMetadata
+ *
+ * @template TKey of array-key
+ * @template T of object
+ * @template-extends Collection<TKey, T>
  */
 interface PersistentCollectionInterface extends Collection
 {
     /**
      * Sets the document manager and unit of work (used during merge operations).
+     *
+     * @return void
      */
     public function setDocumentManager(DocumentManager $dm);
 
     /**
      * Sets the array of raw mongo data that will be used to initialize this collection.
      *
      * @param array $mongoData
+     *
+     * @return void
      */
     public function setMongoData(array $mongoData);
 
@@ -39,6 +49,8 @@ public function getMongoData();
      * Set hints to account for during reconstitution/lookup of the documents.
      *
      * @param array $hints
+     *
+     * @return void
      */
     public function setHints(array $hints);
 
@@ -52,6 +64,8 @@ public function getHints();
     /**
      * Initializes the collection by loading its contents from the database
      * if the collection is not yet initialized.
+     *
+     * @return void
      */
     public function initialize();
 
@@ -67,12 +81,18 @@ public function isDirty();
      * Sets a boolean flag, indicating whether this collection is dirty.
      *
      * @param bool $dirty Whether the collection should be marked dirty or not.
+     *
+     * @return void
      */
     public function setDirty($dirty);
 
     /**
-     * Sets the collection's owning entity together with the AssociationMapping that
+     * Sets the collection's owning document together with the AssociationMapping that
      * describes the association between the owner and the elements of the collection.
+     *
+     * @psalm-param FieldMapping $mapping
+     *
+     * @return void
      */
     public function setOwner(object $document, array $mapping);
 
@@ -81,12 +101,16 @@ public function setOwner(object $document, array $mapping);
      * itself numerically if using save strategy that is enforcing BSON array.
      * Reindexing is safe as snapshot is taken only after synchronizing collection
      * with database or clearing it.
+     *
+     * @return void
      */
     public function takeSnapshot();
 
     /**
      * Clears the internal snapshot information and sets isDirty to true if the collection
      * has elements.
+     *
+     * @return void
      */
     public function clearSnapshot();
 
@@ -128,11 +152,13 @@ public function getOwner(): ?object;
 
     /**
      * @return array
+     * @psalm-return FieldMapping
      */
     public function getMapping();
 
     /**
      * @return ClassMetadata
+     * @psalm-return ClassMetadata<T>
      *
      * @throws MongoDBException
      */
@@ -142,6 +168,8 @@ public function getTypeClass();
      * Sets the initialized flag of the collection, forcing it into that state.
      *
      * @param bool $bool
+     *
+     * @return void
      */
     public function setInitialized($bool);
 
@@ -155,7 +183,7 @@ public function isInitialized();
     /**
      * Returns the wrapped Collection instance.
      *
-     * @return Collection
+     * @return Collection<TKey, T>
      */
     public function unwrap();
 }

lib/Doctrine/ODM/MongoDB/PersistentCollection/PersistentCollectionTrait.php

@@ -7,6 +7,7 @@
 use Closure;
 use Doctrine\Common\Collections\Collection as BaseCollection;
 use Doctrine\ODM\MongoDB\DocumentManager;
+use Doctrine\ODM\MongoDB\Mapping\ClassMetadata;
 use Doctrine\ODM\MongoDB\MongoDBException;
 use Doctrine\ODM\MongoDB\UnitOfWork;
 use Doctrine\ODM\MongoDB\Utility\CollectionHelper;
@@ -21,25 +22,32 @@
 
 /**
  * Trait with methods needed to implement PersistentCollectionInterface.
+ *
+ * @psalm-import-type FieldMapping from ClassMetadata
+ * @template TKey of array-key
+ * @template T of object
  */
 trait PersistentCollectionTrait
 {
     /**
      * A snapshot of the collection at the moment it was fetched from the database.
      * This is used to create a diff of the collection at commit time.
      *
-     * @var array
+     * @var array<TKey, T>
      */
     private $snapshot = [];
 
     /**
-     * Collection's owning entity
+     * Collection's owning document
      *
      * @var object|null
      */
     private $owner;
 
-    /** @var array|null */
+    /**
+     * @var array|null
+     * @psalm-var FieldMapping|null
+     */
     private $mapping;
 
     /**
@@ -60,7 +68,7 @@ trait PersistentCollectionTrait
     /**
      * The wrapped Collection instance.
      *
-     * @var BaseCollection
+     * @var BaseCollection<TKey, T>
      */
     private $coll;
 
@@ -130,6 +138,7 @@ public function initialize()
             return;
         }
 
+        /** @psalm-var array<TKey, T> $newObjects */
         $newObjects = [];
 
         if ($this->isDirty) {
@@ -164,7 +173,7 @@ public function initialize()
     /**
      * Marks this collection as changed/dirty.
      */
-    private function changed()
+    private function changed(): void
     {
         if ($this->isDirty) {
             return;
@@ -681,7 +690,7 @@ public function unwrap()
      * 2. New collection is not dirty, if reused on other document nothing
      * changes.
      * 3. Snapshot leads to invalid diffs being generated.
-     * 4. Lazy loading grabs entities from old owner object.
+     * 4. Lazy loading grabs documents from old owner object.
      * 5. New collection is connected to old owner and leads to duplicate keys.
      */
     public function __clone()
@@ -730,11 +739,10 @@ private function doAdd($value, $arrayAccess)
      * Actual logic for removing element by its key.
      *
      * @param mixed $offset
-     * @param bool  $arrayAccess
      *
-     * @return bool
+     * @return bool|T|null
      */
-    private function doRemove($offset, $arrayAccess)
+    private function doRemove($offset, bool $arrayAccess)
     {
         $this->initialize();
         if ($arrayAccess) {
@@ -758,9 +766,8 @@ private function doRemove($offset, $arrayAccess)
      *
      * @param mixed $offset
      * @param mixed $value
-     * @param bool  $arrayAccess
      */
-    private function doSet($offset, $value, $arrayAccess)
+    private function doSet($offset, $value, bool $arrayAccess): void
     {
         $arrayAccess ? $this->coll->offsetSet($offset, $value) : $this->coll->set($offset, $value);
 
@@ -777,10 +784,8 @@ private function doSet($offset, $value, $arrayAccess)
      *
      * Embedded documents are automatically considered as "orphan removal enabled" because they might have references
      * that require to trigger cascade remove operations.
-     *
-     * @return bool
      */
-    private function isOrphanRemovalEnabled()
+    private function isOrphanRemovalEnabled(): bool
     {
         if ($this->mapping === null) {
             return false;
@@ -795,10 +800,8 @@ private function isOrphanRemovalEnabled()
 
     /**
      * Checks whether collection owner needs to be scheduled for dirty change in case the collection is modified.
-     *
-     * @return bool
      */
-    private function needsSchedulingForSynchronization()
+    private function needsSchedulingForSynchronization(): bool
     {
         return $this->owner && $this->dm && ! empty($this->mapping['isOwningSide'])
             && $this->dm->getClassMetadata(get_class($this->owner))->isChangeTrackingNotify();

phpstan-baseline.neon

@@ -228,3 +228,27 @@ parameters:
             message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\Tests\\\\SchemaManagerTest\\:\\:writeOptions\\(\\) should return PHPUnit\\\\Framework\\\\Constraint\\\\Constraint but returns PHPUnit\\\\Framework\\\\Constraint\\\\ArraySubset\\.$#"
             count: 1
             path: tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php
+
+        # import type in PHPStan does not work, see https://github.com/phpstan/phpstan/issues/5091
+        -
+            message: "#^Access to offset '.+' on an unknown class Doctrine\\\\ODM\\\\MongoDB\\\\PersistentCollection\\\\FieldMapping\\.$#"
+            count: 12
+            path: lib/Doctrine/ODM/MongoDB/PersistentCollection.php
+
+        # import type in PHPStan does not work, see https://github.com/phpstan/phpstan/issues/5091
+        -
+            message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\PersistentCollection\\:\\:getMapping\\(\\) should return array\\('type' \\=\\> string, 'fieldName' \\=\\> string, 'name' \\=\\> string, 'isCascadeRemove' \\=\\> bool, 'isCascadePersist' \\=\\> bool, 'isCascadeRefresh' \\=\\> bool, 'isCascadeMerge' \\=\\> bool, 'isCascadeDetach' \\=\\> bool, \\.\\.\\.\\) but returns Doctrine\\\\ODM\\\\MongoDB\\\\PersistentCollection\\\\FieldMapping\\|null\\.$#"
+            count: 1
+            path: lib/Doctrine/ODM/MongoDB/PersistentCollection.php
+
+        # import type in PHPStan does not work, see https://github.com/phpstan/phpstan/issues/5091
+        -
+            message: "#^Property Doctrine\\\\ODM\\\\MongoDB\\\\PersistentCollection\\:\\:\\$mapping has unknown class Doctrine\\\\ODM\\\\MongoDB\\\\PersistentCollection\\\\FieldMapping as its type\\.$#"
+            count: 1
+            path: lib/Doctrine/ODM/MongoDB/PersistentCollection.php
+
+        # import type in PHPStan does not work, see https://github.com/phpstan/phpstan/issues/5091
+        -
+            message: "#^Property Doctrine\\\\ODM\\\\MongoDB\\\\PersistentCollection\\<TKey of \\(int\\|string\\),T of object\\>\\:\\:\\$mapping \\(Doctrine\\\\ODM\\\\MongoDB\\\\PersistentCollection\\\\FieldMapping\\|null\\) does not accept array\\<string, array\\<int\\|string, mixed\\>\\|bool\\|int\\|string\\|null\\>\\.$#"
+            count: 1
+            path: lib/Doctrine/ODM/MongoDB/PersistentCollection.php

tests/Doctrine/ODM/MongoDB/Tests/PersistentCollectionTest.php

@@ -42,7 +42,19 @@ public function testExceptionForGetTypeClassWithoutDocumentManager()
         $unserialized = unserialize($serialized);
         assert($unserialized instanceof PersistentCollection);
 
-        $unserialized->setOwner($owner, ['targetDocument' => stdClass::class]);
+        $unserialized->setOwner($owner, [
+            'type' => ClassMetadata::ONE,
+            'name' => 'name',
+            'fieldName' => 'fieldName',
+            'isCascadeRemove' => false,
+            'isCascadePersist' => false,
+            'isCascadeRefresh' => false,
+            'isCascadeMerge' => false,
+            'isCascadeDetach' => false,
+            'isOwningSide' => false,
+            'isInverseSide' => false,
+            'targetDocument' => stdClass::class,
+        ]);
         $this->expectException(MongoDBException::class);
         $this->expectExceptionMessage(
             'No DocumentManager is associated with this PersistentCollection, ' .
@@ -274,9 +286,10 @@ public function testOffsetExistsIsForwarded()
     public function testOffsetGetIsForwarded()
     {
         $collection = $this->getMockCollection();
-        $collection->expects($this->once())->method('offsetGet')->willReturn(2);
+        $object     = new stdClass();
+        $collection->expects($this->once())->method('offsetGet')->willReturn($object);
         $pcoll = new PersistentCollection($collection, $this->dm, $this->uow);
-        $this->assertSame(2, $pcoll[0]);
+        $this->assertSame($object, $pcoll[0]);
     }
 
     public function testOffsetUnsetIsForwarded()
@@ -302,12 +315,12 @@ public function testOffsetSetIsForwarded()
         $collection = $this->getMockCollection();
         $collection->expects($this->exactly(2))->method('offsetSet');
         $pcoll    = new PersistentCollection($collection, $this->dm, $this->uow);
-        $pcoll[]  = 1;
-        $pcoll[1] = 2;
+        $pcoll[]  = new stdClass();
+        $pcoll[1] = new stdClass();
         $collection->expects($this->once())->method('add');
-        $pcoll->add(3);
+        $pcoll->add(new stdClass());
         $collection->expects($this->once())->method('set');
-        $pcoll->set(3, 4);
+        $pcoll->set(3, new stdClass());
     }
 
     public function testIsEmptyIsForwardedWhenCollectionIsInitialized()