diff --git a/lib/Doctrine/ORM/Internal/TopologicalSort.php b/lib/Doctrine/ORM/Internal/TopologicalSort.php index 2bf1624ced8..aed71f3b206 100644 --- a/lib/Doctrine/ORM/Internal/TopologicalSort.php +++ b/lib/Doctrine/ORM/Internal/TopologicalSort.php @@ -7,8 +7,6 @@ use Doctrine\ORM\Internal\TopologicalSort\CycleDetectedException; use function array_keys; -use function array_reverse; -use function array_unshift; use function spl_object_id; /** @@ -93,18 +91,14 @@ public function addEdge($from, $to, bool $optional): void /** * 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. + * A and B, then B will be listed before A in the result. Visually speaking, when ordering + * the nodes in the result order from left to right, all edges point to the left. * * @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) { + foreach (array_keys($this->nodes) as $oid) { if ($this->states[$oid] === self::NOT_VISITED) { $this->visit($oid); } @@ -147,7 +141,7 @@ private function visit(int $oid): void } // 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 + // is to backtrack 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]); @@ -160,6 +154,6 @@ private function visit(int $oid): void // So we're done with this vertex as well. $this->states[$oid] = self::VISITED; - array_unshift($this->sortResult, $this->nodes[$oid]); + $this->sortResult[] = $this->nodes[$oid]; } } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index b1fef6d2012..b3285a8845c 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1378,9 +1378,10 @@ private function computeInsertExecutionOrder(): array $joinColumns = reset($assoc['joinColumns']); $isNullable = ! isset($joinColumns['nullable']) || $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); + // Add dependency. The dependency direction implies that "$entity depends on $targetEntity". The + // topological sort result will output the depended-upon nodes first, which means we can insert + // entities in that order. + $sort->addEdge($entity, $targetEntity, $isNullable); } } @@ -1432,9 +1433,10 @@ private function computeDeleteExecutionOrder(): array 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); + // Add dependency. The dependency direction implies that "$targetEntity depends on $entity + // being deleted first". The topological sort will output the depended-upon nodes first, + // so we can work through the result in the returned order. + $sort->addEdge($targetEntity, $entity, false); } } diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH11058Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH11058Test.php new file mode 100644 index 00000000000..a40ef8c2763 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH11058Test.php @@ -0,0 +1,146 @@ +setUpEntitySchema([ + GH11058Parent::class, + GH11058Child::class, + ]); + } + + public function testChildrenInsertedInOrderOfPersistCalls1WhenParentPersistedLast(): void + { + [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities(); + + $this->_em->persist($child1); + $this->_em->persist($child2); + $this->_em->persist($parent); + $this->_em->flush(); + + self::assertTrue($child1->id < $child2->id); + } + + public function testChildrenInsertedInOrderOfPersistCalls2WhenParentPersistedLast(): void + { + [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities(); + + $this->_em->persist($child2); + $this->_em->persist($child1); + $this->_em->persist($parent); + $this->_em->flush(); + + self::assertTrue($child2->id < $child1->id); + } + + public function testChildrenInsertedInOrderOfPersistCalls1WhenParentPersistedFirst(): void + { + [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities(); + + $this->_em->persist($parent); + $this->_em->persist($child1); + $this->_em->persist($child2); + $this->_em->flush(); + + self::assertTrue($child1->id < $child2->id); + } + + public function testChildrenInsertedInOrderOfPersistCalls2WhenParentPersistedFirst(): void + { + [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities(); + + $this->_em->persist($parent); + $this->_em->persist($child2); + $this->_em->persist($child1); + $this->_em->flush(); + + self::assertTrue($child2->id < $child1->id); + } + + private function createParentWithTwoChildEntities(): array + { + $parent = new GH11058Parent(); + $child1 = new GH11058Child(); + $child2 = new GH11058Child(); + + $parent->addChild($child1); + $parent->addChild($child2); + + return [$parent, $child1, $child2]; + } +} + +/** + * @ORM\Entity() + */ +class GH11058Parent +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\OneToMany(targetEntity="GH11058Child", mappedBy="parent") + * + * @var Collection + */ + public $children; + + public function __construct() + { + $this->children = new ArrayCollection(); + } + + public function addChild(GH11058Child $child): void + { + if (! $this->children->contains($child)) { + $this->children->add($child); + $child->setParent($this); + } + } +} + +/** + * @ORM\Entity() + */ +class GH11058Child +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity="GH11058Parent", inversedBy="children") + * + * @var GH11058Parent + */ + public $parent; + + public function setParent(GH11058Parent $parent): void + { + $this->parent = $parent; + $parent->addChild($this); + } +} diff --git a/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php b/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php index bba00d2d44e..d0dd9178c15 100644 --- a/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php +++ b/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php @@ -34,7 +34,7 @@ public function testSimpleOrdering(): void $this->addEdge('E', 'A'); // There is only 1 valid ordering for this constellation - self::assertSame(['E', 'A', 'B', 'C'], $this->computeResult()); + self::assertSame(['C', 'B', 'A', 'E'], $this->computeResult()); } public function testSkipOptionalEdgeToBreakCycle(): void @@ -44,7 +44,7 @@ public function testSkipOptionalEdgeToBreakCycle(): void $this->addEdge('A', 'B', true); $this->addEdge('B', 'A', false); - self::assertSame(['B', 'A'], $this->computeResult()); + self::assertSame(['A', 'B'], $this->computeResult()); } public function testBreakCycleByBacktracking(): void @@ -57,7 +57,7 @@ public function testBreakCycleByBacktracking(): void $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()); + self::assertSame(['B', 'A', 'D', 'C'], $this->computeResult()); } public function testCycleRemovedByEliminatingLastOptionalEdge(): void @@ -75,7 +75,7 @@ public function testCycleRemovedByEliminatingLastOptionalEdge(): void $this->addEdge('B', 'D', true); $this->addEdge('D', 'A'); - self::assertSame(['C', 'D', 'A', 'B'], $this->computeResult()); + self::assertSame(['B', 'A', 'C', 'D'], $this->computeResult()); } public function testGH7180Example(): void @@ -89,7 +89,7 @@ public function testGH7180Example(): void $this->addEdge('F', 'E'); $this->addEdge('E', 'D'); - self::assertSame(['F', 'E', 'D', 'G'], $this->computeResult()); + self::assertSame(['G', 'D', 'E', 'F'], $this->computeResult()); } public function testCommitOrderingFromGH7259Test(): void @@ -106,9 +106,9 @@ public function testCommitOrderingFromGH7259Test(): void // 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'], + ['C', 'B', 'A', 'D'], + ['B', 'C', 'A', 'D'], + ['B', 'A', 'C', 'D'], ]; self::assertContains($this->computeResult(), $correctOrders); @@ -124,12 +124,12 @@ public function testCommitOrderingFromGH8349Case1Test(): void $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). + // Many orderings are possible here, but the bottom line is A must be before D (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); + self::assertTrue($indexD > $indexA); } public function testCommitOrderingFromGH8349Case2Test(): void @@ -141,7 +141,7 @@ public function testCommitOrderingFromGH8349Case2Test(): void $this->addEdge('A', 'B', true); // The B -> A requirement determines the result here - self::assertSame(['B', 'A'], $this->computeResult()); + self::assertSame(['A', 'B'], $this->computeResult()); } public function testNodesMaintainOrderWhenNoDepencency(): void @@ -153,6 +153,58 @@ public function testNodesMaintainOrderWhenNoDepencency(): void self::assertSame(['A', 'B', 'C'], $this->computeResult()); } + public function testNodesReturnedInDepthFirstOrder(): void + { + $this->addNodes('A', 'B', 'C'); + $this->addEdge('A', 'B'); + $this->addEdge('A', 'C'); + + // We start on A and find that it has two dependencies on B and C, + // added (as dependencies) in that order. + // So, first we continue the DFS on B, because that edge was added first. + // This gives the result order B, C, A. + self::assertSame(['B', 'C', 'A'], $this->computeResult()); + } + + public function testNodesReturnedInDepthFirstOrderWithEdgesInDifferentOrderThanNodes(): void + { + $this->addNodes('A', 'B', 'C'); + $this->addEdge('A', 'C'); + $this->addEdge('A', 'B'); + + // This is like testNodesReturnedInDepthFirstOrder, but it shows that for the two + // nodes B and C that A depends upon, the result will follow the order in which + // the edges were added. + self::assertSame(['C', 'B', 'A'], $this->computeResult()); + } + + public function testNodesReturnedInDepthFirstOrderWithDependingNodeLast(): void + { + $this->addNodes('B', 'C', 'A'); + $this->addEdge('A', 'B'); + $this->addEdge('A', 'C'); + + // This again is like testNodesReturnedInDepthFirstOrder, but this + // time the node A that depends on B and C is added as the last node. + // That means processing can go over B and C in the order they were given. + // The order in which edges are added is not relevant (!), since at the time + // the edges are evaluated, the nodes they point to have already been finished. + self::assertSame(['B', 'C', 'A'], $this->computeResult()); + } + + public function testNodesReturnedInDepthFirstOrderWithDependingNodeLastAndEdgeOrderInversed(): void + { + $this->addNodes('B', 'C', 'A'); + $this->addEdge('A', 'C'); + $this->addEdge('A', 'B'); + + // This again is like testNodesReturnedInDepthFirstOrderWithDependingNodeLast, but adds + // the edges in the opposing order. Still, the result order is the same (!). + // This may be surprising when comparing with testNodesReturnedInDepthFirstOrderWithEdgesInDifferentOrderThanNodes, + // where the result order depends upon the _edge_ order. + self::assertSame(['B', 'C', 'A'], $this->computeResult()); + } + public function testDetectSmallCycle(): void { $this->addNodes('A', 'B'); @@ -205,7 +257,7 @@ public function testDetectLargerCycleNotIncludingStartNode(): void $this->computeResult(); } catch (CycleDetectedException $exception) { self::assertEquals( - [$this->nodes['D'], $this->nodes['B'], $this->nodes['C'], $this->nodes['D']], + [$this->nodes['B'], $this->nodes['C'], $this->nodes['D'], $this->nodes['B']], $exception->getCycle() ); }