Merge pull request #11150 from nextcloud/feature/noid/unit-test-find-webroot

Extract logic for webroot into method and add test

lib/private/Setup.php

@@ -43,6 +43,7 @@ namespace OC;
 
 use bantu\IniGetWrapper\IniGetWrapper;
 use Exception;
+use InvalidArgumentException;
 use OC\App\AppStore\Bundles\BundleFetcher;
 use OC\Authentication\Token\DefaultTokenCleanupJob;
 use OC\Authentication\Token\DefaultTokenProvider;
@@ -431,27 +432,46 @@ class Setup {
 	}
 
 	/**
-	 * Append the correct ErrorDocument path for Apache hosts
-	 * @return bool True when success, False otherwise
+	 * Find webroot from config
+	 *
+	 * @param SystemConfig $config
+	 * @return string
+	 * @throws InvalidArgumentException when invalid value for overwrite.cli.url
 	 */
-	public static function updateHtaccess() {
-		$config = \OC::$server->getSystemConfig();
-
+	private static function findWebRoot(SystemConfig $config): string {
 		// For CLI read the value from overwrite.cli.url
-		if(\OC::$CLI) {
+		if (\OC::$CLI) {
 			$webRoot = $config->getValue('overwrite.cli.url', '');
-			if($webRoot === '') {
-				return false;
+			if ($webRoot === '') {
+				throw new InvalidArgumentException('overwrite.cli.url is empty');
 			}
 			$webRoot = parse_url($webRoot, PHP_URL_PATH);
 			if ($webRoot === null) {
-				return false;
+				throw new InvalidArgumentException('invalid value for overwrite.cli.url');
 			}
 			$webRoot = rtrim($webRoot, '/');
 		} else {
 			$webRoot = !empty(\OC::$WEBROOT) ? \OC::$WEBROOT : '/';
 		}
 
+		return $webRoot;
+	}
+
+	/**
+	 * Append the correct ErrorDocument path for Apache hosts
+	 *
+	 * @return bool True when success, False otherwise
+	 * @throws \OCP\AppFramework\QueryException
+	 */
+	public static function updateHtaccess() {
+		$config = \OC::$server->getSystemConfig();
+
+		try {
+			$webRoot = self::findWebRoot($config);
+		} catch (InvalidArgumentException $e) {
+			return false;
+		}
+
 		$setupHelper = new \OC\Setup(
 			$config,
 			\OC::$server->getIniWrapper(),
@@ -467,10 +487,10 @@ class Setup {
 		$htaccessContent = explode($content, $htaccessContent, 2)[0];
 
 		//custom 403 error page
-		$content.= "\nErrorDocument 403 ".$webRoot."/";
+		$content .= "\nErrorDocument 403 " . $webRoot . '/';
 
 		//custom 404 error page
-		$content.= "\nErrorDocument 404 ".$webRoot."/";
+		$content .= "\nErrorDocument 404 " . $webRoot . '/';
 
 		// Add rewrite rules if the RewriteBase is configured
 		$rewriteBase = $config->getValue('htaccess.RewriteBase', '');

tests/lib/SetupTest.php

@@ -10,6 +10,7 @@ namespace Test;
 
 use bantu\IniGetWrapper\IniGetWrapper;
 use OC\Installer;
+use OC\Setup;
 use OC\SystemConfig;
 use OCP\Defaults;
 use OCP\IL10N;
@@ -45,7 +46,7 @@ class SetupTest extends \Test\TestCase {
 		$this->logger = $this->createMock(ILogger::class);
 		$this->random = $this->createMock(ISecureRandom::class);
 		$this->installer = $this->createMock(Installer::class);
-		$this->setupClass = $this->getMockBuilder('\OC\Setup')
+		$this->setupClass = $this->getMockBuilder(Setup::class)
 			->setMethods(['class_exists', 'is_callable', 'getAvailableDbDriversForPdo'])
 			->setConstructorArgs([$this->config, $this->iniWrapper, $this->l10n, $this->defaults, $this->logger, $this->random, $this->installer])
 			->getMock();
@@ -56,7 +57,7 @@ class SetupTest extends \Test\TestCase {
 			->expects($this->once())
 			->method('getValue')
 			->will($this->returnValue(
-				array('sqlite', 'mysql', 'oci')
+				['sqlite', 'mysql', 'oci']
 			));
 		$this->setupClass
 			->expects($this->once())
@@ -67,9 +68,9 @@ class SetupTest extends \Test\TestCase {
 			->method('getAvailableDbDriversForPdo')
 			->will($this->returnValue(['sqlite']));
 		$result = $this->setupClass->getSupportedDatabases();
-		$expectedResult = array(
+		$expectedResult = [
 			'sqlite' => 'SQLite'
-		);
+		];
 
 		$this->assertSame($expectedResult, $result);
 	}
@@ -79,7 +80,7 @@ class SetupTest extends \Test\TestCase {
 			->expects($this->once())
 			->method('getValue')
 			->will($this->returnValue(
-				array('sqlite', 'mysql', 'oci', 'pgsql')
+				['sqlite', 'mysql', 'oci', 'pgsql']
 			));
 		$this->setupClass
 			->expects($this->any())
@@ -91,7 +92,7 @@ class SetupTest extends \Test\TestCase {
 			->will($this->returnValue([]));
 		$result = $this->setupClass->getSupportedDatabases();
 
-		$this->assertSame(array(), $result);
+		$this->assertSame([], $result);
 	}
 
 	public function testGetSupportedDatabasesWithAllWorking() {
@@ -99,7 +100,7 @@ class SetupTest extends \Test\TestCase {
 			->expects($this->once())
 			->method('getValue')
 			->will($this->returnValue(
-				array('sqlite', 'mysql', 'pgsql', 'oci')
+				['sqlite', 'mysql', 'pgsql', 'oci']
 			));
 		$this->setupClass
 			->expects($this->any())
@@ -110,12 +111,12 @@ class SetupTest extends \Test\TestCase {
 			->method('getAvailableDbDriversForPdo')
 			->will($this->returnValue(['sqlite', 'mysql', 'pgsql']));
 		$result = $this->setupClass->getSupportedDatabases();
-		$expectedResult = array(
+		$expectedResult = [
 			'sqlite' => 'SQLite',
 			'mysql' => 'MySQL/MariaDB',
 			'pgsql' => 'PostgreSQL',
 			'oci' => 'Oracle'
-		);
+		];
 		$this->assertSame($expectedResult, $result);
 	}
 
@@ -130,4 +131,37 @@ class SetupTest extends \Test\TestCase {
 			->will($this->returnValue('NotAnArray'));
 		$this->setupClass->getSupportedDatabases();
 	}
+
+	/**
+	 * @dataProvider findWebRootProvider
+	 * @param $url
+	 * @param $expected
+	 */
+	public function testFindWebRootCli($url, $expected) {
+		$cliState = \OC::$CLI;
+
+		$this->config
+			->expects($this->once())
+			->method('getValue')
+			->will($this->returnValue($url));
+		\OC::$CLI = true;
+
+		try {
+			$webRoot = self::invokePrivate($this->setupClass, 'findWebRoot', [$this->config]);
+		} catch (\InvalidArgumentException $e) {
+			$webRoot = false;
+		}
+
+		\OC::$CLI = $cliState;
+		$this->assertEquals($webRoot, $expected);
+	}
+
+	public function findWebRootProvider(): array {
+		return [
+			'https://www.example.com/nextcloud' => ['https://www.example.com/nextcloud', '/nextcloud'],
+			'https://www.example.com/' => ['https://www.example.com/', ''],
+			'https://www.example.com' => ['https://www.example.com', false],
+			'empty' => ['', false],
+		];
+	}
 }