From 960a437d46e6bc0df5049da0ae5e9a9f5d067516 Mon Sep 17 00:00:00 2001
From: Marco Pivetta <ocramius@gmail.com>
Date: Sun, 16 Dec 2018 15:37:45 +0100
Subject: [PATCH] #7527 failing test case:
`UnitOfWork#getSingleIdentifierValue()` should not be called for a well
specified parameter type
As previously reported by @flaushi in https://github.com/doctrine/doctrine2/pull/7471#discussion_r241949045, we discovered
that binding a parameter causes a `ClassMetadataFactory#getClassMetadata()` call, which in turn leads to large performance
regression when using any `object` type as parameter.
Following two snippets lead to an internal `ClassMetadataFactory#getClassMetadata()` call, which in turn leads to an
exception being thrown and garbage collected, plus multiple associated performance implications:
```php
$query->setParameter('foo', new DateTime());
$query->getResult();
```
```php
$query->setParameter('foo', new DateTime(), DateTimeType::NAME);
$query->getResult();
```
This is due to following portion of code:
https://github.com/doctrine/doctrine2/blob/434820973cadf2da2d66e7184be370084cc32ca8/lib/Doctrine/ORM/Query.php#L406-L409
Notice how `$value = $this->processParameterValue($value);` happens before attempting to infer the type for the parameter value.
That call leads to this segment being reached, which leads to the regression:
https://github.com/doctrine/doctrine2/blob/434820973cadf2da2d66e7184be370084cc32ca8/lib/Doctrine/ORM/AbstractQuery.php#L423-L433
Assuming the bound parameter type is provided, we can completely skip attempting to introspect the given object:
```php
$query->setParameter('foo', new DateTime(), DateTimeType::NAME);
$query->getResult();
```
Processing the parameter value is not needed in this case, so we can safely skip that logic for all known parameters.
In order to not introduce a BC break or change the `AbstractQuery#processParameterValue()` implementation, we could filter
out all parameters for which the type is given upfront, and later on merge them back in instead.
The test expectation to be set is for `UnitOfWork#getSingleIdentifierValue()` to never be called.
---
tests/Doctrine/Tests/ORM/Query/QueryTest.php | 30 ++++++++++++++++----
tests/Doctrine/Tests/OrmTestCase.php | 7 ++---
2 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/tests/Doctrine/Tests/ORM/Query/QueryTest.php b/tests/Doctrine/Tests/ORM/Query/QueryTest.php
index 8a3caf66f..0045647a0 100644
--- a/tests/Doctrine/Tests/ORM/Query/QueryTest.php
+++ b/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());
+ }
}
diff --git a/tests/Doctrine/Tests/OrmTestCase.php b/tests/Doctrine/Tests/OrmTestCase.php
index 46e8ea143..6fdbe1471 100644
--- a/tests/Doctrine/Tests/OrmTestCase.php
+++ b/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)