chore: Use IAppConfig instead of IConfig->getAppValue

Signed-off-by: Ferdinand Thiessen <[email protected]>

apps/theming/lib/Controller/ThemingController.php

@@ -22,6 +22,7 @@
 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;
@@ -40,34 +41,19 @@
 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;
-
 	public function __construct(
 		$appName,
 		IRequest $request,
-		IConfig $config,
-		ThemingDefaults $themingDefaults,
-		IL10N $l,
-		IURLGenerator $urlGenerator,
-		IAppManager $appManager,
-		ImageManager $imageManager,
-		ThemesService $themesService
+		private IConfig $config,
+		private ThemingDefaults $themingDefaults,
+		private IL10N $l10n,
+		private IURLGenerator $urlGenerator,
+		private IAppManager $appManager,
+		private ImageManager $imageManager,
+		private ThemesService $themesService,
+		private IAppConfig $appConfig,
 	) {
 		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;
 	}
 
 	/**
@@ -80,6 +66,7 @@ public function __construct(
 	public function updateStylesheet($setting, $value) {
 		$value = trim($value);
 		$error = null;
+		$saved = false;
 		switch ($setting) {
 			case 'name':
 				if (strlen($value) > 250) {
@@ -118,16 +105,25 @@ public function updateStylesheet($setting, $value) {
 			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;
 		}
@@ -140,7 +136,9 @@ public function updateStylesheet($setting, $value) {
 			], Http::STATUS_BAD_REQUEST);
 		}
 
-		$this->themingDefaults->set($setting, $value);
+		if (!$saved) {
+			$this->themingDefaults->set($setting, $value);
+		}
 
 		return new DataResponse([
 			'data' => [

apps/theming/lib/Migration/SeparatePrimaryColorAndBackground.php

@@ -10,6 +10,7 @@
 namespace OCA\Theming\Migration;
 
 use OCA\Theming\AppInfo\Application;
+use OCP\IAppConfig;
 use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\Migration\IOutput;
@@ -18,6 +19,7 @@ class SeparatePrimaryColorAndBackground implements \OCP\Migration\IRepairStep {
 
 	public function __construct(
 		private IConfig $config,
+		private IAppConfig $appConfig,
 		private IDBConnection $connection,
 	) {
 	}
@@ -27,25 +29,24 @@ public function getName() {
 	}
 
 	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 +60,6 @@ public function run(IOutput $output) {
 			$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);
 	}
 }

apps/theming/lib/ThemingDefaults.php

@@ -11,6 +11,7 @@
 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 @@ public function getColorBackground(): string {
 	 */
 	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 @@ public function getDefaultColorPrimary(): string {
 	 * 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 @@ public function getScssVariables() {
 		$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 @@ public function getDefaultTextColorPrimary() {
 	 * 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');
 	}
 }

apps/theming/tests/ThemingDefaultsTest.php

@@ -10,8 +10,8 @@
 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\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 @@ protected function setUp(): void {
 			->willReturn('');
 		$this->template = new ThemingDefaults(
 			$this->config,
+			$this->appConfig,
 			$this->l10n,
 			$this->userSession,
 			$this->urlGenerator,
@@ -398,63 +399,69 @@ public function testGetShortFooterInvalidPrivacy($invalidPrivacyUrl) {
 	}
 
 	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());
 	}
 
 	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,21 +472,24 @@ public function dataGetColorPrimary() {
 	/**
 	 * @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')
 			->willReturn($user);
 		$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 @@ public function testGetScssVariables() {
 				['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);