From 33675ff4a96c1b4b51e119bd95a5fb03d9518a14 Mon Sep 17 00:00:00 2001 From: Michael Roterman Date: Tue, 6 Jun 2023 18:18:26 +0200 Subject: [PATCH 1/2] fix: Update baseline and assertions for OneToManyPersister --- .../ORM/Persisters/Collection/OneToManyPersister.php | 9 ++++++++- psalm-baseline.xml | 8 -------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php index db5024d257b..70e195e6ec2 100644 --- a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php +++ b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php @@ -16,6 +16,7 @@ use function array_values; use function assert; use function implode; +use function is_int; use function is_string; /** @@ -183,7 +184,11 @@ private function deleteEntityCollection(PersistentCollection $collection): int $statement = 'DELETE FROM ' . $this->quoteStrategy->getTableName($targetClass, $this->platform) . ' WHERE ' . implode(' = ? AND ', $columns) . ' = ?'; - return $this->conn->executeStatement($statement, $parameters); + $numAffected = $this->conn->executeStatement($statement, $parameters); + + assert(is_int($numAffected)); + + return $numAffected; } /** @@ -247,6 +252,8 @@ private function deleteJoinedEntityCollection(PersistentCollection $collection): $this->conn->executeStatement($statement); + assert(is_int($numDeleted)); + return $numDeleted; } } diff --git a/psalm-baseline.xml b/psalm-baseline.xml index b480604632d..aab77a75ede 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1236,14 +1236,6 @@ $mapping['indexBy'] => $index, ]]]> - - $numDeleted - conn->executeStatement($statement, $parameters)]]> - - - int - int - getOwner()]]> getOwner()]]> From 3c0d140e525747c2bcecb8686fc62b5b78e916b0 Mon Sep 17 00:00:00 2001 From: Michael Roterman Date: Tue, 6 Jun 2023 18:19:20 +0200 Subject: [PATCH 2/2] OneToManyPersister does not take custom identifier types into account for orphan removal. In my case a custom doctrine type of Uuid object is converted to string by simply casting it, resulting in a hex DELETE FROM x WHERE id = ? query, whilst it should've been converted along the way to it's binary representation. This leads to no deletions being made at all as you would expect making use of doctrine custom type's as an identifier. This commit fixes usage of ramsey/uuid or symfony/uid as custom id types when making use of orphan removal. --- .../Collection/OneToManyPersister.php | 4 +- .../Tests/ORM/Functional/GH10747Test.php | 195 ++++++++++++++++++ 2 files changed, 198 insertions(+), 1 deletion(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/GH10747Test.php diff --git a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php index 70e195e6ec2..f39415fc0c9 100644 --- a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php +++ b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php @@ -175,16 +175,18 @@ private function deleteEntityCollection(PersistentCollection $collection): int $targetClass = $this->em->getClassMetadata($mapping['targetEntity']); $columns = []; $parameters = []; + $types = []; foreach ($targetClass->associationMappings[$mapping['mappedBy']]['joinColumns'] as $joinColumn) { $columns[] = $this->quoteStrategy->getJoinColumnName($joinColumn, $targetClass, $this->platform); $parameters[] = $identifier[$sourceClass->getFieldForColumn($joinColumn['referencedColumnName'])]; + $types[] = PersisterHelper::getTypeOfColumn($joinColumn['referencedColumnName'], $sourceClass, $this->em); } $statement = 'DELETE FROM ' . $this->quoteStrategy->getTableName($targetClass, $this->platform) . ' WHERE ' . implode(' = ? AND ', $columns) . ' = ?'; - $numAffected = $this->conn->executeStatement($statement, $parameters); + $numAffected = $this->conn->executeStatement($statement, $parameters, $types); assert(is_int($numAffected)); diff --git a/tests/Doctrine/Tests/ORM/Functional/GH10747Test.php b/tests/Doctrine/Tests/ORM/Functional/GH10747Test.php new file mode 100644 index 00000000000..019449a4164 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/GH10747Test.php @@ -0,0 +1,195 @@ +setUpEntitySchema([GH10747Article::class, GH10747Credit::class]); + } + + public function testOrphanedOneToManyDeletesCollection(): void + { + $object = new GH10747Article( + new CustomIdObject('article') + ); + + $creditOne = new GH10747Credit( + $object, + 'credit1' + ); + + $creditTwo = new GH10747Credit( + $object, + 'credit2' + ); + + $object->setCredits(new ArrayCollection([$creditOne, $creditTwo])); + + $this->_em->persist($object); + $this->_em->persist($creditOne); + $this->_em->persist($creditTwo); + $this->_em->flush(); + + $id = $object->id; + + $object2 = $this->_em->find(GH10747Article::class, $id); + + $creditThree = new GH10747Credit( + $object2, + 'credit3' + ); + + $object2->setCredits(new ArrayCollection([$creditThree])); + + $this->_em->persist($object2); + $this->_em->persist($creditThree); + $this->_em->flush(); + + $currentDatabaseCredits = $this->_em->createQueryBuilder() + ->select('c.id') + ->from(GH10747Credit::class, 'c') + ->getQuery() + ->execute(); + + self::assertCount(1, $currentDatabaseCredits); + } +} + +/** + * @Entity + * @Table + */ +class GH10747Article +{ + /** + * @Id + * @Column(type="Doctrine\Tests\ORM\Functional\GH10747CustomIdObjectHashType") + * @var CustomIdObject + */ + public $id; + + /** + * @ORM\OneToMany(targetEntity="GH10747Credit", mappedBy="article", orphanRemoval=true) + * + * @var Collection + */ + public $credits; + + public function __construct(CustomIdObject $id) + { + $this->id = $id; + $this->credits = new ArrayCollection(); + } + + public function setCredits(Collection $credits): void + { + $this->credits = $credits; + } + + /** @return Collection */ + public function getCredits(): Collection + { + return $this->credits; + } +} + +/** + * @Entity + * @Table + */ +class GH10747Credit +{ + /** + * @ORM\Column(type="integer") + * @ORM\GeneratedValue() + * + * @Id() + * @var int|null + */ + public $id = null; + + /** @var string */ + public $name; + + /** + * @ORM\ManyToOne(targetEntity="GH10747Article", inversedBy="credits") + * + * @var GH10747Article + */ + public $article; + + public function __construct(GH10747Article $article, string $name) + { + $this->article = $article; + $this->name = $name; + } +} + +class GH10747CustomIdObjectHashType extends DBALType +{ + /** + * {@inheritDoc} + */ + public function convertToDatabaseValue($value, AbstractPlatform $platform) + { + return $value->id . '_test'; + } + + /** + * {@inheritDoc} + */ + public function convertToPHPValue($value, AbstractPlatform $platform) + { + return new CustomIdObject(str_replace('_test', '', $value)); + } + + /** + * {@inheritDoc} + */ + public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform) + { + if (method_exists($platform, 'getStringTypeDeclarationSQL')) { + return $platform->getStringTypeDeclarationSQL($fieldDeclaration); + } + + return $platform->getVarcharTypeDeclarationSQL($fieldDeclaration); + } + + /** + * {@inheritDoc} + */ + public function getName() + { + return self::class; + } +}