Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use psr/cache for result caching #4620

Merged
merged 1 commit into from
Apr 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,21 @@ awareness about deprecated code.
- Use of our low-overhead runtime deprecation API, details:
https://github.com/doctrine/deprecations/

# Upgrade to 3.2

## Introduction of PSR-6 for result caching

Instead of relying on the deprecated `doctrine/cache` library, a PSR-6 cache
can now be used for result caching. Please use the following new methods for
this purpose:

| class | old method | new method |
| ------------------- | ------------------------ | ------------------ |
| `Configuration` | `setResultCacheImpl()` | `setResultCache()` |
| `Configuration` | `getResultCacheImpl()` | `getResultCache()` |
| `QueryCacheProfile` | `setResultCacheDriver()` | `setResultCache()` |
| `QueryCacheProfile` | `getResultCacheDriver()` | `getResultCache()` |

# Upgrade to 3.1

## Deprecated schema- and namespace-related methods
Expand Down
6 changes: 4 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@
"require": {
"php": "^7.3 || ^8.0",
"composer/package-versions-deprecated": "^1.11.99",
"doctrine/cache": "^1.0",
"doctrine/cache": "^1.11",
"doctrine/deprecations": "^0.5.3",
"doctrine/event-manager": "^1.0"
"doctrine/event-manager": "^1.0",
"psr/cache": "^1|^2|^3"
},
"require-dev": {
"doctrine/coding-standard": "8.2.0",
Expand All @@ -45,6 +46,7 @@
"phpunit/phpunit": "9.5.0",
"psalm/plugin-phpunit": "0.13.0",
"squizlabs/php_codesniffer": "3.6.0",
"symfony/cache": "^5.2",
derrabus marked this conversation as resolved.
Show resolved Hide resolved
"symfony/console": "^2.0.5|^3.0|^4.0|^5.0",
"vimeo/psalm": "4.6.4"
},
Expand Down
6 changes: 6 additions & 0 deletions phpstan.neon.dist
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,11 @@ parameters:
message: '~^Instanceof between Doctrine\\DBAL\\Platforms\\Keywords\\KeywordList and Doctrine\\DBAL\\Platforms\\Keywords\\KeywordList will always evaluate to true\.~'
paths:
- %currentWorkingDirectory%/src/Platforms/AbstractPlatform.php

# We're checking for invalid invalid input
derrabus marked this conversation as resolved.
Show resolved Hide resolved
-
message: "#^Strict comparison using \\!\\=\\= between null and null will always evaluate to false\\.$#"
count: 1
path: src/Cache/QueryCacheProfile.php
includes:
- vendor/phpstan/phpstan-strict-rules/rules.neon
12 changes: 12 additions & 0 deletions psalm.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@
is no longer supported.
-->
<file name="src/Tools/Console/ConsoleRunner.php"/>
<!--
This suppression should be removed once Doctrine Cache
is no longer supported.
-->
<file name="tests/Functional/ResultCacheTest.php"/>
</errorLevel>
</DeprecatedClass>
<DeprecatedMethod>
Expand Down Expand Up @@ -87,6 +92,12 @@
See https://github.com/doctrine/dbal/pull/4518
-->
<file name="src/Connection.php"/>

<!--
This suppression should be removed in 4.0.x
See https://github.com/doctrine/dbal/pull/4620
-->
<file name="src/Configuration.php"/>
</errorLevel>
</DeprecatedProperty>
<DocblockTypeContradiction>
Expand All @@ -100,6 +111,7 @@
1. Union types not supported at the language level (require dropping PHP 7 support)
2. Associative arrays with typed elements used instead of classes (require breaking API changes)
-->
<file name="src/Cache/QueryCacheProfile.php"/>
<file name="src/Connection.php"/>
<file name="src/Driver/IBMDB2/Statement.php"/>
<file name="src/DriverManager.php"/>
Expand Down
19 changes: 10 additions & 9 deletions src/Cache/CachingResult.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

namespace Doctrine\DBAL\Cache;

use Doctrine\Common\Cache\Cache;
use Doctrine\DBAL\Driver\Exception;
use Doctrine\DBAL\Driver\FetchUtils;
use Doctrine\DBAL\Driver\Result as DriverResult;
use Doctrine\DBAL\Result;
use Psr\Cache\CacheItemPoolInterface;

