Make indexBy/orderBy easier to understand for SA (doctrine#10687)

Interfaces cannot have properties, and we do not have a concept of sealed classes available to us without installing third party packages. Interfaces can have methods however, which allows us to simplify calling code. I've been avoiding introducing getters for mapping properties because I do not know what the performance implications are, but here, I think it is sensible to make an exception, given the benefits.
derrabus · May 8, 2023 · 05b5a64 · 05b5a64
1 parent 05678dc
commit 05b5a64
Show file tree

Hide file tree

Showing 18 changed files with 126 additions and 41 deletions.
diff --git a/lib/Doctrine/ORM/Cache/Persister/Collection/AbstractCollectionPersister.php b/lib/Doctrine/ORM/Cache/Persister/Collection/AbstractCollectionPersister.php
@@ -92,7 +92,7 @@ public function storeCollectionCache(CollectionCacheKey $key, Collection|array $
         $targetHydrator = $targetPersister->getEntityHydrator();
 
         // Only preserve ordering if association configured it
-        if (! (isset($associationMapping['indexBy']) && $associationMapping['indexBy'])) {
+        if (! $associationMapping->isIndexed()) {
             // Elements may be an array or a Collection
             $elements = array_values($elements instanceof Collection ? $elements->getValues() : $elements);
         }

diff --git a/lib/Doctrine/ORM/Cache/Persister/Collection/NonStrictReadWriteCachedCollectionPersister.php b/lib/Doctrine/ORM/Cache/Persister/Collection/NonStrictReadWriteCachedCollectionPersister.php
@@ -56,7 +56,7 @@ public function update(PersistentCollection $collection): void
         $key     = new CollectionCacheKey($this->sourceEntity->rootEntityName, $this->association['fieldName'], $ownerId);
 
        // Invalidate non initialized collections OR ordered collection
-        if ($isDirty && ! $isInitialized || isset($this->association['orderBy'])) {
+        if ($isDirty && ! $isInitialized || $this->association->isOrdered()) {
             $this->persister->update($collection);
 
             $this->queuedCache['delete'][spl_object_id($collection)] = $key;

diff --git a/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php b/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -14,6 +14,7 @@
 
 use function array_fill_keys;
 use function array_keys;
+use function assert;
 use function count;
 use function is_array;
 use function key;
@@ -177,6 +178,7 @@ private function initRelatedCollection(
         }
 
         if (! $value instanceof PersistentCollection) {
+            assert($relation->isToMany());
             $value = new PersistentCollection(
                 $this->em,
                 $this->metadataCache[$relation['targetEntity']],

diff --git a/lib/Doctrine/ORM/Mapping/AssociationMapping.php b/lib/Doctrine/ORM/Mapping/AssociationMapping.php
@@ -205,6 +205,18 @@ final public function isManyToMany(): bool
         return $this instanceof ManyToManyAssociationMapping;
     }
 
+    /** @psalm-assert-if-true ToManyAssociationMapping $this */
+    final public function isOrdered(): bool
+    {
+        return $this->isToMany() && $this->orderBy() !== [];
+    }
+
+    /** @psalm-assert-if-true ToManyAssociationMapping $this */
+    public function isIndexed(): bool
+    {
+        return false;
+    }
+
     final public function type(): int
     {
         return match (true) {

diff --git a/lib/Doctrine/ORM/Mapping/ToManyAssociationMapping.php b/lib/Doctrine/ORM/Mapping/ToManyAssociationMapping.php
@@ -6,4 +6,11 @@
 
 interface ToManyAssociationMapping
 {
+    /** @psalm-assert-if-true string $this->indexBy() */
+    public function isIndexed(): bool;
+
+    public function indexBy(): string;
+
+    /** @return array<string, 'asc'|'desc'> */
+    public function orderBy(): array;
 }
diff --git a/lib/Doctrine/ORM/Mapping/ToManyAssociationMappingImplementation.php b/lib/Doctrine/ORM/Mapping/ToManyAssociationMappingImplementation.php
@@ -4,6 +4,10 @@
 
 namespace Doctrine\ORM\Mapping;
 
+use LogicException;
+
+use function sprintf;
+
 /** @internal */
 trait ToManyAssociationMappingImplementation
 {
@@ -18,19 +22,46 @@ trait ToManyAssociationMappingImplementation
     /**
      * A map of field names (of the target entity) to sorting directions
      *
-     * @var array<string, 'asc'|'desc'>|null
+     * @var array<string, 'asc'|'desc'>
      */
-    public array|null $orderBy = null;
+    public array $orderBy = [];
+
+    /** @return array<string, 'asc'|'desc'> */
+    final public function orderBy(): array
+    {
+        return $this->orderBy;
+    }
+
+    /** @psalm-assert-if-true !null $this->indexBy */
+    final public function isIndexed(): bool
+    {
+        return $this->indexBy !== null;
+    }
+
+    final public function indexBy(): string
+    {
+        if (! $this->isIndexed()) {
+            throw new LogicException(sprintf(
+                'This mapping is not indexed. Use %s::isIndexed() to check that before calling %s.',
+                self::class,
+                __METHOD__,
+            ));
+        }
+
+        return $this->indexBy;
+    }
 
     /** @return list<string> */
     public function __sleep(): array
     {
         $serialized = parent::__sleep();
 
-        foreach (['indexBy', 'orderBy'] as $stringOrArrayKey) {
-            if ($this->$stringOrArrayKey !== null) {
-                $serialized[] = $stringOrArrayKey;
-            }
+        if ($this->indexBy !== null) {
+            $serialized[] = 'indexBy';
+        }
+
+        if ($this->orderBy !== []) {
+            $serialized[] = 'orderBy';
         }
 
         return $serialized;

diff --git a/lib/Doctrine/ORM/PersistentCollection.php b/lib/Doctrine/ORM/PersistentCollection.php
@@ -12,6 +12,7 @@
 use Doctrine\ORM\Mapping\AssociationMapping;
 use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\Mapping\ManyToManyOwningSideMapping;
+use Doctrine\ORM\Mapping\ToManyAssociationMapping;
 use RuntimeException;
 use UnexpectedValueException;
 
@@ -56,6 +57,8 @@ final class PersistentCollection extends AbstractLazyCollection implements Selec
     /**
      * The association mapping the collection belongs to.
      * This is currently either a OneToManyMapping or a ManyToManyMapping.
+     *
+     * @var (AssociationMapping&ToManyAssociationMapping)|null
      */
     private AssociationMapping|null $association = null;
 
@@ -98,7 +101,7 @@ public function __construct(
      * Sets the collection's owning entity together with the AssociationMapping that
      * describes the association between the owner and the elements of the collection.
      */
-    public function setOwner(object $entity, AssociationMapping $assoc): void
+    public function setOwner(object $entity, AssociationMapping&ToManyAssociationMapping $assoc): void
     {
         $this->owner            = $entity;
         $this->association      = $assoc;
@@ -245,7 +248,7 @@ public function getInsertDiff(): array
     }
 
     /** INTERNAL: Gets the association mapping of the collection. */
-    public function getMapping(): AssociationMapping
+    public function getMapping(): AssociationMapping&ToManyAssociationMapping
     {
         if ($this->association === null) {
             throw new UnexpectedValueException('The underlying association mapping is null although it should not be');
@@ -600,7 +603,7 @@ public function matching(Criteria $criteria): Collection
 
         $criteria = clone $criteria;
         $criteria->where($expression);
-        $criteria->orderBy($criteria->getOrderings() ?: $association['orderBy'] ?? []);
+        $criteria->orderBy($criteria->getOrderings() ?: $association->orderBy());
 
         $persister = $this->getUnitOfWork()->getEntityPersister($association['targetEntity']);
 

diff --git a/lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php b/lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php
@@ -83,7 +83,7 @@ public function get(PersistentCollection $collection, mixed $index): object|null
     {
         $mapping = $collection->getMapping();
 
-        if (! isset($mapping->indexBy)) {
+        if (! $mapping->isIndexed()) {
             throw new BadMethodCallException('Selecting a collection by index is only supported on indexed collections.');
         }
 
@@ -95,7 +95,7 @@ public function get(PersistentCollection $collection, mixed $index): object|null
         assert($mappedKey !== null);
 
         return $persister->load(
-            [$mappedKey => $collection->getOwner(), $mapping->indexBy => $index],
+            [$mappedKey => $collection->getOwner(), $mapping->indexBy() => $index],
             null,
             $mapping,
             [],
@@ -177,7 +177,7 @@ public function containsKey(PersistentCollection $collection, mixed $key): bool
     {
         $mapping = $collection->getMapping();
 
-        if (! isset($mapping->indexBy)) {
+        if (! $mapping->isIndexed()) {
             throw new BadMethodCallException('Selecting a collection by index is only supported on indexed collections.');
         }
 
@@ -574,11 +574,10 @@ private function getJoinTableRestrictionsWithKey(
     ): array {
         $filterMapping = $collection->getMapping();
         $mapping       = $filterMapping;
-        assert(isset($mapping->indexBy));
-        $indexBy     = $mapping->indexBy;
-        $id          = $this->uow->getEntityIdentifier($collection->getOwner());
-        $sourceClass = $this->em->getClassMetadata($mapping->sourceEntity);
-        $targetClass = $this->em->getClassMetadata($mapping->targetEntity);
+        $indexBy       = $mapping->indexBy();
+        $id            = $this->uow->getEntityIdentifier($collection->getOwner());
+        $sourceClass   = $this->em->getClassMetadata($mapping->sourceEntity);
+        $targetClass   = $this->em->getClassMetadata($mapping->targetEntity);
 
         if (! $mapping->isOwningSide()) {
             assert($mapping instanceof InverseSideMapping);

diff --git a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php
@@ -54,17 +54,18 @@ public function update(PersistentCollection $collection): void
     public function get(PersistentCollection $collection, mixed $index): object|null
     {
         $mapping = $collection->getMapping();
+        assert($mapping->isOneToMany());
 
-        if (! isset($mapping['indexBy'])) {
+        if (! $mapping->isIndexed()) {
             throw new BadMethodCallException('Selecting a collection by index is only supported on indexed collections.');
         }
 
         $persister = $this->uow->getEntityPersister($mapping['targetEntity']);
 
         return $persister->load(
             [
-                $mapping['mappedBy'] => $collection->getOwner(),
-                $mapping['indexBy']  => $index,
+                $mapping->mappedBy  => $collection->getOwner(),
+                $mapping->indexBy() => $index,
             ],
             null,
             $mapping,
@@ -102,7 +103,7 @@ public function containsKey(PersistentCollection $collection, mixed $key): bool
     {
         $mapping = $collection->getMapping();
 
-        if (! isset($mapping['indexBy'])) {
+        if (! $mapping->isIndexed()) {
             throw new BadMethodCallException('Selecting a collection by index is only supported on indexed collections.');
         }
 
@@ -113,8 +114,8 @@ public function containsKey(PersistentCollection $collection, mixed $key): bool
         // 'mappedBy' field.
         $criteria = new Criteria();
 
-        $criteria->andWhere(Criteria::expr()->eq($mapping['mappedBy'], $collection->getOwner()));
-        $criteria->andWhere(Criteria::expr()->eq($mapping['indexBy'], $key));
+        $criteria->andWhere(Criteria::expr()->eq($mapping->mappedBy, $collection->getOwner()));
+        $criteria->andWhere(Criteria::expr()->eq($mapping->indexBy(), $key));
 
         return (bool) $persister->count($criteria);
     }

diff --git a/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php b/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php
@@ -918,9 +918,9 @@ private function loadArrayFromResult(AssociationMapping $assoc, Result $stmt): a
         $rsm   = $this->currentPersisterContext->rsm;
         $hints = [UnitOfWork::HINT_DEFEREAGERLOAD => true];
 
-        if (isset($assoc->indexBy)) {
+        if ($assoc->isIndexed()) {
             $rsm = clone $this->currentPersisterContext->rsm; // this is necessary because the "default rsm" should be changed.
-            $rsm->addIndexBy('r', $assoc->indexBy);
+            $rsm->addIndexBy('r', $assoc->indexBy());
         }
 
         return $this->em->newHydrator(Query::HYDRATE_OBJECT)->hydrateAll($stmt, $rsm, $hints);
@@ -942,9 +942,9 @@ private function loadCollectionFromStatement(
             'collection' => $coll,
         ];
 
-        if (isset($assoc->indexBy)) {
+        if ($assoc->isIndexed()) {
             $rsm = clone $this->currentPersisterContext->rsm; // this is necessary because the "default rsm" should be changed.
-            $rsm->addIndexBy('r', $assoc->indexBy);
+            $rsm->addIndexBy('r', $assoc->indexBy());
         }
 
         return $this->em->newHydrator(Query::HYDRATE_OBJECT)->hydrateAll($stmt, $rsm, $hints);
@@ -1049,8 +1049,8 @@ public function getSelectSQL(
             $joinSql = $this->getSelectManyToManyJoinSQL($assoc);
         }
 
-        if ($assoc !== null && isset($assoc->orderBy)) {
-            $orderBy = $assoc->orderBy;
+        if ($assoc !== null && $assoc->isOrdered()) {
+            $orderBy = $assoc->orderBy();
         }
 
         if ($orderBy) {
@@ -1244,8 +1244,8 @@ protected function getSelectColumnsSQL(): string
             $association   = $assoc;
             $joinCondition = [];
 
-            if (isset($assoc->indexBy)) {
-                $this->currentPersisterContext->rsm->addIndexBy($assocAlias, $assoc->indexBy);
+            if ($assoc->isIndexed()) {
+                $this->currentPersisterContext->rsm->addIndexBy($assocAlias, $assoc->indexBy());
             }
 
             if (! $assoc->isOwningSide()) {

diff --git a/lib/Doctrine/ORM/Persisters/Entity/JoinedSubclassPersister.php b/lib/Doctrine/ORM/Persisters/Entity/JoinedSubclassPersister.php
@@ -271,8 +271,8 @@ public function getSelectSQL(
 
         $orderBySql = '';
 
-        if ($assoc !== null && isset($assoc['orderBy'])) {
-            $orderBy = $assoc['orderBy'];
+        if ($assoc !== null && $assoc->isOrdered()) {
+            $orderBy = $assoc->orderBy();
         }
 
         if ($orderBy) {

diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php
@@ -1027,8 +1027,8 @@ public function walkJoinAssociationDeclaration(
         if ($indexBy) {
             // For Many-To-One or One-To-One associations this obviously makes no sense, but is ignored silently.
             $this->walkIndexBy($indexBy);
-        } elseif (isset($relation['indexBy'])) {
-            $this->rsm->addIndexBy($joinedDqlAlias, $relation['indexBy']);
+        } elseif ($relation->isIndexed()) {
+            $this->rsm->addIndexBy($joinedDqlAlias, $relation->indexBy());
         }
 
         return $sql;

diff --git a/lib/Doctrine/ORM/Tools/SchemaValidator.php b/lib/Doctrine/ORM/Tools/SchemaValidator.php
@@ -214,8 +214,8 @@ public function validateClass(ClassMetadata $class): array
                 }
             }
 
-            if (isset($assoc['orderBy']) && $assoc['orderBy'] !== null) {
-                foreach ($assoc['orderBy'] as $orderField => $orientation) {
+            if ($assoc->isOrdered()) {
+                foreach ($assoc->orderBy() as $orderField => $orientation) {
                     if (! $targetMetadata->hasField($orderField) && ! $targetMetadata->hasAssociation($orderField)) {
                         $ce[] = 'The association ' . $class->name . '#' . $fieldName . ' is ordered by a foreign field ' .
                                 $orderField . ' that is not a field on the target entity ' . $targetMetadata->name . '.';

diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php
@@ -575,6 +575,7 @@ public function computeChangeSet(ClassMetadata $class, object $entity): void
                 }
 
                 $assoc = $class->associationMappings[$name];
+                assert($assoc->isToMany());
 
                 // Inject PersistentCollection
                 $value = new PersistentCollection(
@@ -671,6 +672,7 @@ public function computeChangeSet(ClassMetadata $class, object $entity): void
                 // created one. This can only mean it was cloned and replaced
                 // on another entity.
                 if ($actualValue instanceof PersistentCollection) {
+                    assert($assoc->isToMany());
                     $owner = $actualValue->getOwner();
                     if ($owner === null) { // cloned
                         $actualValue->setOwner($entity, $assoc);
@@ -2433,6 +2435,7 @@ public function createEntity(string $className, array $data, array &$hints = [])
                     break;
 
                 default:
+                    assert($assoc->isToMany());
                     // Ignore if its a cached collection
                     if (isset($hints[Query::HINT_CACHE_ENABLED]) && $class->getFieldValue($entity, $field) instanceof PersistentCollection) {
                         break;

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -125,6 +125,11 @@ parameters:
 			count: 1
 			path: lib/Doctrine/ORM/EntityRepository.php
 
+		-
+			message: "#^Parameter \\#2 \\$assoc of method Doctrine\\\\ORM\\\\PersistentCollection\\<\\(int\\|string\\),mixed\\>\\:\\:setOwner\\(\\) expects Doctrine\\\\ORM\\\\Mapping\\\\AssociationMapping&Doctrine\\\\ORM\\\\Mapping\\\\ToManyAssociationMapping, Doctrine\\\\ORM\\\\Mapping\\\\AssociationMapping given\\.$#"
+			count: 1
+			path: lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
+
 		-
 			message: "#^Method Doctrine\\\\ORM\\\\Mapping\\\\ClassMetadata\\:\\:fullyQualifiedClassName\\(\\) should return class\\-string\\|null but returns string\\|null\\.$#"
 			count: 1
@@ -300,6 +305,16 @@ parameters:
 			count: 1
 			path: lib/Doctrine/ORM/Tools/SchemaTool.php
 
+		-
+			message: "#^Parameter \\#2 \\$assoc of method Doctrine\\\\ORM\\\\PersistentCollection\\<\\(int\\|string\\),mixed\\>\\:\\:setOwner\\(\\) expects Doctrine\\\\ORM\\\\Mapping\\\\AssociationMapping&Doctrine\\\\ORM\\\\Mapping\\\\ToManyAssociationMapping, Doctrine\\\\ORM\\\\Mapping\\\\AssociationMapping given\\.$#"
+			count: 4
+			path: lib/Doctrine/ORM/UnitOfWork.php
+
+		-
+			message: "#^Parameter \\#2 \\$assoc of method Doctrine\\\\ORM\\\\PersistentCollection\\<\\*NEVER\\*,\\*NEVER\\*\\>\\:\\:setOwner\\(\\) expects Doctrine\\\\ORM\\\\Mapping\\\\AssociationMapping&Doctrine\\\\ORM\\\\Mapping\\\\ToManyAssociationMapping, Doctrine\\\\ORM\\\\Mapping\\\\AssociationMapping given\\.$#"
+			count: 1
+			path: lib/Doctrine/ORM/UnitOfWork.php
+
 		-
 			message: "#^Parameter \\#3 \\$changeSet of class Doctrine\\\\ORM\\\\Event\\\\PreUpdateEventArgs constructor is passed by reference, so it expects variables only$#"
 			count: 1