diff --git a/CHANGELOG.md b/CHANGELOG.md index ea079941e4..4abd3a9f8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,7 @@ a release. ### Fixed - References: fixed condition in `XML` Driver that did not allow to retrieve from the entity definition the `mappedBy` and `inversedBy` fields. - Fix bug collecting metadata for inherited mapped classes +- Tree: Fix issue with null ids inserted into closure table when persisting more than one type of entity using the closure strategy (#2653) ## [3.12.0] - 2023-07-08 ### Added diff --git a/src/Tree/Strategy/ORM/Closure.php b/src/Tree/Strategy/ORM/Closure.php index d455224df6..79fdad74c4 100644 --- a/src/Tree/Strategy/ORM/Closure.php +++ b/src/Tree/Strategy/ORM/Closure.php @@ -285,12 +285,20 @@ public function processPostPersist($em, $entity, AdapterInterface $ea) $uow = $em->getUnitOfWork(); $emHash = spl_object_id($em); - while ($node = array_shift($this->pendingChildNodeInserts[$emHash])) { + foreach ($this->pendingChildNodeInserts[$emHash] as $node) { $meta = $em->getClassMetadata(get_class($node)); - $config = $this->listener->getConfiguration($em, $meta->getName()); - $identifier = $meta->getSingleIdentifierFieldName(); $nodeId = $meta->getReflectionProperty($identifier)->getValue($node); + + if (null === $nodeId) { + // Do not update if the node has not been persisted yet. + continue; + } + + // The closure for this node will now be inserted. Remove the node from the list of pending inserts to indicate this. + unset($this->pendingChildNodeInserts[$emHash][spl_object_id($node)]); + + $config = $this->listener->getConfiguration($em, $meta->getName()); $parent = $meta->getReflectionProperty($config['parent'])->getValue($node); $closureClass = $config['closure']; @@ -349,6 +357,13 @@ public function processPostPersist($em, $entity, AdapterInterface $ea) } foreach ($entries as $closure) { + $existingClosure = $em->getRepository($closureClass)->findBy([ + $ancestorColumnName => $closure[$ancestorColumnName], + $descendantColumnName => $closure[$descendantColumnName], + ]); + if ([] !== $existingClosure) { + continue; + } if (!$em->getConnection()->insert($closureTable, $closure)) { throw new RuntimeException('Failed to insert new Closure record'); } @@ -500,13 +515,15 @@ protected function getJoinColumnFieldName($association) */ protected function setLevelFieldOnPendingNodes(ObjectManager $em) { - if (!empty($this->pendingNodesLevelProcess)) { + while (!empty($this->pendingNodesLevelProcess)) { + // Nodes need to be processed one class at a time. Each iteration through the while loop will process one type, starting with the first item on the list. $first = array_slice($this->pendingNodesLevelProcess, 0, 1); $first = array_shift($first); assert(null !== $first); - $meta = $em->getClassMetadata(get_class($first)); + $className = get_class($first); + $meta = $em->getClassMetadata($className); unset($first); $identifier = $meta->getIdentifier(); $mapping = $meta->getFieldMapping($identifier[0]); @@ -516,6 +533,10 @@ protected function setLevelFieldOnPendingNodes(ObjectManager $em) $uow = $em->getUnitOfWork(); foreach ($this->pendingNodesLevelProcess as $node) { + if (get_class($node) !== $className) { + // Only process nodes of the same type as the first element + continue; + } $children = $em->getRepository($meta->getName())->children($node); foreach ($children as $child) { @@ -543,6 +564,14 @@ protected function setLevelFieldOnPendingNodes(ObjectManager $em) // Now we update levels foreach ($this->pendingNodesLevelProcess as $nodeId => $node) { + if (get_class($node) !== $className) { + // Only process nodes of the same time as the first element + continue; + } + + // This node will now be processed. Therefore, remove it from the list of pending nodes. + unset($this->pendingNodesLevelProcess[$nodeId]); + // Update new level $level = $levels[$nodeId]; $levelProp = $meta->getReflectionProperty($config['level']); @@ -555,8 +584,6 @@ protected function setLevelFieldOnPendingNodes(ObjectManager $em) $levelProp->setValue($node, $level); $uow->setOriginalEntityProperty(spl_object_id($node), $config['level'], $level); } - - $this->pendingNodesLevelProcess = []; } } diff --git a/tests/Gedmo/Tree/Fixture/Issue2652/Category2.php b/tests/Gedmo/Tree/Fixture/Issue2652/Category2.php new file mode 100644 index 0000000000..50e6e67642 --- /dev/null +++ b/tests/Gedmo/Tree/Fixture/Issue2652/Category2.php @@ -0,0 +1,119 @@ + http://www.gediminasm.org + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Gedmo\Tests\Tree\Fixture\Issue2652; + +use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\Collections\Collection; +use Doctrine\DBAL\Types\Types; +use Doctrine\ORM\Mapping as ORM; +use Gedmo\Mapping\Annotation as Gedmo; +use Gedmo\Tree\Entity\Repository\ClosureTreeRepository; + +/** + * @Gedmo\Tree(type="closure") + * @Gedmo\TreeClosure(class="Category2Closure") + * + * @ORM\Entity(repositoryClass="Gedmo\Tree\Entity\Repository\ClosureTreeRepository") + */ +#[ORM\Entity(repositoryClass: ClosureTreeRepository::class)] +#[Gedmo\Tree(type: 'closure')] +#[Gedmo\TreeClosure(class: Category2Closure::class)] +class Category2 +{ + /** + * @var int|null + * + * @ORM\Id + * @ORM\GeneratedValue + * @ORM\Column(type="integer") + */ + #[ORM\Id] + #[ORM\GeneratedValue] + #[ORM\Column(type: Types::INTEGER)] + private $id; + + /** + * @ORM\Column(name="title", type="string", length=64) + */ + #[ORM\Column(name: 'title', type: Types::STRING, length: 64)] + private ?string $title = null; + + /** + * @ORM\Column(name="level", type="integer", nullable=true) + * + * @Gedmo\TreeLevel + */ + #[ORM\Column(name: 'level', type: Types::INTEGER, nullable: true)] + #[Gedmo\TreeLevel] + private ?int $level = null; + + /** + * @Gedmo\TreeParent + * + * @ORM\JoinColumn(name="parent_id", referencedColumnName="id", onDelete="CASCADE") + * @ORM\ManyToOne(targetEntity="Category2", inversedBy="children") + */ + #[ORM\ManyToOne(targetEntity: self::class, inversedBy: 'children')] + #[ORM\JoinColumn(name: 'category2_id', referencedColumnName: 'id', onDelete: 'CASCADE')] + #[Gedmo\TreeParent] + private ?\Gedmo\Tests\Tree\Fixture\Issue2652\Category2 $parent = null; + + /** + * @var Collection + */ + private $closures; + + public function __construct() + { + $this->closures = new ArrayCollection(); + } + + public function getId(): ?int + { + return $this->id; + } + + public function setTitle(?string $title): void + { + $this->title = $title; + } + + public function getTitle(): ?string + { + return $this->title; + } + + public function setParent(?self $parent = null): void + { + $this->parent = $parent; + } + + public function getParent(): ?self + { + return $this->parent; + } + + public function addClosure(Category2Closure $closure): void + { + $this->closures[] = $closure; + } + + public function setLevel(?int $level): void + { + $this->level = $level; + } + + public function getLevel(): ?int + { + return $this->level; + } +} diff --git a/tests/Gedmo/Tree/Fixture/Issue2652/Category2Closure.php b/tests/Gedmo/Tree/Fixture/Issue2652/Category2Closure.php new file mode 100644 index 0000000000..751c00f158 --- /dev/null +++ b/tests/Gedmo/Tree/Fixture/Issue2652/Category2Closure.php @@ -0,0 +1,46 @@ + http://www.gediminasm.org + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Gedmo\Tests\Tree\Fixture\Issue2652; + +use Doctrine\ORM\Mapping as ORM; +use Gedmo\Tree\Entity\MappedSuperclass\AbstractClosure; + +/** + * @ORM\Entity + * @ORM\Table( + * indexes={@ORM\Index(name="closure_category2_depth_idx", columns={"depth"})}, + * uniqueConstraints={@ORM\UniqueConstraint(name="closure_category2_unique_idx", columns={ + * "ancestor", "descendant" + * })} + * ) + */ +#[ORM\Entity] +#[ORM\UniqueConstraint(name: 'closure_category2_unique_idx', columns: ['ancestor', 'descendant'])] +#[ORM\Index(name: 'closure_category2_depth_idx', columns: ['depth'])] +class Category2Closure extends AbstractClosure +{ + /** + * @ORM\ManyToOne(targetEntity="Gedmo\Tests\Tree\Fixture\Issue2652\Category2") + * @ORM\JoinColumn(name="ancestor", referencedColumnName="id", nullable=false, onDelete="CASCADE") + */ + #[ORM\ManyToOne(targetEntity: Category2::class)] + #[ORM\JoinColumn(name: 'ancestor', referencedColumnName: 'id', nullable: false, onDelete: 'CASCADE')] + protected $ancestor; + + /** + * @ORM\ManyToOne(targetEntity="Gedmo\Tests\Tree\Fixture\Issue2652\Category2") + * @ORM\JoinColumn(name="descendant", referencedColumnName="id", nullable=false, onDelete="CASCADE") + */ + #[ORM\ManyToOne(targetEntity: Category2::class)] + #[ORM\JoinColumn(name: 'descendant', referencedColumnName: 'id', nullable: false, onDelete: 'CASCADE')] + protected $descendant; +} diff --git a/tests/Gedmo/Tree/Issue/Issue2652Test.php b/tests/Gedmo/Tree/Issue/Issue2652Test.php new file mode 100644 index 0000000000..b7255025f1 --- /dev/null +++ b/tests/Gedmo/Tree/Issue/Issue2652Test.php @@ -0,0 +1,94 @@ + http://www.gediminasm.org + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Gedmo\Tests\Tree; + +use Doctrine\Common\EventManager; +use Gedmo\Tests\Tool\BaseTestCaseORM; +use Gedmo\Tests\Tree\Fixture\Closure\Category; +use Gedmo\Tests\Tree\Fixture\Closure\CategoryClosure; +use Gedmo\Tests\Tree\Fixture\Issue2652\Category2; +use Gedmo\Tests\Tree\Fixture\Issue2652\Category2Closure; +use Gedmo\Tree\TreeListener; + +/** + * These are tests for Tree behavior + * + * @author Gustavo Adrian + * @author Gediminas Morkevicius + */ +final class Issue2652Test extends BaseTestCaseORM +{ + private const CATEGORY = Category::class; + private const CLOSURE = CategoryClosure::class; + private const CATEGORY2 = Category2::class; + private const CLOSURE2 = Category2Closure::class; + + private TreeListener $listener; + + protected function setUp(): void + { + parent::setUp(); + + $this->listener = new TreeListener(); + + $evm = new EventManager(); + $evm->addEventSubscriber($this->listener); + + $this->getDefaultMockSqliteEntityManager($evm); + } + + public function testAddMultipleEntityTypes(): void + { + $food = new Category(); + $food->setTitle('Food'); + $this->em->persist($food); + + $monkey = new Category2(); + $monkey->setTitle('Monkey'); + $this->em->persist($monkey); + + // Before issue #2652 was fixed, null values would be inserted into the closure table at this next line and an exception would be thrown + $this->em->flush(); + + // Ensure that the closures were correctly inserted + $repo = $this->em->getRepository(self::CATEGORY); + + $food = $repo->findOneBy(['title' => 'Food']); + $dql = 'SELECT c FROM '.self::CLOSURE.' c'; + $dql .= ' WHERE c.ancestor = :ancestor'; + $query = $this->em->createQuery($dql); + $query->setParameter('ancestor', $food); + + $foodClosures = $query->getResult(); + static::assertCount(1, $foodClosures); + + $repo = $this->em->getRepository(self::CATEGORY2); + $monkey = $repo->findOneBy(['title' => 'Monkey']); + $dql = 'SELECT c FROM '.self::CLOSURE2.' c'; + $dql .= ' WHERE c.ancestor = :ancestor'; + $query = $this->em->createQuery($dql); + $query->setParameter('ancestor', $monkey); + + $monkeyClosures = $query->getResult(); + static::assertCount(1, $monkeyClosures); + } + + protected function getUsedEntityFixtures(): array + { + return [ + self::CATEGORY, + self::CLOSURE, + self::CATEGORY2, + self::CLOSURE2, + ]; + } +}