Handle null comparisons in ManyToManyPersister (#10587)

* Add test case for #7717 * Do not hide null equality checks in `SqlValueVisitor::walkComparison` * Annotate `GH7717Parent::$children` type
doctrine · Apr 12, 2023 · fca1ef7 · fca1ef7
1 parent e5fb1a4
commit fca1ef7
Show file tree

Hide file tree

Showing 6 changed files with 121 additions and 21 deletions.
diff --git a/lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php b/lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php
@@ -6,6 +6,7 @@
 
 use BadMethodCallException;
 use Doctrine\Common\Collections\Criteria;
+use Doctrine\Common\Collections\Expr\Comparison;
 use Doctrine\DBAL\Exception as DBALException;
 use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\PersistentCollection;
@@ -246,10 +247,15 @@ public function loadCriteria(PersistentCollection $collection, Criteria $criteri
         foreach ($parameters as $parameter) {
             [$name, $value, $operator] = $parameter;
 
-            $field          = $this->quoteStrategy->getColumnName($name, $targetClass, $this->platform);
-            $whereClauses[] = sprintf('te.%s %s ?', $field, $operator);
-            $params[]       = $value;
-            $paramTypes[]   = PersisterHelper::getTypeOfField($name, $targetClass, $this->em)[0];
+            $field = $this->quoteStrategy->getColumnName($name, $targetClass, $this->platform);
+
+            if ($value === null && ($operator === Comparison::EQ || $operator === Comparison::NEQ)) {
+                $whereClauses[] = sprintf('te.%s %s NULL', $field, $operator === Comparison::EQ ? 'IS' : 'IS NOT');
+            } else {
+                $whereClauses[] = sprintf('te.%s %s ?', $field, $operator);
+                $params[]       = $value;
+                $paramTypes[]   = PersisterHelper::getTypeOfField($name, $targetClass, $this->em)[0];
+            }
         }
 
         $tableName = $this->quoteStrategy->getTableName($targetClass, $this->platform);

diff --git a/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php b/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php
@@ -890,15 +890,17 @@ public function expandCriteriaParameters(Criteria $criteria)
 
         $valueVisitor->dispatch($expression);
 
-        [$params, $types] = $valueVisitor->getParamsAndTypes();
-
-        foreach ($params as $param) {
-            $sqlParams = array_merge($sqlParams, $this->getValues($param));
-        }
+        [, $types] = $valueVisitor->getParamsAndTypes();
 
         foreach ($types as $type) {
-            [$field, $value] = $type;
-            $sqlTypes        = array_merge($sqlTypes, $this->getTypes($field, $value, $this->class));
+            [$field, $value, $operator] = $type;
+
+            if ($value === null && ($operator === Comparison::EQ || $operator === Comparison::NEQ)) {
+                continue;
+            }
+
+            $sqlParams = array_merge($sqlParams, $this->getValues($value));
+            $sqlTypes  = array_merge($sqlTypes, $this->getTypes($field, $value, $this->class));
         }
 
         return [$sqlParams, $sqlTypes];

diff --git a/lib/Doctrine/ORM/Persisters/SqlValueVisitor.php b/lib/Doctrine/ORM/Persisters/SqlValueVisitor.php
@@ -27,18 +27,10 @@ class SqlValueVisitor extends ExpressionVisitor
      */
     public function walkComparison(Comparison $comparison)
     {
-        $value    = $this->getValueFromComparison($comparison);
-        $field    = $comparison->getField();
-        $operator = $comparison->getOperator();
-
-        if (($operator === Comparison::EQ || $operator === Comparison::IS) && $value === null) {
-            return null;
-        } elseif ($operator === Comparison::NEQ && $value === null) {
-            return null;
-        }
+        $value = $this->getValueFromComparison($comparison);
 
         $this->values[] = $value;
-        $this->types[]  = [$field, $value, $operator];
+        $this->types[]  = [$comparison->getField(), $value, $comparison->getOperator()];
 
         return null;
     }

diff --git a/tests/Doctrine/Tests/Models/GH7717/GH7717Child.php b/tests/Doctrine/Tests/Models/GH7717/GH7717Child.php
@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\Models\GH7717;
+
+use Doctrine\ORM\Mapping as ORM;
+
+/**
+ * @ORM\Entity
+ * @ORM\Table(name="gh7717_children")
+ */
+class GH7717Child
+{
+    /**
+     * @ORM\Id
+     * @ORM\Column(type="integer")
+     * @ORM\GeneratedValue
+     */
+    public ?int $id = null;
+
+    /**
+     * @ORM\Column(type="string", nullable=true)
+     */
+    public ?string $nullableProperty = null;
+}
diff --git a/tests/Doctrine/Tests/Models/GH7717/GH7717Parent.php b/tests/Doctrine/Tests/Models/GH7717/GH7717Parent.php
@@ -0,0 +1,29 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\Models\GH7717;
+
+use Doctrine\Common\Collections\Selectable;
+use Doctrine\ORM\Mapping as ORM;
+
+/**
+ * @ORM\Entity
+ * @ORM\Table(name="gh7717_parents")
+ */
+class GH7717Parent
+{
+    /**
+     * @ORM\Id
+     * @ORM\Column(type="integer")
+     * @ORM\GeneratedValue
+     */
+    public ?int $id = null;
+
+    /**
+     * @ORM\ManyToMany(targetEntity="GH7717Child", cascade={"persist"})
+     *
+     * @var Selectable<int, GH7717Child>
+     */
+    public Selectable $children;
+}
diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7717Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7717Test.php
@@ -0,0 +1,45 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\Common\Collections\ArrayCollection;
+use Doctrine\Common\Collections\Criteria;
+use Doctrine\Tests\Models\GH7717\GH7717Child;
+use Doctrine\Tests\Models\GH7717\GH7717Parent;
+use Doctrine\Tests\OrmFunctionalTestCase;
+
+/**
+ * @requires PHP 7.4
+ */
+final class GH7717Test extends OrmFunctionalTestCase
+{
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        $this->createSchemaForModels(
+            GH7717Parent::class,
+            GH7717Child::class
+        );
+    }
+
+    public function testManyToManyPersisterIsNullComparison(): void
+    {
+        $childWithNullProperty                      = new GH7717Child();
+        $childWithoutNullProperty                   = new GH7717Child();
+        $childWithoutNullProperty->nullableProperty = 'nope';
+
+        $parent           = new GH7717Parent();
+        $parent->children = new ArrayCollection([$childWithNullProperty, $childWithoutNullProperty]);
+
+        $this->_em->persist($parent);
+        $this->_em->flush();
+        $this->_em->clear();
+
+        $parent = $this->_em->find(GH7717Parent::class, 1);
+
+        $this->assertCount(1, $parent->children->matching(new Criteria(Criteria::expr()->isNull('nullableProperty'))));
+    }
+}