Merge pull request #7528 from Ocramius/fix/#7527-prevent-unit-of-work-lookup-for-known-value-types

Fix #7527:  prevent `UnitOfWork` lookup for DBAL types specified in `Doctrine\ORM\Query#setParameter()`

lib/Doctrine/ORM/AbstractQuery.php

@@ -72,7 +72,7 @@ abstract class AbstractQuery
     /**
      * The parameter map of this query.
      *
-     * @var \Doctrine\Common\Collections\ArrayCollection
+     * @var ArrayCollection|Parameter[]
      */
     protected $parameters;
 
@@ -306,7 +306,7 @@ abstract class AbstractQuery
     /**
      * Get all defined parameters.
      *
-     * @return \Doctrine\Common\Collections\ArrayCollection The defined query parameters.
+     * @return ArrayCollection The defined query parameters.
      */
     public function getParameters()
     {
@@ -336,7 +336,7 @@ abstract class AbstractQuery
     /**
      * Sets a collection of query parameters.
      *
-     * @param \Doctrine\Common\Collections\ArrayCollection|array $parameters
+     * @param ArrayCollection|mixed[] $parameters
      *
      * @return static This query instance.
      */

lib/Doctrine/ORM/Query.php

@@ -19,15 +19,18 @@
 
 namespace Doctrine\ORM;
 
+use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\DBAL\LockMode;
+use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\Query\Exec\AbstractSqlExecutor;
+use Doctrine\ORM\Query\Parameter;
+use Doctrine\ORM\Query\ParameterTypeInferer;
 use Doctrine\ORM\Query\Parser;
 use Doctrine\ORM\Query\ParserResult;
 use Doctrine\ORM\Query\QueryException;
-use Doctrine\ORM\Mapping\ClassMetadata;
-use Doctrine\ORM\Query\ParameterTypeInferer;
-use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\ORM\Utility\HierarchyDiscriminatorResolver;
+use function array_keys;
+use function assert;
 
 /**
  * A Query object represents a DQL query.
@@ -387,26 +390,13 @@ final class Query extends AbstractQuery
         $types     = [];
 
         foreach ($this->parameters as $parameter) {
-            $key    = $parameter->getName();
-            $value  = $parameter->getValue();
-            $rsm    = $this->getResultSetMapping();
+            $key = $parameter->getName();
 
             if ( ! isset($paramMappings[$key])) {
                 throw QueryException::unknownParameter($key);
             }
 
-            if (isset($rsm->metadataParameterMapping[$key]) && $value instanceof ClassMetadata) {
-                $value = $value->getMetadataValue($rsm->metadataParameterMapping[$key]);
-            }
-
-            if (isset($rsm->discriminatorParameters[$key]) && $value instanceof ClassMetadata) {
-                $value = array_keys(HierarchyDiscriminatorResolver::resolveDiscriminatorsForClass($value, $this->_em));
-            }
-
-            $value = $this->processParameterValue($value);
-            $type  = ($parameter->getValue() === $value)
-                ? $parameter->getType()
-                : ParameterTypeInferer::inferType($value);
+            [$value, $type] = $this->resolveParameterValue($parameter);
 
             foreach ($paramMappings[$key] as $position) {
                 $types[$position] = $type;
@@ -439,6 +429,38 @@ final class Query extends AbstractQuery
         return [$sqlParams, $types];
     }
 
+    /** @return mixed[] tuple of (value, type) */
+    private function resolveParameterValue(Parameter $parameter) : array
+    {
+        if ($parameter->typeWasSpecified()) {
+            return [$parameter->getValue(), $parameter->getType()];
+        }
+
+        $key           = $parameter->getName();
+        $originalValue = $parameter->getValue();
+        $value         = $originalValue;
+        $rsm           = $this->getResultSetMapping();
+
+        assert($rsm !== null);
+
+        if ($value instanceof ClassMetadata && isset($rsm->metadataParameterMapping[$key])) {
+            $value = $value->getMetadataValue($rsm->metadataParameterMapping[$key]);
+        }
+
+        if ($value instanceof ClassMetadata && isset($rsm->discriminatorParameters[$key])) {
+            $value = array_keys(HierarchyDiscriminatorResolver::resolveDiscriminatorsForClass($value, $this->_em));
+        }
+
+        $processedValue = $this->processParameterValue($value);
+
+        return [
+            $processedValue,
+            $originalValue === $processedValue
+                ? $parameter->getType()
+                : ParameterTypeInferer::inferType($processedValue),
+        ];
+    }
+
     /**
      * Defines a cache driver to be used for caching queries.
      *

lib/Doctrine/ORM/Query/Parameter.php

@@ -19,6 +19,8 @@
 
 namespace Doctrine\ORM\Query;
 
+use function trim;
+
 /**
  * Defines a Query Parameter.
  *
@@ -49,6 +51,13 @@ class Parameter
      */
     private $type;
 
+    /**
+     * Whether the parameter type was explicitly specified or not
+     *
+     * @var bool
+     */
+    private $typeSpecified;
+
     /**
      * Constructor.
      *
@@ -58,7 +67,8 @@ class Parameter
      */
     public function __construct($name, $value, $type = null)
     {
-        $this->name = trim($name, ':');
+        $this->name          = trim($name, ':');
+        $this->typeSpecified = $type !== null;
 
         $this->setValue($value, $type);
     }
@@ -104,4 +114,9 @@ class Parameter
         $this->value = $value;
         $this->type  = $type ?: ParameterTypeInferer::inferType($value);
     }
