From 11a8bd17e3e6a66d1df6ffee22355c9f95398b2b Mon Sep 17 00:00:00 2001 From: Philipp Date: Wed, 11 Jan 2023 21:53:34 +0100 Subject: [PATCH] Assure that deleted cat/keys are working as expected - A deleted cache-key would delete a merged cache-key as well - Deleting a key in the Model results in reloading the config to assure any value from underlying files --- src/Core/Config/Model/Config.php | 4 +- src/Core/Config/ValueObject/Cache.php | 19 ++++++++ tests/src/Core/Config/Cache/CacheTest.php | 21 +++++++++ .../Config/Cache/ConfigFileManagerTest.php | 43 ++----------------- tests/src/Core/Config/ConfigTest.php | 20 ++++++++- 5 files changed, 65 insertions(+), 42 deletions(-) diff --git a/src/Core/Config/Model/Config.php b/src/Core/Config/Model/Config.php index c96750694..5258033a7 100644 --- a/src/Core/Config/Model/Config.php +++ b/src/Core/Config/Model/Config.php @@ -71,6 +71,8 @@ class Config implements IManageConfigValues { try { $this->configFileManager->saveData($this->configCache); + // reload after the save to possible reload default values of lower source-priorities again + $this->reload(); } catch (ConfigFileException $e) { throw new ConfigPersistenceException('Cannot save config', $e); } @@ -116,7 +118,7 @@ class Config implements IManageConfigValues /** {@inheritDoc} */ public function delete(string $cat, string $key): bool { - if ($this->configCache->delete($cat, $key, Cache::SOURCE_DATA)) { + if ($this->configCache->delete($cat, $key)) { $this->save(); return true; } else { diff --git a/src/Core/Config/ValueObject/Cache.php b/src/Core/Config/ValueObject/Cache.php index db972431a..be42ae118 100644 --- a/src/Core/Config/ValueObject/Cache.php +++ b/src/Core/Config/ValueObject/Cache.php @@ -55,6 +55,11 @@ class Cache */ private $source = []; + /** + * @var array + */ + private $delConfig = []; + /** * @var bool */ @@ -232,6 +237,7 @@ class Cache if (isset($this->config[$cat][$key])) { unset($this->config[$cat][$key]); unset($this->source[$cat][$key]); + $this->delConfig[$cat][$key] = true; if (count($this->config[$cat]) == 0) { unset($this->config[$cat]); unset($this->source[$cat]); @@ -313,6 +319,19 @@ class Cache } } + $delCategories = array_keys($cache->delConfig); + + foreach ($delCategories as $category) { + if (is_array($cache->delConfig[$category])) { + $keys = array_keys($cache->delConfig[$category]); + + foreach ($keys as $key) { + unset($newConfig[$category][$key]); + unset($newSource[$category][$key]); + } + } + } + $newCache = new Cache(); $newCache->config = $newConfig; $newCache->source = $newSource; diff --git a/tests/src/Core/Config/Cache/CacheTest.php b/tests/src/Core/Config/Cache/CacheTest.php index a4e12e23a..c4e536871 100644 --- a/tests/src/Core/Config/Cache/CacheTest.php +++ b/tests/src/Core/Config/Cache/CacheTest.php @@ -576,4 +576,25 @@ class CacheTest extends MockedTest $cache->delete('config', 'a'); self::assertArrayNotHasKey('config', $cache->getAll()); } + + /** + * Test that deleted keys are working with merge + * + * @dataProvider dataTests + */ + public function testDeleteAndMergeWithDefault($data) + { + $cache = new Cache(); + $cache->load($data, Cache::SOURCE_FILE); + + $cache2 = new Cache(); + $cache2->set('system', 'test', 'overrride'); + $cache2->delete('system', 'test'); + + self::assertEquals('it', $cache->get('system', 'test')); + self::assertNull($cache2->get('system', 'test')); + + $mergedCache = $cache->merge($cache2); + self::assertNull($mergedCache->get('system', 'test')); + } } diff --git a/tests/src/Core/Config/Cache/ConfigFileManagerTest.php b/tests/src/Core/Config/Cache/ConfigFileManagerTest.php index 6c2c41ef6..e5b630a20 100644 --- a/tests/src/Core/Config/Cache/ConfigFileManagerTest.php +++ b/tests/src/Core/Config/Cache/ConfigFileManagerTest.php @@ -443,7 +443,7 @@ class ConfigFileManagerTest extends MockedTest } /** - * If we delete something with the Cache::delete() functionality, be sure to override the underlying source as well + * If we delete something with the Cache::delete() functionality, be sure to probably reset it to the underlying key */ public function testDeleteKeyOverwrite() { @@ -465,51 +465,16 @@ class ConfigFileManagerTest extends MockedTest $configFileManager->setupCache($configCache); - $configCache->delete('system', 'default_timezone', Cache::SOURCE_DATA); + $configCache->delete('system', 'default_timezone'); $configFileManager->saveData($configCache); - // assert that system.default_timezone is now null, even it's set with settings.conf.php + // assert that system.default_timezone is now the restored 'UTC' from the defaults $configCache = new Cache(); $configFileManager->setupCache($configCache); - self::assertNull($configCache->get('system', 'default_timezone')); - } - - /** - * If we delete something with the Cache::delete() functionality, be sure to override the underlying source as well - */ - public function testDeleteCategoryOverwrite() - { - $this->delConfigFile('node.config.php'); - - $fileDir = dirname(__DIR__) . DIRECTORY_SEPARATOR . - '..' . DIRECTORY_SEPARATOR . - '..' . DIRECTORY_SEPARATOR . - '..' . DIRECTORY_SEPARATOR . - 'datasets' . DIRECTORY_SEPARATOR . - 'config' . DIRECTORY_SEPARATOR; - - vfsStream::newFile('B.config.php') - ->at($this->root->getChild('config')) - ->setContent(file_get_contents($fileDir . 'B.config.php')); - - $configFileManager = (new Config())->createConfigFileManager($this->root->url()); - $configCache = new Cache(); - - $configFileManager->setupCache($configCache); - - $configCache->delete('system'); - - $configFileManager->saveData($configCache); - - // assert that system.default_timezone is now null, even it's set with settings.conf.php - $configCache = new Cache(); - - $configFileManager->setupCache($configCache); - - self::assertNull($configCache->get('system', 'default_timezone')); + self::assertEquals('UTC', $configCache->get('system', 'default_timezone')); } /** diff --git a/tests/src/Core/Config/ConfigTest.php b/tests/src/Core/Config/ConfigTest.php index f7d7c070d..f24404474 100644 --- a/tests/src/Core/Config/ConfigTest.php +++ b/tests/src/Core/Config/ConfigTest.php @@ -363,8 +363,6 @@ class ConfigTest extends MockedTest self::assertTrue($this->testedConfig->delete('test', 'it')); self::assertNull($this->testedConfig->get('test', 'it')); self::assertNull($this->testedConfig->getCache()->get('test', 'it')); - - self::assertEquals(['test' => null], $this->testedConfig->getCache()->getAll()); } /** @@ -532,4 +530,22 @@ class ConfigTest extends MockedTest self::assertEquals($assertion, $config->get($category)); } + + /** + * Tests, if an overwritten value of an existing key will reset to it's default after deletion + */ + public function testDeleteReturnsDefault() + { + vfsStream::newFile(ConfigFileManager::CONFIG_DATA_FILE) + ->at($this->root->getChild('config')) + ->setContent(ConfigFileTransformer::encode([ + 'config' => ['sitename' => 'overritten'], + ])); + + $config = $this->getInstance(); + self::assertEquals('overritten', $config->get('config', 'sitename')); + + $config->delete('config', 'sitename'); + self::assertEquals('Friendica Social Network', $config->get('config', 'sitename')); + } }