From c057954896ee9984513473acb4041751ecad6339 Mon Sep 17 00:00:00 2001 From: Philipp Date: Wed, 4 Jan 2023 08:14:00 +0100 Subject: [PATCH] Make BaseURL check/save transactional and make the whole process easier --- src/App/BaseURL.php | 53 +++---- .../ISetConfigValuesTransactionally.php | 4 - tests/src/Util/BaseURLTest.php | 148 +++++------------- 3 files changed, 59 insertions(+), 146 deletions(-) diff --git a/src/App/BaseURL.php b/src/App/BaseURL.php index 89dded3002..e557f712f0 100644 --- a/src/App/BaseURL.php +++ b/src/App/BaseURL.php @@ -172,54 +172,37 @@ class BaseURL */ public function save($hostname = null, $sslPolicy = null, $urlPath = null): bool { - $currHostname = $this->hostname; - $currSSLPolicy = $this->sslPolicy; - $currURLPath = $this->urlPath; - $currUrl = $this->url; + $currUrl = $this->url; + + $configTransaction = $this->config->beginTransaction(); + $savable = false; 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; + $savable = true; } 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; + $savable = true; } 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; + $savable = true; } $this->determineBaseUrl(); if ($this->url !== $currUrl) { - if (!$this->config->set('system', 'url', $this->url)) { - $this->hostname = $currHostname; - $this->sslPolicy = $currSSLPolicy; - $this->urlPath = $currURLPath; - $this->determineBaseUrl(); + $configTransaction->set('system', 'url', $this->url); + $savable = true; + } - $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 ($savable) { + $configTransaction->commit(); } return true; diff --git a/src/Core/Config/Capability/ISetConfigValuesTransactionally.php b/src/Core/Config/Capability/ISetConfigValuesTransactionally.php index 501e24f738..a9fe36b022 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/tests/src/Util/BaseURLTest.php b/tests/src/Util/BaseURLTest.php index e108385f05..b9c23a1d8b 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 [ @@ -330,30 +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']); - - $baseUrl = new BaseURL($configMock, []); - - if (isset($save['hostname']) && ($save['hostname'] !== $input['hostname'])) { - $configMock->shouldReceive('set')->with('config', 'hostname', $save['hostname'])->andReturn(true)->once(); - } - - if (isset($save['urlPath']) && ($save['urlPath'] !== $input['urlPath'])) { - $configMock->shouldReceive('set')->with('system', 'urlpath', $save['urlPath'])->andReturn(true)->once(); - } - - if (isset($save['sslPolicy']) && ($save['sslPolicy'] !== $input['sslPolicy'])) { - $configMock->shouldReceive('set')->with('system', 'ssl_policy', $save['sslPolicy'])->andReturn(true)->once(); - } + $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, + ], + ])); - if ($input['url'] !== $url) { - $configMock->shouldReceive('set')->with('system', 'url', $url)->andReturn(true)->once(); - } + $baseUrl = new BaseURL($config, []); $baseUrl->save($save['hostname'], $save['sslPolicy'], $save['urlPath']); @@ -370,30 +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']); - - $baseUrl = new BaseURL($configMock, []); - - if (isset($save['hostname']) && ($save['hostname'] !== $input['hostname'])) { - $configMock->shouldReceive('set')->with('config', 'hostname', $save['hostname'])->andReturn(true)->once(); - } - - if (isset($save['urlPath']) && ($save['urlPath'] !== $input['urlPath'])) { - $configMock->shouldReceive('set')->with('system', 'urlpath', $save['urlPath'])->andReturn(true)->once(); - } - - if (isset($save['sslPolicy']) && ($save['sslPolicy'] !== $input['sslPolicy'])) { - $configMock->shouldReceive('set')->with('system', 'ssl_policy', $save['sslPolicy'])->andReturn(true)->once(); - } + $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, + ], + ])); - if ($input['url'] !== $url) { - $configMock->shouldReceive('set')->with('system', 'url', $url)->andReturn(true)->once(); - } + $baseUrl = new BaseURL($config, []); $baseUrl->saveByURL($url); @@ -517,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()); - } } -- 2.39.5