Fix expire result cache #6417
Fix expire result cache #6417
Conversation
lib/Doctrine/ORM/Query.php
Outdated
| @@ -322,6 +322,12 @@ protected function _doExecute() | |||
|
|
|||
| list($sqlParams, $types) = $this->processParameterMappings($paramMappings); | |||
|
|
|||
| if ($this->_queryCacheProfile && $this->getExpireResultCache()) { | |||
| $this->_queryCacheProfile->getResultCacheDriver()->delete( | |||
| $this->_queryCacheProfile->generateCacheKeys($executor->getSqlStatements(), $sqlParams, $types)[0] | |||
There was a problem hiding this comment.
If this is a list, iterate over it (even though now it only returns one item) and delete each record.
There was a problem hiding this comment.
I thought that but although the docblock says it returns an array it can actually be string 😱
lcobucci
force-pushed
the
fix-expire-result-cache
branch
2 times, most recently
from
611816f
to
4a6c0c6
Compare
| @@ -682,6 +689,11 @@ protected function _getEntityManager($config = null, $eventManager = null) { | |||
| self::$_queryCacheImpl = new ArrayCache(); | |||
| } | |||
|
|
|||
| if (is_null(self::$_resultCacheImpl)) { | |||
| * | ||
| * @var \Doctrine\Common\Cache\Cache|null | ||
| */ | ||
| private static $_resultCacheImpl = null; |
There was a problem hiding this comment.
If we introduce new properties, we shouldn't use the prefix... Also, the fact that this is a static smells a lot: what if it is accidentally not unset between test cycles? Is it supposed to always be set?
There was a problem hiding this comment.
True thing, I'll keep it non-static and leave the instantiation to the test that actually uses it. Thanks
lib/Doctrine/ORM/Query.php
Outdated
|
|
||
| $cacheDriver = $this->_queryCacheProfile->getResultCacheDriver(); | ||
|
|
||
| foreach ((array) $executor->getSqlStatements() as $statement) { |
There was a problem hiding this comment.
Why the (array) cast? Smelly :|
There was a problem hiding this comment.
It's indeed... cause: #6417 (comment)
There was a problem hiding this comment.
@lcobucci can you add that as a comment in the code?
lib/Doctrine/ORM/Query.php
Outdated
| $cacheDriver = $this->_queryCacheProfile->getResultCacheDriver(); | ||
|
|
||
| foreach ((array) $executor->getSqlStatements() as $statement) { | ||
| $cacheDriver->delete($this->_queryCacheProfile->generateCacheKeys($statement, $sqlParams, $types)[0]); |
There was a problem hiding this comment.
What do you suggest? current()? list()?
| $this->brand = $brand; | ||
| } | ||
|
|
||
| public function __toString() |
There was a problem hiding this comment.
We can start violently adding type hints everywhere
| */ | ||
| public $brand; | ||
|
|
||
| public function __construct($brand) |
There was a problem hiding this comment.
We can start violently adding type hints everywhere
| /** | ||
| * @group 2947 | ||
| */ | ||
| class GH2947Test extends OrmFunctionalTestCase |
There was a problem hiding this comment.
Do we have a Query-specific test that we can use here?
There was a problem hiding this comment.
Yes - are there no unit tests there? GH-XYZ tests are fine, but they make it hard to track supported features by looking at unit tests.
There was a problem hiding this comment.
Doctrine\Tests\ORM\Query\QueryTest#testResultCacheEviction() not strictly a unit test though but should be enough 😉
lcobucci
force-pushed
the
fix-expire-result-cache
branch
from
4a6c0c6
to
e71272e
Compare
| /** | ||
| * @group 2947 | ||
| */ | ||
| class GH2947Test extends OrmFunctionalTestCase |
|
@lcobucci 🚢 \o/ |
|
@Ocramius awesome thanks! |
Simplifying test provided in #6390 and process the result set cache eviction when flag was passed.