fix(cache): separated cache namespaces should not interfere

Elgg · Jul 28, 2022 · 0a2e40c · 0a2e40c
1 parent 7a4f730
commit 0a2e40c
Show file tree

Hide file tree

Showing 9 changed files with 70 additions and 33 deletions.
diff --git a/engine/classes/Elgg/Cache/CompositeCache.php b/engine/classes/Elgg/Cache/CompositeCache.php
@@ -297,7 +297,7 @@ protected function buildRedisDriver() {
 			return null;
 		}
 
-		$config = \Elgg\Cache\Config\Redis::fromElggConfig($this->config);
+		$config = \Elgg\Cache\Config\Redis::fromElggConfig($this->namespace, $this->config);
 		if (empty($config)) {
 			return null;
 		}
@@ -318,22 +318,12 @@ protected function buildMemcachedDriver() {
 			return null;
 		}
 
-		$config = \Elgg\Cache\Config\Memcached::fromElggConfig($this->config);
+		$config = \Elgg\Cache\Config\Memcached::fromElggConfig($this->namespace, $this->config);
 		if (empty($config)) {
 			return null;
 		}
-
-		$driver = class_exists('Memcached') ? 'Memcached' : 'Memcache';
-
-		if (!empty($this->config->memcache_namespace_prefix)) {
-			if ($driver !== 'Memcached') {
-				elgg_log('The setting memcache_namespace_prefix is only working if you use Memcached (not Memcache)', \Psr\Log\LogLevel::WARNING);
-			} else {
-				$config->setOptPrefix($this->config->memcache_namespace_prefix);
-			}
-		}
 
-		return CacheManager::getInstance($driver, $config, $this->prefixInstanceId('memcache'));
+		return CacheManager::getInstance('Memcached', $config, $this->prefixInstanceId('memcache'));
 	}
 
 	/**
@@ -345,7 +335,7 @@ protected function buildFileSystemDriver() {
 			return null;
 		}
 
-		$config = \Elgg\Cache\Config\Files::fromElggConfig($this->config);
+		$config = \Elgg\Cache\Config\Files::fromElggConfig($this->namespace, $this->config);
 		if (empty($config)) {
 			return null;
 		}
@@ -370,7 +360,7 @@ protected function buildLocalFileSystemDriver() {
 			return null;
 		}
 
-		$config = \Elgg\Cache\Config\LocalFiles::fromElggConfig($this->config);
+		$config = \Elgg\Cache\Config\LocalFiles::fromElggConfig($this->namespace, $this->config);
 		if (empty($config)) {
 			return null;
 		}
@@ -425,7 +415,7 @@ protected function buildBlackHoleDriver() {
 	 * @since 4.2
 	 */
 	public static function isMemcacheAvailable(): bool {
-		return class_exists('Memcached') || class_exists('Memcache');
+		return class_exists('Memcached');
 	}
 
 	/**

diff --git a/engine/classes/Elgg/Cache/Config/Files.php b/engine/classes/Elgg/Cache/Config/Files.php
@@ -15,11 +15,12 @@ class Files extends Config {
 	/**
 	 * Factory to return a config object to be used when starting a driver
 	 *
-	 * @param \Elgg\Config $config Elgg configuration
+	 * @param string       $namespace cache namespace
+	 * @param \Elgg\Config $config    Elgg configuration
 	 *
 	 * @return self|NULL
 	 */
-	public static function fromElggConfig(\Elgg\Config $config): ?self {
+	public static function fromElggConfig(string $namespace, \Elgg\Config $config): ?self {
 
 		$path = $config->cacheroot ?: $config->dataroot;
 		if (!$path) {
@@ -32,7 +33,7 @@ public static function fromElggConfig(\Elgg\Config $config): ?self {
 			'preventCacheSlams' => true,
 			'useStaticItemCaching' => true,
 			'itemDetailedDate' => true,
-			'securityKey' => 'elgg', // to make sure cli and webserver use the same folder
+			'securityKey' => $namespace, // to make sure cli and webserver use the same folder and to separate caches
 		]);
 	}
 }
