fix(settings): Sort all settings - incl declarative settings - by priorityfix/declarative-settings-priority

Previously declarative settings were sorted by priority but behind the "native" settings,
this is now fixed, meaning a declarative setting with higher priority than an `ISetting` will
be correctly rendered before that `ISetting` in the settings list.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

3 files changed, 47 insertions, 54 deletions

diff --git a/apps/settings/lib/Controller/AdminSettingsController.php b/apps/settings/lib/Controller/AdminSettingsController.php
index 25338f64326..2b731c5cdde 100644
--- a/apps/settings/lib/Controller/AdminSettingsController.php
+++ b/apps/settings/lib/Controller/AdminSettingsController.php
@@ -5,7 +5,6 @@
  */
 namespace OCA\Settings\Controller;
 
-use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\Attribute\NoAdminRequired;
 use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
@@ -16,7 +15,6 @@ use OCP\Group\ISubAdmin;
 use OCP\IGroupManager;
 use OCP\INavigationManager;
 use OCP\IRequest;
-use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Settings\IDeclarativeManager;
 use OCP\Settings\IManager as ISettingsManager;
@@ -49,27 +47,14 @@ class AdminSettingsController extends Controller {
 	/**
 	 * @NoSubAdminRequired
 	 * We are checking the permissions in the getSettings method. If there is no allowed
-	 * settings for the given section. The user will be gretted by an error message.
+	 * settings for the given section. The user will be greeted by an error message.
 	 */
 	#[NoAdminRequired]
 	#[NoCSRFRequired]
 	public function index(string $section): TemplateResponse {
-		return $this->getIndexResponse('admin', $section);
-	}
-
-	/**
-	 * @param string $section
-	 * @return array
-	 */
-	protected function getSettings($section) {
-		/** @var IUser $user */
-		$user = $this->userSession->getUser();
-		$settings = $this->settingsManager->getAllowedAdminSettings($section, $user);
-		$declarativeFormIDs = $this->declarativeSettingsManager->getFormIDs($user, 'admin', $section);
-		if (empty($settings) && empty($declarativeFormIDs)) {
-			throw new NotAdminException("Logged in user doesn't have permission to access these settings.");
-		}
-		$formatted = $this->formatSettings($settings);
-		return $formatted;
+		return $this->getIndexResponse(
+			'admin',
+			$section,
+		);
 	}
 }
diff --git a/apps/settings/lib/Controller/CommonSettingsTrait.php b/apps/settings/lib/Controller/CommonSettingsTrait.php
index dd307b67fab..26cbadd76a1 100644
--- a/apps/settings/lib/Controller/CommonSettingsTrait.php
+++ b/apps/settings/lib/Controller/CommonSettingsTrait.php
@@ -6,6 +6,8 @@
 
 namespace OCA\Settings\Controller;
 
+use InvalidArgumentException;
+use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException;
 use OCA\Settings\AppInfo\Application;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Services\IInitialState;
