diff --git a/bin/daemon.php b/bin/daemon.php index f9ed693f3..577e884eb 100755 --- a/bin/daemon.php +++ b/bin/daemon.php @@ -126,7 +126,7 @@ if ($mode == 'status') { unlink($pidfile); - DI::config()->set('system', 'worker_daemon_mode', false); + DI::keyValue()->set('worker_daemon_mode', false); die("Daemon process $pid isn't running.\n"); } diff --git a/src/App/BaseURL.php b/src/App/BaseURL.php index 20fd54916..ab3d03a5b 100644 --- a/src/App/BaseURL.php +++ b/src/App/BaseURL.php @@ -172,53 +172,32 @@ class BaseURL */ public function save($hostname = null, $sslPolicy = null, $urlPath = null): bool { - $currHostname = $this->hostname; - $currSSLPolicy = $this->sslPolicy; - $currURLPath = $this->urlPath; + $currUrl = $this->url; + + $configTransaction = $this->config->beginTransaction(); if (!empty($hostname) && $hostname !== $this->hostname) { - if ($this->config->set('config', 'hostname', $hostname)) { - $this->hostname = $hostname; - } else { - return false; - } + $configTransaction->set('config', 'hostname', $hostname); + $this->hostname = $hostname; } if (isset($sslPolicy) && $sslPolicy !== $this->sslPolicy) { - if ($this->config->set('system', 'ssl_policy', $sslPolicy)) { - $this->sslPolicy = $sslPolicy; - } else { - $this->hostname = $currHostname; - $this->config->set('config', 'hostname', $this->hostname); - return false; - } + $configTransaction->set('system', 'ssl_policy', $sslPolicy); + $this->sslPolicy = $sslPolicy; } if (isset($urlPath) && $urlPath !== $this->urlPath) { - if ($this->config->set('system', 'urlpath', $urlPath)) { - $this->urlPath = $urlPath; - } else { - $this->hostname = $currHostname; - $this->sslPolicy = $currSSLPolicy; - $this->config->set('config', 'hostname', $this->hostname); - $this->config->set('system', 'ssl_policy', $this->sslPolicy); - return false; - } + $configTransaction->set('system', 'urlpath', $urlPath); + $this->urlPath = $urlPath; } $this->determineBaseUrl(); - if (!$this->config->set('system', 'url', $this->url)) { - $this->hostname = $currHostname; - $this->sslPolicy = $currSSLPolicy; - $this->urlPath = $currURLPath; - $this->determineBaseUrl(); - - $this->config->set('config', 'hostname', $this->hostname); - $this->config->set('system', 'ssl_policy', $this->sslPolicy); - $this->config->set('system', 'urlpath', $this->urlPath); - return false; + if ($this->url !== $currUrl) { + $configTransaction->set('system', 'url', $this->url); } + $configTransaction->commit(); + return true; } diff --git a/src/Core/Config/Capability/ISetConfigValuesTransactionally.php b/src/Core/Config/Capability/ISetConfigValuesTransactionally.php index 501e24f73..a9fe36b02 100644 --- a/src/Core/Config/Capability/ISetConfigValuesTransactionally.php +++ b/src/Core/Config/Capability/ISetConfigValuesTransactionally.php @@ -41,8 +41,6 @@ interface ISetConfigValuesTransactionally * @param mixed $value The value to store * * @return static the current instance - * - * @throws ConfigPersistenceException In case the persistence layer throws errors */ public function set(string $cat, string $key, $value): self; @@ -54,8 +52,6 @@ interface ISetConfigValuesTransactionally * * @return static the current instance * - * @throws ConfigPersistenceException In case the persistence layer throws errors - * */ public function delete(string $cat, string $key): self; diff --git a/src/Core/Config/Model/ConfigTransaction.php b/src/Core/Config/Model/ConfigTransaction.php index 296a469c0..26420b078 100644 --- a/src/Core/Config/Model/ConfigTransaction.php +++ b/src/Core/Config/Model/ConfigTransaction.php @@ -37,6 +37,8 @@ class ConfigTransaction implements ISetConfigValuesTransactionally protected $cache; /** @var Cache */ protected $delCache; + /** @var bool field to check if something is to save */ + protected $changedConfig = false; public function __construct(IManageConfigValues $config) { @@ -70,6 +72,7 @@ class ConfigTransaction implements ISetConfigValuesTransactionally public function set(string $cat, string $key, $value): ISetConfigValuesTransactionally { $this->cache->set($cat, $key, $value, Cache::SOURCE_DATA); + $this->changedConfig = true; return $this; } @@ -80,6 +83,7 @@ class ConfigTransaction implements ISetConfigValuesTransactionally { $this->cache->delete($cat, $key); $this->delCache->set($cat, $key, 'deleted'); + $this->changedConfig = true; return $this; } @@ -87,6 +91,11 @@ class ConfigTransaction implements ISetConfigValuesTransactionally /** {@inheritDoc} */ public function commit(): void { + // If nothing changed, just do nothing :) + if (!$this->changedConfig) { + return; + } + try { $newCache = $this->config->getCache()->merge($this->cache); $newCache = $newCache->diff($this->delCache); diff --git a/src/Core/Config/Util/ConfigFileManager.php b/src/Core/Config/Util/ConfigFileManager.php index 378d7fbd4..cc264ea26 100644 --- a/src/Core/Config/Util/ConfigFileManager.php +++ b/src/Core/Config/Util/ConfigFileManager.php @@ -173,7 +173,21 @@ class ConfigFileManager $filename = $this->configDir . '/' . self::CONFIG_DATA_FILE; if (file_exists($filename)) { - $dataArray = include $filename; + + $content = '' . $content); if (!is_array($dataArray)) { throw new ConfigFileException(sprintf('Error loading config file %s', $filename)); @@ -200,8 +214,12 @@ class ConfigFileManager throw new ConfigFileException('config source cannot get encoded'); } - if (!file_put_contents($this->configDir . '/' . self::CONFIG_DATA_FILE, $encodedData)) { - throw new ConfigFileException(sprintf('Cannot save data to file %s/%s', $this->configDir, self::CONFIG_DATA_FILE)); + $configStream = fopen($this->configDir . '/' . self::CONFIG_DATA_FILE, 'w'); + + if (flock($configStream, LOCK_EX)) { + fwrite($configStream, $encodedData); + fflush($configStream); + flock($configStream, LOCK_UN); } } diff --git a/tests/src/Core/Config/Cache/ConfigFileManagerTest.php b/tests/src/Core/Config/Cache/ConfigFileManagerTest.php index 99049426b..55dff2f4a 100644 --- a/tests/src/Core/Config/Cache/ConfigFileManagerTest.php +++ b/tests/src/Core/Config/Cache/ConfigFileManagerTest.php @@ -407,7 +407,7 @@ class ConfigFileManagerTest extends MockedTest $configFileLoader->setupCache($configCache); - $specialChars = '!"§$%&/()(/&%$\'>set('system', 'test', 'it', Cache::SOURCE_DATA); diff --git a/tests/src/Core/Config/ConfigTransactionTest.php b/tests/src/Core/Config/ConfigTransactionTest.php index 2eec9b68f..454c760d4 100644 --- a/tests/src/Core/Config/ConfigTransactionTest.php +++ b/tests/src/Core/Config/ConfigTransactionTest.php @@ -9,6 +9,7 @@ use Friendica\Core\Config\Util\ConfigFileManager; use Friendica\Core\Config\ValueObject\Cache; use Friendica\Test\MockedTest; use Friendica\Test\Util\VFSTrait; +use Mockery\Exception\InvalidCountException; class ConfigTransactionTest extends MockedTest { @@ -108,4 +109,27 @@ class ConfigTransactionTest extends MockedTest // the whole category should be gone self::assertNull($tempData['delete'] ?? null); } + + /** + * This test asserts that in empty transactions, no saveData is called, thus no config file writing was performed + */ + public function testNothingToDo() + { + $this->configFileManager = \Mockery::spy(ConfigFileManager::class); + + $config = new Config($this->configFileManager, new Cache()); + $configTransaction = new ConfigTransaction($config); + + // commit empty transaction + $configTransaction->commit(); + + try { + $this->configFileManager->shouldNotHaveReceived('saveData'); + } catch (InvalidCountException $exception) { + self::fail($exception); + } + + // If not failed, the test ends successfully :) + self::assertTrue(true); + } } diff --git a/tests/src/Util/BaseURLTest.php b/tests/src/Util/BaseURLTest.php index 0be83be0a..b9c23a1d8 100644 --- a/tests/src/Util/BaseURLTest.php +++ b/tests/src/Util/BaseURLTest.php @@ -23,10 +23,25 @@ namespace Friendica\Test\src\Util; use Friendica\App\BaseURL; use Friendica\Core\Config\Capability\IManageConfigValues; +use Friendica\Core\Config\Capability\ISetConfigValuesTransactionally; +use Friendica\Core\Config\Model\Config; +use Friendica\Core\Config\Model\ConfigTransaction; +use Friendica\Core\Config\Util\ConfigFileManager; +use Friendica\Core\Config\ValueObject\Cache; use Friendica\Test\MockedTest; +use Friendica\Test\Util\VFSTrait; class BaseURLTest extends MockedTest { + use VFSTrait; + + protected function setUp(): void + { + parent::setUp(); + + $this->setUpVfsDir(); + } + public function dataDefault() { return [ @@ -231,6 +246,21 @@ class BaseURLTest extends MockedTest public function dataSave() { return [ + 'no_change' => [ + 'input' => [ + 'hostname' => 'friendica.local', + 'urlPath' => 'path', + 'sslPolicy' => BaseURL::SSL_POLICY_FULL, + 'url' => 'https://friendica.local/path', + 'force_ssl' => true, + ], + 'save' => [ + 'hostname' => 'friendica.local', + 'urlPath' => 'path', + 'sslPolicy' => BaseURL::SSL_POLICY_FULL, + ], + 'url' => 'https://friendica.local/path', + ], 'default' => [ 'input' => [ 'hostname' => 'friendica.old', @@ -315,28 +345,20 @@ class BaseURLTest extends MockedTest */ public function testSave($input, $save, $url) { - $configMock = \Mockery::mock(IManageConfigValues::class); - $configMock->shouldReceive('get')->with('config', 'hostname')->andReturn($input['hostname']); - $configMock->shouldReceive('get')->with('system', 'urlpath')->andReturn($input['urlPath']); - $configMock->shouldReceive('get')->with('system', 'ssl_policy')->andReturn($input['sslPolicy']); - $configMock->shouldReceive('get')->with('system', 'url')->andReturn($input['url']); - $configMock->shouldReceive('get')->with('system', 'force_ssl')->andReturn($input['force_ssl']); + $configFileManager = new ConfigFileManager($this->root->url(), $this->root->url() . '/config/', $this->root->url() . '/static/'); + $config = new Config($configFileManager, new Cache([ + 'config' => [ + 'hostname' => $input['hostname'] ?? null, + ], + 'system' => [ + 'urlpath' => $input['urlPath'] ?? null, + 'ssl_policy' => $input['sslPolicy'] ?? null, + 'url' => $input['url'] ?? null, + 'force_ssl' => $input['force_ssl'] ?? null, + ], + ])); - $baseUrl = new BaseURL($configMock, []); - - if (isset($save['hostname'])) { - $configMock->shouldReceive('set')->with('config', 'hostname', $save['hostname'])->andReturn(true)->once(); - } - - if (isset($save['urlPath'])) { - $configMock->shouldReceive('set')->with('system', 'urlpath', $save['urlPath'])->andReturn(true)->once(); - } - - if (isset($save['sslPolicy'])) { - $configMock->shouldReceive('set')->with('system', 'ssl_policy', $save['sslPolicy'])->andReturn(true)->once(); - } - - $configMock->shouldReceive('set')->with('system', 'url', $url)->andReturn(true)->once(); + $baseUrl = new BaseURL($config, []); $baseUrl->save($save['hostname'], $save['sslPolicy'], $save['urlPath']); @@ -353,28 +375,20 @@ class BaseURLTest extends MockedTest */ public function testSaveByUrl($input, $save, $url) { - $configMock = \Mockery::mock(IManageConfigValues::class); - $configMock->shouldReceive('get')->with('config', 'hostname')->andReturn($input['hostname']); - $configMock->shouldReceive('get')->with('system', 'urlpath')->andReturn($input['urlPath']); - $configMock->shouldReceive('get')->with('system', 'ssl_policy')->andReturn($input['sslPolicy']); - $configMock->shouldReceive('get')->with('system', 'url')->andReturn($input['url']); - $configMock->shouldReceive('get')->with('system', 'force_ssl')->andReturn($input['force_ssl']); + $configFileManager = new ConfigFileManager($this->root->url(), $this->root->url() . '/config/', $this->root->url() . '/static/'); + $config = new Config($configFileManager, new Cache([ + 'config' => [ + 'hostname' => $input['hostname'] ?? null, + ], + 'system' => [ + 'urlpath' => $input['urlPath'] ?? null, + 'ssl_policy' => $input['sslPolicy'] ?? null, + 'url' => $input['url'] ?? null, + 'force_ssl' => $input['force_ssl'] ?? null, + ], + ])); - $baseUrl = new BaseURL($configMock, []); - - if (isset($save['hostname'])) { - $configMock->shouldReceive('set')->with('config', 'hostname', $save['hostname'])->andReturn(true)->once(); - } - - if (isset($save['urlPath'])) { - $configMock->shouldReceive('set')->with('system', 'urlpath', $save['urlPath'])->andReturn(true)->once(); - } - - if (isset($save['sslPolicy'])) { - $configMock->shouldReceive('set')->with('system', 'ssl_policy', $save['sslPolicy'])->andReturn(true)->once(); - } - - $configMock->shouldReceive('set')->with('system', 'url', $url)->andReturn(true)->once(); + $baseUrl = new BaseURL($config, []); $baseUrl->saveByURL($url); @@ -498,65 +512,4 @@ class BaseURLTest extends MockedTest self::assertEquals($redirect, $baseUrl->checkRedirectHttps()); } - - public function dataWrongSave() - { - return [ - 'wrongHostname' => [ - 'fail' => 'hostname', - ], - 'wrongSSLPolicy' => [ - 'fail' => 'sslPolicy', - ], - 'wrongURLPath' => [ - 'fail' => 'urlPath', - ], - 'wrongURL' => [ - 'fail' => 'url', - ], - ]; - } - - /** - * Test the save() method with wrong parameters - * @dataProvider dataWrongSave - */ - public function testWrongSave($fail) - { - $configMock = \Mockery::mock(IManageConfigValues::class); - $configMock->shouldReceive('get')->with('config', 'hostname')->andReturn('friendica.local'); - $configMock->shouldReceive('get')->with('system', 'urlpath')->andReturn('new/test'); - $configMock->shouldReceive('get')->with('system', 'ssl_policy')->andReturn(BaseURL::DEFAULT_SSL_SCHEME); - $configMock->shouldReceive('get')->with('system', 'url')->andReturn('http://friendica.local/new/test'); - - switch ($fail) { - case 'hostname': - $configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(false)->once(); - break; - case 'sslPolicy': - $configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(true)->twice(); - $configMock->shouldReceive('set')->with('system', 'ssl_policy', \Mockery::any())->andReturn(false)->once(); - break; - case 'urlPath': - $configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(true)->twice(); - $configMock->shouldReceive('set')->with('system', 'ssl_policy', \Mockery::any())->andReturn(true)->twice(); - $configMock->shouldReceive('set')->with('system', 'urlpath', \Mockery::any())->andReturn(false)->once(); - break; - case 'url': - $configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(true)->twice(); - $configMock->shouldReceive('set')->with('system', 'ssl_policy', \Mockery::any())->andReturn(true)->twice(); - $configMock->shouldReceive('set')->with('system', 'urlpath', \Mockery::any())->andReturn(true)->twice(); - $configMock->shouldReceive('set')->with('system', 'url', \Mockery::any())->andReturn(false)->once(); - break; - } - - $baseUrl = new BaseURL($configMock, []); - self::assertFalse($baseUrl->save('test', 10, 'nope')); - - // nothing should have changed because we never successfully saved anything - self::assertEquals('friendica.local', $baseUrl->getHostname()); - self::assertEquals('new/test', $baseUrl->getUrlPath()); - self::assertEquals(BaseURL::DEFAULT_SSL_SCHEME, $baseUrl->getSSLPolicy()); - self::assertEquals('http://friendica.local/new/test', $baseUrl->get()); - } }