Merge pull request #12653 from nupplaphil/bug/config_revert_null

Revert Cache delete() behavior to stable version

src/Core/Config/Capability/IManageConfigValues.php

@@ -58,20 +58,6 @@ interface IManageConfigValues
 	 */
 	public function get(string $cat, string $key = null, $default_value = null);
 
-	/**
-	 * Load all configuration values from a given cache and saves it back in the configuration node store
-	 * @see	ConfigFileManager::CONFIG_DATA_FILE
-	 *
-	 * All configuration values of the system are stored in the cache.
-	 *
-	 * @param Cache $cache a new cache
-	 *
-	 * @return void
-	 *
-	 * @throws ConfigPersistenceException In case the persistence layer throws errors
-	 */
-	public function load(Cache $cache);
-
 	/**
 	 * Sets a configuration value for system config
 	 *

src/Core/Config/Model/Config.php

@@ -49,6 +49,24 @@ class Config implements IManageConfigValues
 		$this->configCache       = $configCache;
 	}
 
+	/**
+	 * Load all configuration values from a given cache and saves it back in the configuration node store
+	 * @see	ConfigFileManager::CONFIG_DATA_FILE
+	 *
+	 * All configuration values of the system are stored in the cache.
+	 *
+	 * @param Cache $cache a new cache
+	 *
+	 * @return void
+	 *
+	 * @throws ConfigPersistenceException In case the persistence layer throws errors
+	 */
+	public function setCacheAndSave(Cache $cache)
+	{
+		$this->configCache = $cache;
+		$this->save();
+	}
+
 	/**
 	 * {@inheritDoc}
 	 */
@@ -71,6 +89,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);
 		}
@@ -89,13 +109,6 @@ class Config implements IManageConfigValues
 		$this->configCache = $configCache;
 	}
 
-	/** {@inheritDoc} */
-	public function load(Cache $cache)
-	{
-		$this->configCache = $cache;
-		$this->save();
-	}
-
 	/** {@inheritDoc} */
 	public function get(string $cat, string $key = null, $default_value = null)
 	{
@@ -116,7 +129,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 {

src/Core/Config/Model/ConfigTransaction.php

@@ -57,7 +57,7 @@ class ConfigTransaction implements ISetConfigValuesTransactionally
 	/** {@inheritDoc} */
 	public function delete(string $cat, string $key): ISetConfigValuesTransactionally
 	{
-		$this->cache->delete($cat, $key, Cache::SOURCE_DATA);
+		$this->cache->delete($cat, $key);
 		$this->changedConfig = true;
 
 		return $this;
@@ -72,7 +72,7 @@ class ConfigTransaction implements ISetConfigValuesTransactionally
 		}
 
 		try {
-			$this->config->load($this->cache);
+			$this->config->setCacheAndSave($this->cache);
 			$this->cache = clone $this->config->getCache();
 		} catch (\Exception $e) {
 			throw new ConfigPersistenceException('Cannot save config', $e);

src/Core/Config/ValueObject/Cache.php

@@ -55,6 +55,11 @@ class Cache
 	 */
 	private $source = [];
 
+	/**
+	 * @var bool[][]
+	 */
+	private $delConfig = [];
+
 	/**
 	 * @var bool
 	 */
@@ -87,10 +92,11 @@ class Cache
 				$keys = array_keys($config[$category]);
 
 				foreach ($keys as $key) {
-					$this->set($category, $key, $config[$category][$key] ?? null, $source);
+					$value = $config[$category][$key];
+					if (isset($value)) {
+						$this->set($category, $key, $value, $source);
+					}
 				}
-			} else {
-				$this->set($category, null, $config[$category], $source);
 			}
 		}
 	}
@@ -149,8 +155,6 @@ class Cache
 						$data[$category][$key] = $this->config[$category][$key];
 					}
 				}
-			} elseif (is_int($this->source[$category])) {
-				$data[$category] = null;
 			}
 		}
 
@@ -160,49 +164,40 @@ class Cache
 	/**
 	 * Sets a value in the config cache. Accepts raw output from the config table
 	 *
-	 * @param string  $cat    Config category
+	 * @param string $cat    Config category
-	 * @param ?string $key    Config key
+	 * @param string $key    Config key
-	 * @param ?mixed  $value  Value to set
+	 * @param mixed  $value  Value to set
-	 * @param int     $source The source of the current config key
+	 * @param int    $source The source of the current config key
 	 *
 	 * @return bool True, if the value is set
 	 */
