Fix collection filtering API for IN/NOT IN comparisons that require type conversions

phpstan-baseline.neon

@@ -2946,12 +2946,6 @@ parameters:
 			count: 1
 			path: src/Persisters/Entity/BasicEntityPersister.php
 
-		-
-			message: '#^Strict comparison using \=\=\= between string and null will always evaluate to false\.$#'
-			identifier: identical.alwaysFalse
-			count: 1
-			path: src/Persisters/Entity/BasicEntityPersister.php
-
 		-
 			message: '#^Method Doctrine\\ORM\\Persisters\\Entity\\CachedPersisterContext\:\:__construct\(\) has parameter \$class with generic interface Doctrine\\Persistence\\Mapping\\ClassMetadata but does not specify its types\: T$#'
 			identifier: missingType.generics

src/Persisters/Collection/ManyToManyPersister.php

@@ -257,14 +257,14 @@ public function loadCriteria(PersistentCollection $collection, Criteria $criteri
 
             if ($value === null && ($operator === Comparison::EQ || $operator === Comparison::NEQ)) {
                 $whereClauses[] = sprintf('te.%s %s NULL', $field, $operator === Comparison::EQ ? 'IS' : 'IS NOT');
-            } else {
-                if ($operator === Comparison::IN) {
-                    $whereClauses[] = sprintf('te.%s IN (?)', $field);
-                } elseif ($operator === Comparison::NIN) {
-                    $whereClauses[] = sprintf('te.%s NOT IN (?)', $field);
-                } else {
-                    $whereClauses[] = sprintf('te.%s %s ?', $field, $operator);
+            } elseif ($operator === Comparison::IN || $operator === Comparison::NIN) {
+                $whereClauses[] = sprintf('te.%s %s (%s)', $field, $operator === Comparison::IN ? 'IN' : 'NOT IN', implode(', ', array_fill(0, count($value), '?')));
+                foreach ($value as $item) {
+                    $params     = array_merge($params, PersisterHelper::convertToParameterValue($item, $this->em));
+                    $paramTypes = array_merge($paramTypes, PersisterHelper::inferParameterTypes($name, $item, $targetClass, $this->em));
                 }
+            } else {
+                $whereClauses[] = sprintf('te.%s %s ?', $field, $operator);
 
                 $params     = array_merge($params, PersisterHelper::convertToParameterValue($value, $this->em));
                 $paramTypes = array_merge($paramTypes, PersisterHelper::inferParameterTypes($name, $value, $targetClass, $this->em));

src/Persisters/Entity/BasicEntityPersister.php

@@ -34,10 +34,12 @@
 use LengthException;
 
 use function array_combine;
+use function array_diff_key;
+use function array_fill;
+use function array_flip;
 use function array_keys;
 use function array_map;
 use function array_merge;
-use function array_search;
 use function array_unique;
 use function array_values;
 use function assert;
@@ -951,6 +953,29 @@ public function expandCriteriaParameters(Criteria $criteria)
                 continue;
             }
 
+            if ($operator === Comparison::IN || $operator === Comparison::NIN) {
+                if (! is_array($value)) {
+                    $value = [$value];
+                }
+
+                foreach ($value as $item) {
+                    if ($item === null) {
+                        /*
+                         * Compare this to how \Doctrine\ORM\Persisters\Entity\BasicEntityPersister::getSelectConditionStatementSQL
+                         * creates the "[NOT] IN (...)" expression - for NULL values, it does _not_ insert a placeholder in the
+                         * SQL and instead adds an extra ... OR ... IS NULL condition. So we need to skip NULL values here as
+                         * well to create a parameters list that matches the SQL.
+                         */
+                        continue;
+                    }
+
+                    $sqlParams = array_merge($sqlParams, PersisterHelper::convertToParameterValue($item, $this->em));
+                    $sqlTypes  = array_merge($sqlTypes, PersisterHelper::inferParameterTypes($field, $item, $this->class, $this->em));
+                }
+
+                continue;
+            }
+
             $sqlParams = array_merge($sqlParams, PersisterHelper::convertToParameterValue($value, $this->em));
             $sqlTypes  = array_merge($sqlTypes, PersisterHelper::inferParameterTypes($field, $value, $this->class, $this->em));
         }
@@ -1689,6 +1714,8 @@ protected function getSelectConditionCriteriaSQL(Criteria $criteria)
      */
     public function getSelectConditionStatementSQL($field, $value, $assoc = null, $comparison = null)
     {
+        $comparison = $comparison ?? (is_array($value) ? Comparison::IN : Comparison::EQ);
+
         $selectedColumns = [];
         $columns         = $this->getSelectConditionStatementColumnSQL($field, $assoc);
 
@@ -1708,46 +1735,45 @@ public function getSelectConditionStatementSQL($field, $value, $assoc = null, $c
                 $placeholder = $type->convertToDatabaseValueSQL($placeholder, $this->platform);
             }
 
-            if ($comparison !== null) {
-                // special case null value handling
-                if (($comparison === Comparison::EQ || $comparison === Comparison::IS) && $value === null) {
-                    $selectedColumns[] = $column . ' IS NULL';
-
-                    continue;
-                }
-
-                if ($comparison === Comparison::NEQ && $value === null) {
-                    $selectedColumns[] = $column . ' IS NOT NULL';
-
-                    continue;
-                }
-
-                $selectedColumns[] = $column . ' ' . sprintf(self::$comparisonMap[$comparison], $placeholder);
+            // special case null value handling
+            if (($comparison === Comparison::EQ || $comparison === Comparison::IS) && $value === null) {
+                $selectedColumns[] = $column . ' IS NULL';
 
                 continue;
             }
 
-            if (is_array($value)) {
-                $in = sprintf('%s IN (%s)', $column, $placeholder);
+            if ($comparison === Comparison::NEQ && $value === null) {
+                $selectedColumns[] = $column . ' IS NOT NULL';
 
-                if (array_search(null, $value, true) !== false) {
-                    $selectedColumns[] = sprintf('(%s OR %s IS NULL)', $in, $column);
+                continue;
+            }
 
-                    continue;
+            if ($comparison === Comparison::IN || $comparison === Comparison::NIN) {
+                if (! is_array($value)) {
+                    $value = [$value];
                 }
 
-                $selectedColumns[] = $in;
+                $nullKeys      = array_keys($value, null, true);
+                $nonNullValues = array_diff_key($value, array_flip($nullKeys));
 
-                continue;
-            }
+                $placeholders = implode(', ', array_fill(0, count($nonNullValues), $placeholder));
 
-            if ($value === null) {
-                $selectedColumns[] = sprintf('%s IS NULL', $column);
+                $in = $column . ' ' . sprintf(self::$comparisonMap[$comparison], $placeholders);
+
+                if ($nullKeys) {
+                    if ($nonNullValues) {
+                        $selectedColumns[] = sprintf('(%s OR %s IS NULL)', $in, $column);
+                    } else {
+                        $selectedColumns[] = $column . ' IS NULL';
+                    }
+                } else {
+                    $selectedColumns[] = $in;
+                }
 
                 continue;
             }
 
-            $selectedColumns[] = sprintf('%s = %s', $column, $placeholder);
+            $selectedColumns[] = $column . ' ' . sprintf(self::$comparisonMap[$comparison], $placeholder);
         }
 
         return implode(' AND ', $selectedColumns);
@@ -1938,6 +1964,16 @@ public function expandParameters($criteria)
                 continue; // skip null values.
             }
 
+            if (is_array($value)) {
+                $nonNullValues = array_diff_key($value, array_flip(array_keys($value, null, true)));
+                foreach ($nonNullValues as $item) {
+                    $types  = array_merge($types, PersisterHelper::inferParameterTypes($field, $item, $this->class, $this->em));
+                    $params = array_merge($params, PersisterHelper::convertToParameterValue($item, $this->em));
+                }
+
+                continue;
+            }
+
             $types  = array_merge($types, PersisterHelper::inferParameterTypes($field, $value, $this->class, $this->em));
             $params = array_merge($params, PersisterHelper::convertToParameterValue($value, $this->em));
         }

src/Persisters/Entity/EntityPersister.php

@@ -73,7 +73,7 @@ public function getCountSQL($criteria = []);
     /**
      * Expands the parameters from the given criteria and use the correct binding types if found.
      *
-     * @param string[] $criteria
+     * @param array<string, mixed> $criteria
      *
      * @phpstan-return array{list<mixed>, list<int|string|null>}
      */

tests/Tests/Models/ValueConversionType/InversedManyToManyEntity.php

@@ -25,6 +25,12 @@ class InversedManyToManyEntity
      */
     public $id1;
 
+    /**
+     * @var string
+     * @Column(type="rot13", length=255, nullable=true)
+     */
+    public $field = null;
+
     /**
      * @phpstan-var Collection<int, OwningManyToManyEntity>
      * @ManyToMany(targetEntity="OwningManyToManyEntity", mappedBy="associatedEntities")

tests/Tests/Models/ValueConversionType/InversedOneToManyEntity.php

@@ -33,7 +33,7 @@ class InversedOneToManyEntity
 
     /**
      * @var string
-     * @Column(type="string", name="some_property", length=255)
+     * @Column(type="string", name="some_property", length=255, nullable=true)
      */
     public $someProperty;
 

tests/Tests/Models/ValueConversionType/OwningManyToManyEntity.php

@@ -27,6 +27,12 @@ class OwningManyToManyEntity
      */
     public $id2;
 
+    /**
+     * @var string
+     * @Column(type="rot13", length=255, nullable=true)
+     */
+    public $field = null;
+
     /**
      * @var Collection<int, InversedManyToManyEntity>
      * @ManyToMany(targetEntity="InversedManyToManyEntity", inversedBy="associatedEntities")

tests/Tests/Models/ValueConversionType/OwningManyToOneEntity.php

@@ -24,6 +24,12 @@ class OwningManyToOneEntity
      */
     public $id2;
 
+    /**
+     * @var string
+     * @Column(type="rot13", length=255, nullable=true)
+     */
+    public $field = null;
+
     /**
      * @var InversedOneToManyEntity
      * @ManyToOne(targetEntity="InversedOneToManyEntity", inversedBy="associatedEntities")

tests/Tests/ORM/Functional/ValueConversionType/ManyToManyCriteriaMatchingTest.php

@@ -0,0 +1,65 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\ValueConversionType;
+
+use Doctrine\Common\Collections\Criteria;
+use Doctrine\Common\Collections\Expr\Comparison;
+use Doctrine\Tests\Models\ValueConversionType\InversedManyToManyEntity;
+use Doctrine\Tests\Models\ValueConversionType\OwningManyToManyEntity;
+use Doctrine\Tests\OrmFunctionalTestCase;
+use Generator;
+
+class ManyToManyCriteriaMatchingTest extends OrmFunctionalTestCase
+{
+    protected function setUp(): void
+    {
+        $this->useModelSet('vct_manytomany');
+
+        parent::setUp();
+    }
+
+    /**
+     * @dataProvider provideMatchingExpressions
+     */
+    public function testCriteriaMatchingOnFieldInManyToManyTarget(Comparison $comparison): void
+    {
+        $associated      = new InversedManyToManyEntity();
+        $associated->id1 = 'associated';
+
+        $matching        = new OwningManyToManyEntity();
+        $matching->id2   = 'first';
+        $matching->field = 'match this';
+        $matching->associatedEntities->add($associated);
+
+        $nonMatching        = new OwningManyToManyEntity();
+        $nonMatching->id2   = 'second';
+        $nonMatching->field = 'this is no match';
+        $nonMatching->associatedEntities->add($associated);
+
+        $this->_em->persist($associated);
+        $this->_em->persist($matching);
+        $this->_em->persist($nonMatching);
+        $this->_em->flush();
+        $this->_em->clear();
+
+        $this->getQueryLog()->reset()->enable();
+
+        $associated = $this->_em->find(InversedManyToManyEntity::class, 'associated');
+        self::assertFalse($associated->associatedEntities->isInitialized(), 'Pre-condition: lazy collection');
+
+        $result = $associated->associatedEntities->matching(Criteria::create()->where($comparison));
+
+        $l = $this->getQueryLog();
+
+        self::assertCount(1, $result);
+        self::assertSame('first', $result[0]->id2);
+    }
+
+    public function provideMatchingExpressions(): Generator
+    {
+        yield [Criteria::expr()->eq('field', 'match this')];
+        yield [Criteria::expr()->in('field', ['match this'])];
+    }
+}

tests/Tests/ORM/Functional/ValueConversionType/OneToManyCriteriaMatchingTest.php

@@ -0,0 +1,65 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\ValueConversionType;
+
+use Doctrine\Common\Collections\Criteria;
+use Doctrine\Common\Collections\Expr\Comparison;
+use Doctrine\Tests\Models\ValueConversionType\InversedOneToManyEntity;
+use Doctrine\Tests\Models\ValueConversionType\OwningManyToOneEntity;
+use Doctrine\Tests\OrmFunctionalTestCase;
+use Generator;
+
+class OneToManyCriteriaMatchingTest extends OrmFunctionalTestCase
+{
+    protected function setUp(): void
+    {
+        $this->useModelSet('vct_onetomany');
+
+        parent::setUp();
+    }
+
+    /**
+     * @dataProvider provideMatchingExpressions
+     */
+    public function testCriteriaMatchingOnFieldInOneToManyTarget(Comparison $comparison): void
+    {
+        $entityWithCollection      = new InversedOneToManyEntity();
+        $entityWithCollection->id1 = 'associated';
+
+        $matching                   = new OwningManyToOneEntity();
+        $matching->id2              = 'first';
+        $matching->field            = 'match this';
+        $matching->associatedEntity = $entityWithCollection;
+
+        $notMatching                   = new OwningManyToOneEntity();
+        $notMatching->id2              = 'second';
+        $notMatching->field            = 'this is no match';
+        $notMatching->associatedEntity = $entityWithCollection;
+
+        $this->_em->persist($entityWithCollection);
+        $this->_em->persist($matching);
+        $this->_em->persist($notMatching);
+        $this->_em->flush();
+        $this->_em->clear();
+
+        $this->getQueryLog()->reset()->enable();
+
+        $entityWithCollection = $this->_em->find(InversedOneToManyEntity::class, 'associated');
+        self::assertFalse($entityWithCollection->associatedEntities->isInitialized(), 'Pre-condition: lazy collection');
+
+        $result = $entityWithCollection->associatedEntities->matching(Criteria::create()->where($comparison));
+
+        $l = $this->getQueryLog();
+
+        self::assertCount(1, $result);
+        self::assertSame('first', $result[0]->id2);
+    }
+
+    public function provideMatchingExpressions(): Generator
+    {
+        yield [Criteria::expr()->eq('field', 'match this')];
+        yield [Criteria::expr()->in('field', ['match this'])];
+    }
+}

tests/Tests/ORM/Persisters/BasicEntityPersisterTypeValueSqlTest.php

@@ -133,7 +133,7 @@ public function testSelectConditionStatementNeqNull(): void
     public function testSelectConditionStatementWithMultipleValuesContainingNull(): void
     {
         self::assertEquals(
-            '(t0.id IN (?) OR t0.id IS NULL)',
+            't0.id IS NULL',
             $this->persister->getSelectConditionStatementSQL('id', [null])
         );
 
@@ -146,6 +146,11 @@ public function testSelectConditionStatementWithMultipleValuesContainingNull():
             '(t0.id IN (?) OR t0.id IS NULL)',
             $this->persister->getSelectConditionStatementSQL('id', [123, null])
         );
+
+        self::assertEquals(
+            '(t0.id IN (?, ?) OR t0.id IS NULL)',
+            $this->persister->getSelectConditionStatementSQL('id', [123, null, 234])
+        );
     }
 
     public function testCountCondition(): void