From 3e3de05d4ca94a7c5ab658bc7b5ff91e407327dc Mon Sep 17 00:00:00 2001 From: Valentin SALMERON Date: Fri, 21 Jun 2024 08:55:25 +0200 Subject: [PATCH 1/6] Fix issue #11481 --- .../Collection/ManyToManyPersister.php | 6 +- .../Entity/BasicEntityPersister.php | 61 +----------- src/Persisters/Traits/ResolveValuesHelper.php | 74 +++++++++++++++ tests/Tests/Models/Enums/Book.php | 52 ++++++++++ tests/Tests/Models/Enums/BookCategory.php | 52 ++++++++++ tests/Tests/Models/Enums/BookColor.php | 11 +++ tests/Tests/Models/Enums/Library.php | 50 ++++++++++ tests/Tests/ORM/Functional/EnumTest.php | 95 +++++++++++++++++++ 8 files changed, 341 insertions(+), 60 deletions(-) create mode 100644 src/Persisters/Traits/ResolveValuesHelper.php create mode 100644 tests/Tests/Models/Enums/Book.php create mode 100644 tests/Tests/Models/Enums/BookCategory.php create mode 100644 tests/Tests/Models/Enums/BookColor.php create mode 100644 tests/Tests/Models/Enums/Library.php diff --git a/src/Persisters/Collection/ManyToManyPersister.php b/src/Persisters/Collection/ManyToManyPersister.php index 7cf993d5997..c9d6b3ec454 100644 --- a/src/Persisters/Collection/ManyToManyPersister.php +++ b/src/Persisters/Collection/ManyToManyPersister.php @@ -15,10 +15,12 @@ use Doctrine\ORM\Mapping\ManyToManyAssociationMapping; use Doctrine\ORM\PersistentCollection; use Doctrine\ORM\Persisters\SqlValueVisitor; +use Doctrine\ORM\Persisters\Traits\ResolveValuesHelper; use Doctrine\ORM\Query; use Doctrine\ORM\Utility\PersisterHelper; use function array_fill; +use function array_merge; use function array_pop; use function assert; use function count; @@ -32,6 +34,8 @@ */ class ManyToManyPersister extends AbstractCollectionPersister { + use ResolveValuesHelper; + public function delete(PersistentCollection $collection): void { $mapping = $this->getMapping($collection); @@ -249,7 +253,7 @@ public function loadCriteria(PersistentCollection $collection, Criteria $criteri $whereClauses[] = sprintf('te.%s %s NULL', $field, $operator === Comparison::EQ ? 'IS' : 'IS NOT'); } else { $whereClauses[] = sprintf('te.%s %s ?', $field, $operator); - $params[] = $value; + $params = array_merge($params, $this->getValues($value)); $paramTypes[] = PersisterHelper::getTypeOfField($name, $targetClass, $this->em)[0]; } } diff --git a/src/Persisters/Entity/BasicEntityPersister.php b/src/Persisters/Entity/BasicEntityPersister.php index 377e03ce274..399abccee81 100644 --- a/src/Persisters/Entity/BasicEntityPersister.php +++ b/src/Persisters/Entity/BasicEntityPersister.php @@ -4,7 +4,6 @@ namespace Doctrine\ORM\Persisters\Entity; -use BackedEnum; use Doctrine\Common\Collections\Criteria; use Doctrine\Common\Collections\Expr\Comparison; use Doctrine\Common\Collections\Order; @@ -31,7 +30,7 @@ use Doctrine\ORM\Persisters\Exception\UnrecognizedField; use Doctrine\ORM\Persisters\SqlExpressionVisitor; use Doctrine\ORM\Persisters\SqlValueVisitor; -use Doctrine\ORM\Proxy\DefaultProxyClassNameResolver; +use Doctrine\ORM\Persisters\Traits\ResolveValuesHelper; use Doctrine\ORM\Query; use Doctrine\ORM\Query\QueryException; use Doctrine\ORM\Query\ResultSetMapping; @@ -53,7 +52,6 @@ use function count; use function implode; use function is_array; -use function is_object; use function reset; use function spl_object_id; use function sprintf; @@ -99,6 +97,7 @@ class BasicEntityPersister implements EntityPersister { use LockSqlHelper; + use ResolveValuesHelper; /** @var array */ private static array $comparisonMap = [ @@ -1912,62 +1911,6 @@ private function getArrayBindingType(ParameterType|int|string $type): ArrayParam }; } - /** - * Retrieves the parameters that identifies a value. - * - * @return mixed[] - */ - private function getValues(mixed $value): array - { - if (is_array($value)) { - $newValue = []; - - foreach ($value as $itemValue) { - $newValue = array_merge($newValue, $this->getValues($itemValue)); - } - - return [$newValue]; - } - - return $this->getIndividualValue($value); - } - - /** - * Retrieves an individual parameter value. - * - * @psalm-return list - */ - private function getIndividualValue(mixed $value): array - { - if (! is_object($value)) { - return [$value]; - } - - if ($value instanceof BackedEnum) { - return [$value->value]; - } - - $valueClass = DefaultProxyClassNameResolver::getClass($value); - - if ($this->em->getMetadataFactory()->isTransient($valueClass)) { - return [$value]; - } - - $class = $this->em->getClassMetadata($valueClass); - - if ($class->isIdentifierComposite) { - $newValue = []; - - foreach ($class->getIdentifierValues($value) as $innerValue) { - $newValue = array_merge($newValue, $this->getValues($innerValue)); - } - - return $newValue; - } - - return [$this->em->getUnitOfWork()->getSingleIdentifierValue($value)]; - } - public function exists(object $entity, Criteria|null $extraConditions = null): bool { $criteria = $this->class->getIdentifierValues($entity); diff --git a/src/Persisters/Traits/ResolveValuesHelper.php b/src/Persisters/Traits/ResolveValuesHelper.php new file mode 100644 index 00000000000..edef7f04829 --- /dev/null +++ b/src/Persisters/Traits/ResolveValuesHelper.php @@ -0,0 +1,74 @@ +getValues($itemValue)); + } + + return [$newValue]; + } + + return $this->getIndividualValue($value); + } + + /** + * Retrieves an individual parameter value. + * + * @psalm-return list + */ + private function getIndividualValue(mixed $value): array + { + if (! is_object($value)) { + return [$value]; + } + + if ($value instanceof BackedEnum) { + return [$value->value]; + } + + $valueClass = DefaultProxyClassNameResolver::getClass($value); + + if ($this->em->getMetadataFactory()->isTransient($valueClass)) { + return [$value]; + } + + $class = $this->em->getClassMetadata($valueClass); + + if ($class->isIdentifierComposite) { + $newValue = []; + + foreach ($class->getIdentifierValues($value) as $innerValue) { + $newValue = array_merge($newValue, $this->getValues($innerValue)); + } + + return $newValue; + } + + return [$this->em->getUnitOfWork()->getSingleIdentifierValue($value)]; + } +} diff --git a/tests/Tests/Models/Enums/Book.php b/tests/Tests/Models/Enums/Book.php new file mode 100644 index 00000000000..ec7f960927a --- /dev/null +++ b/tests/Tests/Models/Enums/Book.php @@ -0,0 +1,52 @@ +categories = new ArrayCollection(); + } + + public function setLibrary(Library $library): void + { + $this->library = $library; + } + + public function addCategory(BookCategory $bookCategory): void + { + $this->categories->add($bookCategory); + $bookCategory->addBook($this); + } +} diff --git a/tests/Tests/Models/Enums/BookCategory.php b/tests/Tests/Models/Enums/BookCategory.php new file mode 100644 index 00000000000..4096a8a0b1c --- /dev/null +++ b/tests/Tests/Models/Enums/BookCategory.php @@ -0,0 +1,52 @@ +books = new ArrayCollection(); + } + + public function addBook(Book $book): void + { + $this->books->add($book); + } + + public function getBooks(): Collection + { + return $this->books; + } + + public function getBooksWithColor(BookColor $bookColor): Collection + { + $criteria = Criteria::create() + ->andWhere(Criteria::expr()->eq('bookColor', $bookColor)); + + return $this->books->matching($criteria); + } +} diff --git a/tests/Tests/Models/Enums/BookColor.php b/tests/Tests/Models/Enums/BookColor.php new file mode 100644 index 00000000000..66c3e8664e0 --- /dev/null +++ b/tests/Tests/Models/Enums/BookColor.php @@ -0,0 +1,11 @@ +books = new ArrayCollection(); + } + + public function getBooksWithColor(BookColor $bookColor): Collection + { + $criteria = Criteria::create() + ->andWhere(Criteria::expr()->eq('bookColor', $bookColor)); + + return $this->books->matching($criteria); + } + + public function getBooks(): Collection + { + return $this->books; + } + + public function addBook(Book $book): void + { + $this->books->add($book); + $book->setLibrary($this); + } +} diff --git a/tests/Tests/ORM/Functional/EnumTest.php b/tests/Tests/ORM/Functional/EnumTest.php index 75ccac8b506..e5d25ba3e6f 100644 --- a/tests/Tests/ORM/Functional/EnumTest.php +++ b/tests/Tests/ORM/Functional/EnumTest.php @@ -12,9 +12,13 @@ use Doctrine\ORM\Tools\SchemaTool; use Doctrine\Tests\Models\DataTransferObjects\DtoWithArrayOfEnums; use Doctrine\Tests\Models\DataTransferObjects\DtoWithEnum; +use Doctrine\Tests\Models\Enums\Book; +use Doctrine\Tests\Models\Enums\BookCategory; +use Doctrine\Tests\Models\Enums\BookColor; use Doctrine\Tests\Models\Enums\Card; use Doctrine\Tests\Models\Enums\CardWithDefault; use Doctrine\Tests\Models\Enums\CardWithNullable; +use Doctrine\Tests\Models\Enums\Library; use Doctrine\Tests\Models\Enums\Product; use Doctrine\Tests\Models\Enums\Quantity; use Doctrine\Tests\Models\Enums\Scale; @@ -516,4 +520,95 @@ public function testEnumWithDefault(): void self::assertSame(Suit::Hearts, $card->suit); } + + public function testEnumCollectionMatchingWithOneToMany(): void + { + $this->setUpEntitySchema([Book::class, Library::class]); + + $redBook = new Book(); + $redBook->bookColor = BookColor::RED; + + $blueBook = new Book(); + $blueBook->bookColor = BookColor::BLUE; + + $library = new Library(); + $library->addBook($blueBook); + $library->addBook($redBook); + + $this->_em->persist($library); + $this->_em->persist($blueBook); + $this->_em->persist($redBook); + + $this->_em->flush(); + $libraryId = $library->id; + + unset($library, $redBook, $blueBook); + + $this->_em->clear(); + + // Case 1: load collection first, then use matching() + + $library = $this->_em->find(Library::class, $libraryId); + $this->assertInstanceOf(Library::class, $library); + + // Load books collection first + $this->assertCount(2, $library->getBooks()); + + $this->assertCount(1, $library->getBooksWithColor(BookColor::RED)); + + $this->_em->clear(); + + // Case 2: use matching() with uninitialized collection + + $library = $this->_em->find(Library::class, $libraryId); + $this->assertCount(1, $library->getBooksWithColor(BookColor::RED)); + } + + public function testEnumCollectionMatchingWithManyToMany(): void + { + $this->setUpEntitySchema([Book::class, BookCategory::class, Library::class]); + + $thrillerCategory = new BookCategory(); + $thrillerCategory->name = 'thriller'; + + $fantasyCategory = new BookCategory(); + $fantasyCategory->name = 'fantasy'; + + $redBook = new Book(); + $redBook->addCategory($fantasyCategory); + $redBook->addCategory($thrillerCategory); + $redBook->bookColor = BookColor::RED; + + $blueBook = new Book(); + $blueBook->addCategory($thrillerCategory); + $blueBook->bookColor = BookColor::BLUE; + + $this->_em->persist($thrillerCategory); + $this->_em->persist($fantasyCategory); + $this->_em->persist($blueBook); + $this->_em->persist($redBook); + + $this->_em->flush(); + $thrillerCategoryId = $thrillerCategory->id; + + $this->_em->clear(); + unset($thrillerCategory, $fantasyCategory, $blueBook, $redBook); + + // Case 1: load collection first, then use matching() + + $thrillerCategory = $this->_em->find(BookCategory::class, $thrillerCategoryId); + $this->assertInstanceOf(BookCategory::class, $thrillerCategory); + + // Load books collection first + $this->assertCount(2, $thrillerCategory->getBooks()); + + $this->assertCount(1, $thrillerCategory->getBooksWithColor(BookColor::RED)); + + $this->_em->clear(); + + // Case 2: use matching() with uninitialized collection + + $thrillerCategory = $this->_em->find(BookCategory::class, $thrillerCategoryId); + $this->assertCount(1, $thrillerCategory->getBooksWithColor(BookColor::RED)); + } } From e9fa4265866d14b9f02d5193c627876474b4060b Mon Sep 17 00:00:00 2001 From: Valentin SALMERON Date: Mon, 24 Jun 2024 10:52:44 +0200 Subject: [PATCH 2/6] Apply review's suggestions --- .../Collection/ManyToManyPersister.php | 6 +- src/Persisters/Traits/ResolveValuesHelper.php | 18 ++--- tests/Tests/ORM/Functional/EnumTest.php | 77 +++++++++++++++---- 3 files changed, 75 insertions(+), 26 deletions(-) diff --git a/src/Persisters/Collection/ManyToManyPersister.php b/src/Persisters/Collection/ManyToManyPersister.php index c9d6b3ec454..f819099a48b 100644 --- a/src/Persisters/Collection/ManyToManyPersister.php +++ b/src/Persisters/Collection/ManyToManyPersister.php @@ -243,6 +243,7 @@ public function loadCriteria(PersistentCollection $collection, Criteria $criteri } $parameters = $this->expandCriteriaParameters($criteria); + $paramsValues = []; foreach ($parameters as $parameter) { [$name, $value, $operator] = $parameter; @@ -253,11 +254,14 @@ public function loadCriteria(PersistentCollection $collection, Criteria $criteri $whereClauses[] = sprintf('te.%s %s NULL', $field, $operator === Comparison::EQ ? 'IS' : 'IS NOT'); } else { $whereClauses[] = sprintf('te.%s %s ?', $field, $operator); - $params = array_merge($params, $this->getValues($value)); + $paramsValues[] = $this->getValues($value); $paramTypes[] = PersisterHelper::getTypeOfField($name, $targetClass, $this->em)[0]; } } + $params = array_merge($params, ...$paramsValues); + unset($paramsValues); + $tableName = $this->quoteStrategy->getTableName($targetClass, $this->platform); $joinTable = $this->quoteStrategy->getJoinTableName($mapping, $associationSourceClass, $this->platform); diff --git a/src/Persisters/Traits/ResolveValuesHelper.php b/src/Persisters/Traits/ResolveValuesHelper.php index edef7f04829..cde84544c23 100644 --- a/src/Persisters/Traits/ResolveValuesHelper.php +++ b/src/Persisters/Traits/ResolveValuesHelper.php @@ -5,17 +5,17 @@ namespace Doctrine\ORM\Persisters\Traits; use BackedEnum; -use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Proxy\DefaultProxyClassNameResolver; use function array_merge; use function is_array; use function is_object; +/** + * @internal + */ trait ResolveValuesHelper { - protected EntityManagerInterface $em; - /** * Retrieves the parameters that identifies a value. * @@ -24,13 +24,13 @@ trait ResolveValuesHelper private function getValues(mixed $value): array { if (is_array($value)) { - $newValue = []; + $newValues = []; foreach ($value as $itemValue) { - $newValue = array_merge($newValue, $this->getValues($itemValue)); + $newValues[] = $this->getValues($itemValue); } - return [$newValue]; + return [array_merge(...$newValues)]; } return $this->getIndividualValue($value); @@ -60,13 +60,13 @@ private function getIndividualValue(mixed $value): array $class = $this->em->getClassMetadata($valueClass); if ($class->isIdentifierComposite) { - $newValue = []; + $newValues = []; foreach ($class->getIdentifierValues($value) as $innerValue) { - $newValue = array_merge($newValue, $this->getValues($innerValue)); + $newValues[] = $this->getValues($innerValue); } - return $newValue; + return array_merge(...$newValues); } return [$this->em->getUnitOfWork()->getSingleIdentifierValue($value)]; diff --git a/tests/Tests/ORM/Functional/EnumTest.php b/tests/Tests/ORM/Functional/EnumTest.php index e5d25ba3e6f..6b707c8ab6a 100644 --- a/tests/Tests/ORM/Functional/EnumTest.php +++ b/tests/Tests/ORM/Functional/EnumTest.php @@ -521,7 +521,7 @@ public function testEnumWithDefault(): void self::assertSame(Suit::Hearts, $card->suit); } - public function testEnumCollectionMatchingWithOneToMany(): void + public function testEnumLazyCollectionMatchingWithOneToMany(): void { $this->setUpEntitySchema([Book::class, Library::class]); @@ -546,7 +546,34 @@ public function testEnumCollectionMatchingWithOneToMany(): void $this->_em->clear(); - // Case 1: load collection first, then use matching() + $library = $this->_em->find(Library::class, $libraryId); + $this->assertCount(1, $library->getBooksWithColor(BookColor::RED)); + } + + public function testEnumInitializedCollectionMatchingWithOneToMany(): void + { + $this->setUpEntitySchema([Book::class, Library::class]); + + $redBook = new Book(); + $redBook->bookColor = BookColor::RED; + + $blueBook = new Book(); + $blueBook->bookColor = BookColor::BLUE; + + $library = new Library(); + $library->addBook($blueBook); + $library->addBook($redBook); + + $this->_em->persist($library); + $this->_em->persist($blueBook); + $this->_em->persist($redBook); + + $this->_em->flush(); + $libraryId = $library->id; + + unset($library, $redBook, $blueBook); + + $this->_em->clear(); $library = $this->_em->find(Library::class, $libraryId); $this->assertInstanceOf(Library::class, $library); @@ -555,16 +582,43 @@ public function testEnumCollectionMatchingWithOneToMany(): void $this->assertCount(2, $library->getBooks()); $this->assertCount(1, $library->getBooksWithColor(BookColor::RED)); + } - $this->_em->clear(); + public function testEnumLazyCollectionMatchingWithManyToMany(): void + { + $this->setUpEntitySchema([Book::class, BookCategory::class, Library::class]); - // Case 2: use matching() with uninitialized collection + $thrillerCategory = new BookCategory(); + $thrillerCategory->name = 'thriller'; - $library = $this->_em->find(Library::class, $libraryId); - $this->assertCount(1, $library->getBooksWithColor(BookColor::RED)); + $fantasyCategory = new BookCategory(); + $fantasyCategory->name = 'fantasy'; + + $redBook = new Book(); + $redBook->addCategory($fantasyCategory); + $redBook->addCategory($thrillerCategory); + $redBook->bookColor = BookColor::RED; + + $blueBook = new Book(); + $blueBook->addCategory($thrillerCategory); + $blueBook->bookColor = BookColor::BLUE; + + $this->_em->persist($thrillerCategory); + $this->_em->persist($fantasyCategory); + $this->_em->persist($blueBook); + $this->_em->persist($redBook); + + $this->_em->flush(); + $thrillerCategoryId = $thrillerCategory->id; + + $this->_em->clear(); + unset($thrillerCategory, $fantasyCategory, $blueBook, $redBook); + + $thrillerCategory = $this->_em->find(BookCategory::class, $thrillerCategoryId); + $this->assertCount(1, $thrillerCategory->getBooksWithColor(BookColor::RED)); } - public function testEnumCollectionMatchingWithManyToMany(): void + public function testEnumInitializedCollectionMatchingWithManyToMany(): void { $this->setUpEntitySchema([Book::class, BookCategory::class, Library::class]); @@ -594,8 +648,6 @@ public function testEnumCollectionMatchingWithManyToMany(): void $this->_em->clear(); unset($thrillerCategory, $fantasyCategory, $blueBook, $redBook); - // Case 1: load collection first, then use matching() - $thrillerCategory = $this->_em->find(BookCategory::class, $thrillerCategoryId); $this->assertInstanceOf(BookCategory::class, $thrillerCategory); @@ -603,12 +655,5 @@ public function testEnumCollectionMatchingWithManyToMany(): void $this->assertCount(2, $thrillerCategory->getBooks()); $this->assertCount(1, $thrillerCategory->getBooksWithColor(BookColor::RED)); - - $this->_em->clear(); - - // Case 2: use matching() with uninitialized collection - - $thrillerCategory = $this->_em->find(BookCategory::class, $thrillerCategoryId); - $this->assertCount(1, $thrillerCategory->getBooksWithColor(BookColor::RED)); } } From 7d9904a7c7d0aca7bfd795ad0031d09831e0270e Mon Sep 17 00:00:00 2001 From: Valentin Salmeron Date: Mon, 24 Jun 2024 21:02:29 +0200 Subject: [PATCH 3/6] Update ResolveValuesHelper.php --- src/Persisters/Traits/ResolveValuesHelper.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Persisters/Traits/ResolveValuesHelper.php b/src/Persisters/Traits/ResolveValuesHelper.php index cde84544c23..d7847a20279 100644 --- a/src/Persisters/Traits/ResolveValuesHelper.php +++ b/src/Persisters/Traits/ResolveValuesHelper.php @@ -11,9 +11,7 @@ use function is_array; use function is_object; -/** - * @internal - */ +/** @internal */ trait ResolveValuesHelper { /** From 6ec521b0d293c6ad520e577911f00c772852e16d Mon Sep 17 00:00:00 2001 From: Valentin Salmeron Date: Mon, 24 Jun 2024 21:04:08 +0200 Subject: [PATCH 4/6] Update ManyToManyPersister.php --- src/Persisters/Collection/ManyToManyPersister.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Persisters/Collection/ManyToManyPersister.php b/src/Persisters/Collection/ManyToManyPersister.php index f819099a48b..bfb390e2374 100644 --- a/src/Persisters/Collection/ManyToManyPersister.php +++ b/src/Persisters/Collection/ManyToManyPersister.php @@ -242,7 +242,7 @@ public function loadCriteria(PersistentCollection $collection, Criteria $criteri $paramTypes[] = PersisterHelper::getTypeOfColumn($value, $ownerMetadata, $this->em); } - $parameters = $this->expandCriteriaParameters($criteria); + $parameters = $this->expandCriteriaParameters($criteria); $paramsValues = []; foreach ($parameters as $parameter) { From 480b36287eb68cd0a9386763e2abe733d0e42a5b Mon Sep 17 00:00:00 2001 From: Valentin SALMERON Date: Fri, 5 Jul 2024 08:12:05 +0200 Subject: [PATCH 5/6] Fix pipeline errors --- src/Persisters/Traits/ResolveValuesHelper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Persisters/Traits/ResolveValuesHelper.php b/src/Persisters/Traits/ResolveValuesHelper.php index d7847a20279..abfaaf1d5b3 100644 --- a/src/Persisters/Traits/ResolveValuesHelper.php +++ b/src/Persisters/Traits/ResolveValuesHelper.php @@ -17,7 +17,7 @@ trait ResolveValuesHelper /** * Retrieves the parameters that identifies a value. * - * @return mixed[] + * @psalm-return list */ private function getValues(mixed $value): array { From adf4a5312243e1a36c4d4e570fcb94233e9d0f0d Mon Sep 17 00:00:00 2001 From: Valentin SALMERON Date: Thu, 8 Aug 2024 14:21:53 +0200 Subject: [PATCH 6/6] Update psalm-baseline.xml --- psalm-baseline.xml | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 7a84afd4f83..bf9b9f9ed5f 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -667,16 +667,6 @@ true]]]> true]]]> - - - - - - - - - ]]> -