From 858451a9cd8763ce8d12ca5b77c98cc028484c7a Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 19 Jun 2018 15:19:01 -0700 Subject: [PATCH] (dev/core#174) CRM_Utils_Cache_Interface::set() should match PSR-16 Comparing `CRM_Utils_Cache_Interface::set()` and `Psr\SimpleCache\CacheInterface::set()`, they agree on key details: 1. The first value is a string key. 2. The second value is a mixed field (string or number or array...). 3. The return value should be a boolean indicating success/failure. There are two differences: 1. Our old interface didn't actually enforce the return-value in all cases. Should be better now. 2. PSR-16 accepts a third argument, `$ttl`, which defaults to `NULL`. In the NULL/default case, the semantics are the same (i.e. up to the discretion of the driver). 1. Since no existing callers actually use this, we can (for moment) throw an error if someone tries to pass `$ttl` to a driver that doesn't support it. 3. The `$value` is pass-by-reference. However, this probably doesn't mean anything. 1. In older Civi code, there was a tendenacy to pass-by-reference without any real need -- perhaps just speculation about performance. 2. Most cache drivers serialize the value and send it elsewhere. References will not survive the process. If there's some code/caller/use-case that requires the references to work, then it's already broken for most drivers (except maybe `ArrayCache`). 3. The old signature meant that callers had to say `$bar=123; $cache->set('foo', $bar)` rather than `$cache->set('foo', 123)`. This change relaxes the contract -- so that you can pass either `$bar` or `123. See also: https://www.php-fig.org/psr/psr-16/ --- CRM/Utils/Cache/APCcache.php | 6 +++++- CRM/Utils/Cache/ArrayCache.php | 8 +++++++- CRM/Utils/Cache/Interface.php | 4 +++- CRM/Utils/Cache/Memcache.php | 6 +++++- CRM/Utils/Cache/Memcached.php | 6 +++++- CRM/Utils/Cache/NoCache.php | 3 ++- CRM/Utils/Cache/Redis.php | 6 +++++- CRM/Utils/Cache/SerializeCache.php | 12 +++++++++--- CRM/Utils/Cache/SqlGroup.php | 8 +++++++- 9 files changed, 48 insertions(+), 11 deletions(-) diff --git a/CRM/Utils/Cache/APCcache.php b/CRM/Utils/Cache/APCcache.php index 64c1de94b6..cb08b7cd16 100644 --- a/CRM/Utils/Cache/APCcache.php +++ b/CRM/Utils/Cache/APCcache.php @@ -72,10 +72,14 @@ class CRM_Utils_Cache_APCcache implements CRM_Utils_Cache_Interface { /** * @param $key * @param $value + * @param null|int|\DateInterval $ttl * * @return bool */ - public function set($key, &$value) { + public function set($key, $value, $ttl = NULL) { + if ($ttl !== NULL) { + throw new \RuntimeException("FIXME: " . __CLASS__ . "::set() should support non-NULL TTL"); + } if (!apc_store($this->_prefix . $key, $value, $this->_timeout)) { return FALSE; } diff --git a/CRM/Utils/Cache/ArrayCache.php b/CRM/Utils/Cache/ArrayCache.php index 1c1b2b2979..5d3fc7c050 100644 --- a/CRM/Utils/Cache/ArrayCache.php +++ b/CRM/Utils/Cache/ArrayCache.php @@ -56,9 +56,15 @@ class CRM_Utils_Cache_Arraycache implements CRM_Utils_Cache_Interface { /** * @param string $key * @param mixed $value + * @param null|int|\DateInterval $ttl + * @return bool */ - public function set($key, &$value) { + public function set($key, $value, $ttl = NULL) { + if ($ttl !== NULL) { + throw new \RuntimeException("FIXME: " . __CLASS__ . "::set() should support non-NULL TTL"); + } $this->_cache[$key] = $value; + return TRUE; } /** diff --git a/CRM/Utils/Cache/Interface.php b/CRM/Utils/Cache/Interface.php index d1d1576a3b..0f95b99ffd 100644 --- a/CRM/Utils/Cache/Interface.php +++ b/CRM/Utils/Cache/Interface.php @@ -62,8 +62,10 @@ interface CRM_Utils_Cache_Interface { * * @param string $key * @param mixed $value + * @param null|int|\DateInterval $ttl + * @return bool */ - public function set($key, &$value); + public function set($key, $value, $ttl = NULL); /** * Get a value from the cache. diff --git a/CRM/Utils/Cache/Memcache.php b/CRM/Utils/Cache/Memcache.php index 85a1376500..dcab723283 100644 --- a/CRM/Utils/Cache/Memcache.php +++ b/CRM/Utils/Cache/Memcache.php @@ -109,10 +109,14 @@ class CRM_Utils_Cache_Memcache implements CRM_Utils_Cache_Interface { /** * @param $key * @param $value + * @param null|int|\DateInterval $ttl * * @return bool */ - public function set($key, &$value) { + public function set($key, $value, $ttl = NULL) { + if ($ttl !== NULL) { + throw new \RuntimeException("FIXME: " . __CLASS__ . "::set() should support non-NULL TTL"); + } if (!$this->_cache->set($this->_prefix . $key, $value, FALSE, $this->_timeout)) { return FALSE; } diff --git a/CRM/Utils/Cache/Memcached.php b/CRM/Utils/Cache/Memcached.php index c653154483..82910da114 100644 --- a/CRM/Utils/Cache/Memcached.php +++ b/CRM/Utils/Cache/Memcached.php @@ -110,11 +110,15 @@ class CRM_Utils_Cache_Memcached implements CRM_Utils_Cache_Interface { /** * @param $key * @param $value + * @param null|int|\DateInterval $ttl * * @return bool * @throws Exception */ - public function set($key, &$value) { + public function set($key, $value, $ttl = NULL) { + if ($ttl !== NULL) { + throw new \RuntimeException("FIXME: " . __CLASS__ . "::set() should support non-NULL TTL"); + } $key = $this->cleanKey($key); if (!$this->_cache->set($key, $value, $this->_timeout)) { CRM_Core_Error::debug('Result Code: ', $this->_cache->getResultMessage()); diff --git a/CRM/Utils/Cache/NoCache.php b/CRM/Utils/Cache/NoCache.php index a7adecbe73..119f646155 100644 --- a/CRM/Utils/Cache/NoCache.php +++ b/CRM/Utils/Cache/NoCache.php @@ -54,10 +54,11 @@ class CRM_Utils_Cache_NoCache implements CRM_Utils_Cache_Interface { /** * @param string $key * @param mixed $value + * @param null|int|\DateInterval $ttl * * @return bool */ - public function set($key, &$value) { + public function set($key, $value, $ttl = NULL) { return FALSE; } diff --git a/CRM/Utils/Cache/Redis.php b/CRM/Utils/Cache/Redis.php index 27fcb159a1..664a2f2cb5 100644 --- a/CRM/Utils/Cache/Redis.php +++ b/CRM/Utils/Cache/Redis.php @@ -113,11 +113,15 @@ class CRM_Utils_Cache_Redis implements CRM_Utils_Cache_Interface { /** * @param $key * @param $value + * @param null|int|\DateInterval $ttl * * @return bool * @throws Exception */ - public function set($key, &$value) { + public function set($key, $value, $ttl = NULL) { + if ($ttl !== NULL) { + throw new \RuntimeException("FIXME: " . __CLASS__ . "::set() should support non-NULL TTL"); + } if (!$this->_cache->set($this->_prefix . $key, serialize($value), $this->_timeout)) { if (PHP_SAPI === 'cli' || (Civi\Core\Container::isContainerBooted() && CRM_Core_Permission::check('view debug output'))) { CRM_Core_Error::fatal("Redis set ($key) failed: " . $this->_cache->getLastError()); diff --git a/CRM/Utils/Cache/SerializeCache.php b/CRM/Utils/Cache/SerializeCache.php index 7ee639bd60..5e21c31726 100644 --- a/CRM/Utils/Cache/SerializeCache.php +++ b/CRM/Utils/Cache/SerializeCache.php @@ -90,13 +90,19 @@ class CRM_Utils_Cache_SerializeCache implements CRM_Utils_Cache_Interface { /** * @param string $key * @param mixed $value + * @param null|int|\DateInterval $ttl + * @return bool */ - public function set($key, &$value) { + public function set($key, $value, $ttl = NULL) { + if ($ttl !== NULL) { + throw new \RuntimeException("FIXME: " . __CLASS__ . "::set() should support non-NULL TTL"); + } if (file_exists($this->fileName($key))) { - return; + return FALSE; // WTF, write-once cache?! } $this->_cache[$key] = $value; - file_put_contents($this->fileName($key), "fileName($key), "group, $key, $this->componentID); $this->frontCache[$key] = $value; + return TRUE; } /** -- 2.25.1