Merge pull request #6705 from Majkl578/ticket/6699

Fix parameter name comparison in AbstractQuery regarding different types (fixes #6699) Fixes #6699
doctrine · Nov 24, 2017 · b7cace8 · b7cace8
2 parents b47a39b + b8fd708
commit b7cace8
Show file tree

Hide file tree

Showing 5 changed files with 213 additions and 36 deletions.
diff --git a/lib/Doctrine/ORM/AbstractQuery.php b/lib/Doctrine/ORM/AbstractQuery.php
@@ -321,14 +321,14 @@ public function getParameters()
     public function getParameter($key)
     {
         $filteredParameters = $this->parameters->filter(
-            function ($parameter) use ($key)
-            {
-                // Must not be identical because of string to integer conversion
-                return ($key == $parameter->getName());
+            function (Query\Parameter $parameter) use ($key) : bool {
+                $parameterName = $parameter->getName();
+
+                return $key === $parameterName || (string) $key === (string) $parameterName;
             }
         );
 
-        return count($filteredParameters) ? $filteredParameters->first() : null;
+        return ! $filteredParameters->isEmpty() ? $filteredParameters->first() : null;
     }
 
     /**
@@ -369,17 +369,10 @@ public function setParameters($parameters)
      */
     public function setParameter($key, $value, $type = null)
     {
-        $filteredParameters = $this->parameters->filter(
-            function ($parameter) use ($key)
-            {
-                // Must not be identical because of string to integer conversion
-                return ($key == $parameter->getName());
-            }
-        );
+        $existingParameter = $this->getParameter($key);
 
-        if (count($filteredParameters)) {
-            $parameter = $filteredParameters->first();
-            $parameter->setValue($value, $type);
+        if ($existingParameter !== null) {
+            $existingParameter->setValue($value, $type);
 
             return $this;
         }

diff --git a/lib/Doctrine/ORM/QueryBuilder.php b/lib/Doctrine/ORM/QueryBuilder.php
@@ -532,26 +532,15 @@ public function getRootEntities()
      */
     public function setParameter($key, $value, $type = null)
     {
-        $filteredParameters = $this->parameters->filter(
-            function ($parameter) use ($key)
-            {
-                /* @var Query\Parameter $parameter */
-                // Must not be identical because of string to integer conversion
-                return ($key == $parameter->getName());
-            }
-        );
+        $existingParameter = $this->getParameter($key);
 
-        if (count($filteredParameters)) {
-            /* @var Query\Parameter $parameter */
-            $parameter = $filteredParameters->first();
-            $parameter->setValue($value, $type);
+        if ($existingParameter !== null) {
+            $existingParameter->setValue($value, $type);
 
             return $this;
         }
 
-        $parameter = new Query\Parameter($key, $value, $type);
-
-        $this->parameters->add($parameter);
+        $this->parameters->add(new Query\Parameter($key, $value, $type));
 
         return $this;
     }
@@ -614,15 +603,14 @@ public function getParameters()
     public function getParameter($key)
     {
         $filteredParameters = $this->parameters->filter(
-            function ($parameter) use ($key)
-            {
-                /* @var Query\Parameter $parameter */
-                // Must not be identical because of string to integer conversion
-                return ($key == $parameter->getName());
+            function (Query\Parameter $parameter) use ($key) : bool {
+                $parameterName = $parameter->getName();
+
+                return $key === $parameterName || (string) $key === (string) $parameterName;
             }
         );
 
-        return count($filteredParameters) ? $filteredParameters->first() : null;
+        return ! $filteredParameters->isEmpty() ? $filteredParameters->first() : null;
     }
 
     /**

diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6699Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6699Test.php
@@ -0,0 +1,56 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\Tests\Models\CMS\CmsUser;
+use Doctrine\Tests\OrmFunctionalTestCase;
+
+/**
+ * @group 6699
+ */
+final class GH6699Test extends OrmFunctionalTestCase
+{
+    protected function setUp() : void
+    {
+        $this->useModelSet('cms');
+
+        parent::setUp();
+    }
+
+    public function testMixedParametersWithZeroNumber() : void
+    {
+        $query = $this->_em->createQueryBuilder()
+                           ->select('u')
+                           ->from(CmsUser::class, 'u')
+                           ->andWhere('u.username = :username')
+                           ->andWhere('u.id = ?0')
+                           ->getQuery();
+
+        $query->setParameter('username', 'bar');
+        $query->setParameter(0, 0);
+
+        $query->execute();
+
+        self::assertCount(2, $query->getParameters());
+        self::assertSame(0, $query->getParameter(0)->getValue());
+        self::assertSame('bar', $query->getParameter('username')->getValue());
+    }
+
+    public function testMixedParametersWithZeroNumberOnQueryBuilder() : void
+    {
+        $query = $this->_em->createQueryBuilder()
+                           ->select('u')
+                           ->from(CmsUser::class, 'u')
+                           ->andWhere('u.username = :username')
+                           ->andWhere('u.id = ?0')
+                           ->setParameter('username', 'bar')
+                           ->setParameter(0, 0)
+                           ->getQuery();
+
+        $query->execute();
+
+        self::assertCount(2, $query->getParameters());
+        self::assertSame(0, $query->getParameter(0)->getValue());
+        self::assertSame('bar', $query->getParameter('username')->getValue());
+    }
+}
diff --git a/tests/Doctrine/Tests/ORM/Query/QueryTest.php b/tests/Doctrine/Tests/ORM/Query/QueryTest.php
@@ -12,6 +12,7 @@
 use Doctrine\Tests\Mocks\DriverConnectionMock;
 use Doctrine\Tests\Mocks\StatementArrayMock;
 use Doctrine\Tests\Models\CMS\CmsAddress;
+use Doctrine\Tests\Models\CMS\CmsUser;
 use Doctrine\Tests\OrmTestCase;
 
 class QueryTest extends OrmTestCase
@@ -303,4 +304,66 @@ public function testSelectFromSubquery()
         $this->expectExceptionMessage('Subquery');
         $query->getSQL();
     }
+
+    /**
+     * @group 6699
+     */
+    public function testGetParameterTypeJuggling() : void
+    {
+        $query = $this->_em->createQuery('select u from ' . CmsUser::class . ' u where u.id = ?0');
+
+        $query->setParameter(0, 0);
+
+        self::assertCount(1, $query->getParameters());
+        self::assertSame(0, $query->getParameter(0)->getValue());
+        self::assertSame(0, $query->getParameter('0')->getValue());
+    }
+
+    /**
+     * @group 6699
+     */
+    public function testSetParameterWithNameZeroIsNotOverridden() : void
+    {
+        $query = $this->_em->createQuery('select u from ' . CmsUser::class . ' u where u.id != ?0 and u.username = :name');
+
+        $query->setParameter(0, 0);
+        $query->setParameter('name', 'Doctrine');
+
+        self::assertCount(2, $query->getParameters());
+        self::assertSame(0, $query->getParameter('0')->getValue());
+        self::assertSame('Doctrine', $query->getParameter('name')->getValue());
+    }
+
+    /**
+     * @group 6699
+     */
+    public function testSetParameterWithNameZeroDoesNotOverrideAnotherParameter() : void
+    {
+        $query = $this->_em->createQuery('select u from ' . CmsUser::class . ' u where u.id != ?0 and u.username = :name');
+
+        $query->setParameter('name', 'Doctrine');
+        $query->setParameter(0, 0);
+
+        self::assertCount(2, $query->getParameters());
+        self::assertSame(0, $query->getParameter(0)->getValue());
+        self::assertSame('Doctrine', $query->getParameter('name')->getValue());
+    }
+
+    /**
+     * @group 6699
+     */
+    public function testSetParameterWithTypeJugglingWorks() : void
+    {
+        $query = $this->_em->createQuery('select u from ' . CmsUser::class . ' u where u.id != ?0 and u.username = :name');
+
+        $query->setParameter('0', 1);
+        $query->setParameter('name', 'Doctrine');
+        $query->setParameter(0, 2);
+        $query->setParameter('0', 3);
+
+        self::assertCount(2, $query->getParameters());
+        self::assertSame(3, $query->getParameter(0)->getValue());
+        self::assertSame(3, $query->getParameter('0')->getValue());
+        self::assertSame('Doctrine', $query->getParameter('name')->getValue());
+    }
 }
diff --git a/tests/Doctrine/Tests/ORM/QueryBuilderTest.php b/tests/Doctrine/Tests/ORM/QueryBuilderTest.php
@@ -1158,4 +1158,81 @@ public function testGetAllAliasesWithJoins()
 
         $this->assertEquals(['u', 'g'], $aliases);
     }
+
+    /**
+     * @group 6699
+     */
+    public function testGetParameterTypeJuggling() : void
+    {
+        $builder = $this->_em->createQueryBuilder()
+                             ->select('u')
+                             ->from(CmsUser::class, 'u')
+                             ->where('u.id = ?0');
+
+        $builder->setParameter(0, 0);
+
+        self::assertCount(1, $builder->getParameters());
+        self::assertSame(0, $builder->getParameter(0)->getValue());
+        self::assertSame(0, $builder->getParameter('0')->getValue());
+    }
+
+    /**
+     * @group 6699
+     */
+    public function testSetParameterWithNameZeroIsNotOverridden() : void
+    {
+        $builder = $this->_em->createQueryBuilder()
+                             ->select('u')
+                             ->from(CmsUser::class, 'u')
+                             ->where('u.id != ?0')
+                             ->andWhere('u.username = :name');
+
+        $builder->setParameter(0, 0);
+        $builder->setParameter('name', 'Doctrine');
+
+        self::assertCount(2, $builder->getParameters());
+        self::assertSame(0, $builder->getParameter('0')->getValue());
+        self::assertSame('Doctrine', $builder->getParameter('name')->getValue());
+    }
+
+    /**
+     * @group 6699
+     */
+    public function testSetParameterWithNameZeroDoesNotOverrideAnotherParameter() : void
+    {
+        $builder = $this->_em->createQueryBuilder()
+                             ->select('u')
+                             ->from(CmsUser::class, 'u')
+                             ->where('u.id != ?0')
+                             ->andWhere('u.username = :name');
+
+        $builder->setParameter('name', 'Doctrine');
+        $builder->setParameter(0, 0);
+
+        self::assertCount(2, $builder->getParameters());
+        self::assertSame(0, $builder->getParameter(0)->getValue());
+        self::assertSame('Doctrine', $builder->getParameter('name')->getValue());
+    }
+
+    /**
+     * @group 6699
+     */
+    public function testSetParameterWithTypeJugglingWorks() : void
+    {
+        $builder = $this->_em->createQueryBuilder()
+                             ->select('u')
+                             ->from(CmsUser::class, 'u')
+                             ->where('u.id != ?0')
+                             ->andWhere('u.username = :name');
+
+        $builder->setParameter('0', 1);
+        $builder->setParameter('name', 'Doctrine');
+        $builder->setParameter(0, 2);
+        $builder->setParameter('0', 3);
+
+        self::assertCount(2, $builder->getParameters());
+        self::assertSame(3, $builder->getParameter(0)->getValue());
+        self::assertSame(3, $builder->getParameter('0')->getValue());
+        self::assertSame('Doctrine', $builder->getParameter('name')->getValue());
+    }
 }