chore: Use IAppConfig instead of IConfig->getAppValue

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
2026-05-28 04:32:30 -04:00 · 2024-09-01 13:16:49 +02:00 · 2024-09-01 13:16:49 +02:00 · c7d9068be9
commit c7d9068be9
parent ee1585d809
6 changed files with 109 additions and 99 deletions
--- a/apps/theming/lib/Controller/ThemingController.php
+++ b/apps/theming/lib/Controller/ThemingController.php
@ -22,6 +22,7 @@ use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\FileDisplayResponse;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\NotFoundResponse;
+use OCP\AppFramework\Services\IAppConfig;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
 use OCP\IConfig;
@ -41,37 +42,20 @@ use ScssPhp\ScssPhp\Compiler;
 class ThemingController extends Controller {
 	public const VALID_UPLOAD_KEYS = ['header', 'logo', 'logoheader', 'background', 'favicon'];

-	private ThemingDefaults $themingDefaults;
-	private IL10N $l10n;
-	private IConfig $config;
-	private IURLGenerator $urlGenerator;
-	private IAppManager $appManager;
-	private ImageManager $imageManager;
-	private ThemesService $themesService;
-	private INavigationManager $navigationManager;
-
 	public function __construct(
 		$appName,
 		IRequest $request,
-		IConfig $config,
-		ThemingDefaults $themingDefaults,
-		IL10N $l,
-		IURLGenerator $urlGenerator,
-		IAppManager $appManager,
-		ImageManager $imageManager,
-		ThemesService $themesService,
-		INavigationManager $navigationManager,
+		private IConfig $config,
+		private IAppConfig $appConfig,
+		private ThemingDefaults $themingDefaults,
+		private IL10N $l10n,
+		private IURLGenerator $urlGenerator,
+		private IAppManager $appManager,
+		private ImageManager $imageManager,
+		private ThemesService $themesService,
+		private INavigationManager $navigationManager,
 	) {
 		parent::__construct($appName, $request);
-
-		$this->themingDefaults = $themingDefaults;
-		$this->l10n = $l;
-		$this->config = $config;
-		$this->urlGenerator = $urlGenerator;
-		$this->appManager = $appManager;
-		$this->imageManager = $imageManager;
-		$this->themesService = $themesService;
-		$this->navigationManager = $navigationManager;
 	}

 	/**
@ -84,6 +68,7 @@ class ThemingController extends Controller {
 	public function updateStylesheet($setting, $value) {
 		$value = trim($value);
 		$error = null;
+		$saved = false;
 		switch ($setting) {
 			case 'name':
 				if (strlen($value) > 250) {
@ -122,16 +107,25 @@ class ThemingController extends Controller {
 			case 'primary_color':
 				if (!preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $value)) {
 					$error = $this->l10n->t('The given color is invalid');
+				} else {
+					$this->appConfig->setAppValueString('primary_color', $value);
+					$saved = true;
 				}
 				break;
 			case 'background_color':
 				if (!preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $value)) {
 					$error = $this->l10n->t('The given color is invalid');
+				} else {
+					$this->appConfig->setAppValueString('background_color', $value);
+					$saved = true;
 				}
 				break;
 			case 'disable-user-theming':
-				if ($value !== 'yes' && $value !== 'no') {
+				if (!in_array($value, ['yes', 'true', 'no', 'false'])) {
 					$error = $this->l10n->t('Disable-user-theming should be true or false');
+				} else {
+					$this->appConfig->setAppValueBool('disable-user-theming', $value === 'yes' || $value === 'true');
+					$saved = true;
 				}
 				break;
 		}
@ -144,7 +138,9 @@ class ThemingController extends Controller {
 			], Http::STATUS_BAD_REQUEST);
 		}

-		$this->themingDefaults->set($setting, $value);
+		if (!$saved) {
+			$this->themingDefaults->set($setting, $value);
+		}

 		return new DataResponse([
 			'data' => [
--- a/apps/theming/lib/Migration/SeparatePrimaryColorAndBackground.php
+++ b/apps/theming/lib/Migration/SeparatePrimaryColorAndBackground.php
@ -10,14 +10,14 @@ declare(strict_types=1);
 namespace OCA\Theming\Migration;

 use OCA\Theming\AppInfo\Application;
-use OCP\IConfig;
+use OCP\IAppConfig;
 use OCP\IDBConnection;
 use OCP\Migration\IOutput;

 class SeparatePrimaryColorAndBackground implements \OCP\Migration\IRepairStep {

 	public function __construct(
-		private IConfig $config,
+		private IAppConfig $appConfig,
 		private IDBConnection $connection,
 	) {
 	}
@ -27,25 +27,24 @@ class SeparatePrimaryColorAndBackground implements \OCP\Migration\IRepairStep {
 	}

 	public function run(IOutput $output) {
-		$defaultColor = $this->config->getAppValue(Application::APP_ID, 'color', '');
+		$defaultColor = $this->appConfig->getValueString(Application::APP_ID, 'color', '');
 		if ($defaultColor !== '') {
 			// Restore legacy value into new field
-			$this->config->setAppValue(Application::APP_ID, 'background_color', $defaultColor);
-			$this->config->setAppValue(Application::APP_ID, 'primary_color', $defaultColor);
+			$this->appConfig->setValueString(Application::APP_ID, 'background_color', $defaultColor);
+			$this->appConfig->setValueString(Application::APP_ID, 'primary_color', $defaultColor);
 			// Delete legacy field
-			$this->config->deleteAppValue(Application::APP_ID, 'color');
+			$this->appConfig->deleteKey(Application::APP_ID, 'color');
 			// give some feedback
 			$output->info('Global primary color restored');
 		}

 		// This can only be executed once because `background_color` is again used with Nextcloud 30,
 		// so this part only works when updating -> Nextcloud 29 -> 30
-		$migrated = $this->config->getAppValue('theming', 'nextcloud_30_migration', 'false') === 'true';
-		if ($migrated) {
+		if ($this->appConfig->getValueBool('theming', 'nextcloud_30_migration')) {
 			return;
 		}

-		$userThemingEnabled = $this->config->getAppValue('theming', 'disable-user-theming', 'no') !== 'yes';
+		$userThemingEnabled = $this->appConfig->getValueBool('theming', 'disable-user-theming');
 		if ($userThemingEnabled) {
 			$output->info('Restoring user primary color');
 			// For performance let the DB handle this
@ -59,6 +58,6 @@ class SeparatePrimaryColorAndBackground implements \OCP\Migration\IRepairStep {
 			$qb->executeStatement();
 			$output->info('Primary color of users restored');
 		}
-		$this->config->setAppValue('theming', 'nextcloud_30_migration', 'true');
+		$this->appConfig->setValueBool('theming', 'nextcloud_30_migration', true);
 	}
 }
--- a/apps/theming/lib/ThemingDefaults.php
+++ b/apps/theming/lib/ThemingDefaults.php
@ -11,6 +11,7 @@ use OCP\App\AppPathNotFoundException;
 use OCP\App\IAppManager;
 use OCP\Files\NotFoundException;
 use OCP\Files\SimpleFS\ISimpleFile;
+use OCP\IAppConfig;
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IL10N;
@ -39,6 +40,7 @@ class ThemingDefaults extends \OC_Defaults {
 	 */
 	public function __construct(
 		private IConfig $config,
+		private IAppConfig $appConfig,
 		private IL10N $l,
 		private IUserSession $userSession,
 		private IURLGenerator $urlGenerator,
@ -221,7 +223,7 @@ class ThemingDefaults extends \OC_Defaults {
 	 */
 	public function getDefaultColorPrimary(): string {
 		// try admin color
-		$defaultColor = $this->config->getAppValue(Application::APP_ID, 'primary_color', '');
+		$defaultColor = $this->appConfig->getValueString(Application::APP_ID, 'primary_color', '');
 		if (preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $defaultColor)) {
 			return $defaultColor;
 		}
@ -234,7 +236,7 @@ class ThemingDefaults extends \OC_Defaults {
 	 * Default background color only taking admin setting into account
 	 */
 	public function getDefaultColorBackground(): string {
-		$defaultColor = $this->config->getAppValue(Application::APP_ID, 'background_color', '');
+		$defaultColor = $this->appConfig->getValueString(Application::APP_ID, 'background_color');
 		if (preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $defaultColor)) {
 			return $defaultColor;
 		}
@ -344,7 +346,7 @@ class ThemingDefaults extends \OC_Defaults {
 		$variables['image-login-background'] = "url('".$this->imageManager->getImageUrl('background')."')";
 		$variables['image-login-plain'] = 'false';

-		if ($this->config->getAppValue('theming', 'primary_color', '') !== '') {
+		if ($this->appConfig->getValueString(Application::APP_ID, 'primary_color', '') !== '') {
 			$variables['color-primary'] = $this->getColorPrimary();
 			$variables['color-primary-text'] = $this->getTextColorPrimary();
 			$variables['color-primary-element'] = $this->util->elementColor($this->getColorPrimary());
@ -520,6 +522,6 @@ class ThemingDefaults extends \OC_Defaults {
 	 * Has the admin disabled user customization
 	 */
 	public function isUserThemingDisabled(): bool {
-		return $this->config->getAppValue('theming', 'disable-user-theming', 'no') === 'yes';
+		return $this->appConfig->getValueBool(Application::APP_ID, 'disable-user-theming');
 	}
 }
--- a/apps/theming/tests/Controller/ThemingControllerTest.php
+++ b/apps/theming/tests/Controller/ThemingControllerTest.php
@ -13,6 +13,7 @@ use OCA\Theming\ThemingDefaults;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
+use OCP\AppFramework\Services\IAppConfig;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\NotFoundException;
 use OCP\Files\SimpleFS\ISimpleFile;
@ -25,30 +26,24 @@ use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;

 class ThemingControllerTest extends TestCase {
-	/** @var IRequest|MockObject */
-	private $request;
-	/** @var IConfig|MockObject */
-	private $config;
-	/** @var ThemingDefaults|MockObject */
-	private $themingDefaults;
-	/** @var IL10N|MockObject */
-	private $l10n;
-	/** @var ThemingController */
-	private $themingController;
-	/** @var IAppManager|MockObject */
-	private $appManager;
-	/** @var ImageManager|MockObject */
-	private $imageManager;
-	/** @var IURLGenerator|MockObject */
-	private $urlGenerator;
-	/** @var ThemesService|MockObject */
-	private $themesService;
-	/** @var INavigationManager|MockObject */
-	private $navigationManager;
+
+	private IRequest&MockObject $request;
+	private IConfig&MockObject $config;
+	private IAppConfig&MockObject $appConfig;
+	private ThemingDefaults&MockObject $themingDefaults;
+	private IL10N&MockObject $l10n;
+	private IAppManager&MockObject $appManager;
+	private ImageManager&MockObject $imageManager;
+	private IURLGenerator&MockObject $urlGenerator;
+	private ThemesService&MockObject $themesService;
+	private INavigationManager&MockObject $navigationManager;
+
+	private ThemingController $themingController;

 	protected function setUp(): void {
 		$this->request = $this->createMock(IRequest::class);
 		$this->config = $this->createMock(IConfig::class);
+		$this->appConfig = $this->createMock(IAppConfig::class);
 		$this->themingDefaults = $this->createMock(ThemingDefaults::class);
 		$this->l10n = $this->createMock(L10N::class);
 		$this->appManager = $this->createMock(IAppManager::class);
@ -68,6 +63,7 @@ class ThemingControllerTest extends TestCase {
 			'theming',
 			$this->request,
 			$this->config,
+			$this->appConfig,
 			$this->themingDefaults,
 			$this->l10n,
 			$this->urlGenerator,
--- a/apps/theming/tests/ThemingDefaultsTest.php
+++ b/apps/theming/tests/ThemingDefaultsTest.php
@ -10,8 +10,8 @@ use OCA\Theming\Service\BackgroundService;
 use OCA\Theming\ThemingDefaults;
 use OCA\Theming\Util;
 use OCP\App\IAppManager;
-use OCP\Files\IAppData;
 use OCP\Files\NotFoundException;
+use OCP\IAppConfig;
 use OCP\ICache;
 use OCP\ICacheFactory;
 use OCP\IConfig;
@ -20,21 +20,20 @@ use OCP\INavigationManager;
 use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserSession;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;

 class ThemingDefaultsTest extends TestCase {
-	/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
-	private $config;
+	private IAppConfig&MockObject $appConfig;
+	private IConfig&MockObject $config;
+	private \OC_Defaults $defaults;
+
 	/** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */
 	private $l10n;
 	/** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */
 	private $userSession;
 	/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
 	private $urlGenerator;
-	/** @var \OC_Defaults|\PHPUnit\Framework\MockObject\MockObject */
-	private $defaults;
-	/** @var IAppData|\PHPUnit\Framework\MockObject\MockObject */
-	private $appData;
 	/** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */
 	private $cacheFactory;
 	/** @var ThemingDefaults */
@ -54,6 +53,7 @@ class ThemingDefaultsTest extends TestCase {

 	protected function setUp(): void {
 		parent::setUp();
+		$this->appConfig = $this->createMock(IAppConfig::class);
 		$this->config = $this->createMock(IConfig::class);
 		$this->l10n = $this->createMock(IL10N::class);
 		$this->userSession = $this->createMock(IUserSession::class);
@ -72,6 +72,7 @@ class ThemingDefaultsTest extends TestCase {
 			->willReturn('');
 		$this->template = new ThemingDefaults(
 			$this->config,
+			$this->appConfig,
 			$this->l10n,
 			$this->userSession,
 			$this->urlGenerator,
@ -398,25 +399,31 @@ class ThemingDefaultsTest extends TestCase {
 	}

 	public function testGetColorPrimaryWithDefault() {
-		$this->config
-			->expects($this->exactly(2))
-			->method('getAppValue')
-			->willReturnMap([
-				['theming', 'disable-user-theming', 'no', 'no'],
-				['theming', 'primary_color', '', $this->defaults->getColorPrimary()],
-			]);
+		$this->appConfig
+			->expects(self::once())
+			->method('getValueBool')
+			->with('theming', 'disable-user-theming')
+			->willReturn(false);
+		$this->appConfig
+			->expects(self::once())
+			->method('getValueString')
+			->with('theming', 'primary_color', '')
+			->willReturn($this->defaults->getColorPrimary());

 		$this->assertEquals($this->defaults->getColorPrimary(), $this->template->getColorPrimary());
 	}

 	public function testGetColorPrimaryWithCustom() {
-		$this->config
-			->expects($this->exactly(2))
-			->method('getAppValue')
-			->willReturnMap([
-				['theming', 'disable-user-theming', 'no', 'no'],
-				['theming', 'primary_color', '', '#fff'],
-			]);
+		$this->appConfig
+			->expects(self::once())
+			->method('getValueBool')
+			->with('theming', 'disable-user-theming')
+			->willReturn(false);
+		$this->appConfig
+			->expects(self::once())
+			->method('getValueString')
+			->with('theming', 'primary_color', '')
+			->willReturn('#fff');

 		$this->assertEquals('#fff', $this->template->getColorPrimary());
 	}
@ -424,37 +431,37 @@ class ThemingDefaultsTest extends TestCase {
 	public function dataGetColorPrimary() {
 		return [
 			'with fallback default' => [
-				'disableTheming' => 'no',
+				'disableTheming' => false,
 				'primaryColor' => '',
 				'userPrimaryColor' => '',
 				'expected' => BackgroundService::DEFAULT_COLOR,
 			],
 			'with custom admin primary' => [
-				'disableTheming' => 'no',
+				'disableTheming' => false,
 				'primaryColor' => '#aaa',
 				'userPrimaryColor' => '',
 				'expected' => '#aaa',
 			],
 			'with custom invalid admin primary' => [
-				'disableTheming' => 'no',
+				'disableTheming' => false,
 				'primaryColor' => 'invalid',
 				'userPrimaryColor' => '',
 				'expected' => BackgroundService::DEFAULT_COLOR,
 			],
 			'with custom invalid user primary' => [
-				'disableTheming' => 'no',
+				'disableTheming' => false,
 				'primaryColor' => '',
 				'userPrimaryColor' => 'invalid-name',
 				'expected' => BackgroundService::DEFAULT_COLOR,
 			],
 			'with custom user primary' => [
-				'disableTheming' => 'no',
+				'disableTheming' => false,
 				'primaryColor' => '',
 				'userPrimaryColor' => '#bbb',
 				'expected' => '#bbb',
 			],
 			'with disabled user theming primary' => [
-				'disableTheming' => 'yes',
+				'disableTheming' => true,
 				'primaryColor' => '#aaa',
 				'userPrimaryColor' => '#bbb',
 				'expected' => '#aaa',
@ -465,7 +472,7 @@ class ThemingDefaultsTest extends TestCase {
 	/**
 	 * @dataProvider dataGetColorPrimary
 	 */
-	public function testGetColorPrimary(string $disableTheming, string $primaryColor, string $userPrimaryColor, string $expected) {
+	public function testGetColorPrimary(bool $disableTheming, string $primaryColor, string $userPrimaryColor, string $expected) {
 		$user = $this->createMock(IUser::class);
 		$this->userSession->expects($this->any())
 			->method('getUser')
@ -473,13 +480,16 @@ class ThemingDefaultsTest extends TestCase {
 		$user->expects($this->any())
 			->method('getUID')
 			->willReturn('user');
-		$this->config
-			->expects($this->any())
-			->method('getAppValue')
-			->willReturnMap([
-				['theming', 'disable-user-theming', 'no', $disableTheming],
-				['theming', 'primary_color', '', $primaryColor],
-			]);
+		$this->appConfig
+			->expects(self::any())
+			->method('getValueBool')
+			->with('theming', 'disable-user-theming')
+			->willReturn($disableTheming);
+		$this->appConfig
+			->expects(self::any())
+			->method('getValueString')
+			->with('theming', 'primary_color', '')
+			->willReturn($primaryColor);
 		$this->config
 			->expects($this->any())
 			->method('getUserValue')
@ -699,8 +709,14 @@ class ThemingDefaultsTest extends TestCase {
 				['theming', 'backgroundMime', '', 'jpeg'],
 				['theming', 'logoheaderMime', '', 'jpeg'],
 				['theming', 'faviconMime', '', 'jpeg'],
-				['theming', 'primary_color', '', $this->defaults->getColorPrimary()],
-				['theming', 'primary_color', $this->defaults->getColorPrimary(), $this->defaults->getColorPrimary()],
+			]);
+		
+		$this->appConfig
+			->expects(self::atLeastOnce())
+			->method('getValueString')
+			->willReturnMap([
+				['theming', 'primary_color', '', false, $this->defaults->getColorPrimary()],
+				['theming', 'primary_color', $this->defaults->getColorPrimary(), false, $this->defaults->getColorPrimary()],
 			]);

 		$this->util->expects($this->any())->method('invertTextColor')->with($this->defaults->getColorPrimary())->willReturn(false);
--- a/lib/private/Server.php
+++ b/lib/private/Server.php
@ -1167,6 +1167,7 @@ class Server extends ServerContainer implements IServerContainer {
 				);
 				return new ThemingDefaults(
 					$c->get(\OCP\IConfig::class),
+					$c->get(\OCP\IAppConfig::class),
 					$c->getL10N('theming'),
 					$c->get(IUserSession::class),
 					$c->get(IURLGenerator::class),