+
+    public function typeWasSpecified() : bool
+    {
+        return $this->typeSpecified;
+    }
 }

tests/Doctrine/Performance/EntityManagerFactory.php

@@ -1,13 +1,21 @@
 <?php
 
+declare(strict_types=1);
+
 namespace Doctrine\Performance;
 
+use Doctrine\Common\EventManager;
+use Doctrine\DBAL\Cache\ArrayStatement;
+use Doctrine\DBAL\Cache\QueryCacheProfile;
+use Doctrine\DBAL\Connection;
 use Doctrine\DBAL\Driver\PDOSqlite\Driver;
 use Doctrine\ORM\Configuration;
 use Doctrine\ORM\EntityManager;
 use Doctrine\ORM\EntityManagerInterface;
 use Doctrine\ORM\Proxy\ProxyFactory;
 use Doctrine\ORM\Tools\SchemaTool;
+use function array_map;
+use function realpath;
 
 final class EntityManagerFactory
 {
@@ -20,7 +28,7 @@ final class EntityManagerFactory
         $config->setAutoGenerateProxyClasses(ProxyFactory::AUTOGENERATE_EVAL);
         $config->setMetadataDriverImpl($config->newDefaultAnnotationDriver([
             realpath(__DIR__ . '/Models/Cache'),
-            realpath(__DIR__ . '/Models/GeoNames')
+            realpath(__DIR__ . '/Models/GeoNames'),
         ], true));
 
         $entityManager = EntityManager::create(
@@ -36,4 +44,30 @@ final class EntityManagerFactory
 
         return $entityManager;
     }
+
+    public static function makeEntityManagerWithNoResultsConnection() : EntityManagerInterface
+    {
+        $config = new Configuration();
+
+        $config->setProxyDir(__DIR__ . '/../Tests/Proxies');
+        $config->setProxyNamespace('Doctrine\Tests\Proxies');
+        $config->setAutoGenerateProxyClasses(ProxyFactory::AUTOGENERATE_EVAL);
+        $config->setMetadataDriverImpl($config->newDefaultAnnotationDriver([
+            realpath(__DIR__ . '/Models/Cache'),
+            realpath(__DIR__ . '/Models/Generic'),
+            realpath(__DIR__ . '/Models/GeoNames'),
+        ], true));
+
+        // A connection that doesn't really do anything
+        $connection = new class ([], new Driver(), null, new EventManager()) extends Connection
+        {
+            /** {@inheritdoc} */
+            public function executeQuery($query, array $params = [], $types = [], ?QueryCacheProfile $qcp = null)
+            {
+                return new ArrayStatement([]);
+            }
+        };
+
+        return EntityManager::create($connection, $config);
+    }
 }

tests/Doctrine/Performance/Query/QueryBoundParameterProcessingBench.php

@@ -0,0 +1,78 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Performance\Query;
+
+use DateTime;
+use Doctrine\DBAL\Types\DateTimeType;
+use Doctrine\ORM\Query;
+use Doctrine\Performance\EntityManagerFactory;
+use PhpBench\Benchmark\Metadata\Annotations\BeforeMethods;
+use function range;
+
+/**
+ * @BeforeMethods({"init"})
+ */
+final class QueryBoundParameterProcessingBench
+{
+    /** @var Query */
+    private $parsedQueryWithInferredParameterType;
+
+    /** @var Query */
+    private $parsedQueryWithDeclaredParameterType;
+
+    public function init() : void
+    {
+        $entityManager = EntityManagerFactory::makeEntityManagerWithNoResultsConnection();
+
+        // Note: binding a lot of parameters because DQL operations are noisy due to hydrators and other components
+        //       kicking in, so we make the parameter operations more noticeable.
+        $dql = <<<'DQL'
+SELECT e
+FROM Doctrine\Tests\Models\Generic\DateTimeModel e
+WHERE
+    e.datetime = :parameter1
+    OR
+    e.datetime = :parameter2
+    OR
+    e.datetime = :parameter3
+    OR
+    e.datetime = :parameter4
+    OR
+    e.datetime = :parameter5
+    OR
+    e.datetime = :parameter6
+    OR
+    e.datetime = :parameter7
+    OR
+    e.datetime = :parameter8
+    OR
+    e.datetime = :parameter9
+    OR
+    e.datetime = :parameter10
+DQL;
+
+        $this->parsedQueryWithInferredParameterType = $entityManager->createQuery($dql);
+        $this->parsedQueryWithDeclaredParameterType = $entityManager->createQuery($dql);
+
+        foreach (range(1, 10) as $index) {
+            $this->parsedQueryWithInferredParameterType->setParameter('parameter' . $index, new DateTime());
+            $this->parsedQueryWithDeclaredParameterType->setParameter('parameter' . $index, new DateTime(), DateTimeType::DATETIME);
+        }
+
+        // Force parsing upfront - we don't benchmark that bit in this scenario
+        $this->parsedQueryWithInferredParameterType->getSQL();
+        $this->parsedQueryWithDeclaredParameterType->getSQL();
+    }
+
+    public function benchExecuteParsedQueryWithInferredParameterType() : void
+    {
+        $this->parsedQueryWithInferredParameterType->execute();
+    }
+
+    public function benchExecuteParsedQueryWithDeclaredParameterType() : void
+    {
+        $this->parsedQueryWithDeclaredParameterType->execute();
+    }
+}