@@ -21,7 +23,8 @@ use OCP\Settings\ISettings;
 use OCP\Util;
 
 /**
- * @psalm-import-type DeclarativeSettingsFormField from IDeclarativeSettingsForm
+ * @psalm-import-type DeclarativeSettingsFormSchemaWithValues from IDeclarativeSettingsForm
+ * @psalm-import-type DeclarativeSettingsFormSchemaWithoutValues from IDeclarativeSettingsForm
  */
 trait CommonSettingsTrait {
 
@@ -106,16 +109,26 @@ trait CommonSettingsTrait {
 	}
 
 	/**
-	 * @param array<int, list<\OCP\Settings\ISettings>> $settings
+	 * @param list<\OCP\Settings\ISettings> $settings
+	 * @param list<DeclarativeSettingsFormSchemaWithValues> $declarativeSettings
 	 * @return array{content: string}
 	 */
-	private function formatSettings(array $settings): array {
+	private function formatSettings(array $settings, array $declarativeSettings): array {
+		$settings = array_merge($settings, $declarativeSettings);
+
+		usort($settings, function ($first, $second) {
+			$priorityOne = $first instanceof ISettings ? $first->getPriority() : $first['priority'];
+			$priorityTwo = $second instanceof ISettings ? $second->getPriority() : $second['priority'];
+			return $priorityOne - $priorityTwo;
+		});
+
 		$html = '';
-		foreach ($settings as $prioritizedSettings) {
-			foreach ($prioritizedSettings as $setting) {
-				/** @var ISettings $setting */
+		foreach ($settings as $setting) {
+			if ($setting instanceof ISettings) {
 				$form = $setting->getForm();
 				$html .= $form->renderAs('')->render();
+			} else {
+				$html .= '<div id="' . $setting['app'] . '_' . $setting['id'] . '"></div>';
 			}
 		}
 		return ['content' => $html];
@@ -125,34 +138,38 @@ trait CommonSettingsTrait {
 	 * @psalm-param 'admin'|'personal' $type
 	 */
 	private function getIndexResponse(string $type, string $section): TemplateResponse {
+		$user = $this->userSession->getUser();
+		assert($user !== null, 'No user logged in for settings');
+
+		$this->declarativeSettingsManager->loadSchemas();
+		$declarativeSettings = $this->declarativeSettingsManager->getFormsWithValues($user, $type, $section);
+
 		if ($type === 'personal') {
+			$settings = array_values($this->settingsManager->getPersonalSettings($section));
 			if ($section === 'theming') {
 				$this->navigationManager->setActiveEntry('accessibility_settings');
 			} else {
 				$this->navigationManager->setActiveEntry('settings');
 			}
 		} elseif ($type === 'admin') {
+			$settings = array_values($this->settingsManager->getAllowedAdminSettings($section, $user));
+			if (empty($settings) && empty($declarativeSettings)) {
+				throw new NotAdminException('Logged in user does not have permission to access these settings.');
+			}
 			$this->navigationManager->setActiveEntry('admin_settings');
+		} else {
+			throw new InvalidArgumentException('$type must be either "admin" or "personal"');
 		}
 
-		$this->declarativeSettingsManager->loadSchemas();
-
-		$templateParams = [];
-		$templateParams = array_merge($templateParams, $this->getNavigationParameters($type, $section));
-		$templateParams = array_merge($templateParams, $this->getSettings($section));
-
-		/** @psalm-suppress PossiblyNullArgument */
-		$declarativeFormIDs = $this->declarativeSettingsManager->getFormIDs($this->userSession->getUser(), $type, $section);
-		if (!empty($declarativeFormIDs)) {
-			foreach ($declarativeFormIDs as $app => $ids) {
-				/** @psalm-suppress PossiblyUndefinedArrayOffset */
-				$templateParams['content'] .= join(array_map(fn (string $id) => '<div id="' . $app . '_' . $id . '"></div>', $ids));
-			}
+		if (!empty($declarativeSettings)) {
 			Util::addScript(Application::APP_ID, 'declarative-settings-forms');
-			/** @psalm-suppress PossiblyNullArgument */
-			$this->initialState->provideInitialState('declarative-settings-forms', $this->declarativeSettingsManager->getFormsWithValues($this->userSession->getUser(), $type, $section));
+			$this->initialState->provideInitialState('declarative-settings-forms', $declarativeSettings);
 		}
 
+		$settings = array_merge(...$settings);
+		$templateParams = $this->formatSettings($settings, $declarativeSettings);
+		$templateParams = array_merge($templateParams, $this->getNavigationParameters($type, $section));
+
 		$activeSection = $this->settingsManager->getSection($type, $section);
 		if ($activeSection) {
 			$templateParams['pageTitle'] = $activeSection->getName();
@@ -162,6 +179,4 @@ trait CommonSettingsTrait {
 
 		return new TemplateResponse('settings', 'settings/frame', $templateParams);
 	}
-
-	abstract protected function getSettings($section);
 }
diff --git a/apps/settings/lib/Controller/PersonalSettingsController.php b/apps/settings/lib/Controller/PersonalSettingsController.php
index 1a31a20eb04..0a87181c7d7 100644
--- a/apps/settings/lib/Controller/PersonalSettingsController.php
+++ b/apps/settings/lib/Controller/PersonalSettingsController.php
@@ -50,16 +50,9 @@ class PersonalSettingsController extends Controller {
 	#[NoAdminRequired]
 	#[NoCSRFRequired]
 	public function index(string $section): TemplateResponse {
-		return $this->getIndexResponse('personal', $section);
-	}
-
-	/**
-	 * @param string $section
-	 * @return array
-	 */
-	protected function getSettings($section) {
-		$settings = $this->settingsManager->getPersonalSettings($section);
-		$formatted = $this->formatSettings($settings);
-		return $formatted;
+		return $this->getIndexResponse(
+			'personal',
+			$section,
+		);
 	}
 }