Fix ldap testing and move stuff to dependency injection

Signed-off-by: Carl Schwan <carl@carlschwan.eu>

Author: CarlSchwan

apps/user_ldap/lib/AccessFactory.php

@@ -56,7 +56,7 @@ public function __construct(
 		$this->dispatcher = $dispatcher;
 	}
 
-	public function get(Connection $connection) {
+	public function get(Connection $connection): Access {
 		return new Access(
 			$connection,
 			$this->ldap,

apps/user_ldap/lib/Jobs/Sync.php

@@ -5,6 +5,7 @@
  * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
  * @author Christoph Wurst <christoph@winzerhof-wurst.at>
  * @author Joas Schilling <coding@schilljs.com>
+ * @author Carl Schwan <carl@carlschwan.eu>
  *
  * @license GNU AGPL version 3 or any later version
  *
@@ -24,7 +25,9 @@
  */
 namespace OCA\User_LDAP\Jobs;
 
-use OC\BackgroundJob\TimedJob;
+use OCA\User_LDAP\User\User;
+use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\BackgroundJob\TimedJob;
 use OC\ServerNotAvailableException;
 use OCA\User_LDAP\AccessFactory;
 use OCA\User_LDAP\Configuration;
@@ -44,51 +47,77 @@
 class Sync extends TimedJob {
 	public const MAX_INTERVAL = 12 * 60 * 60; // 12h
 	public const MIN_INTERVAL = 30 * 60; // 30min
-	/** @var  Helper */
-	protected $ldapHelper;
-	/** @var  LDAP */
-	protected $ldap;
+
+	protected Helper $ldapHelper;
+	protected LDAP $ldap;
 	protected Manager $userManager;
-	/** @var UserMapping */
-	protected $mapper;
-	/** @var  IConfig */
-	protected $config;
-	/** @var  IAvatarManager */
-	protected $avatarManager;
-	/** @var  IDBConnection */
-	protected $dbc;
-	/** @var  IUserManager */
-	protected $ncUserManager;
-	/** @var  LoggerInterface */
-	protected $logger;
-	/** @var  IManager */
-	protected $notificationManager;
-	/** @var ConnectionFactory */
-	protected $connectionFactory;
-	/** @var AccessFactory */
-	protected $accessFactory;
+	protected UserMapping $mapper;
+	protected IConfig $config;
+	protected IAvatarManager $avatarManager;
+	protected IDBConnection $dbc;
+	protected IUserManager $ncUserManager;
+	protected LoggerInterface $logger;
+	protected IManager $notificationManager;
+	protected ConnectionFactory $connectionFactory;
+	protected AccessFactory $accessFactory;
 	protected IEventDispatcher $dispatcher;
 
-	public function __construct(Manager $userManager) {
+	public function __construct(
+		Manager $userManager,
+		IEventDispatcher $dispatcher,
+		IConfig $config,
+		ITimeFactory $timeFactory,
+		IDBConnection $dbConnection,
+		IAvatarManager $avatarManager,
+		IUserManager $ncUserManager,
+		LoggerInterface $logger,
+		IManager $notificationManager,
+		UserMapping $userMapping,
+		Helper $ldapHelper,
+		ConnectionFactory $connectionFactory
+	) {
+		parent::__construct($timeFactory);
 		$this->userManager = $userManager;
+		$this->dispatcher = $dispatcher;
+		$this->config = $config;
 		$this->setInterval(
-			\OC::$server->getConfig()->getAppValue(
+			(int)$config->getAppValue(
 				'user_ldap',
 				'background_sync_interval',
 				self::MIN_INTERVAL
 			)
 		);
-		$this->dispatcher = \OC::$server->get(IEventDispatcher::class);
+
+		$this->ldapHelper = $ldapHelper;
+		$this->ldap = new LDAP($this->config->getSystemValueString('ldap_log_file'));
+		$this->avatarManager = $avatarManager;
+		$this->dbc = $dbConnection;
+		$this->ncUserManager = $ncUserManager;
+		$this->logger = $logger;
+		$this->userManager = $userManager;
+		$this->notificationManager = $notificationManager;
+		$this->mapper = $userMapping;
+		$this->connectionFactory = $connectionFactory;
+
+		$this->accessFactory = new AccessFactory(
+			$this->ldap,
+			$this->userManager,
+			$this->ldapHelper,
+			$this->config,
+			$this->ncUserManager,
+			$this->logger,
+			$this->dispatcher
+		);
 	}
 
 	/**
-	 * updates the interval
+	 * Updates the interval
 	 *
-	 * the idea is to adjust the interval depending on the amount of known users
+	 * The idea is to adjust the interval depending on the amount of known users
 	 * and the attempt to update each user one day. At most it would run every
 	 * 30 minutes, and at least every 12 hours.
 	 */
-	public function updateInterval() {
+	public function updateInterval(): void {
 		$minPagingSize = $this->getMinPagingSize();
 		$mappedUsers = $this->mapper->count();
 
@@ -101,10 +130,9 @@ public function updateInterval() {
 	}
 
 	/**
-	 * returns the smallest configured paging size
-	 * @return int
+	 * Returns the smallest configured paging size
 	 */
-	protected function getMinPagingSize() {
+	protected function getMinPagingSize(): int {
 		$configKeys = $this->config->getAppKeys('user_ldap');
 		$configKeys = array_filter($configKeys, function ($key) {
 			return strpos($key, 'ldap_paging_size') !== false;
@@ -121,8 +149,6 @@ protected function getMinPagingSize() {
 	 * @param array $argument
 	 */
 	public function run($argument) {
-		$this->setArgument($argument);
-
 		$isBackgroundJobModeAjax = $this->config
 				->getAppValue('core', 'backgroundjobs_mode', 'ajax') === 'ajax';
 		if ($isBackgroundJobModeAjax) {
@@ -157,10 +183,10 @@ public function run($argument) {
 	}
 
 	/**
-	 * @param array $cycleData
+	 * @param array{offset: int, prefix: string} $cycleData
 	 * @return bool whether more results are expected from the same configuration
 	 */
-	public function runCycle($cycleData) {
+	public function runCycle(array $cycleData): bool {
 		$connection = $this->connectionFactory->get($cycleData['prefix']);
 		$access = $this->accessFactory->get($connection);
 		$access->setUserMapper($this->mapper);
@@ -185,24 +211,23 @@ public function runCycle($cycleData) {
 	}
 
 	/**
-	 * returns the info about the current cycle that should be run, if any,
+	 * Returns the info about the current cycle that should be run, if any,
 	 * otherwise null
-	 *
-	 * @return array|null
+	 * @return ?array{offset: int, prefix: string}
 	 */
-	public function getCycle() {
+	public function getCycle(): ?array {
 		$prefixes = $this->ldapHelper->getServerConfigurationPrefixes(true);
 		if (count($prefixes) === 0) {
 			return null;
 		}
 
 		$cycleData = [
-			'prefix' => $this->config->getAppValue('user_ldap', 'background_sync_prefix', null),
-			'offset' => (int)$this->config->getAppValue('user_ldap', 'background_sync_offset', 0),
+			'prefix' => $this->config->getAppValue('user_ldap', 'background_sync_prefix', 'none'),
+			'offset' => (int)$this->config->getAppValue('user_ldap', 'background_sync_offset', '0'),
 		];
 
 		if (
-			$cycleData['prefix'] !== null
+			$cycleData['prefix'] !== 'none'
 			&& in_array($cycleData['prefix'], $prefixes)
 		) {
 			return $cycleData;
@@ -214,21 +239,21 @@ public function getCycle() {
 	/**
 	 * Save the provided cycle information in the DB
 	 *
-	 * @param array $cycleData
+	 * @param array{prefix: ?string, offset: int} $cycleData
 	 */
-	public function setCycle(array $cycleData) {
+	public function setCycle(array $cycleData): void {
 		$this->config->setAppValue('user_ldap', 'background_sync_prefix', $cycleData['prefix']);
-		$this->config->setAppValue('user_ldap', 'background_sync_offset', $cycleData['offset']);
+		$this->config->setAppValue('user_ldap', 'background_sync_offset', (string)$cycleData['offset']);
 	}
 
 	/**
 	 * returns data about the next cycle that should run, if any, otherwise
 	 * null. It also always goes for the next LDAP configuration!
 	 *
 	 * @param array|null $cycleData the old cycle
-	 * @return array|null
+	 * @return ?array{prefix: string, offset: int}
 	 */
-	public function determineNextCycle(array $cycleData = null) {
+	public function determineNextCycle(?array $cycleData = null): ?array {
 		$prefixes = $this->ldapHelper->getServerConfigurationPrefixes(true);
 		if (count($prefixes) === 0) {
 			return null;
@@ -242,19 +267,18 @@ public function determineNextCycle(array $cycleData = null) {
 		}
 		$cycleData['prefix'] = $prefix;
 		$cycleData['offset'] = 0;
-		$this->setCycle(['prefix' => $prefix, 'offset' => 0]);
+		$this->setCycle($cycleData);
 
 		return $cycleData;
 	}
 
 	/**
-	 * Checks whether the provided cycle should be run. Currently only the
+	 * Checks whether the provided cycle should be run. Currently, only the
 	 * last configuration change goes into account (at least one hour).
 	 *
-	 * @param $cycleData
-	 * @return bool
+	 * @param $cycleData{prefix: string}
 	 */
-	public function qualifiesToRun($cycleData) {
+	public function qualifiesToRun(array $cycleData): bool {
 		$lastChange = $this->config->getAppValue('user_ldap', $cycleData['prefix'] . '_lastChange', 0);
 		if ((time() - $lastChange) > 60 * 30) {
 			return true;
@@ -263,23 +287,20 @@ public function qualifiesToRun($cycleData) {
 	}
 
 	/**
-	 * increases the offset of the current cycle for the next run
+	 * Increases the offset of the current cycle for the next run
 	 *
-	 * @param $cycleData
+	 * @param array{prefix: string, offset: int} $cycleData
 	 */
-	protected function increaseOffset($cycleData) {
+	protected function increaseOffset(array $cycleData): void {
 		$ldapConfig = new Configuration($cycleData['prefix']);
-		$cycleData['offset'] += (int)$ldapConfig->ldapPagingSize;
+		$cycleData['offset'] += $ldapConfig->ldapPagingSize;
 		$this->setCycle($cycleData);
 	}
 
 	/**
-	 * determines the next configuration prefix based on the last one (if any)
-	 *
-	 * @param string|null $lastPrefix
-	 * @return string|null
+	 * Determines the next configuration prefix based on the last one (if any)
 	 */
-	protected function getNextPrefix($lastPrefix) {
+	protected function getNextPrefix(?string $lastPrefix): ?string {
 		$prefixes = $this->ldapHelper->getServerConfigurationPrefixes(true);
 		$noOfPrefixes = count($prefixes);
 		if ($noOfPrefixes === 0) {
@@ -299,87 +320,10 @@ protected function getNextPrefix($lastPrefix) {
 	}
 
 	/**
-	 * "fixes" DI
-	 *
-	 * @param array $argument
+	 * Only used in tests
 	 */
-	public function setArgument($argument) {
-		if (isset($argument['config'])) {
-			$this->config = $argument['config'];
-		} else {
-			$this->config = \OC::$server->getConfig();
-		}
-
-		if (isset($argument['helper'])) {
-			$this->ldapHelper = $argument['helper'];
-		} else {
-			$this->ldapHelper = new Helper($this->config, \OC::$server->getDatabaseConnection());
-		}
-
-		if (isset($argument['ldapWrapper'])) {
-			$this->ldap = $argument['ldapWrapper'];
-		} else {
-			$this->ldap = new LDAP($this->config->getSystemValueString('ldap_log_file'));
-		}
-
-		if (isset($argument['avatarManager'])) {
-			$this->avatarManager = $argument['avatarManager'];
-		} else {
-			$this->avatarManager = \OC::$server->getAvatarManager();
-		}
-
-		if (isset($argument['dbc'])) {
-			$this->dbc = $argument['dbc'];
-		} else {
-			$this->dbc = \OC::$server->getDatabaseConnection();
-		}
-
-		if (isset($argument['ncUserManager'])) {
-			$this->ncUserManager = $argument['ncUserManager'];
-		} else {
-			$this->ncUserManager = \OC::$server->getUserManager();
-		}
-
-		if (isset($argument['logger'])) {
-			$this->logger = $argument['logger'];
-		} else {
-			$this->logger = \OC::$server->get(LoggerInterface::class);
-		}
-
-		if (isset($argument['notificationManager'])) {
-			$this->notificationManager = $argument['notificationManager'];
-		} else {
-			$this->notificationManager = \OC::$server->getNotificationManager();
-		}
-
-		if (isset($argument['userManager'])) {
-			$this->userManager = $argument['userManager'];
-		}
-
-		if (isset($argument['mapper'])) {
-			$this->mapper = $argument['mapper'];
-		} else {
-			$this->mapper = new UserMapping($this->dbc);
-		}
-
-		if (isset($argument['connectionFactory'])) {
-			$this->connectionFactory = $argument['connectionFactory'];
-		} else {
-			$this->connectionFactory = new ConnectionFactory($this->ldap);
-		}
-
-		if (isset($argument['accessFactory'])) {
-			$this->accessFactory = $argument['accessFactory'];
-		} else {
-			$this->accessFactory = new AccessFactory(
-				$this->ldap,
-				$this->userManager,
-				$this->ldapHelper,
-				$this->config,
-				$this->ncUserManager,
-				$this->logger,
-				$this->dispatcher,
-			);
-		}
+	public function overwritePropertiesForTest(LDAP $ldapWrapper, AccessFactory $accessFactory): void {
+		$this->ldap = $ldapWrapper;
+		$this->accessFactory = $accessFactory;
 	}
 }