Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix matching lazy collections with enums criteria #11516

Open
wants to merge 6 commits into
base: 3.2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions psalm-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -667,16 +667,6 @@
<code><![CDATA[[UnitOfWork::HINT_DEFEREAGERLOAD => true]]]></code>
<code><![CDATA[[UnitOfWork::HINT_DEFEREAGERLOAD => true]]]></code>
</InvalidArgument>
<LessSpecificReturnStatement>
<code><![CDATA[$newValue]]></code>
<code><![CDATA[[$params, $types]]]></code>
<code><![CDATA[[$sqlParams, $sqlTypes]]]></code>
</LessSpecificReturnStatement>
<MoreSpecificReturnType>
<code><![CDATA[array]]></code>
<code><![CDATA[array]]></code>
<code><![CDATA[list<mixed>]]></code>
</MoreSpecificReturnType>
<PossiblyNullReference>
<code><![CDATA[getValue]]></code>
<code><![CDATA[getValue]]></code>
Expand Down
12 changes: 10 additions & 2 deletions src/Persisters/Collection/ManyToManyPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -32,6 +34,8 @@
*/
class ManyToManyPersister extends AbstractCollectionPersister
{
use ResolveValuesHelper;

public function delete(PersistentCollection $collection): void
{
$mapping = $this->getMapping($collection);
Expand Down Expand Up @@ -238,7 +242,8 @@ 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) {
[$name, $value, $operator] = $parameter;
Expand All @@ -249,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[] = $value;
$paramsValues[] = $this->getValues($value);
$paramTypes[] = PersisterHelper::getTypeOfField($name, $targetClass, $this->em)[0];
}
}

$params = array_merge($params, ...$paramsValues);
unset($paramsValues);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this unset, its not needed.


$tableName = $this->quoteStrategy->getTableName($targetClass, $this->platform);
$joinTable = $this->quoteStrategy->getJoinTableName($mapping, $associationSourceClass, $this->platform);

Expand Down
61 changes: 2 additions & 59 deletions src/Persisters/Entity/BasicEntityPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -99,6 +97,7 @@
class BasicEntityPersister implements EntityPersister
{
use LockSqlHelper;
use ResolveValuesHelper;

/** @var array<string,string> */
private static array $comparisonMap = [
Expand Down Expand Up @@ -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<mixed>
*/
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);
Expand Down
72 changes: 72 additions & 0 deletions src/Persisters/Traits/ResolveValuesHelper.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?php

declare(strict_types=1);

namespace Doctrine\ORM\Persisters\Traits;

use BackedEnum;
use Doctrine\ORM\Proxy\DefaultProxyClassNameResolver;

use function array_merge;
use function is_array;
use function is_object;

/** @internal */
trait ResolveValuesHelper
kira0269 marked this conversation as resolved.
Show resolved Hide resolved
{
/**
* Retrieves the parameters that identifies a value.
*
* @psalm-return list<mixed>
*/
private function getValues(mixed $value): array
{
if (is_array($value)) {
$newValues = [];

foreach ($value as $itemValue) {
$newValues[] = $this->getValues($itemValue);
}

return [array_merge(...$newValues)];
}

return $this->getIndividualValue($value);
}

/**
* Retrieves an individual parameter value.
*
* @psalm-return list<mixed>
*/
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) {
$newValues = [];

foreach ($class->getIdentifierValues($value) as $innerValue) {
$newValues[] = $this->getValues($innerValue);
}

return array_merge(...$newValues);
}

return [$this->em->getUnitOfWork()->getSingleIdentifierValue($value)];
}
}
52 changes: 52 additions & 0 deletions tests/Tests/Models/Enums/Book.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\Models\Enums;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\JoinColumn;
use Doctrine\ORM\Mapping\ManyToMany;
use Doctrine\ORM\Mapping\ManyToOne;
use Doctrine\ORM\Mapping\Table;

#[Entity]
#[Table(name: 'books')]
class Book
{
#[Id]
#[GeneratedValue]
#[Column]
public int $id;

#[ManyToOne(targetEntity: Library::class, inversedBy: 'books')]
#[JoinColumn(name: 'library_id', referencedColumnName: 'id')]
public Library $library;

#[Column(enumType: BookColor::class)]
public BookColor $bookColor;

#[ManyToMany(targetEntity: BookCategory::class, mappedBy: 'books')]
public Collection $categories;

public function __construct()
{
$this->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);
}
}
52 changes: 52 additions & 0 deletions tests/Tests/Models/Enums/BookCategory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\Models\Enums;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\Common\Collections\Criteria;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\ManyToMany;

#[Entity]
class BookCategory
{
#[Id]
#[Column]
#[GeneratedValue]
public int $id;

#[Column]
public string $name;

#[ManyToMany(targetEntity: Book::class, inversedBy: 'categories')]
public Collection $books;

public function __construct()
{
$this->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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if you move this code into the test, because reading the test code now its not easy to grasp how test name matches to code. Same for the other Criteria code in Library.

->andWhere(Criteria::expr()->eq('bookColor', $bookColor));

return $this->books->matching($criteria);
}
}
11 changes: 11 additions & 0 deletions tests/Tests/Models/Enums/BookColor.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\Models\Enums;

enum BookColor: string
{
case RED = 'red';
case BLUE = 'blue';
}
Loading
Loading