Merge pull request #11398 from nextcloud/refactor/settings-manager-complexity

Reduce settings manager complexity by loading sections via DI

3 files changed, 62 insertions, 150 deletions

diff --git a/lib/private/Server.php b/lib/private/Server.php
index 5962012604a..c79f16567d8 100644
--- a/lib/private/Server.php
+++ b/lib/private/Server.php
@@ -1081,18 +1081,9 @@ class Server extends ServerContainer implements IServerContainer {
 		$this->registerService('SettingsManager', function (Server $c) {
 			$manager = new \OC\Settings\Manager(
 				$c->getLogger(),
-				$c->getDatabaseConnection(),
 				$c->getL10N('lib'),
-				$c->getConfig(),
-				$c->getEncryptionManager(),
-				$c->getUserManager(),
-				$c->getLockingProvider(),
-				$c->getRequest(),
 				$c->getURLGenerator(),
-				$c->query(AccountManager::class),
-				$c->getGroupManager(),
-				$c->getL10NFactory(),
-				$c->getAppManager()
+				$c
 			);
 			return $manager;
 		});
diff --git a/lib/private/Settings/Manager.php b/lib/private/Settings/Manager.php
index f1baf35b7ab..48883582ef4 100644
--- a/lib/private/Settings/Manager.php
+++ b/lib/private/Settings/Manager.php
@@ -29,96 +29,39 @@
 
 namespace OC\Settings;
 
-use OC\Accounts\AccountManager;
-use OCP\App\IAppManager;
 use OCP\AppFramework\QueryException;
-use OCP\Encryption\IManager as EncryptionManager;
-use OCP\IConfig;
-use OCP\IDBConnection;
-use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\ILogger;
-use OCP\IRequest;
+use OCP\IServerContainer;
 use OCP\IURLGenerator;
-use OCP\IUserManager;
-use OCP\L10N\IFactory;
-use OCP\Lock\ILockingProvider;
 use OCP\Settings\ISettings;
 use OCP\Settings\IManager;
 use OCP\Settings\ISection;
-use OCP\Util;
 
 class Manager implements IManager {
+
 	/** @var ILogger */
 	private $log;
-	/** @var IDBConnection */
-	private $dbc;
+
 	/** @var IL10N */
 	private $l;
-	/** @var IConfig */
-	private $config;
-	/** @var EncryptionManager */
-	private $encryptionManager;
-	/** @var IUserManager */
-	private $userManager;
-	/** @var ILockingProvider */
-	private $lockingProvider;
-	/** @var IRequest */
-	private $request;
+
 	/** @var IURLGenerator */
 	private $url;
-	/** @var AccountManager */
-	private $accountManager;
-	/** @var IGroupManager */
-	private $groupManager;
-	/** @var IFactory */
-	private $l10nFactory;
-	/** @var IAppManager */
-	private $appManager;
 
-	/**
-	 * @param ILogger $log
-	 * @param IDBConnection $dbc
-	 * @param IL10N $l
-	 * @param IConfig $config
-	 * @param EncryptionManager $encryptionManager
-	 * @param IUserManager $userManager
-	 * @param ILockingProvider $lockingProvider
-	 * @param IRequest $request
-	 * @param IURLGenerator $url
-	 * @param AccountManager $accountManager
-	 * @param IGroupManager $groupManager
-	 * @param IFactory $l10nFactory
-	 * @param IAppManager $appManager
-	 */
+	/** @var IServerContainer */
+	private $container;
+
 	public function __construct(
 		ILogger $log,
-		IDBConnection $dbc,
-		IL10N $l,
-		IConfig $config,
-		EncryptionManager $encryptionManager,
-		IUserManager $userManager,
-		ILockingProvider $lockingProvider,
-		IRequest $request,
+		IL10N $l10n,
 		IURLGenerator $url,
-		AccountManager $accountManager,
-		IGroupManager $groupManager,
-		IFactory $l10nFactory,
-		IAppManager $appManager
+		IServerContainer $container
 	) {
 		$this->log = $log;
-		$this->dbc = $dbc;
-		$this->l = $l;
-		$this->config = $config;
-		$this->encryptionManager = $encryptionManager;
-		$this->userManager = $userManager;
-		$this->lockingProvider = $lockingProvider;
-		$this->request = $request;
+		$this->l = $l10n;
 		$this->url = $url;
-		$this->accountManager = $accountManager;
-		$this->groupManager = $groupManager;
-		$this->l10nFactory = $l10nFactory;
-		$this->appManager = $appManager;
+		$this->container = $container;
 	}
 
 	/** @var array */
@@ -130,6 +73,7 @@ class Manager implements IManager {
 	/**
 	 * @param string $type 'admin' or 'personal'
 	 * @param string $section Class must implement OCP\Settings\ISection
+	 *
 	 * @return void
 	 */
 	public function registerSection(string $type, string $section) {
@@ -142,6 +86,7 @@ class Manager implements IManager {
 
 	/**
 	 * @param string $type 'admin' or 'personal'
+	 *
 	 * @return ISection[]
 	 */
 	protected function getSections(string $type): array {
@@ -191,6 +136,7 @@ class Manager implements IManager {
 	/**
 	 * @param string $type 'admin' or 'personal'
 	 * @param string $setting Class must implement OCP\Settings\ISetting
+	 *
 	 * @return void
 	 */
 	public function registerSetting(string $type, string $setting) {
@@ -200,6 +146,7 @@ class Manager implements IManager {
 	/**
 	 * @param string $type 'admin' or 'personal'
 	 * @param string $section
+	 *
 	 * @return ISettings[]
 	 */
 	protected function getSettings(string $type, string $section): array {
@@ -267,6 +214,7 @@ class Manager implements IManager {
 
 	/**
 	 * @param string $section
+	 *
 	 * @return ISection[]
 	 */
 	private function getBuiltInAdminSettings($section): array {
@@ -274,24 +222,24 @@ class Manager implements IManager {
 
 		if ($section === 'overview') {
 			/** @var ISettings $form */
-			$form = new Admin\Overview($this->config);
+			$form = $this->container->query(Admin\Overview::class);
 			$forms[$form->getPriority()] = [$form];
 		}
 		if ($section === 'server') {
 			/** @var ISettings $form */
-			$form = new Admin\Server($this->dbc, $this->request, $this->config, $this->lockingProvider, $this->l);
+			$form = $this->container->query(Admin\Server::class);
 			$forms[$form->getPriority()] = [$form];
-			$form = new Admin\Mail($this->config);
+			$form = $this->container->query(Admin\Mail::class);
 			$forms[$form->getPriority()] = [$form];
 		}
 		if ($section === 'security') {
 			/** @var ISettings $form */
-			$form = new Admin\Encryption($this->encryptionManager, $this->userManager);
+			$form = $this->container->query(Admin\Encryption::class);
 			$forms[$form->getPriority()] = [$form];
 		}
 		if ($section === 'sharing') {
 			/** @var ISettings $form */
-			$form = new Admin\Sharing($this->config, $this->l);
+			$form = $this->container->query(Admin\Sharing::class);
 			$forms[$form->getPriority()] = [$form];
 		}
 
@@ -300,6 +248,7 @@ class Manager implements IManager {
 
 	/**
 	 * @param string $section
+	 *
 	 * @return ISection[]
 	 */
 	private function getBuiltInPersonalSettings($section): array {
@@ -307,27 +256,19 @@ class Manager implements IManager {
 
 		if ($section === 'personal-info') {
 			/** @var ISettings $form */
-			$form = new Personal\PersonalInfo(
-				$this->config,
-				$this->userManager,
-				$this->groupManager,
-				$this->accountManager,
-				$this->appManager,
-				$this->l10nFactory,
-				$this->l
-			);
+			$form = $this->container->query(Personal\PersonalInfo::class);
 			$forms[$form->getPriority()] = [$form];
 			$form = new Personal\ServerDevNotice();
 			$forms[$form->getPriority()] = [$form];
 		}
-		if($section === 'security') {
+		if ($section === 'security') {
 			/** @var ISettings $form */
-			$form = new Personal\Security($this->userManager);
+			$form = $this->container->query(Personal\Security::class);
 			$forms[$form->getPriority()] = [$form];
 		}
 		if ($section === 'additional') {
 			/** @var ISettings $form */
-			$form = new Personal\Additional();
+			$form = $this->container->query(Personal\Additional::class);
 			$forms[$form->getPriority()] = [$form];
 		}
 
@@ -363,7 +304,7 @@ class Manager implements IManager {
 		];
 
 		$legacyForms = \OC_App::getForms('personal');
-		if(!empty($legacyForms) && $this->hasLegacyPersonalSettingsToRender($legacyForms)) {
+		if (!empty($legacyForms) && $this->hasLegacyPersonalSettingsToRender($legacyForms)) {
 			$sections[98] = [new Section('additional', $this->l->t('Additional settings'), 0, $this->url->imagePath('core', 'actions/settings-dark.svg'))];
 		}
 
@@ -385,11 +326,12 @@ class Manager implements IManager {
 
 	/**
 	 * @param string[] $forms
+	 *
 	 * @return bool
 	 */
 	private function hasLegacyPersonalSettingsToRender(array $forms): bool {
 		foreach ($forms as $form) {
-			if(trim($form) !== '') {
+			if (trim($form) !== '') {
 				return true;
 			}
 		}
diff --git a/tests/lib/Settings/ManagerTest.php b/tests/lib/Settings/ManagerTest.php
index 6f9af39d591..b82fb5bc3ca 100644
--- a/tests/lib/Settings/ManagerTest.php
+++ b/tests/lib/Settings/ManagerTest.php
@@ -23,87 +23,44 @@
 
 namespace Tests\Settings;
 
-use OC\Accounts\AccountManager;
 use OC\Settings\Admin\Sharing;
 use OC\Settings\Manager;
 use OC\Settings\Mapper;
 use OC\Settings\Personal\Security;
 use OC\Settings\Section;
-use OCP\App\IAppManager;
-use OCP\Encryption\IManager;
-use OCP\IConfig;
 use OCP\IDBConnection;
-use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\ILogger;
-use OCP\IRequest;
+use OCP\IServerContainer;
 use OCP\IURLGenerator;
-use OCP\IUserManager;
-use OCP\L10N\IFactory;
-use OCP\Lock\ILockingProvider;
 use Test\TestCase;
 
 class ManagerTest extends TestCase {
+
 	/** @var Manager|\PHPUnit_Framework_MockObject_MockObject */
 	private $manager;
 	/** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */
 	private $logger;
 	/** @var IDBConnection|\PHPUnit_Framework_MockObject_MockObject */
-	private $dbConnection;
-	/** @var IL10N|\PHPUnit_Framework_MockObject_MockObject */
 	private $l10n;
-	/** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */
-	private $config;
-	/** @var IManager|\PHPUnit_Framework_MockObject_MockObject */
-	private $encryptionManager;
-	/** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */
-	private $userManager;
-	/** @var ILockingProvider|\PHPUnit_Framework_MockObject_MockObject */
-	private $lockingProvider;
-	/** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */
-	private $request;
 	/** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */
 	private $url;
-	/** @var AccountManager|\PHPUnit_Framework_MockObject_MockObject */
-	private $accountManager;
-	/** @var  IGroupManager|\PHPUnit_Framework_MockObject_MockObject */
-	private $groupManager;
-	/** @var  IFactory|\PHPUnit_Framework_MockObject_MockObject */
-	private $l10nFactory;
-	/** @var IAppManager */
-	private $appManager;
+	/** @var IServerContainer|\PHPUnit_Framework_MockObject_MockObject */
+	private $container;
 
 	public function setUp() {
 		parent::setUp();
 
 		$this->logger = $this->createMock(ILogger::class);
-		$this->dbConnection = $this->createMock(IDBConnection::class);
 		$this->l10n = $this->createMock(IL10N::class);
-		$this->config = $this->createMock(IConfig::class);
-		$this->encryptionManager = $this->createMock(IManager::class);
-		$this->userManager = $this->createMock(IUserManager::class);
-		$this->lockingProvider = $this->createMock(ILockingProvider::class);
-		$this->request = $this->createMock(IRequest::class);
 		$this->url = $this->createMock(IURLGenerator::class);
-		$this->accountManager = $this->createMock(AccountManager::class);
-		$this->groupManager = $this->createMock(IGroupManager::class);
-		$this->l10nFactory = $this->createMock(IFactory::class);
-		$this->appManager = $this->createMock(IAppManager::class);
+		$this->container = $this->createMock(IServerContainer::class);
 
 		$this->manager = new Manager(
 			$this->logger,
-			$this->dbConnection,
 			$this->l10n,
-			$this->config,
-			$this->encryptionManager,
-			$this->userManager,
-			$this->lockingProvider,
-			$this->request,
 			$this->url,
-			$this->accountManager,
-			$this->groupManager,
-			$this->l10nFactory,
-			$this->appManager
+			$this->container
 		);
 	}
 
@@ -210,15 +167,37 @@ class ManagerTest extends TestCase {
 	}
 
 	public function testGetAdminSettings() {
+		$section = $this->createMock(Sharing::class);
+		$section->expects($this->once())
+			->method('getPriority')
+			->willReturn(13);
+		$this->container->expects($this->once())
+			->method('query')
+			->with(Sharing::class)
+			->willReturn($section);
+
+		$settings = $this->manager->getAdminSettings('sharing');
+
 		$this->assertEquals([
-			0 => [new Sharing($this->config, $this->l10n)],
-		], $this->manager->getAdminSettings('sharing'));
+			13 => [$section]
+		], $settings);
 	}
 
 	public function testGetPersonalSettings() {
+		$section = $this->createMock(Security::class);
+		$section->expects($this->once())
+			->method('getPriority')
+			->willReturn(16);
+		$this->container->expects($this->once())
+			->method('query')
+			->with(Security::class)
+			->willReturn($section);
+
+		$settings = $this->manager->getPersonalSettings('security');
+
 		$this->assertEquals([
-			10 => [new Security($this->userManager)],
-		], $this->manager->getPersonalSettings('security'));
+			16 => [$section]
+		], $settings);
 	}
 
 	public function testSameSectionAsPersonalAndAdmin() {