diff --git a/UPGRADE.md b/UPGRADE.md index bf2d9b385db..ef703efa09d 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,12 @@ # Upgrade to 2.16 +## Deprecated `\Doctrine\ORM\Internal\CommitOrderCalculator` and related classes + +With changes made to the commit order computation, the internal classes +`\Doctrine\ORM\Internal\CommitOrderCalculator`, `\Doctrine\ORM\Internal\CommitOrder\Edge`, +`\Doctrine\ORM\Internal\CommitOrder\Vertex` and `\Doctrine\ORM\Internal\CommitOrder\VertexState` +have been deprecated and will be removed in ORM 3.0. + ## Deprecated returning post insert IDs from `EntityPersister::executeInserts()` Persisters implementing `\Doctrine\ORM\Persisters\Entity\EntityPersister` should no longer diff --git a/docs/en/reference/events.rst b/docs/en/reference/events.rst index c2dfcecc9fc..0307429b864 100644 --- a/docs/en/reference/events.rst +++ b/docs/en/reference/events.rst @@ -707,8 +707,8 @@ not directly mapped by Doctrine. ``UPDATE`` statement. - The ``postPersist`` event occurs for an entity after the entity has been made persistent. It will be invoked after the - database insert operations. Generated primary key values are - available in the postPersist event. + database insert operation for that entity. A generated primary key value for + the entity will be available in the postPersist event. - The ``postRemove`` event occurs for an entity after the entity has been deleted. It will be invoked after the database delete operations. It is not called for a DQL ``DELETE`` statement. diff --git a/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php b/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php index 2878db50e07..2ea4c132ba8 100644 --- a/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php +++ b/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php @@ -23,6 +23,7 @@ use Doctrine\ORM\Persisters\Entity\EntityPersister; use Doctrine\ORM\UnitOfWork; +use function array_merge; use function assert; use function serialize; use function sha1; @@ -314,7 +315,13 @@ public function getOwningTable($fieldName) */ public function executeInserts() { - $this->queuedCache['insert'] = $this->persister->getInserts(); + // The commit order/foreign key relationships may make it necessary that multiple calls to executeInsert() + // are performed, so collect all the new entities. + $newInserts = $this->persister->getInserts(); + + if ($newInserts) { + $this->queuedCache['insert'] = array_merge($this->queuedCache['insert'] ?? [], $newInserts); + } return $this->persister->executeInserts(); } diff --git a/lib/Doctrine/ORM/Internal/CommitOrder/Edge.php b/lib/Doctrine/ORM/Internal/CommitOrder/Edge.php index f1457755ee1..98bb7378955 100644 --- a/lib/Doctrine/ORM/Internal/CommitOrder/Edge.php +++ b/lib/Doctrine/ORM/Internal/CommitOrder/Edge.php @@ -4,7 +4,12 @@ namespace Doctrine\ORM\Internal\CommitOrder; -/** @internal */ +use Doctrine\Deprecations\Deprecation; + +/** + * @internal + * @deprecated + */ final class Edge { /** @@ -27,6 +32,13 @@ final class Edge public function __construct(string $from, string $to, int $weight) { + Deprecation::triggerIfCalledFromOutside( + 'doctrine/orm', + 'https://github.com/doctrine/orm/pull/10547', + 'The %s class is deprecated and will be removed in ORM 3.0', + self::class + ); + $this->from = $from; $this->to = $to; $this->weight = $weight; diff --git a/lib/Doctrine/ORM/Internal/CommitOrder/Vertex.php b/lib/Doctrine/ORM/Internal/CommitOrder/Vertex.php index c4747e032d1..c748bd7eab0 100644 --- a/lib/Doctrine/ORM/Internal/CommitOrder/Vertex.php +++ b/lib/Doctrine/ORM/Internal/CommitOrder/Vertex.php @@ -4,9 +4,13 @@ namespace Doctrine\ORM\Internal\CommitOrder; +use Doctrine\Deprecations\Deprecation; use Doctrine\ORM\Mapping\ClassMetadata; -/** @internal */ +/** + * @internal + * @deprecated + */ final class Vertex { /** @@ -32,6 +36,13 @@ final class Vertex public function __construct(string $hash, ClassMetadata $value) { + Deprecation::triggerIfCalledFromOutside( + 'doctrine/orm', + 'https://github.com/doctrine/orm/pull/10547', + 'The %s class is deprecated and will be removed in ORM 3.0', + self::class + ); + $this->hash = $hash; $this->value = $value; } diff --git a/lib/Doctrine/ORM/Internal/CommitOrder/VertexState.php b/lib/Doctrine/ORM/Internal/CommitOrder/VertexState.php index 395db58d554..24d2fb54ee1 100644 --- a/lib/Doctrine/ORM/Internal/CommitOrder/VertexState.php +++ b/lib/Doctrine/ORM/Internal/CommitOrder/VertexState.php @@ -4,7 +4,12 @@ namespace Doctrine\ORM\Internal\CommitOrder; -/** @internal */ +use Doctrine\Deprecations\Deprecation; + +/** + * @internal + * @deprecated + */ final class VertexState { public const NOT_VISITED = 0; @@ -13,5 +18,11 @@ final class VertexState private function __construct() { + Deprecation::triggerIfCalledFromOutside( + 'doctrine/orm', + 'https://github.com/doctrine/orm/pull/10547', + 'The %s class is deprecated and will be removed in ORM 3.0', + self::class + ); } } diff --git a/lib/Doctrine/ORM/Internal/CommitOrderCalculator.php b/lib/Doctrine/ORM/Internal/CommitOrderCalculator.php index 22ee1ab13bd..f0f8112ba23 100644 --- a/lib/Doctrine/ORM/Internal/CommitOrderCalculator.php +++ b/lib/Doctrine/ORM/Internal/CommitOrderCalculator.php @@ -4,6 +4,7 @@ namespace Doctrine\ORM\Internal; +use Doctrine\Deprecations\Deprecation; use Doctrine\ORM\Internal\CommitOrder\Edge; use Doctrine\ORM\Internal\CommitOrder\Vertex; use Doctrine\ORM\Internal\CommitOrder\VertexState; @@ -17,6 +18,8 @@ * using a depth-first searching (DFS) to traverse the graph built in memory. * This algorithm have a linear running time based on nodes (V) and dependency * between the nodes (E), resulting in a computational complexity of O(V + E). + * + * @deprecated */ class CommitOrderCalculator { @@ -45,6 +48,16 @@ class CommitOrderCalculator */ private $sortedNodeList = []; + public function __construct() + { + Deprecation::triggerIfCalledFromOutside( + 'doctrine/orm', + 'https://github.com/doctrine/orm/pull/10547', + 'The %s class is deprecated and will be removed in ORM 3.0', + self::class + ); + } + /** * Checks for node (vertex) existence in graph. * diff --git a/lib/Doctrine/ORM/Internal/TopologicalSort.php b/lib/Doctrine/ORM/Internal/TopologicalSort.php new file mode 100644 index 00000000000..2bf1624ced8 --- /dev/null +++ b/lib/Doctrine/ORM/Internal/TopologicalSort.php @@ -0,0 +1,165 @@ + + */ + private $nodes = []; + + /** + * DFS state for the different nodes, indexed by node object id and using one of + * this class' constants as value. + * + * @var array + */ + private $states = []; + + /** + * Edges between the nodes. The first-level key is the object id of the outgoing + * node; the second array maps the destination node by object id as key. The final + * boolean value indicates whether the edge is optional or not. + * + * @var array> + */ + private $edges = []; + + /** + * Builds up the result during the DFS. + * + * @var list + */ + private $sortResult = []; + + /** @param object $node */ + public function addNode($node): void + { + $id = spl_object_id($node); + $this->nodes[$id] = $node; + $this->states[$id] = self::NOT_VISITED; + $this->edges[$id] = []; + } + + /** @param object $node */ + public function hasNode($node): bool + { + return isset($this->nodes[spl_object_id($node)]); + } + + /** + * Adds a new edge between two nodes to the graph + * + * @param object $from + * @param object $to + * @param bool $optional This indicates whether the edge may be ignored during the topological sort if it is necessary to break cycles. + */ + public function addEdge($from, $to, bool $optional): void + { + $fromId = spl_object_id($from); + $toId = spl_object_id($to); + + if (isset($this->edges[$fromId][$toId]) && $this->edges[$fromId][$toId] === false) { + return; // we already know about this dependency, and it is not optional + } + + $this->edges[$fromId][$toId] = $optional; + } + + /** + * Returns a topological sort of all nodes. When we have an edge A->B between two nodes + * A and B, then A will be listed before B in the result. + * + * @return list + */ + public function sort(): array + { + /* + * When possible, keep objects in the result in the same order in which they were added as nodes. + * Since nodes are unshifted into $this->>sortResult (see the visit() method), that means we + * need to work them in array_reverse order here. + */ + foreach (array_reverse(array_keys($this->nodes)) as $oid) { + if ($this->states[$oid] === self::NOT_VISITED) { + $this->visit($oid); + } + } + + return $this->sortResult; + } + + private function visit(int $oid): void + { + if ($this->states[$oid] === self::IN_PROGRESS) { + // This node is already on the current DFS stack. We've found a cycle! + throw new CycleDetectedException($this->nodes[$oid]); + } + + if ($this->states[$oid] === self::VISITED) { + // We've reached a node that we've already seen, including all + // other nodes that are reachable from here. We're done here, return. + return; + } + + $this->states[$oid] = self::IN_PROGRESS; + + // Continue the DFS downwards the edge list + foreach ($this->edges[$oid] as $adjacentId => $optional) { + try { + $this->visit($adjacentId); + } catch (CycleDetectedException $exception) { + if ($exception->isCycleCollected()) { + // There is a complete cycle downstream of the current node. We cannot + // do anything about that anymore. + throw $exception; + } + + if ($optional) { + // The current edge is part of a cycle, but it is optional and the closest + // such edge while backtracking. Break the cycle here by skipping the edge + // and continuing with the next one. + continue; + } + + // We have found a cycle and cannot break it at $edge. Best we can do + // is to retreat from the current vertex, hoping that somewhere up the + // stack this can be salvaged. + $this->states[$oid] = self::NOT_VISITED; + $exception->addToCycle($this->nodes[$oid]); + + throw $exception; + } + } + + // We have traversed all edges and visited all other nodes reachable from here. + // So we're done with this vertex as well. + + $this->states[$oid] = self::VISITED; + array_unshift($this->sortResult, $this->nodes[$oid]); + } +} diff --git a/lib/Doctrine/ORM/Internal/TopologicalSort/CycleDetectedException.php b/lib/Doctrine/ORM/Internal/TopologicalSort/CycleDetectedException.php new file mode 100644 index 00000000000..9b0bc49d257 --- /dev/null +++ b/lib/Doctrine/ORM/Internal/TopologicalSort/CycleDetectedException.php @@ -0,0 +1,55 @@ + */ + private $cycle; + + /** @var object */ + private $startNode; + + /** + * Do we have the complete cycle collected? + * + * @var bool + */ + private $cycleCollected = false; + + /** @param object $startNode */ + public function __construct($startNode) + { + parent::__construct('A cycle has been detected, so a topological sort is not possible. The getCycle() method provides the list of nodes that form the cycle.'); + + $this->startNode = $startNode; + $this->cycle = [$startNode]; + } + + /** @return list */ + public function getCycle(): array + { + return $this->cycle; + } + + /** @param object $node */ + public function addToCycle($node): void + { + array_unshift($this->cycle, $node); + + if ($node === $this->startNode) { + $this->cycleCollected = true; + } + } + + public function isCycleCollected(): bool + { + return $this->cycleCollected; + } +} diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index a37cdbcb306..5bf8a6b6085 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -28,6 +28,7 @@ use Doctrine\ORM\Id\AssignedGenerator; use Doctrine\ORM\Internal\CommitOrderCalculator; use Doctrine\ORM\Internal\HydrationCompleteHandler; +use Doctrine\ORM\Internal\TopologicalSort; use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Mapping\MappingException; use Doctrine\ORM\Mapping\Reflection\ReflectionPropertiesGetter; @@ -56,11 +57,9 @@ use function array_key_exists; use function array_map; use function array_merge; -use function array_pop; use function array_sum; use function array_values; use function assert; -use function count; use function current; use function func_get_arg; use function func_num_args; @@ -74,6 +73,7 @@ use function reset; use function spl_object_id; use function sprintf; +use function strtolower; /** * The UnitOfWork is responsible for tracking changes to objects during an @@ -419,9 +419,6 @@ public function commit($entity = null) $this->dispatchOnFlushEvent(); - // Now we need a commit order to maintain referential integrity - $commitOrder = $this->getCommitOrder(); - $conn = $this->em->getConnection(); $conn->beginTransaction(); @@ -437,32 +434,37 @@ public function commit($entity = null) } if ($this->entityInsertions) { - foreach ($commitOrder as $class) { - $this->executeInserts($class); - } + // Perform entity insertions first, so that all new entities have their rows in the database + // and can be referred to by foreign keys. The commit order only needs to take new entities + // into account (new entities referring to other new entities), since all other types (entities + // with updates or scheduled deletions) are currently not a problem, since they are already + // in the database. + $this->executeInserts(); } if ($this->entityUpdates) { - foreach ($commitOrder as $class) { - $this->executeUpdates($class); - } + // Updates do not need to follow a particular order + $this->executeUpdates(); } // Extra updates that were requested by persisters. + // This may include foreign keys that could not be set when an entity was inserted, + // which may happen in the case of circular foreign key relationships. if ($this->extraUpdates) { $this->executeExtraUpdates(); } // Collection updates (deleteRows, updateRows, insertRows) + // No particular order is necessary, since all entities themselves are already + // in the database foreach ($this->collectionUpdates as $collectionToUpdate) { $this->getCollectionPersister($collectionToUpdate->getMapping())->update($collectionToUpdate); } - // Entity deletions come last and need to be in reverse commit order + // Entity deletions come last. Their order only needs to take care of other deletions + // (first delete entities depending upon others, before deleting depended-upon entities). if ($this->entityDeletions) { - for ($count = count($commitOrder), $i = $count - 1; $i >= 0 && $this->entityDeletions; --$i) { - $this->executeDeletions($commitOrder[$i]); - } + $this->executeDeletions(); } // Commit failed silently @@ -1156,58 +1158,46 @@ public function recomputeSingleEntityChangeSet(ClassMetadata $class, $entity) } /** - * Executes all entity insertions for entities of the specified type. + * Executes entity insertions */ - private function executeInserts(ClassMetadata $class): void + private function executeInserts(): void { - $entities = []; - $className = $class->name; - $persister = $this->getEntityPersister($className); - $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist); - - $insertionsForClass = []; - - foreach ($this->entityInsertions as $oid => $entity) { - if ($this->em->getClassMetadata(get_class($entity))->name !== $className) { - continue; - } + $entities = $this->computeInsertExecutionOrder(); - $insertionsForClass[$oid] = $entity; + foreach ($entities as $entity) { + $oid = spl_object_id($entity); + $class = $this->em->getClassMetadata(get_class($entity)); + $persister = $this->getEntityPersister($class->name); + $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist); $persister->addInsert($entity); unset($this->entityInsertions[$oid]); - if ($invoke !== ListenersInvoker::INVOKE_NONE) { - $entities[] = $entity; - } - } - - $postInsertIds = $persister->executeInserts(); + $postInsertIds = $persister->executeInserts(); - if (is_array($postInsertIds)) { - Deprecation::trigger( - 'doctrine/orm', - 'https://github.com/doctrine/orm/pull/10743/', - 'Returning post insert IDs from \Doctrine\ORM\Persisters\Entity\EntityPersister::executeInserts() is deprecated and will not be supported in Doctrine ORM 3.0. Make the persister call Doctrine\ORM\UnitOfWork::assignPostInsertId() instead.' - ); + if (is_array($postInsertIds)) { + Deprecation::trigger( + 'doctrine/orm', + 'https://github.com/doctrine/orm/pull/10743/', + 'Returning post insert IDs from \Doctrine\ORM\Persisters\Entity\EntityPersister::executeInserts() is deprecated and will not be supported in Doctrine ORM 3.0. Make the persister call Doctrine\ORM\UnitOfWork::assignPostInsertId() instead.' + ); - // Persister returned post-insert IDs - foreach ($postInsertIds as $postInsertId) { - $this->assignPostInsertId($postInsertId['entity'], $postInsertId['generatedId']); + // Persister returned post-insert IDs + foreach ($postInsertIds as $postInsertId) { + $this->assignPostInsertId($postInsertId['entity'], $postInsertId['generatedId']); + } } - } - foreach ($insertionsForClass as $oid => $entity) { if (! isset($this->entityIdentifiers[$oid])) { //entity was not added to identity map because some identifiers are foreign keys to new entities. //add it now $this->addToEntityIdentifiersAndEntityMap($class, $oid, $entity); } - } - foreach ($entities as $entity) { - $this->listenersInvoker->invoke($class, Events::postPersist, $entity, new PostPersistEventArgs($entity, $this->em), $invoke); + if ($invoke !== ListenersInvoker::INVOKE_NONE) { + $this->listenersInvoker->invoke($class, Events::postPersist, $entity, new PostPersistEventArgs($entity, $this->em), $invoke); + } } } @@ -1245,19 +1235,15 @@ private function addToEntityIdentifiersAndEntityMap( } /** - * Executes all entity updates for entities of the specified type. + * Executes all entity updates */ - private function executeUpdates(ClassMetadata $class): void + private function executeUpdates(): void { - $className = $class->name; - $persister = $this->getEntityPersister($className); - $preUpdateInvoke = $this->listenersInvoker->getSubscribedSystems($class, Events::preUpdate); - $postUpdateInvoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postUpdate); - foreach ($this->entityUpdates as $oid => $entity) { - if ($this->em->getClassMetadata(get_class($entity))->name !== $className) { - continue; - } + $class = $this->em->getClassMetadata(get_class($entity)); + $persister = $this->getEntityPersister($class->name); + $preUpdateInvoke = $this->listenersInvoker->getSubscribedSystems($class, Events::preUpdate); + $postUpdateInvoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postUpdate); if ($preUpdateInvoke !== ListenersInvoker::INVOKE_NONE) { $this->listenersInvoker->invoke($class, Events::preUpdate, $entity, new PreUpdateEventArgs($entity, $this->em, $this->getEntityChangeSet($entity)), $preUpdateInvoke); @@ -1278,18 +1264,17 @@ private function executeUpdates(ClassMetadata $class): void } /** - * Executes all entity deletions for entities of the specified type. + * Executes all entity deletions */ - private function executeDeletions(ClassMetadata $class): void + private function executeDeletions(): void { - $className = $class->name; - $persister = $this->getEntityPersister($className); - $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postRemove); + $entities = $this->computeDeleteExecutionOrder(); - foreach ($this->entityDeletions as $oid => $entity) { - if ($this->em->getClassMetadata(get_class($entity))->name !== $className) { - continue; - } + foreach ($entities as $entity) { + $oid = spl_object_id($entity); + $class = $this->em->getClassMetadata(get_class($entity)); + $persister = $this->getEntityPersister($class->name); + $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postRemove); $persister->delete($entity); @@ -1313,73 +1298,116 @@ private function executeDeletions(ClassMetadata $class): void } } - /** - * Gets the commit order. - * - * @return list - */ - private function getCommitOrder(): array + /** @return list */ + private function computeInsertExecutionOrder(): array { - $calc = $this->getCommitOrderCalculator(); + $sort = new TopologicalSort(); - // See if there are any new classes in the changeset, that are not in the - // commit order graph yet (don't have a node). - // We have to inspect changeSet to be able to correctly build dependencies. - // It is not possible to use IdentityMap here because post inserted ids - // are not yet available. - $newNodes = []; + // First make sure we have all the nodes + foreach ($this->entityInsertions as $entity) { + $sort->addNode($entity); + } - foreach (array_merge($this->entityInsertions, $this->entityUpdates, $this->entityDeletions) as $entity) { + // Now add edges + foreach ($this->entityInsertions as $entity) { $class = $this->em->getClassMetadata(get_class($entity)); - if ($calc->hasNode($class->name)) { - continue; - } - - $calc->addNode($class->name, $class); - - $newNodes[] = $class; - } - - // Calculate dependencies for new nodes - while ($class = array_pop($newNodes)) { foreach ($class->associationMappings as $assoc) { + // We only need to consider the owning sides of to-one associations, + // since many-to-many associations are persisted at a later step and + // have no insertion order problems (all entities already in the database + // at that time). if (! ($assoc['isOwningSide'] && $assoc['type'] & ClassMetadata::TO_ONE)) { continue; } - $targetClass = $this->em->getClassMetadata($assoc['targetEntity']); + $targetEntity = $class->getFieldValue($entity, $assoc['fieldName']); - if (! $calc->hasNode($targetClass->name)) { - $calc->addNode($targetClass->name, $targetClass); + // If there is no entity that we need to refer to, or it is already in the + // database (i. e. does not have to be inserted), no need to consider it. + if ($targetEntity === null || ! $sort->hasNode($targetEntity)) { + continue; + } - $newNodes[] = $targetClass; + // An entity that references back to itself _and_ uses an application-provided ID + // (the "NONE" generator strategy) can be exempted from commit order computation. + // See https://github.com/doctrine/orm/pull/10735/ for more details on this edge case. + // A non-NULLable self-reference would be a cycle in the graph. + if ($targetEntity === $entity && $class->isIdentifierNatural()) { + continue; } + // According to https://www.doctrine-project.org/projects/doctrine-orm/en/2.14/reference/annotations-reference.html#annref_joincolumn, + // the default for "nullable" is true. Unfortunately, it seems this default is not applied at the metadata driver, factory or other + // level, but in fact we may have an undefined 'nullable' key here, so we must assume that default here as well. + // + // Same in \Doctrine\ORM\Tools\EntityGenerator::isAssociationIsNullable or \Doctrine\ORM\Persisters\Entity\BasicEntityPersister::getJoinSQLForJoinColumns, + // to give two examples. + assert(isset($assoc['joinColumns'])); $joinColumns = reset($assoc['joinColumns']); + $isNullable = ! isset($joinColumns['nullable']) || $joinColumns['nullable']; - $calc->addDependency($targetClass->name, $class->name, (int) empty($joinColumns['nullable'])); + // Add dependency. The dependency direction implies that "$targetEntity has to go before $entity", + // so we can work through the topo sort result from left to right (with all edges pointing right). + $sort->addEdge($targetEntity, $entity, $isNullable); + } + } - // If the target class has mapped subclasses, these share the same dependency. - if (! $targetClass->subClasses) { - continue; - } + return $sort->sort(); + } + + /** @return list */ + private function computeDeleteExecutionOrder(): array + { + $sort = new TopologicalSort(); + + // First make sure we have all the nodes + foreach ($this->entityDeletions as $entity) { + $sort->addNode($entity); + } - foreach ($targetClass->subClasses as $subClassName) { - $targetSubClass = $this->em->getClassMetadata($subClassName); + // Now add edges + foreach ($this->entityDeletions as $entity) { + $class = $this->em->getClassMetadata(get_class($entity)); - if (! $calc->hasNode($subClassName)) { - $calc->addNode($targetSubClass->name, $targetSubClass); + foreach ($class->associationMappings as $assoc) { + // We only need to consider the owning sides of to-one associations, + // since many-to-many associations can always be (and have already been) + // deleted in a preceding step. + if (! ($assoc['isOwningSide'] && $assoc['type'] & ClassMetadata::TO_ONE)) { + continue; + } - $newNodes[] = $targetSubClass; + // For associations that implement a database-level cascade/set null operation, + // we do not have to follow a particular order: If the referred-to entity is + // deleted first, the DBMS will either delete the current $entity right away + // (CASCADE) or temporarily set the foreign key to NULL (SET NULL). + // Either way, we can skip it in the computation. + assert(isset($assoc['joinColumns'])); + $joinColumns = reset($assoc['joinColumns']); + if (isset($joinColumns['onDelete'])) { + $onDeleteOption = strtolower($joinColumns['onDelete']); + if ($onDeleteOption === 'cascade' || $onDeleteOption === 'set null') { + continue; } + } + + $targetEntity = $class->getFieldValue($entity, $assoc['fieldName']); - $calc->addDependency($targetSubClass->name, $class->name, 1); + // If the association does not refer to another entity or that entity + // is not to be deleted, there is no ordering problem and we can + // skip this particular association. + if ($targetEntity === null || ! $sort->hasNode($targetEntity)) { + continue; } + + // Add dependency. The dependency direction implies that "$entity has to be removed before $targetEntity", + // so we can work through the topo sort result from left to right (with all edges pointing right). + $sort->addEdge($entity, $targetEntity, false); } } - return $calc->sort(); + return $sort->sort(); } /** diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 145b78e82f1..2b887162e50 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -2793,7 +2793,6 @@ setValue - @@ -2802,10 +2801,6 @@ unwrap unwrap - - = 0 && $this->entityDeletions]]> - entityDeletions]]> - is_array($entity) diff --git a/psalm.xml b/psalm.xml index 262a6ed0aa6..9cb91ee7714 100644 --- a/psalm.xml +++ b/psalm.xml @@ -45,6 +45,10 @@ + + + + diff --git a/tests/Doctrine/Tests/ORM/Functional/OneToOneSelfReferentialAssociationTest.php b/tests/Doctrine/Tests/ORM/Functional/OneToOneSelfReferentialAssociationTest.php index 34b7823713e..93b89acd242 100644 --- a/tests/Doctrine/Tests/ORM/Functional/OneToOneSelfReferentialAssociationTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/OneToOneSelfReferentialAssociationTest.php @@ -73,9 +73,11 @@ public function testFind(): void public function testEagerLoadsAssociation(): void { - $this->createFixture(); + $customerId = $this->createFixture(); + + $query = $this->_em->createQuery('select c, m from Doctrine\Tests\Models\ECommerce\ECommerceCustomer c left join c.mentor m where c.id = :id'); + $query->setParameter('id', $customerId); - $query = $this->_em->createQuery('select c, m from Doctrine\Tests\Models\ECommerce\ECommerceCustomer c left join c.mentor m order by c.id asc'); $result = $query->getResult(); $customer = $result[0]; $this->assertLoadingOfAssociation($customer); diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10348Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10348Test.php new file mode 100644 index 00000000000..1e4f0c47c94 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10348Test.php @@ -0,0 +1,108 @@ +setUpEntitySchema([ + GH10348Person::class, + GH10348Company::class, + ]); + } + + public function testTheORMRemovesReferencedEmployeeBeforeReferencingEmployee(): void + { + $person1 = new GH10348Person(); + $person2 = new GH10348Person(); + $person2->mentor = $person1; + + $company = new GH10348Company(); + $company->addEmployee($person1)->addEmployee($person2); + + $this->_em->persist($company); + $this->_em->flush(); + + $company = $this->_em->find(GH10348Company::class, $company->id); + + $this->_em->remove($company); + $this->_em->flush(); + + self::assertEmpty($this->_em->createQuery('SELECT c FROM ' . GH10348Company::class . ' c')->getResult()); + self::assertEmpty($this->_em->createQuery('SELECT p FROM ' . GH10348Person::class . ' p')->getResult()); + } +} + +/** + * @ORM\Entity + */ +class GH10348Person +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var ?int + */ + public $id = null; + + /** + * @ORM\ManyToOne(targetEntity="GH10348Company", inversedBy="employees") + * + * @var ?GH10348Company + */ + public $employer = null; + + /** + * @ORM\ManyToOne(targetEntity="GH10348Person", cascade={"remove"}) + * + * @var ?GH10348Person + */ + public $mentor = null; +} + +/** + * @ORM\Entity + */ +class GH10348Company +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var ?int + */ + public $id = null; + + /** + * @ORM\OneToMany(targetEntity="GH10348Person", mappedBy="emplo", cascade={"persist", "remove"}) + * + * @var Collection + */ + private $employees; + + public function __construct() + { + $this->employees = new ArrayCollection(); + } + + public function addEmployee(GH10348Person $person): self + { + $person->employer = $this; + $this->employees->add($person); + + return $this; + } +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10531Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10531Test.php new file mode 100644 index 00000000000..2b0f4fa5c19 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10531Test.php @@ -0,0 +1,151 @@ +createSchemaForModels( + GH10531A::class, + GH10531B::class + ); + } + + public function tearDown(): void + { + $conn = static::$sharedConn; + $conn->executeStatement('DELETE FROM gh10531_b'); + $conn->executeStatement('DELETE FROM gh10531_a'); + } + + public function testInserts(): void + { + $a = new GH10531A(); + $b1 = new GH10531B(); + $b2 = new GH10531B(); + $b3 = new GH10531B(); + + $b1->parent = $b2; + $b3->parent = $b2; + $b2->parent = $a; + + /* + * The following would force a working commit order, but that's not what + * we want (the ORM shall sort this out internally). + * + * $this->_em->persist($a); + * $this->_em->persist($b2); + * $this->_em->flush(); + * $this->_em->persist($b1); + * $this->_em->persist($b3); + * $this->_em->flush(); + */ + + // Pass $b2 to persist() between $b1 and $b3, so that any potential reliance upon the + // order of persist() calls is spotted: No matter if it is in the order that persist() + // was called or the other way round, in both cases there is an entity that will come + // "before" $b2 but depend on its primary key, so the ORM must re-order the inserts. + + $this->_em->persist($a); + $this->_em->persist($b1); + $this->_em->persist($b2); + $this->_em->persist($b3); + $this->_em->flush(); + + self::assertNotNull($a->id); + self::assertNotNull($b1->id); + self::assertNotNull($b2->id); + self::assertNotNull($b3->id); + } + + public function testDeletes(): void + { + $this->expectNotToPerformAssertions(); + $con = $this->_em->getConnection(); + + // The "a" entity + $con->insert('gh10531_a', ['id' => 1, 'discr' => 'A']); + $a = $this->_em->find(GH10531A::class, 1); + + // The "b2" entity + $con->insert('gh10531_a', ['id' => 2, 'discr' => 'B']); + $con->insert('gh10531_b', ['id' => 2, 'parent_id' => 1]); + $b2 = $this->_em->find(GH10531B::class, 2); + + // The "b1" entity + $con->insert('gh10531_a', ['id' => 3, 'discr' => 'B']); + $con->insert('gh10531_b', ['id' => 3, 'parent_id' => 2]); + $b1 = $this->_em->find(GH10531B::class, 3); + + // The "b3" entity + $con->insert('gh10531_a', ['id' => 4, 'discr' => 'B']); + $con->insert('gh10531_b', ['id' => 4, 'parent_id' => 2]); + $b3 = $this->_em->find(GH10531B::class, 4); + + /* + * The following would make the deletions happen in an order + * where the not-nullable foreign key constraints would not be + * violated. But, we want the ORM to be able to sort this out + * internally. + * + * $this->_em->remove($b1); + * $this->_em->remove($b3); + * $this->_em->remove($b2); + */ + + // As before, put $b2 in between $b1 and $b3 so that the order of the + // remove() calls alone (in either direction) does not solve the problem. + // The ORM will have to sort $b2 to be deleted last, after $b1 and $b3. + $this->_em->remove($b1); + $this->_em->remove($b2); + $this->_em->remove($b3); + + $this->_em->flush(); + } +} + +/** + * @ORM\Entity + * @ORM\Table(name="gh10531_a") + * @ORM\DiscriminatorColumn(name="discr", type="string") + * @ORM\DiscriminatorMap({ "A": "GH10531A", "B": "GH10531B" }) + * @ORM\InheritanceType("JOINED") + * + * We are using JTI here, since STI would relax the not-nullable constraint for the "parent" + * column (it has to be NULL when the row contains a GH10531A instance). Causes another error, + * but not the constraint violation I'd like to point out. + */ +class GH10531A +{ + /** + * @ORM\Id + * @ORM\GeneratedValue(strategy="AUTO") + * @ORM\Column(type="integer") + * + * @var int + */ + public $id; +} + +/** + * @ORM\Entity + * @ORM\Table(name="gh10531_b") + */ +class GH10531B extends GH10531A +{ + /** + * @ORM\ManyToOne(targetEntity="GH10531A") + * @ORM\JoinColumn(nullable=false, name="parent_id") + * + * @var GH10531A + */ + public $parent; +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10532Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10532Test.php new file mode 100644 index 00000000000..b3762b068e4 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10532Test.php @@ -0,0 +1,186 @@ +createSchemaForModels( + GH10532A::class, + GH10532B::class, + GH10532C::class, + GH10532X::class + ); + } + + public function tearDown(): void + { + $conn = static::$sharedConn; + $conn->executeStatement('DELETE FROM gh10532_c'); + $conn->executeStatement('DELETE FROM gh10532_b'); + $conn->executeStatement('DELETE FROM gh10532_a'); + $conn->executeStatement('DELETE FROM gh10532_x'); + } + + public function testInserts(): void + { + // Dependencies are $a1 -> $b -> $a2 -> $c + + $a1 = new GH10532A(); + $b = new GH10532B(); + $a2 = new GH10532A(); + $c = new GH10532C(); + + $a1->x = $b; + $b->a = $a2; + $a2->x = $c; + + /* + * The following would force a working commit order, but that's not what + * we want (the ORM shall sort this out internally). + * + * $this->_em->persist($c); + * $this->_em->persist($a2); + * $this->_em->flush(); + * $this->_em->persist($b); + * $this->_em->persist($a1); + * $this->_em->flush(); + */ + + $this->_em->persist($a1); + $this->_em->persist($a2); + $this->_em->persist($b); + $this->_em->persist($c); + $this->_em->flush(); + + self::assertNotNull($a1->id); + self::assertNotNull($b->id); + self::assertNotNull($a2->id); + self::assertNotNull($c->id); + } + + public function testDeletes(): void + { + // Dependencies are $a1 -> $b -> $a2 -> $c + + $this->expectNotToPerformAssertions(); + $con = $this->_em->getConnection(); + + // The "c" entity + $con->insert('gh10532_x', ['id' => 1, 'discr' => 'C']); + $con->insert('gh10532_c', ['id' => 1]); + $c = $this->_em->find(GH10532C::class, 1); + + // The "a2" entity + $con->insert('gh10532_a', ['id' => 2, 'gh10532x_id' => 1]); + $a2 = $this->_em->find(GH10532A::class, 2); + + // The "b" entity + $con->insert('gh10532_x', ['id' => 3, 'discr' => 'B']); + $con->insert('gh10532_b', ['id' => 3, 'gh10532a_id' => 2]); + $b = $this->_em->find(GH10532B::class, 3); + + // The "a1" entity + $con->insert('gh10532_a', ['id' => 4, 'gh10532x_id' => 3]); + $a1 = $this->_em->find(GH10532A::class, 4); + + /* + * The following would make the deletions happen in an order + * where the not-nullable foreign key constraints would not be + * violated. But, we want the ORM to be able to sort this out + * internally. + * + * $this->_em->remove($a1); + * $this->_em->flush(); + * $this->_em->remove($b); + * $this->_em->flush(); + * $this->_em->remove($a2); + * $this->_em->remove($c); + * $this->_em->flush(); + */ + + $this->_em->remove($a1); + $this->_em->remove($a2); + $this->_em->remove($b); + $this->_em->remove($c); + + $this->_em->flush(); + } +} + +/** + * @ORM\Entity + * @ORM\Table(name="gh10532_x") + * @ORM\DiscriminatorColumn(name="discr", type="string") + * @ORM\DiscriminatorMap({ "B": "GH10532B", "C": "GH10532C" }) + * @ORM\InheritanceType("JOINED") + * + * We are using JTI here, since STI would relax the not-nullable constraint for the "parent" + * column. Causes another error, but not the constraint violation I'd like to point out. + */ +abstract class GH10532X +{ + /** + * @ORM\Id + * @ORM\GeneratedValue(strategy="AUTO") + * @ORM\Column(type="integer") + * + * @var int + */ + public $id; +} + +/** + * @ORM\Entity + * @ORM\Table(name="gh10532_b") + */ +class GH10532B extends GH10532X +{ + /** + * @ORM\ManyToOne(targetEntity="GH10532A") + * @ORM\JoinColumn(nullable=false, name="gh10532a_id") + * + * @var GH10532A + */ + public $a; +} + +/** + * @ORM\Entity + * @ORM\Table(name="gh10532_c") + */ +class GH10532C extends GH10532X +{ +} + +/** + * @ORM\Entity + * @ORM\Table(name="gh10532_a") + */ +class GH10532A +{ + /** + * @ORM\Id + * @ORM\GeneratedValue(strategy="AUTO") + * @ORM\Column(type="integer") + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity="GH10532X") + * @ORM\JoinColumn(nullable=false, name="gh10532x_id") + * + * @var GH10532X + */ + public $x; +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10566Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10566Test.php new file mode 100644 index 00000000000..91b6174a560 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10566Test.php @@ -0,0 +1,177 @@ +createSchemaForModels( + GH10566A::class, + GH10566B::class, + GH10566C::class + ); + } + + /** + * @dataProvider provideEntityClasses + */ + public function testInsertion(string $startEntityClass): void + { + $a = new GH10566A(); + $b = new GH10566B(); + $c = new GH10566C(); + + $a->other = $b; + $b->other = $c; + $c->other = $a; + + foreach ([$a, $b, $c] as $candidate) { + if (is_a($candidate, $startEntityClass)) { + $this->_em->persist($candidate); + } + } + + // Since all associations are nullable, the ORM has no problem finding an insert order, + // it can always schedule "deferred updates" to fill missing foreign key values. + $this->_em->flush(); + + self::assertNotNull($a->id); + self::assertNotNull($b->id); + self::assertNotNull($c->id); + } + + /** + * @dataProvider provideEntityClasses + */ + public function testRemoval(string $startEntityClass): void + { + $a = new GH10566A(); + $b = new GH10566B(); + $c = new GH10566C(); + + $a->other = $b; + $b->other = $c; + $c->other = $a; + + $this->_em->persist($a); + $this->_em->flush(); + + $aId = $a->id; + $bId = $b->id; + $cId = $c->id; + + // In the removal case, the ORM currently does not schedule "extra updates" + // to break association cycles before entities are removed. So, we must not + // look at "nullable" for associations to find a delete commit order. + // + // To make it work, the user needs to have a database-level "ON DELETE SET NULL" + // on an association. That's where the cycle can be broken. Commit order computation + // for the removal case needs to look at this property. + // + // In this example, only A -> B can be used to break the cycle. So, regardless which + // entity we start with, the ORM-level cascade will always remove all three entities, + // and the order of database deletes always has to be (can only be) from B, then C, then A. + + foreach ([$a, $b, $c] as $candidate) { + if (is_a($candidate, $startEntityClass)) { + $this->_em->remove($candidate); + } + } + + $this->_em->flush(); + + self::assertFalse($this->_em->getConnection()->fetchOne('SELECT id FROM gh10566_a WHERE id = ?', [$aId])); + self::assertFalse($this->_em->getConnection()->fetchOne('SELECT id FROM gh10566_b WHERE id = ?', [$bId])); + self::assertFalse($this->_em->getConnection()->fetchOne('SELECT id FROM gh10566_c WHERE id = ?', [$cId])); + } + + public function provideEntityClasses(): Generator + { + yield [GH10566A::class]; + yield [GH10566B::class]; + yield [GH10566C::class]; + } +} + +/** + * @ORM\Entity + * @ORM\Table(name="gh10566_a") + */ +class GH10566A +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue() + * + * @var int + */ + public $id; + + /** + * @ORM\OneToOne(targetEntity="GH10566B", cascade={"all"}) + * @ORM\JoinColumn(nullable=true, onDelete="SET NULL") + * + * @var GH10566B + */ + public $other; +} + +/** + * @ORM\Entity + * @ORM\Table(name="gh10566_b") + */ +class GH10566B +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue() + * + * @var int + */ + public $id; + + /** + * @ORM\OneToOne(targetEntity="GH10566C", cascade={"all"}) + * @ORM\JoinColumn(nullable=true) + * + * @var GH10566C + */ + public $other; +} + +/** + * @ORM\Entity + * @ORM\Table(name="gh10566_c") + */ +class GH10566C +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue() + * + * @var int + */ + public $id; + + /** + * @ORM\OneToOne(targetEntity="GH10566A", cascade={"all"}) + * @ORM\JoinColumn(nullable=true) + * + * @var GH10566A + */ + public $other; +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6499OneToManyRelationshipTest.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6499OneToManyRelationshipTest.php new file mode 100644 index 00000000000..f638e79f013 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6499OneToManyRelationshipTest.php @@ -0,0 +1,155 @@ +createSchemaForModels(Application::class, Person::class, ApplicationPerson::class); + } + + /** + * Test for the bug described in issue #6499. + */ + public function testIssue(): void + { + $person = new Person(); + $this->_em->persist($person); + + $application = new Application(); + $this->_em->persist($application); + + $applicationPerson = new ApplicationPerson($person, $application); + + $this->_em->persist($applicationPerson); + $this->_em->flush(); + $this->_em->clear(); + + $personFromDatabase = $this->_em->find(Person::class, $person->id); + $applicationFromDatabase = $this->_em->find(Application::class, $application->id); + + self::assertEquals($personFromDatabase->id, $person->id, 'Issue #6499 will result in an integrity constraint violation before reaching this point.'); + self::assertFalse($personFromDatabase->getApplicationPeople()->isEmpty()); + + self::assertEquals($applicationFromDatabase->id, $application->id, 'Issue #6499 will result in an integrity constraint violation before reaching this point.'); + self::assertFalse($applicationFromDatabase->getApplicationPeople()->isEmpty()); + } +} + +/** + * @ORM\Entity + * @ORM\Table("GH6499OTM_application") + */ +class Application +{ + /** + * @ORM\Id + * @ORM\GeneratedValue + * @ORM\Column(type="integer") + * + * @var int + */ + public $id; + + /** + * @ORM\OneToMany(targetEntity=ApplicationPerson::class, mappedBy="application", orphanRemoval=true, cascade={"persist"}) + * @ORM\JoinColumn(nullable=false) + * + * @var Collection + */ + private $applicationPeople; + + public function __construct() + { + $this->applicationPeople = new ArrayCollection(); + } + + public function getApplicationPeople(): Collection + { + return $this->applicationPeople; + } +} +/** + * @ORM\Entity() + * @ORM\Table("GH6499OTM_person") + */ +class Person +{ + /** + * @ORM\Id + * @ORM\GeneratedValue + * @ORM\Column(type="integer") + * + * @var int + */ + public $id; + + /** + * @ORM\OneToMany(targetEntity=ApplicationPerson::class, mappedBy="person", orphanRemoval=true, cascade={"persist"}) + * @ORM\JoinColumn(nullable=false) + * + * @var Collection + */ + private $applicationPeople; + + public function __construct() + { + $this->applicationPeople = new ArrayCollection(); + } + + public function getApplicationPeople(): Collection + { + return $this->applicationPeople; + } +} + +/** + * @ORM\Entity() + * @ORM\Table("GH6499OTM_application_person") + */ +class ApplicationPerson +{ + /** + * @ORM\Id + * @ORM\GeneratedValue + * @ORM\Column(type="integer") + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity=Application::class, inversedBy="applicationPeople", cascade={"persist"}) + * @ORM\JoinColumn(nullable=false) + * + * @var Application + */ + public $application; + + /** + * @ORM\ManyToOne(targetEntity=Person::class, inversedBy="applicationPeople", cascade={"persist"}) + * @ORM\JoinColumn(nullable=false) + * + * @var Person + */ + public $person; + + public function __construct(Person $person, Application $application) + { + $this->person = $person; + $this->application = $application; + } +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6499OneToOneRelationshipTest.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6499OneToOneRelationshipTest.php new file mode 100644 index 00000000000..2922c674733 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6499OneToOneRelationshipTest.php @@ -0,0 +1,78 @@ +createSchemaForModels(GH6499OTOA::class, GH6499OTOB::class); + } + + /** + * Test for the bug described in issue #6499. + */ + public function testIssue(): void + { + $a = new GH6499OTOA(); + + $this->_em->persist($a); + $this->_em->flush(); + $this->_em->clear(); + + self::assertEquals( + $this->_em->find(GH6499OTOA::class, $a->id)->b->id, + $a->b->id, + 'Issue #6499 will result in an integrity constraint violation before reaching this point.' + ); + } +} + +/** @ORM\Entity */ +class GH6499OTOA +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\OneToOne(targetEntity="GH6499OTOB", cascade={"persist"}) + * @ORM\JoinColumn(nullable=false) + * + * @var GH6499OTOB + */ + public $b; + + public function __construct() + { + $this->b = new GH6499OTOB(); + } +} + +/** @ORM\Entity */ +class GH6499OTOB +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6499Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6499Test.php new file mode 100644 index 00000000000..a6672801ca4 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6499Test.php @@ -0,0 +1,103 @@ +createSchemaForModels(GH6499A::class, GH6499B::class); + } + + public function testIssue(): void + { + $b = new GH6499B(); + $a = new GH6499A(); + + $this->_em->persist($a); + + $a->b = $b; + + $this->_em->persist($b); + + $this->_em->flush(); + + self::assertIsInt($a->id); + self::assertIsInt($b->id); + } + + public function testIssueReversed(): void + { + $b = new GH6499B(); + $a = new GH6499A(); + + $a->b = $b; + + $this->_em->persist($b); + $this->_em->persist($a); + + $this->_em->flush(); + + self::assertIsInt($a->id); + self::assertIsInt($b->id); + } +} + +/** + * @ORM\Entity + */ +class GH6499A +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\JoinColumn(nullable=false) + * @ORM\OneToOne(targetEntity=GH6499B::class) + * + * @var GH6499B + */ + public $b; +} + +/** + * @ORM\Entity + */ +class GH6499B +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity=GH6499A::class) + * + * @var GH6499A + */ + private $a; +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7006Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7006Test.php new file mode 100644 index 00000000000..e4be3ed83b8 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7006Test.php @@ -0,0 +1,135 @@ +createSchemaForModels(GH7006Book::class, GH7006PCT::class, GH7006PCTFee::class); + } + + public function testIssue(): void + { + $book = new GH7006Book(); + $book->exchangeCode = 'first'; + $this->_em->persist($book); + + $book->exchangeCode = 'second'; // change sth. + + $paymentCardTransaction = new GH7006PCT(); + $paymentCardTransaction->book = $book; + $paymentCardTransactionFee = new GH7006PCTFee($paymentCardTransaction); + + $this->_em->persist($paymentCardTransaction); + + $this->_em->flush(); + + self::assertIsInt($book->id); + self::assertIsInt($paymentCardTransaction->id); + self::assertIsInt($paymentCardTransactionFee->id); + } +} + +/** + * @ORM\Entity + */ +class GH7006Book +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue(strategy="AUTO") + * + * @var int + */ + public $id; + + /** + * @ORM\Column(type="string", length=255, nullable=true) + * + * @var string + */ + public $exchangeCode; + + /** + * @ORM\OneToOne(targetEntity="GH7006PCT", cascade={"persist", "remove"}) + * @ORM\JoinColumn(name="paymentCardTransactionId", referencedColumnName="id") + * + * @var GH7006PCT + */ + public $paymentCardTransaction; +} + +/** + * @ORM\Entity + */ +class GH7006PCT +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue(strategy="AUTO") + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity="GH7006Book") + * @ORM\JoinColumn(name="bookingId", referencedColumnName="id", nullable=false) + * + * @var GH7006Book + */ + public $book; + + /** + * @ORM\OneToMany(targetEntity="GH7006PCTFee", mappedBy="pct", cascade={"persist", "remove"}) + * @ORM\OrderBy({"id" = "ASC"}) + * + * @var Collection + */ + public $fees; + + public function __construct() + { + $this->fees = new ArrayCollection(); + } +} + +/** + * @ORM\Entity + */ +class GH7006PCTFee +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue(strategy="AUTO") + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity="GH7006PCT", inversedBy="fees") + * @ORM\JoinColumn(name="paymentCardTransactionId", referencedColumnName="id", nullable=false) + * + * @var GH7006PCT + */ + public $pct; + + public function __construct(GH7006PCT $pct) + { + $this->pct = $pct; + $pct->fees->add($this); + } +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7180Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7180Test.php new file mode 100644 index 00000000000..46431647f75 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7180Test.php @@ -0,0 +1,223 @@ +setUpEntitySchema([GH7180A::class, GH7180B::class, GH7180C::class, GH7180D::class, GH7180E::class, GH7180F::class, GH7180G::class]); + } + + public function testIssue(): void + { + $a = new GH7180A(); + $b = new GH7180B(); + $c = new GH7180C(); + + $a->b = $b; + $b->a = $a; + $c->a = $a; + + $this->_em->persist($a); + $this->_em->persist($b); + $this->_em->persist($c); + + $this->_em->flush(); + + self::assertIsInt($a->id); + self::assertIsInt($b->id); + self::assertIsInt($c->id); + } + + public function testIssue3NodeCycle(): void + { + $d = new GH7180D(); + $e = new GH7180E(); + $f = new GH7180F(); + $g = new GH7180G(); + + $d->e = $e; + $e->f = $f; + $f->d = $d; + $g->d = $d; + + $this->_em->persist($d); + $this->_em->persist($e); + $this->_em->persist($f); + $this->_em->persist($g); + + $this->_em->flush(); + + self::assertIsInt($d->id); + self::assertIsInt($e->id); + self::assertIsInt($f->id); + self::assertIsInt($g->id); + } +} + +/** + * @Entity + */ +class GH7180A +{ + /** + * @GeneratedValue() + * @Id + * @Column(type="integer") + * @var int + */ + public $id; + + /** + * @OneToOne(targetEntity=GH7180B::class, inversedBy="a") + * @JoinColumn(nullable=false) + * @var GH7180B + */ + public $b; +} + +/** + * @Entity + */ +class GH7180B +{ + /** + * @GeneratedValue() + * @Id + * @Column(type="integer") + * @var int + */ + public $id; + + /** + * @OneToOne(targetEntity=GH7180A::class, mappedBy="b") + * @JoinColumn(nullable=true) + * @var GH7180A + */ + public $a; +} + +/** + * @Entity + */ +class GH7180C +{ + /** + * @GeneratedValue() + * @Id + * @Column(type="integer") + * @var int + */ + public $id; + + /** + * @ManyToOne(targetEntity=GH7180A::class) + * @JoinColumn(nullable=false) + * @var GH7180A + */ + public $a; +} + +/** + * @Entity + */ +class GH7180D +{ + /** + * @GeneratedValue() + * @Id + * @Column(type="integer") + * @var int + */ + public $id; + + /** + * @OneToOne(targetEntity=GH7180E::class) + * @JoinColumn(nullable=false) + * @var GH7180E + */ + public $e; +} + +/** + * @Entity + */ +class GH7180E +{ + /** + * @GeneratedValue() + * @Id + * @Column(type="integer") + * @var int + */ + public $id; + + /** + * @OneToOne(targetEntity=GH7180F::class) + * @JoinColumn(nullable=false) + * @var GH7180F + */ + public $f; +} + +/** + * @Entity + */ +class GH7180F +{ + /** + * @GeneratedValue() + * @Id + * @Column(type="integer") + * @var int + */ + public $id; + + /** + * @ManyToOne(targetEntity=GH7180D::class) + * @JoinColumn(nullable=true) + * @var GH7180D + */ + public $d; +} + +/** + * @Entity + */ +class GH7180G +{ + /** + * @GeneratedValue() + * @Id + * @Column(type="integer") + * @var int + */ + public $id; + + /** + * @ManyToOne(targetEntity=GH7180D::class) + * @JoinColumn(nullable=false) + * @var GH7180D + */ + public $d; +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH9192Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH9192Test.php new file mode 100644 index 00000000000..216c4f2dff9 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH9192Test.php @@ -0,0 +1,136 @@ +createSchemaForModels(GH9192A::class, GH9192B::class, GH9192C::class); + } + + public function testIssue(): void + { + $a = new GH9192A(); + + $b = new GH9192B(); + $b->a = $a; + $a->bs->add($b); + + $c = new GH9192C(); + $c->b = $b; + $b->cs->add($c); + + $a->c = $c; + + $this->_em->persist($a); + $this->_em->persist($b); + $this->_em->persist($c); + $this->_em->flush(); + + $this->expectNotToPerformAssertions(); + + $this->_em->remove($a); + $this->_em->flush(); + } +} + +/** + * @ORM\Entity + */ +class GH9192A +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue(strategy="AUTO") + * + * @var int + */ + public $id; + + /** + * @ORM\OneToMany(targetEntity="GH9192B", mappedBy="a", cascade={"remove"}) + * + * @var Collection + */ + public $bs; + + /** + * @ORM\OneToOne(targetEntity="GH9192C") + * @ORM\JoinColumn(nullable=true, onDelete="SET NULL") + * + * @var GH9192C + */ + public $c; + + public function __construct() + { + $this->bs = new ArrayCollection(); + } +} + +/** + * @ORM\Entity + */ +class GH9192B +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue(strategy="AUTO") + * + * @var int + */ + public $id; + + /** + * @ORM\OneToMany(targetEntity="GH9192C", mappedBy="b", cascade={"remove"}) + * + * @var Collection + */ + public $cs; + + /** + * @ORM\ManyToOne(targetEntity="GH9192A", inversedBy="bs") + * + * @var GH9192A + */ + public $a; + + public function __construct() + { + $this->cs = new ArrayCollection(); + } +} + +/** + * @ORM\Entity + */ +class GH9192C +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue(strategy="AUTO") + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity="GH9192B", inversedBy="cs") + * + * @var GH9192B + */ + public $b; +} diff --git a/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php b/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php new file mode 100644 index 00000000000..bba00d2d44e --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php @@ -0,0 +1,248 @@ + */ + private $nodes = []; + + /** @var TopologicalSort */ + private $topologicalSort; + + protected function setUp(): void + { + $this->topologicalSort = new TopologicalSort(); + } + + public function testSimpleOrdering(): void + { + $this->addNodes('C', 'B', 'A', 'E'); + + $this->addEdge('A', 'B'); + $this->addEdge('B', 'C'); + $this->addEdge('E', 'A'); + + // There is only 1 valid ordering for this constellation + self::assertSame(['E', 'A', 'B', 'C'], $this->computeResult()); + } + + public function testSkipOptionalEdgeToBreakCycle(): void + { + $this->addNodes('A', 'B'); + + $this->addEdge('A', 'B', true); + $this->addEdge('B', 'A', false); + + self::assertSame(['B', 'A'], $this->computeResult()); + } + + public function testBreakCycleByBacktracking(): void + { + $this->addNodes('A', 'B', 'C', 'D'); + + $this->addEdge('A', 'B'); + $this->addEdge('B', 'C', true); + $this->addEdge('C', 'D'); + $this->addEdge('D', 'A'); // closes the cycle + + // We can only break B -> C, so the result must be C -> D -> A -> B + self::assertSame(['C', 'D', 'A', 'B'], $this->computeResult()); + } + + public function testCycleRemovedByEliminatingLastOptionalEdge(): void + { + // The cycle-breaking algorithm is currently very naive. It breaks the cycle + // at the last optional edge while it backtracks. In this example, we might + // get away with one extra update if we'd break A->B; instead, we break up + // B->C and B->D. + + $this->addNodes('A', 'B', 'C', 'D'); + + $this->addEdge('A', 'B', true); + $this->addEdge('B', 'C', true); + $this->addEdge('C', 'A'); + $this->addEdge('B', 'D', true); + $this->addEdge('D', 'A'); + + self::assertSame(['C', 'D', 'A', 'B'], $this->computeResult()); + } + + public function testGH7180Example(): void + { + // Example given in https://github.com/doctrine/orm/pull/7180#issuecomment-381341943 + + $this->addNodes('E', 'F', 'D', 'G'); + + $this->addEdge('D', 'G'); + $this->addEdge('D', 'F', true); + $this->addEdge('F', 'E'); + $this->addEdge('E', 'D'); + + self::assertSame(['F', 'E', 'D', 'G'], $this->computeResult()); + } + + public function testCommitOrderingFromGH7259Test(): void + { + // this test corresponds to the GH7259Test::testPersistFileBeforeVersion functional test + $this->addNodes('A', 'B', 'C', 'D'); + + $this->addEdge('D', 'A'); + $this->addEdge('A', 'B'); + $this->addEdge('D', 'C'); + $this->addEdge('A', 'D', true); + + // There is only multiple valid ordering for this constellation, but + // the D -> A -> B ordering is important to break the cycle + // on the nullable link. + $correctOrders = [ + ['D', 'A', 'B', 'C'], + ['D', 'A', 'C', 'B'], + ['D', 'C', 'A', 'B'], + ]; + + self::assertContains($this->computeResult(), $correctOrders); + } + + public function testCommitOrderingFromGH8349Case1Test(): void + { + $this->addNodes('A', 'B', 'C', 'D'); + + $this->addEdge('D', 'A'); + $this->addEdge('A', 'B', true); + $this->addEdge('B', 'D', true); + $this->addEdge('B', 'C', true); + $this->addEdge('C', 'D', true); + + // Many orderings are possible here, but the bottom line is D must be before A (it's the only hard requirement). + $result = $this->computeResult(); + + $indexA = array_search('A', $result, true); + $indexD = array_search('D', $result, true); + self::assertTrue($indexD < $indexA); + } + + public function testCommitOrderingFromGH8349Case2Test(): void + { + $this->addNodes('A', 'B'); + + $this->addEdge('B', 'A'); + $this->addEdge('B', 'A', true); // interesting: We have two edges in that direction + $this->addEdge('A', 'B', true); + + // The B -> A requirement determines the result here + self::assertSame(['B', 'A'], $this->computeResult()); + } + + public function testNodesMaintainOrderWhenNoDepencency(): void + { + $this->addNodes('A', 'B', 'C'); + + // Nodes that are not constrained by dependencies shall maintain the order + // in which they were added + self::assertSame(['A', 'B', 'C'], $this->computeResult()); + } + + public function testDetectSmallCycle(): void + { + $this->addNodes('A', 'B'); + + $this->addEdge('A', 'B'); + $this->addEdge('B', 'A'); + + $this->expectException(CycleDetectedException::class); + $this->computeResult(); + } + + public function testMultipleEdges(): void + { + // There may be more than one association between two given entities. + // For the commit order, we only need to track this once, since the + // result is the same (one entity must be processed before the other). + // + // In case one of the associations is optional and the other one is not, + // we must honor the non-optional one, regardless of the order in which + // they were declared. + + $this->addNodes('A', 'B'); + + $this->addEdge('A', 'B', true); // optional comes first + $this->addEdge('A', 'B', false); + $this->addEdge('B', 'A', false); + $this->addEdge('B', 'A', true); // optional comes last + + // Both edges A -> B and B -> A are non-optional, so this is a cycle + // that cannot be broken. + + $this->expectException(CycleDetectedException::class); + $this->computeResult(); + } + + public function testDetectLargerCycleNotIncludingStartNode(): void + { + $this->addNodes('A', 'B', 'C', 'D'); + + $this->addEdge('A', 'B'); + $this->addEdge('B', 'C'); + $this->addEdge('C', 'D'); + $this->addEdge('D', 'B'); + + // The sort has to start with the last node being added to make it possible that + // the result is in the order the nodes were added (if permitted by edges). + // That means the cycle will be detected when starting at D, so it is D -> B -> C -> D. + + try { + $this->computeResult(); + } catch (CycleDetectedException $exception) { + self::assertEquals( + [$this->nodes['D'], $this->nodes['B'], $this->nodes['C'], $this->nodes['D']], + $exception->getCycle() + ); + } + } + + private function addNodes(string ...$names): void + { + foreach ($names as $name) { + $node = new Node($name); + $this->nodes[$name] = $node; + $this->topologicalSort->addNode($node); + } + } + + private function addEdge(string $from, string $to, bool $optional = false): void + { + $this->topologicalSort->addEdge($this->nodes[$from], $this->nodes[$to], $optional); + } + + /** + * @return list + */ + private function computeResult(): array + { + return array_map(static function (Node $n): string { + return $n->name; + }, array_values($this->topologicalSort->sort())); + } +} + +class Node +{ + /** @var string */ + public $name; + + public function __construct(string $name) + { + $this->name = $name; + } +} diff --git a/tests/Doctrine/Tests/OrmFunctionalTestCase.php b/tests/Doctrine/Tests/OrmFunctionalTestCase.php index b3b842f409e..07bf94eb654 100644 --- a/tests/Doctrine/Tests/OrmFunctionalTestCase.php +++ b/tests/Doctrine/Tests/OrmFunctionalTestCase.php @@ -421,6 +421,7 @@ protected function tearDown(): void $conn->executeStatement('DELETE FROM ecommerce_products_categories'); $conn->executeStatement('DELETE FROM ecommerce_products_related'); $conn->executeStatement('DELETE FROM ecommerce_carts'); + $conn->executeStatement('DELETE FROM ecommerce_customers WHERE mentor_id IS NOT NULL'); $conn->executeStatement('DELETE FROM ecommerce_customers'); $conn->executeStatement('DELETE FROM ecommerce_features'); $conn->executeStatement('DELETE FROM ecommerce_products');