use function array_map;
use function array_values;
Expand All @@ -26,7 +26,7 @@
*/
class CachingResult implements DriverResult
{
/** @var Cache */
/** @var CacheItemPoolInterface */
private $cache;

/** @var string */
Expand All @@ -49,7 +49,7 @@ class CachingResult implements DriverResult
* @param string $realKey
* @param int $lifetime
*/
public function __construct(Result $result, Cache $cache, $cacheKey, $realKey, $lifetime)
public function __construct(Result $result, CacheItemPoolInterface $cache, $cacheKey, $realKey, $lifetime)
{
$this->result = $result;
$this->cache = $cache;
Expand Down Expand Up @@ -171,14 +171,15 @@ private function saveToCache(): void
return;
}

$data = $this->cache->fetch($this->cacheKey);
$item = $this->cache->getItem($this->cacheKey);
$data = $item->isHit() ? $item->get() : [];
$data[$this->realKey] = $this->data;

if ($data === false) {
$data = [];
$item->set($data);
if ($this->lifetime > 0) {
$item->expiresAfter($this->lifetime);
}

$data[$this->realKey] = $this->data;

$this->cache->save($this->cacheKey, $data, $this->lifetime);
$this->cache->save($item);
}
}
56 changes: 44 additions & 12 deletions src/Cache/QueryCacheProfile.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,17 @@
namespace Doctrine\DBAL\Cache;

use Doctrine\Common\Cache\Cache;
use Doctrine\Common\Cache\Psr6\CacheAdapter;
use Doctrine\Common\Cache\Psr6\DoctrineProvider;
use Doctrine\DBAL\Types\Type;
use Psr\Cache\CacheItemPoolInterface;
use TypeError;

use function get_class;
use function hash;
use function serialize;
use function sha1;
use function sprintf;

/**
* Query Cache Profile handles the data relevant for query caching.
Expand All @@ -16,8 +22,8 @@
*/
class QueryCacheProfile
{
/** @var Cache|null */
private $resultCacheDriver;
/** @var CacheItemPoolInterface|null */
private $resultCache;

/** @var int */
private $lifetime = 0;
Expand All @@ -26,22 +32,41 @@ class QueryCacheProfile
private $cacheKey;

/**
* @param int $lifetime
* @param string|null $cacheKey
* @param int $lifetime
* @param string|null $cacheKey
* @param CacheItemPoolInterface|Cache|null $resultCache
*/
public function __construct($lifetime = 0, $cacheKey = null, ?Cache $resultCache = null)
public function __construct($lifetime = 0, $cacheKey = null, ?object $resultCache = null)
{
$this->lifetime = $lifetime;
$this->cacheKey = $cacheKey;
if ($resultCache instanceof CacheItemPoolInterface) {
$this->resultCache = $resultCache;
} elseif ($resultCache instanceof Cache) {
$this->resultCache = CacheAdapter::wrap($resultCache);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derrabus my team just stumbled upon this, this is a BC Break I'm afraid, if the cache key contains reserved characters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate? Or better, can you open an issue so we can discuss the BC break an possible solutions?

Copy link
Member

@greg0ire greg0ire Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derrabus sorry, didn't notice your message. I will open an issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #5051

} elseif ($resultCache !== null) {
throw new TypeError(sprintf(
'$resultCache: Expected either null or an instance of %s or %s, got %s.',
CacheItemPoolInterface::class,
Cache::class,
get_class($resultCache)
));
}
}

public function getResultCache(): ?CacheItemPoolInterface
{
$this->lifetime = $lifetime;
$this->cacheKey = $cacheKey;
$this->resultCacheDriver = $resultCache;
return $this->resultCache;
}

/**
* @deprecated Use {@see getResultCache()} instead.
*
* @return Cache|null
*/
public function getResultCacheDriver()
{
return $this->resultCacheDriver;
return $this->resultCache !== null ? DoctrineProvider::wrap($this->resultCache) : null;
}

/**
Expand Down Expand Up @@ -93,12 +118,19 @@ public function generateCacheKeys($sql, $params, $types, array $connectionParams
return [$cacheKey, $realCacheKey];
}

public function setResultCache(CacheItemPoolInterface $cache): QueryCacheProfile
{
return new QueryCacheProfile($this->lifetime, $this->cacheKey, $cache);
}

/**
* @deprecated Use {@see setResultCache()} instead.
*
* @return QueryCacheProfile
*/
public function setResultCacheDriver(Cache $cache)
{
return new QueryCacheProfile($this->lifetime, $this->cacheKey, $cache);
return new QueryCacheProfile($this->lifetime, $this->cacheKey, CacheAdapter::wrap($cache));
}

/**
Expand All @@ -108,7 +140,7 @@ public function setResultCacheDriver(Cache $cache)
*/
public function setCacheKey($cacheKey)
{
return new QueryCacheProfile($this->lifetime, $cacheKey, $this->resultCacheDriver);
return new QueryCacheProfile($this->lifetime, $cacheKey, $this->resultCache);
}

/**
Expand All @@ -118,6 +150,6 @@ public function setCacheKey($cacheKey)
*/
public function setLifetime($lifetime)
{
return new QueryCacheProfile($lifetime, $this->cacheKey, $this->resultCacheDriver);
return new QueryCacheProfile($lifetime, $this->cacheKey, $this->resultCache);
}
}
34 changes: 34 additions & 0 deletions src/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
namespace Doctrine\DBAL;