tests/Doctrine/Tests/ORM/Query/QueryTest.php

@@ -2,24 +2,26 @@
 
 namespace Doctrine\Tests\ORM\Query;
 
+use DateTime;
 use Doctrine\Common\Cache\ArrayCache;
 use Doctrine\Common\Collections\ArrayCollection;
-
-use Doctrine\DBAL\Cache\QueryCacheProfile;
-use Doctrine\ORM\EntityManager;
+use Doctrine\DBAL\Types\Type;
 use Doctrine\ORM\Internal\Hydration\IterableResult;
 use Doctrine\ORM\Query\Parameter;
 use Doctrine\ORM\Query\QueryException;
+use Doctrine\ORM\UnitOfWork;
 use Doctrine\Tests\Mocks\DriverConnectionMock;
+use Doctrine\Tests\Mocks\EntityManagerMock;
 use Doctrine\Tests\Mocks\StatementArrayMock;
 use Doctrine\Tests\Models\CMS\CmsAddress;
 use Doctrine\Tests\Models\CMS\CmsUser;
+use Doctrine\Tests\Models\Generic\DateTimeModel;
 use Doctrine\Tests\OrmTestCase;
 
 class QueryTest extends OrmTestCase
 {
-    /** @var EntityManager */
-    protected $_em = null;
+    /** @var EntityManagerMock */
+    protected $_em;
 
     protected function setUp()
     {
@@ -400,4 +402,22 @@ class QueryTest extends OrmTestCase
 
         self::assertAttributeSame(null, '_queryCacheProfile', $query);
     }
+
+    /** @group 7527 */
+    public function testValuesAreNotBeingResolvedForSpecifiedParameterTypes() : void
+    {
+        $unitOfWork = $this->createMock(UnitOfWork::class);
+
+        $this->_em->setUnitOfWork($unitOfWork);
+
+        $unitOfWork
+            ->expects(self::never())
+            ->method('getSingleIdentifierValue');
+
+        $query = $this->_em->createQuery('SELECT d FROM ' . DateTimeModel::class . ' d WHERE d.datetime = :value');
+
+        $query->setParameter('value', new DateTime(), Type::DATETIME);
+
+        self::assertEmpty($query->getResult());
+    }
 }

tests/Doctrine/Tests/ORM/QueryBuilderTest.php

@@ -609,8 +609,11 @@ class QueryBuilderTest extends OrmTestCase
             ->setParameter('id', 1);
 
         $parameter = new Parameter('id', 1, ParameterTypeInferer::inferType(1));
+        $inferred  = $qb->getParameter('id');
 
-        $this->assertEquals($parameter, $qb->getParameter('id'));
+        self::assertSame($parameter->getValue(), $inferred->getValue());
+        self::assertSame($parameter->getType(), $inferred->getType());
+        self::assertFalse($inferred->typeWasSpecified());
     }
 
     public function testSetParameters()

tests/Doctrine/Tests/OrmTestCase.php

@@ -11,6 +11,7 @@ use Doctrine\ORM\Cache\DefaultCacheFactory;
 use Doctrine\ORM\Configuration;
 use Doctrine\ORM\Mapping\Driver\AnnotationDriver;
 use Doctrine\Tests\Mocks;
+use Doctrine\Tests\Mocks\EntityManagerMock;
 
 /**
  * Base testcase class for all ORM testcases.
@@ -113,10 +114,8 @@ abstract class OrmTestCase extends DoctrineTestCase
      * @param mixed                              $conf
      * @param \Doctrine\Common\EventManager|null $eventManager
      * @param bool                               $withSharedMetadata
-     *
-     * @return \Doctrine\ORM\EntityManager
      */
-    protected function _getTestEntityManager($conn = null, $conf = null, $eventManager = null, $withSharedMetadata = true)
+    protected function _getTestEntityManager($conn = null, $conf = null, $eventManager = null, $withSharedMetadata = true) : EntityManagerMock
     {
         $metadataCache = $withSharedMetadata
             ? self::getSharedMetadataCacheImpl()
@@ -160,7 +159,7 @@ abstract class OrmTestCase extends DoctrineTestCase
             $conn = DriverManager::getConnection($conn, $config, $eventManager);
         }
 
-        return Mocks\EntityManagerMock::create($conn, $config, $eventManager);
+        return EntityManagerMock::create($conn, $config, $eventManager);
     }
 
     protected function enableSecondLevelCache($log = true)