diff --git a/engine/classes/Elgg/Cache/Config/LocalFiles.php b/engine/classes/Elgg/Cache/Config/LocalFiles.php
@@ -15,11 +15,12 @@ class LocalFiles extends Config {
 	/**
 	 * Factory to return a config object to be used when starting a driver
 	 *
-	 * @param \Elgg\Config $config Elgg configuration
+	 * @param string       $namespace cache namespace
+	 * @param \Elgg\Config $config    Elgg configuration
 	 *
 	 * @return self|NULL
 	 */
-	public static function fromElggConfig(\Elgg\Config $config): ?self {
+	public static function fromElggConfig(string $namespace, \Elgg\Config $config): ?self {
 
 		$path = $config->localcacheroot ?: ($config->cacheroot ?: $config->dataroot);
 		if (!$path) {
@@ -32,7 +33,7 @@ public static function fromElggConfig(\Elgg\Config $config): ?self {
 			'secureFileManipulation' => true,
 			'useStaticItemCaching' => true,
 			'itemDetailedDate' => true,
-			'securityKey' => 'elgg', // to make sure cli and webserver use the same folder
+			'securityKey' => $namespace, // to make sure cli and webserver use the same folder and to separate caches
 		]);
 	}
 }
diff --git a/engine/classes/Elgg/Cache/Config/Memcached.php b/engine/classes/Elgg/Cache/Config/Memcached.php
@@ -15,11 +15,12 @@ class Memcached extends Config {
 	/**
 	 * Factory to return a config object to be used when starting a driver
 	 *
-	 * @param \Elgg\Config $config Elgg configuration
+	 * @param string       $namespace cache namespace
+	 * @param \Elgg\Config $config    Elgg configuration
 	 *
 	 * @return self|NULL
 	 */
-	public static function fromElggConfig(\Elgg\Config $config): ?self {
+	public static function fromElggConfig(string $namespace, \Elgg\Config $config): ?self {
 
 		if (!$config->memcache || empty($config->memcache_servers)) {
 			return null;
@@ -48,11 +49,15 @@ public static function fromElggConfig(\Elgg\Config $config): ?self {
 			$servers[] = $server_config;
 		}
 
+		$opt_prefix = (string) $config->memcache_namespace_prefix;
+		$opt_prefix .= $namespace;
+
 		return new self([
 			'servers' => $servers,
 			'preventCacheSlams' => true,
 			'useStaticItemCaching' => true,
 			'itemDetailedDate' => true,
+			'optPrefix' => $opt_prefix,
 		]);
 	}
 }
diff --git a/engine/classes/Elgg/Cache/Config/Redis.php b/engine/classes/Elgg/Cache/Config/Redis.php
@@ -15,11 +15,12 @@ class Redis extends Config {
 	/**
 	 * Factory to return a config object to be used when starting a driver
 	 *
-	 * @param \Elgg\Config $config Elgg configuration
+	 * @param string       $namespace cache namespace
+	 * @param \Elgg\Config $config    Elgg configuration
 	 *
 	 * @return self|NULL
 	 */
-	public static function fromElggConfig(\Elgg\Config $config): ?self {
+	public static function fromElggConfig(string $namespace, \Elgg\Config $config): ?self {
 
 		if (!$config->redis || empty($config->redis_servers) || !is_array($config->redis_servers)) {
 			return null;
@@ -29,6 +30,7 @@ public static function fromElggConfig(\Elgg\Config $config): ?self {
 			'preventCacheSlams' => true,
 			'useStaticItemCaching' => true,
 			'itemDetailedDate' => true,
+			'optPrefix' => $namespace,
 		];
 		if (!empty($config->redis_options) && is_array($config->redis_options)) {
 			$options = $config->redis_options;

diff --git a/engine/tests/phpunit/unit/Elgg/Cache/ElggCacheTestCase.php b/engine/tests/phpunit/unit/Elgg/Cache/ElggCacheTestCase.php
@@ -27,10 +27,12 @@ public function up() {
 	}
 
 	/**
+	 * @param string $namespace
+	 *
 	 * @return CompositeCache
 	 * @throws ConfigurationException
 	 */
-	abstract function createCache();
+	abstract function createCache(string $namespace);
 
 	public function cacheableValuesProvider() {
 		return [
@@ -148,4 +150,40 @@ public function testCanSetVariable() {
 
 		$this->cache->setVariable('foo', null);
 	}
+
+	public function testCanSeparateCachesWithNamespaces() {
+		$cache1 = $this->createCache('cache_namespace1');
+		$cache2 = $this->createCache('cache_namespace2');
+
+		// repeating tests need to start with empty cache
+		$cache1->clear();
+		$cache2->clear();
+
+		$cache1->save('foo1', 'bar');
+		$cache2->save('foo2', 'bar');
+
+		$this->assertEquals('bar', $cache1->load('foo1'));
+		$this->assertEquals('bar', $cache2->load('foo2'));
+
+		// check if values are written to the correct namespaced cache
+		$this->assertNull($cache1->load('foo2'));
+		$this->assertNull($cache2->load('foo1'));
+
+		// redis/memcache do not support flushing namespaces
+		if (static::class !== 'Elgg\Cache\PersistentCacheTest') {
+			// check if clearing namespace 1 does not clear namespace 2
+			$cache1->clear();
+			$this->assertNull($cache1->load('foo1'));
+
+			$reflector = new \ReflectionClass($cache2);
+			$property = $reflector->getProperty('pool');
+			$property->setAccessible(true);
+
+			$pool = $property->getValue($cache2);
+
+			// need to detach to make sure the item is loaded from the backend and does not use static cache
+			$pool->detachAllItems();
+			$this->assertEquals('bar', $cache2->load('foo2'));
+		}
+	}
 }
diff --git a/engine/tests/phpunit/unit/Elgg/Cache/FileSystemCacheTest.php b/engine/tests/phpunit/unit/Elgg/Cache/FileSystemCacheTest.php
@@ -7,8 +7,8 @@
  */
 class FileSystemCacheTest extends ElggCacheTestCase {
 
-	function createCache() {
-		return new CompositeCache('filesystem_test', _elgg_services()->config, ELGG_CACHE_FILESYSTEM);
+	function createCache(string $namespace = 'filesystem_test') {
+		return new CompositeCache($namespace, _elgg_services()->config, ELGG_CACHE_FILESYSTEM);
 	}
 
 }
diff --git a/engine/tests/phpunit/unit/Elgg/Cache/PersistentCacheTest.php b/engine/tests/phpunit/unit/Elgg/Cache/PersistentCacheTest.php
@@ -8,8 +8,8 @@
  */
 class PersistentCacheTest extends ElggCacheTestCase {
 
-	function createCache() {
-		return new CompositeCache('persistent_test', _elgg_services()->config, ELGG_CACHE_PERSISTENT);
+	function createCache(string $namespace = 'persistent_test') {
+		return new CompositeCache($namespace, _elgg_services()->config, ELGG_CACHE_PERSISTENT);
 	}
 
 	public function allowSkip(): bool {

diff --git a/engine/tests/phpunit/unit/Elgg/Cache/RuntimeCacheTest.php b/engine/tests/phpunit/unit/Elgg/Cache/RuntimeCacheTest.php
@@ -7,8 +7,8 @@
  */
 class RuntimeCacheTest extends ElggCacheTestCase {
 
-	function createCache() {
-		return new CompositeCache('test', _elgg_services()->config, ELGG_CACHE_RUNTIME);
+	function createCache(string $namespace = 'runtime_test') {
+		return new CompositeCache($namespace, _elgg_services()->config, ELGG_CACHE_RUNTIME);
 	}
 
 }