-	public function set(string $cat, string $key = null, $value = null, int $source = self::SOURCE_DEFAULT): bool
+	public function set(string $cat, string $key, $value, int $source = self::SOURCE_DEFAULT): bool
 	{
-		if (!isset($this->config[$cat]) && $key !== null) {
+		if (!isset($this->config[$cat])) {
 			$this->config[$cat] = [];
 			$this->source[$cat] = [];
 		}
 
-		if ((isset($this->source[$cat][$key]) && $source < $this->source[$cat][$key]) ||
+		if (isset($this->source[$cat][$key]) &&
-			(is_int($this->source[$cat] ?? null) && $source < $this->source[$cat])) {
+			$source < $this->source[$cat][$key]) {
 			return false;
 		}
 
 		if ($this->hidePasswordOutput &&
 			$key == 'password' &&
 			is_string($value)) {
-			$this->setCatKeyValueSource($cat, $key, new HiddenString($value), $source);
+			$this->config[$cat][$key] = new HiddenString((string)$value);
 		} else if (is_string($value)) {
-			$this->setCatKeyValueSource($cat, $key, self::toConfigValue($value), $source);
+			$this->config[$cat][$key] = self::toConfigValue($value);
 		} else {
-			$this->setCatKeyValueSource($cat, $key, $value, $source);
+			$this->config[$cat][$key] = $value;
 		}
 
+		$this->source[$cat][$key] = $source;
+
 		return true;
 	}
 
-	private function setCatKeyValueSource(string $cat, string $key = null, $value = null, int $source = self::SOURCE_DEFAULT)
-	{
-		if (isset($key)) {
-			$this->config[$cat][$key] = $value;
-			$this->source[$cat][$key] = $source;
-		} else {
-			$this->config[$cat] = $value;
-			$this->source[$cat] = $source;
-		}
-	}
-
 	/**
 	 * Formats a DB value to a config value
 	 * - null   = The db-value isn't set
@@ -214,7 +209,7 @@ class Cache
 	 *
 	 * @param string|null $value
 	 *
-	 * @return null|array|string
+	 * @return mixed
 	 */
 	public static function toConfigValue(?string $value)
 	{
@@ -232,27 +227,25 @@ class Cache
 	/**
 	 * Deletes a value from the config cache.
 	 *
-	 * @param string  $cat Config category
+	 * @param string $cat Config category
-	 * @param ?string $key Config key (if not set, the whole category will get deleted)
+	 * @param string $key Config key
-	 * @param int     $source The source of the current config key
 	 *
 	 * @return bool true, if deleted
 	 */
-	public function delete(string $cat, string $key = null, int $source = self::SOURCE_DEFAULT): bool
+	public function delete(string $cat, string $key): bool
 	{
 		if (isset($this->config[$cat][$key])) {
-			$this->config[$cat][$key] = null;
+			unset($this->config[$cat][$key]);
-			$this->source[$cat][$key] = $source;
+			unset($this->source[$cat][$key]);
-			if (empty(array_filter($this->config[$cat], function($value) { return !is_null($value); }))) {
+			$this->delConfig[$cat][$key] = true;
-				$this->config[$cat] = null;
+			if (count($this->config[$cat]) == 0) {
-				$this->source[$cat] = $source;
+				unset($this->config[$cat]);
+				unset($this->source[$cat]);
 			}
-		} elseif (isset($this->config[$cat])) {
+			return true;
-			$this->config[$cat] = null;
+		} else {
-			$this->source[$cat] = $source;
+			return false;
 		}
-
-		return true;
 	}
 
 	/**
@@ -283,7 +276,7 @@ class Cache
 				$keys = array_keys($config[$category]);
 
 				foreach ($keys as $key) {
-					if (!key_exists($key, $this->config[$category] ?? [])) {
+					if (!isset($this->config[$category][$key])) {
 						$return[$category][$key] = $config[$category][$key];
 					}
 				}
@@ -326,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;

tests/src/Core/Config/Cache/CacheTest.php

@@ -40,7 +40,6 @@ class CacheTest extends MockedTest
 						'int' => 235,
 						'dec' => 2.456,
 						'array' => ['1', 2, '3', true, false],
-						'null' => null,
 					],
 					'config' => [
 						'a' => 'value',
@@ -124,7 +123,7 @@ class CacheTest extends MockedTest
 
 		// wrong dataset
 		$configCache->load(['system' => 'not_array']);
-		self::assertEquals(['system' => 'not_array'], $configCache->getAll());
+		self::assertEquals([], $configCache->getAll());
 
 		// incomplete dataset (key is integer ID of the array)
 		$configCache = new Cache();
@@ -209,16 +208,13 @@ class CacheTest extends MockedTest
 	{
 		$configCache = new Cache($data);
 
-		$assertion = [];
-
 		foreach ($data as $cat => $values) {
-			$assertion[$cat] = null;
 			foreach ($values as $key => $value) {
 				$configCache->delete($cat, $key);
 			}
 		}
 
-		self::assertEquals($assertion, $configCache->getAll());
+		self::assertEmpty($configCache->getAll());
 	}
 
 	/**
@@ -554,4 +550,51 @@ class CacheTest extends MockedTest
 
 		self::assertEquals('new_value', $newCache->get($category, 'new_key'));
 	}
+
+	/**
+	 * Test that keys are removed after a deletion
+	 *
+	 * @dataProvider dataTests
+	 *
+	 */
+	public function testDeleteRemovesKey($data)
+	{
+		$cache = new Cache();
+		$cache->load($data, Cache::SOURCE_FILE);
+
+		$cache->set('system', 'test', 'overwrite!', Cache::SOURCE_DATA);
+		self::assertEquals('overwrite!', $cache->get('system', 'test'));
+
+		// array should now be removed
+		$cache->delete('system', 'test');
+		self::assertArrayNotHasKey('test', $cache->getAll()['system']);
+
+		self::assertArrayHasKey('config', $cache->getAll());
+		self::assertArrayHasKey('a', $cache->getAll()['config']);
+
+		// category should now be removed
+		$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'));
+	}
 }

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'));
+		self::assertEquals('UTC', $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'));
 	}
 
 	/**

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'));
+	}
 }