diff --git a/lib/private/Config.php b/lib/private/Config.php index ecfb9fc3160..ddcc6e1179b 100644 --- a/lib/private/Config.php +++ b/lib/private/Config.php @@ -35,7 +35,8 @@ class Config { */ "; - protected array $cache = []; + protected array $mergedCache = []; + protected array $cacheByPath = []; protected array $envCache = []; protected string $configFilePath; protected bool $isReadOnly; @@ -61,7 +62,7 @@ class Config { * @return array an array of key names */ public function getKeys(): array { - return array_merge(array_keys($this->cache), array_keys($this->envCache)); + return array_merge(array_keys($this->mergedCache), array_keys($this->envCache)); } /** @@ -80,8 +81,8 @@ class Config { return self::trustSystemConfig($this->envCache[$key]); } - if (isset($this->cache[$key])) { - return self::trustSystemConfig($this->cache[$key]); + if (isset($this->mergedCache[$key])) { + return self::trustSystemConfig($this->mergedCache[$key]); } return $default; @@ -153,9 +154,10 @@ class Config { * @throws HintException */ protected function set($key, $value) { - if (!isset($this->cache[$key]) || $this->cache[$key] !== $value) { + if (!isset($this->mergedCache[$key]) || $this->mergedCache[$key] !== $value) { // Add change - $this->cache[$key] = $value; + $this->mergedCache[$key] = $value; + $this->cacheByPath[$this->configFilePath][$key] = $value; return true; } @@ -183,9 +185,9 @@ class Config { * @throws HintException */ protected function delete($key) { - if (isset($this->cache[$key])) { + if (isset($this->mergedCache[$key])) { // Delete key from cache - unset($this->cache[$key]); + unset($this->mergedCache[$key], $this->cacheByPath[$this->configFilePath][$key]); return true; } return false; @@ -254,7 +256,8 @@ class Config { throw new \Exception($errorMessage); } if (isset($CONFIG) && is_array($CONFIG)) { - $this->cache = array_replace_recursive($this->cache, $CONFIG); + $this->mergedCache = array_replace_recursive($this->mergedCache, $CONFIG); + $this->cacheByPath[$file] = $CONFIG; } } @@ -281,22 +284,25 @@ class Config { private function writeData(): void { $this->checkReadOnly(); - if (!is_file(\OC::$configDir . '/CAN_INSTALL') && !isset($this->cache['version'])) { + if (!is_file(\OC::$configDir . '/CAN_INSTALL') && !isset($this->mergedCache['version'])) { throw new HintException(sprintf('Configuration was not read or initialized correctly, not overwriting %s', $this->configFilePath)); } + // Only save the values to config.php that originally came from it or were added in the current process. + $values = $this->cacheByPath[$this->configFilePath]; + // Create a php file ... $content = "cache), true); + $content .= var_export(self::trustSystemConfig($values), true); $content .= ";\n"; touch($this->configFilePath); $filePointer = fopen($this->configFilePath, 'r+'); // Apply permissions for config.php, defaulting to user read-write and group read - $permissions = $this->cache['configfilemode'] ?? 0640; + $permissions = $this->mergedCache['configfilemode'] ?? 0640; chmod($this->configFilePath, $permissions); // File does not exist, this can happen when doing a fresh install diff --git a/tests/lib/ConfigTest.php b/tests/lib/ConfigTest.php index c0a6e4bb53b..8c76ee41802 100644 --- a/tests/lib/ConfigTest.php +++ b/tests/lib/ConfigTest.php @@ -13,7 +13,7 @@ use OCP\ITempManager; use OCP\Server; class ConfigTest extends TestCase { - public const TESTCONTENT = '"bar", "beers" => array("Appenzeller", "Guinness", "Kölsch"), "alcohol_free" => false);'; + public const TESTCONTENT = '"bar", "beers" => array("Appenzeller", "Guinness", "Kölsch"), "alcohol_free" => false, "top" => ["bottom1" => "value1"]);'; /** @var array */ private $initialConfig = ['foo' => 'bar', 'beers' => ['Appenzeller', 'Guinness', 'Kölsch'], 'alcohol_free' => false]; @@ -42,12 +42,12 @@ class ConfigTest extends TestCase { } public function testGetKeys(): void { - $expectedConfig = ['foo', 'beers', 'alcohol_free']; + $expectedConfig = ['foo', 'beers', 'alcohol_free', 'top']; $this->assertSame($expectedConfig, $this->getConfig()->getKeys()); } public function testGetKeysReturnsEnvironmentKeysIfSet() { - $expectedConfig = ['foo', 'beers', 'alcohol_free', 'taste']; + $expectedConfig = ['foo', 'beers', 'alcohol_free', 'top', 'taste']; putenv('NC_taste=great'); $this->assertSame($expectedConfig, $this->getConfig()->getKeys()); putenv('NC_taste'); @@ -102,7 +102,7 @@ class ConfigTest extends TestCase { $expected = " 'moo',\n 'beers' => \n array (\n 0 => 'Appenzeller',\n " - . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n);\n"; + . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n 'top' => \n array (\n 'bottom1' => 'value1',\n ),\n);\n"; $this->assertEquals($expected, $content); $config->setValue('bar', 'red'); @@ -115,7 +115,7 @@ class ConfigTest extends TestCase { $expected = " 'moo',\n 'beers' => \n array (\n 0 => 'Appenzeller',\n " - . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n 'bar' => 'red',\n 'apps' => \n " + . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n 'top' => \n array (\n 'bottom1' => 'value1',\n ),\n 'bar' => 'red',\n 'apps' => \n " . " array (\n 0 => 'files',\n 1 => 'gallery',\n ),\n);\n"; $this->assertEquals($expected, $content); } @@ -148,7 +148,7 @@ class ConfigTest extends TestCase { $expected = " 'moo',\n 'beers' => \n array (\n 0 => 'Appenzeller',\n " - . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n);\n"; + . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'top' => \n array (\n 'bottom1' => 'value1',\n ),\n);\n"; $this->assertEquals($expected, $content); } @@ -161,13 +161,13 @@ class ConfigTest extends TestCase { $expected = " \n array (\n 0 => 'Appenzeller',\n " - . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n);\n"; + . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n 'top' => \n array (\n 'bottom1' => 'value1',\n ),\n);\n"; $this->assertEquals($expected, $content); } public function testConfigMerge(): void { // Create additional config - $additionalConfig = '"totallyOutdated");'; + $additionalConfig = '"totallyOutdated","alcohol_free"=>true);'; $additionalConfigPath = $this->randomTmpDir . 'additionalConfig.testconfig.php'; file_put_contents($additionalConfigPath, $additionalConfig); @@ -178,16 +178,75 @@ class ConfigTest extends TestCase { $this->assertSame('totallyOutdated', $config->getValue('php53', 'bogusValue')); $this->assertEquals(self::TESTCONTENT, file_get_contents($this->configFile)); + // Value is overwritten by additional config file, but not saved to the main config file. + $this->assertTrue($config->getValue('alcohol_free')); + + // Set a new key that's saved to the main config file + $config->setValue('new_key', 'new_value'); + // Write a new value to the config $config->setValue('CoolWebsites', ['demo.owncloud.org', 'owncloud.org', 'owncloud.com']); $expected = " 'bar',\n 'beers' => \n array (\n 0 => 'Appenzeller',\n " - . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n 'php53' => 'totallyOutdated',\n 'CoolWebsites' => \n array (\n " + . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n 'top' => \n array (\n 'bottom1' => 'value1',\n ),\n 'new_key' => 'new_value',\n 'CoolWebsites' => \n array (\n " . " 0 => 'demo.owncloud.org',\n 1 => 'owncloud.org',\n 2 => 'owncloud.com',\n ),\n);\n"; $this->assertEquals($expected, file_get_contents($this->configFile)); // Cleanup unlink($additionalConfigPath); } + + public function testConfigMergeRecursive(): void { + $additionalConfig = ' ["bottom2" => "value2"]];'; + $additionalConfigPath = $this->randomTmpDir . 'additionalConfig.testconfig.php'; + file_put_contents($additionalConfigPath, $additionalConfig); + + $config = new Config($this->randomTmpDir, 'testconfig.php'); + + // Values are merged correctly from the additional config file + $this->assertEquals(['bottom1' => 'value1', 'bottom2' => 'value2'], $config->getValue('top')); + + self::invokePrivate($config, 'writeData'); + + // Values from the additional config file are not saved to the main config file + $expected = " 'bar',\n 'beers' => \n array (\n 0 => 'Appenzeller',\n " + . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n 'top' => \n array (\n 'bottom1' => 'value1',\n ),\n);\n"; + $this->assertEquals($expected, file_get_contents($this->configFile)); + + unlink($additionalConfigPath); + } + + /** + * The current behavior of additional config files is broken. + * Setting or deleting a key in the current process will remove it from the main config file, + * but if the key is specified in an additional config file it will just be overwritten again. + */ + public function testConfigAdditionalSetDelete(): void { + $additionalConfig = ' "value1", "key2" => "value2"];'; + $additionalConfigPath = $this->randomTmpDir . 'additionalConfig.testconfig.php'; + file_put_contents($additionalConfigPath, $additionalConfig); + + $config = new Config($this->randomTmpDir, 'testconfig.php'); + + $config->setValue('key1', 'value3'); + $config->deleteKey('key2'); + + // The updated config is written to the main config file + $expected = " 'bar',\n 'beers' => \n array (\n 0 => 'Appenzeller',\n " + . " 1 => 'Guinness',\n 2 => 'Kölsch',\n ),\n 'alcohol_free' => false,\n 'top' => \n array (\n 'bottom1' => 'value1',\n ),\n 'key1' => 'value3',\n);\n"; + $this->assertEquals($expected, file_get_contents($this->configFile)); + + $config = new Config($this->randomTmpDir, 'testconfig.php'); + + // The additional config file overwrites the values again + $this->assertEquals('value1', $config->getValue('key1')); + $this->assertEquals('value2', $config->getValue('key2')); + + unlink($additionalConfigPath); + } }