fix: Cleanup and fix tests, and fix related issues in code

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

lib/private/Server.php

@@ -1048,7 +1048,7 @@ class Server extends ServerContainer implements IServerContainer {
 				$c->getAppDataDir('js'),
 				$c->get(IURLGenerator::class),
 				$this->get(ICacheFactory::class),
-				$c->get(SystemConfig::class),
+				$c->get(\OCP\IConfig::class),
 				$c->get(LoggerInterface::class)
 			);
 		});

lib/private/Template/JSCombiner.php

@@ -9,13 +9,13 @@ declare(strict_types=1);
 
 namespace OC\Template;
 
-use OC\SystemConfig;
 use OCP\Files\IAppData;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
 use OCP\Files\SimpleFS\ISimpleFolder;
 use OCP\ICache;
 use OCP\ICacheFactory;
+use OCP\IConfig;
 use OCP\IURLGenerator;
 use Psr\Log\LoggerInterface;
 
@@ -26,14 +26,14 @@ class JSCombiner {
 		protected IAppData $appData,
 		protected IURLGenerator $urlGenerator,
 		protected ICacheFactory $cacheFactory,
-		protected SystemConfig $config,
+		protected IConfig $config,
 		protected LoggerInterface $logger,
 	) {
 		$this->depsCache = $this->cacheFactory->createDistributed('JS-' . md5($this->urlGenerator->getBaseUrl()));
 	}
 
 	public function process(string $root, string $file, string $app): bool {
-		if ($this->config->getValue('debug') || !$this->config->getValue('installed')) {
+		if ($this->config->getSystemValueBool('debug') || !$this->config->getSystemValueBool('installed')) {
 			return false;
 		}
 

lib/private/Template/ResourceLocator.php

@@ -18,7 +18,6 @@ abstract class ResourceLocator {
 
 	protected array $mapping;
 	protected string $serverroot;
-	protected string $webroot;
 
 	protected array $resources = [];
 
@@ -30,7 +29,6 @@ abstract class ResourceLocator {
 			\OC::$SERVERROOT => \OC::$WEBROOT
 		];
 		$this->serverroot = \OC::$SERVERROOT;
-		$this->webroot = \OC::$WEBROOT;
 
 		$this->theme = $config->getSystemValueString('theme', '');
 
@@ -53,7 +51,7 @@ abstract class ResourceLocator {
 			try {
 				$this->doFind($resource);
 			} catch (ResourceNotFoundException $e) {
-				$resourceApp = substr($resource, 0, strpos($resource, '/'));
+				[$resourceApp] = explode('/', $resource, 2);
 				$this->logger->debug('Could not find resource file "' . $e->getResourcePath() . '"', ['app' => $resourceApp]);
 			}
 		}
@@ -62,7 +60,7 @@ abstract class ResourceLocator {
 				try {
 					$this->doFindTheme($resource);
 				} catch (ResourceNotFoundException $e) {
-					$resourceApp = substr($resource, 0, strpos($resource, '/'));
+					[$resourceApp] = explode('/', $resource, 2);
 					$this->logger->debug('Could not find resource file in theme "' . $e->getResourcePath() . '"', ['app' => $resourceApp]);
 				}
 			}

lib/private/legacy/OC_App.php

@@ -12,6 +12,7 @@ use OC\AppFramework\Bootstrap\Coordinator;
 use OC\Installer;
 use OC\Repair;
 use OC\Repair\Events\RepairErrorEvent;
+use OCP\App\AppPathNotFoundException;
 use OCP\App\IAppManager;
 use OCP\Authentication\IAlternativeLogin;
 use OCP\EventDispatcher\IEventDispatcher;
@@ -426,7 +427,11 @@ class OC_App {
 					$info['level'] = self::supportedApp;
 				}
 
-				$appPath = self::getAppPath($app);
+				try {
+					$appPath = $appManager->getAppPath($app);
+				} catch (AppPathNotFoundException) {
+					$appPath = false;
+				}
 				if ($appPath !== false) {
 					$appIcon = $appPath . '/img/' . $app . '.svg';
 					if (file_exists($appIcon)) {

tests/lib/Template/CSSResourceLocatorTest.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -12,30 +14,25 @@ use OC\Files\AppData\AppData;
 use OC\Files\AppData\Factory;
 use OC\Template\CSSResourceLocator;
 use OCA\Theming\ThemingDefaults;
+use OCP\App\IAppManager;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\IAppData;
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IURLGenerator;
+use OCP\Server;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 
 class CSSResourceLocatorTest extends \Test\TestCase {
-	/** @var IAppData|\PHPUnit\Framework\MockObject\MockObject */
+	private IAppData&MockObject $appData;
-	protected $appData;
+	private IURLGenerator&MockObject $urlGenerator;
-	/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
+	private IConfig&MockObject $config;
-	protected $urlGenerator;
+	private ThemingDefaults&MockObject $themingDefaults;
-	/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
+	private ICacheFactory&MockObject $cacheFactory;
-	protected $config;
+	private LoggerInterface&MockObject $logger;
-	/** @var ThemingDefaults|\PHPUnit\Framework\MockObject\MockObject */
+	private ITimeFactory&MockObject $timeFactory;
-	protected $themingDefaults;
+	private AppConfig&MockObject $appConfig;
-	/** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */
-	protected $cacheFactory;
-	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
-	protected $logger;
-	/** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
-	private $timeFactory;
-	/** @var AppConfig|\PHPUnit\Framework\MockObject\MockObject */
-	private $appConfig;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -56,6 +53,8 @@ class CSSResourceLocatorTest extends \Test\TestCase {
 		$factory->method('get')->with('css')->willReturn($this->appData);
 		return new CSSResourceLocator(
 			$this->logger,
+			$this->createMock(IConfig::class),
+			Server::get(IAppManager::class),
 			'theme',
 			['core' => 'map'],
 			['3rd' => 'party'],
@@ -74,8 +73,8 @@ class CSSResourceLocatorTest extends \Test\TestCase {
 		return rmdir($directory);
 	}
 
-	private function randomString() {
+	private function randomString(): string {
-		return sha1(uniqid(mt_rand(), true));
+		return sha1(random_bytes(10));
 	}
 
 	public function testFindWithAppPathSymlink(): void {

tests/lib/Template/JSCombinerTest.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -7,7 +9,6 @@
 
 namespace Test\Template;
 
-use OC\SystemConfig;
 use OC\Template\JSCombiner;
 use OCP\Files\IAppData;
 use OCP\Files\NotFoundException;
@@ -16,33 +17,29 @@ use OCP\Files\SimpleFS\ISimpleFile;
 use OCP\Files\SimpleFS\ISimpleFolder;
 use OCP\ICache;
 use OCP\ICacheFactory;
+use OCP\IConfig;
 use OCP\ITempManager;
 use OCP\IURLGenerator;
 use OCP\Server;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 
 class JSCombinerTest extends \Test\TestCase {
-	/** @var IAppData|\PHPUnit\Framework\MockObject\MockObject */
+	private IAppData&MockObject $appData;
-	protected $appData;
+	private IURLGenerator&MockObject $urlGenerator;
-	/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
+	private IConfig&MockObject $config;
-	protected $urlGenerator;
+	private ICache&MockObject $depsCache;
-	/** @var SystemConfig|\PHPUnit\Framework\MockObject\MockObject */
+	private LoggerInterface&MockObject $logger;
-	protected $config;
+	private ICacheFactory&MockObject $cacheFactory;
-	/** @var ICache|\PHPUnit\Framework\MockObject\MockObject */
+
-	protected $depsCache;
+	private JSCombiner $jsCombiner;
-	/** @var JSCombiner */
-	protected $jsCombiner;
-	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
-	protected $logger;
-	/** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */
-	protected $cacheFactory;
 
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->appData = $this->createMock(IAppData::class);
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
-		$this->config = $this->createMock(SystemConfig::class);
+		$this->config = $this->createMock(IConfig::class);
 		$this->cacheFactory = $this->createMock(ICacheFactory::class);
 		$this->depsCache = $this->createMock(ICache::class);
 		$this->cacheFactory->expects($this->atLeastOnce())
@@ -61,7 +58,7 @@ class JSCombinerTest extends \Test\TestCase {
 	public function testProcessDebugMode(): void {
 		$this->config
 			->expects($this->once())
-			->method('getValue')
+			->method('getSystemValueBool')
 			->with('debug')
 			->willReturn(true);
 
@@ -72,7 +69,7 @@ class JSCombinerTest extends \Test\TestCase {
 	public function testProcessNotInstalled(): void {
 		$this->config
 			->expects($this->exactly(2))
-			->method('getValue')
+			->method('getSystemValueBool')
 			->willReturnMap([
 				['debug', false],
 				['installed', false]
@@ -85,10 +82,10 @@ class JSCombinerTest extends \Test\TestCase {
 	public function testProcessUncachedFileNoAppDataFolder(): void {
 		$this->config
 			->expects($this->exactly(2))
-			->method('getValue')
+			->method('getSystemValueBool')
 			->willReturnMap([
-				['debug', '', false],
+				['debug', false],
-				['installed', '', true],
+				['installed', true],
 			]);
 		$folder = $this->createMock(ISimpleFolder::class);
 		$this->appData->expects($this->once())->method('getFolder')->with('awesomeapp')->willThrowException(new NotFoundException());
@@ -121,10 +118,10 @@ class JSCombinerTest extends \Test\TestCase {
 	public function testProcessUncachedFile(): void {
 		$this->config
 			->expects($this->exactly(2))
-			->method('getValue')
+			->method('getSystemValueBool')
 			->willReturnMap([
-				['debug', '', false],
+				['debug', false],
-				['installed', '', true],
+				['installed', true],
 			]);
 		$folder = $this->createMock(ISimpleFolder::class);
 		$this->appData->expects($this->once())->method('getFolder')->with('awesomeapp')->willReturn($folder);
@@ -155,10 +152,10 @@ class JSCombinerTest extends \Test\TestCase {
 	public function testProcessCachedFile(): void {
 		$this->config
 			->expects($this->exactly(2))
-			->method('getValue')
+			->method('getSystemValueBool')
 			->willReturnMap([
-				['debug', '', false],
+				['debug', false],
-				['installed', '', true],
+				['installed', true],
 			]);
 		$folder = $this->createMock(ISimpleFolder::class);
 		$this->appData->expects($this->once())->method('getFolder')->with('awesomeapp')->willReturn($folder);
@@ -192,10 +189,10 @@ class JSCombinerTest extends \Test\TestCase {
 	public function testProcessCachedFileMemcache(): void {
 		$this->config
 			->expects($this->exactly(2))
-			->method('getValue')
+			->method('getSystemValueBool')
 			->willReturnMap([
-				['debug', '', false],
+				['debug', false],
-				['installed', '', true],
+				['installed', true],
 			]);
 		$folder = $this->createMock(ISimpleFolder::class);
 		$this->appData->expects($this->once())

tests/lib/Template/JSResourceLocatorTest.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -7,42 +9,37 @@
 
 namespace Test\Template;
 
-use OC\SystemConfig;
 use OC\Template\JSCombiner;
 use OC\Template\JSResourceLocator;
 use OCP\App\AppPathNotFoundException;
 use OCP\App\IAppManager;
 use OCP\Files\IAppData;
 use OCP\ICacheFactory;
+use OCP\IConfig;
 use OCP\IURLGenerator;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 
 class JSResourceLocatorTest extends \Test\TestCase {
-	/** @var IAppData|\PHPUnit\Framework\MockObject\MockObject */
+	private IAppData&MockObject $appData;
-	protected $appData;
+	private IURLGenerator&MockObject $urlGenerator;
-	/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
+	private IConfig&MockObject $config;
-	protected $urlGenerator;
+	private ICacheFactory&MockObject $cacheFactory;
-	/** @var SystemConfig|\PHPUnit\Framework\MockObject\MockObject */
+	private LoggerInterface&MockObject $logger;
-	protected $config;
+	private IAppManager&MockObject $appManager;
-	/** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */
-	protected $cacheFactory;
-	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
-	protected $logger;
-	/** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject */
-	protected $appManager;
 
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->appData = $this->createMock(IAppData::class);
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
-		$this->config = $this->createMock(SystemConfig::class);
+		$this->config = $this->createMock(IConfig::class);
 		$this->cacheFactory = $this->createMock(ICacheFactory::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->appManager = $this->createMock(IAppManager::class);
 	}
 
-	private function jsResourceLocator() {
+	private function jsResourceLocator(): JSResourceLocator {
 		$jsCombiner = new JSCombiner(
 			$this->appData,
 			$this->urlGenerator,
@@ -52,6 +49,7 @@ class JSResourceLocatorTest extends \Test\TestCase {
 		);
 		return new JSResourceLocator(
 			$this->logger,
+			$this->config,
 			$jsCombiner,
 			$this->appManager,
 		);
@@ -69,8 +67,8 @@ class JSResourceLocatorTest extends \Test\TestCase {
 		return rmdir($directory);
 	}
 
-	private function randomString() {
+	private function randomString(): string {
-		return sha1(uniqid(mt_rand(), true));
+		return sha1(random_bytes(10));
 	}
 
 	public function testFindWithAppPathSymlink(): void {

tests/lib/Template/ResourceLocatorTest.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
@@ -8,34 +10,30 @@
 
 namespace Test\Template;
 
-use OC\SystemConfig;
 use OC\Template\ResourceLocator;
 use OC\Template\ResourceNotFoundException;
+use OCP\IConfig;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 
 class ResourceLocatorTest extends \Test\TestCase {
-	/** @var \PHPUnit\Framework\MockObject\MockObject */
+	private LoggerInterface&MockObject $logger;
-	protected $logger;
+	private IConfig&MockObject $config;
 
 	protected function setUp(): void {
 		parent::setUp();
 		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->config = $this->createMock(IConfig::class);
 	}
 
-	/**
+	public function getResourceLocator(string $theme): ResourceLocator&MockObject {
-	 * @param string $theme
+		$this->config
-	 * @return \PHPUnit\Framework\MockObject\MockObject
-	 */
-	public function getResourceLocator($theme) {
-		$systemConfig = $this->createMock(SystemConfig::class);
-		$systemConfig
 			->expects($this->any())
-			->method('getValue')
+			->method('getSystemValueString')
 			->with('theme', '')
 			->willReturn($theme);
-		$this->overwriteService(SystemConfig::class, $systemConfig);
+		return $this->getMockForAbstractClass(ResourceLocator::class,
-		return $this->getMockForAbstractClass('OC\Template\ResourceLocator',
+			[$this->logger, $this->config],
-			[$this->logger],
 			'', true, true, true, []);
 	}
 
@@ -47,16 +45,10 @@ class ResourceLocatorTest extends \Test\TestCase {
 		$locator->expects($this->once())
 			->method('doFindTheme')
 			->with('foo');
-		/** @var ResourceLocator $locator */
 		$locator->find(['foo']);
 	}
 
 	public function testFindNotFound(): void {
-		$systemConfig = $this->createMock(SystemConfig::class);
-		$systemConfig->method('getValue')
-			->with('theme', '')
-			->willReturn('theme');
-		$this->overwriteService(SystemConfig::class, $systemConfig);
 		$locator = $this->getResourceLocator('theme',
 			['core' => 'map'], ['3rd' => 'party'], ['foo' => 'bar']);
 		$locator->expects($this->once())
@@ -70,13 +62,11 @@ class ResourceLocatorTest extends \Test\TestCase {
 		$this->logger->expects($this->exactly(2))
 			->method('debug')
 			->with($this->stringContains('map/foo'));
-		/** @var ResourceLocator $locator */
 		$locator->find(['foo']);
 	}
 
 	public function testAppendIfExist(): void {
 		$locator = $this->getResourceLocator('theme');
-		/** @var ResourceLocator $locator */
 		$method = new \ReflectionMethod($locator, 'appendIfExist');
 		$method->setAccessible(true);
 

tests/lib/TemplateLayoutTest.php

@@ -44,7 +44,13 @@ class TemplateLayoutTest extends \Test\TestCase {
 	}
 
 	#[\PHPUnit\Framework\Attributes\DataProvider('dataVersionHash')]
-	public function testVersionHash($path, $file, $installed, $debug, $expected): void {
+	public function testVersionHash(
+		string|false $path,
+		string|false $file,
+		bool $installed,
+		bool $debug,
+		string $expected,
+	): void {
 		$this->appManager->expects(self::any())
 			->method('getAppVersion')
 			->willReturnCallback(fn ($appId) => match ($appId) {
@@ -59,8 +65,8 @@ class TemplateLayoutTest extends \Test\TestCase {
 		$this->config->expects(self::atLeastOnce())
 			->method('getSystemValueBool')
 			->willReturnMap([
-				['installed', false, $installed],
+				['installed', $installed],
-				['debug', false, $debug],
+				['debug', $debug],
 			]);
 		$this->config->expects(self::any())
 			->method('getAppValue')