Make BaseURL check/save transactional and make the whole process easier

src/App/BaseURL.php

@@ -172,54 +172,37 @@ class BaseURL
 	 */
 	public function save($hostname = null, $sslPolicy = null, $urlPath = null): bool
 	{
-		$currHostname  = $this->hostname;
+		$currUrl = $this->url;
-		$currSSLPolicy = $this->sslPolicy;
+
-		$currURLPath   = $this->urlPath;
+		$configTransaction = $this->config->beginTransaction();
-		$currUrl       = $this->url;
+		$savable           = false;
 
 		if (!empty($hostname) && $hostname !== $this->hostname) {
-			if ($this->config->set('config', 'hostname', $hostname)) {
+			$configTransaction->set('config', 'hostname', $hostname);
-				$this->hostname = $hostname;
+			$this->hostname = $hostname;
-			} else {
+			$savable        = true;
-				return false;
-			}
 		}
 
 		if (isset($sslPolicy) && $sslPolicy !== $this->sslPolicy) {
-			if ($this->config->set('system', 'ssl_policy', $sslPolicy)) {
+			$configTransaction->set('system', 'ssl_policy', $sslPolicy);
-				$this->sslPolicy = $sslPolicy;
+			$this->sslPolicy = $sslPolicy;
-			} else {
+			$savable         = true;
-				$this->hostname = $currHostname;
-				$this->config->set('config', 'hostname', $this->hostname);
-				return false;
-			}
 		}
 
 		if (isset($urlPath) && $urlPath !== $this->urlPath) {
-			if ($this->config->set('system', 'urlpath', $urlPath)) {
+			$configTransaction->set('system', 'urlpath', $urlPath);
-				$this->urlPath = $urlPath;
+			$this->urlPath = $urlPath;
-			} else {
+			$savable       = true;
-				$this->hostname  = $currHostname;
-				$this->sslPolicy = $currSSLPolicy;
-				$this->config->set('config', 'hostname', $this->hostname);
-				$this->config->set('system', 'ssl_policy', $this->sslPolicy);
-				return false;
-			}
 		}
 
 		$this->determineBaseUrl();
 		if ($this->url !== $currUrl) {
-			if (!$this->config->set('system', 'url', $this->url)) {
+			$configTransaction->set('system', 'url', $this->url);
-				$this->hostname  = $currHostname;
+			$savable = true;
-				$this->sslPolicy = $currSSLPolicy;
+		}
-				$this->urlPath   = $currURLPath;
-				$this->determineBaseUrl();
 
-				$this->config->set('config', 'hostname', $this->hostname);
+		if ($savable) {
-				$this->config->set('system', 'ssl_policy', $this->sslPolicy);
+			$configTransaction->commit();
-				$this->config->set('system', 'urlpath', $this->urlPath);
-				return false;
-			}
 		}
 
 		return true;

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;
 

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);
+		$configFileManager = new ConfigFileManager($this->root->url(), $this->root->url() . '/config/', $this->root->url() . '/static/');
-		$configMock->shouldReceive('get')->with('config', 'hostname')->andReturn($input['hostname']);
+		$config = new Config($configFileManager, new Cache([
-		$configMock->shouldReceive('get')->with('system', 'urlpath')->andReturn($input['urlPath']);
+			'config' => [
-		$configMock->shouldReceive('get')->with('system', 'ssl_policy')->andReturn($input['sslPolicy']);
+				'hostname' => $input['hostname'] ?? null,
-		$configMock->shouldReceive('get')->with('system', 'url')->andReturn($input['url']);
+			],
-		$configMock->shouldReceive('get')->with('system', 'force_ssl')->andReturn($input['force_ssl']);
+			'system' => [
+				'urlpath' => $input['urlPath'] ?? null,
+				'ssl_policy' => $input['sslPolicy'] ?? null,
+				'url' => $input['url'] ?? null,
+				'force_ssl' => $input['force_ssl'] ?? null,
+			],
+		]));
 
-		$baseUrl = new BaseURL($configMock, []);
+		$baseUrl = new BaseURL($config, []);
-
-		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();
-		}
-
-		if ($input['url'] !== $url) {
-			$configMock->shouldReceive('set')->with('system', 'url', $url)->andReturn(true)->once();
-		}
 
 		$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);
+		$configFileManager = new ConfigFileManager($this->root->url(), $this->root->url() . '/config/', $this->root->url() . '/static/');
-		$configMock->shouldReceive('get')->with('config', 'hostname')->andReturn($input['hostname']);
+		$config = new Config($configFileManager, new Cache([
-		$configMock->shouldReceive('get')->with('system', 'urlpath')->andReturn($input['urlPath']);
+			'config' => [
-		$configMock->shouldReceive('get')->with('system', 'ssl_policy')->andReturn($input['sslPolicy']);
+				'hostname' => $input['hostname'] ?? null,
-		$configMock->shouldReceive('get')->with('system', 'url')->andReturn($input['url']);
+			],
-		$configMock->shouldReceive('get')->with('system', 'force_ssl')->andReturn($input['force_ssl']);
+			'system' => [
+				'urlpath' => $input['urlPath'] ?? null,
+				'ssl_policy' => $input['sslPolicy'] ?? null,
+				'url' => $input['url'] ?? null,
+				'force_ssl' => $input['force_ssl'] ?? null,
+			],
+		]));
 
-		$baseUrl = new BaseURL($configMock, []);
+		$baseUrl = new BaseURL($config, []);
-
-		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();
-		}
-
-		if ($input['url'] !== $url) {
-			$configMock->shouldReceive('set')->with('system', 'url', $url)->andReturn(true)->once();
-		}
 
 		$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());
-	}
 }