use Doctrine\Common\Cache\Cache;
use Doctrine\Common\Cache\Psr6\CacheAdapter;
use Doctrine\Common\Cache\Psr6\DoctrineProvider;
use Doctrine\DBAL\Driver\Middleware;
use Doctrine\DBAL\Logging\SQLLogger;
use Psr\Cache\CacheItemPoolInterface;

/**
* Configuration container for the Doctrine DBAL.
Expand All @@ -24,6 +27,15 @@ class Configuration
/**
* The cache driver implementation that is used for query result caching.
*
* @var CacheItemPoolInterface|null
*/
private $resultCache;

/**
* The cache driver implementation that is used for query result caching.
*
* @deprecated Use {@see $resultCache} instead.
*
* @var Cache|null
*/
protected $resultCacheImpl;
Expand Down Expand Up @@ -61,6 +73,16 @@ public function getSQLLogger(): ?SQLLogger
/**
* Gets the cache driver implementation that is used for query result caching.
*/
public function getResultCache(): ?CacheItemPoolInterface
{
return $this->resultCache;
}

/**
* Gets the cache driver implementation that is used for query result caching.
*
* @deprecated Use {@see getResultCache()} instead.
*/
public function getResultCacheImpl(): ?Cache
{
return $this->resultCacheImpl;
Expand All @@ -69,9 +91,21 @@ public function getResultCacheImpl(): ?Cache
/**
* Sets the cache driver implementation that is used for query result caching.
*/
public function setResultCache(CacheItemPoolInterface $cache): void
{
$this->resultCacheImpl = DoctrineProvider::wrap($cache);
$this->resultCache = $cache;
}

/**
* Sets the cache driver implementation that is used for query result caching.
*
* @deprecated Use {@see setResultCache()} instead.
*/
public function setResultCacheImpl(Cache $cacheImpl): void
{
$this->resultCacheImpl = $cacheImpl;
$this->resultCache = CacheAdapter::wrap($cacheImpl);
}

/**
Expand Down
7 changes: 4 additions & 3 deletions src/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -1087,7 +1087,7 @@ public function executeQuery(
*/
public function executeCacheQuery($sql, $params, $types, QueryCacheProfile $qcp): Result
{
$resultCache = $qcp->getResultCacheDriver() ?? $this->_config->getResultCacheImpl();
$resultCache = $qcp->getResultCache() ?? $this->_config->getResultCache();

if ($resultCache === null) {
throw CacheException::noResultDriverConfigured();
Expand All @@ -1099,9 +1099,10 @@ public function executeCacheQuery($sql, $params, $types, QueryCacheProfile $qcp)
[$cacheKey, $realKey] = $qcp->generateCacheKeys($sql, $params, $types, $connectionParams);

// fetch the row pointers entry
$data = $resultCache->fetch($cacheKey);
$item = $resultCache->getItem($cacheKey);

if ($data !== false) {
if ($item->isHit()) {
$data = $item->get();
// is the real key part of this row pointers map or is the cache only pointing to other cache keys?
if (isset($data[$realKey])) {
$result = new ArrayResult($data[$realKey]);
Expand Down