Lower complexity for valid backends (replace hashmap with a "simple" name array)

src/Core/StorageManager.php

@@ -40,12 +40,14 @@ class StorageManager
 	const TABLES = ['photo', 'attach'];
 
 	// Default storage backends
+	/** @var string[]  */
 	const DEFAULT_BACKENDS = [
-		Storage\Filesystem::NAME => Storage\Filesystem::class,
+		Storage\Filesystem::NAME,
-		Storage\Database::NAME   => Storage\Database::class,
+		Storage\Database::NAME,
 	];
 
-	private $backends;
+	/** @var string[] List of valid backend classes */
+	private $validBackends;
 
 	/**
 	 * @var Storage\IStorage[] A local cache for storage instances
@@ -79,7 +81,7 @@ class StorageManager
 		$this->config   = $config;
 		$this->logger   = $logger;
 		$this->l10n     = $l10n;
-		$this->backends = $config->get('storage', 'backends', self::DEFAULT_BACKENDS);
+		$this->validBackends = $config->get('storage', 'backends', self::DEFAULT_BACKENDS);
 
 		$currentName = $this->config->get('storage', 'name');
 
@@ -109,7 +111,7 @@ class StorageManager
 	 */
 	public function getWritableStorageByName(string $name): Storage\IWritableStorage
 	{
-		$storage = $this->getByName($name, $this->backends);
+		$storage = $this->getByName($name, $this->validBackends);
 		if ($storage instanceof Storage\IWritableStorage) {
 			return $storage;
 		} else {
@@ -121,7 +123,7 @@ class StorageManager
 	 * Return storage backend class by registered name
 	 *
 	 * @param string     $name Backend name
-	 * @param array|null $validBackends possible, manual override of the valid backends
+	 * @param string[]|null $validBackends possible, manual override of the valid backends
 	 *
 	 * @return Storage\IStorage
 	 *
@@ -179,18 +181,18 @@ class StorageManager
 	 * Checks, if the storage is a valid backend
 	 *
 	 * @param string|null   $name          The name or class of the backend
-	 * @param array|null  $validBackends Possible, valid backends to check
+	 * @param string[]|null $validBackends Possible, valid backends to check
 	 *
 	 * @return boolean True, if the backend is a valid backend
 	 */
 	public function isValidBackend(string $name = null, array $validBackends = null): bool
 	{
-		$validBackends = $validBackends ?? array_merge($this->backends,
+		$validBackends = $validBackends ?? array_merge($this->validBackends,
 				[
-					Storage\SystemResource::getName()   => '',
+					Storage\SystemResource::getName(),
-					Storage\ExternalResource::getName() => ''
+					Storage\ExternalResource::getName(),
 				]);
-		return array_key_exists($name, $validBackends);
+		return in_array($name, $validBackends);
 	}
 
 	/**
@@ -213,11 +215,11 @@ class StorageManager
 	/**
 	 * Get registered backends
 	 *
-	 * @return array
+	 * @return Storage\IWritableStorage[]
 	 */
 	public function listBackends(): array
 	{
-		return $this->backends;
+		return $this->validBackends;
 	}
 
 	/**
@@ -234,11 +236,15 @@ class StorageManager
 		if (is_subclass_of($class, Storage\IStorage::class)) {
 			/** @var Storage\IStorage $class */
 
-			$backends                    = $this->backends;
+			if (array_search($class::getName(), $this->validBackends, true) !== false) {
-			$backends[$class::getName()] = $class;
+				return true;
+			}
+
+			$backends   = $this->validBackends;
+			$backends[] = $class::getName();
 
 			if ($this->config->set('storage', 'backends', $backends)) {
-				$this->backends = $backends;
+				$this->validBackends = $backends;
 				return true;
 			} else {
 				return false;
@@ -254,20 +260,33 @@ class StorageManager
 	 * @param string $class Backend class name
 	 *
 	 * @return boolean True, if unregistering was successful
+	 *
+	 * @throws Storage\StorageException
 	 */
 	public function unregister(string $class): bool
 	{
 		if (is_subclass_of($class, Storage\IStorage::class)) {
 			/** @var Storage\IStorage $class */
 
-			unset($this->backends[$class::getName()]);
+			if ($this->currentBackend::getName() == $class::getName()) {
-
+				throw new Storage\StorageException(sprintf('Cannot unregister %s, because it\'s currently active.', $class::getName()));
-			if ($this->currentBackend instanceof $class) {
-				$this->config->set('storage', 'name', null);
-				$this->currentBackend = null;
 			}
 
-			return $this->config->set('storage', 'backends', $this->backends);
+			$key = array_search($class::getName(), $this->validBackends);
+
+			if ($key !== false) {
+				$backends = $this->validBackends;
+				unset($backends[$key]);
+				$backends = array_values($backends);
+				if ($this->config->set('storage', 'backends', $backends)) {
+					$this->validBackends = $backends;
+					return true;
+				} else {
+					return false;
+				}
+			} else {
+				return true;
+			}
 		} else {
 			return false;
 		}
@@ -289,7 +308,7 @@ class StorageManager
 	 */
 	public function move(Storage\IWritableStorage $destination, array $tables = self::TABLES, int $limit = 5000): int
 	{
-		if (!$this->isValidBackend($destination, $this->backends)) {
+		if (!$this->isValidBackend($destination, $this->validBackends)) {
 			throw new Storage\StorageException(sprintf("Can't move to storage backend '%s'", $destination::getName()));
 		}
 

tests/src/Core/StorageManagerTest.php

@@ -250,10 +250,38 @@ class StorageManagerTest extends DatabaseTest
 		self::assertTrue($storageManager->register(SampleStorageBackend::class));
 
 		self::assertEquals(array_merge(StorageManager::DEFAULT_BACKENDS, [
-			SampleStorageBackend::getName() => SampleStorageBackend::class,
+			SampleStorageBackend::getName(),
 		]), $storageManager->listBackends());
 		self::assertEquals(array_merge(StorageManager::DEFAULT_BACKENDS, [
-			SampleStorageBackend::getName() => SampleStorageBackend::class,
+			SampleStorageBackend::getName()
+		]), $this->config->get('storage', 'backends'));
+
+		self::assertTrue($storageManager->unregister(SampleStorageBackend::class));
+		self::assertEquals(StorageManager::DEFAULT_BACKENDS, $this->config->get('storage', 'backends'));
+		self::assertEquals(StorageManager::DEFAULT_BACKENDS, $storageManager->listBackends());
+	}
+
+	/**
+	 * tests that an active backend cannot get unregistered
+	 */
+	public function testUnregisterActiveBackend()
+	{
+		/// @todo Remove dice once "Hook" is dynamic and mockable
+		$dice = (new Dice())
+			->addRules(include __DIR__ . '/../../../static/dependencies.config.php')
+			->addRule(Database::class, ['instanceOf' => StaticDatabase::class, 'shared' => true])
+			->addRule(ISession::class, ['instanceOf' => Session\Memory::class, 'shared' => true, 'call' => null]);
+		DI::init($dice);
+
+		$storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n);
+
+		self::assertTrue($storageManager->register(SampleStorageBackend::class));
+
+		self::assertEquals(array_merge(StorageManager::DEFAULT_BACKENDS, [
+			SampleStorageBackend::getName(),
+		]), $storageManager->listBackends());
+		self::assertEquals(array_merge(StorageManager::DEFAULT_BACKENDS, [
+			SampleStorageBackend::getName()
 		]), $this->config->get('storage', 'backends'));
 
 		// inline call to register own class as hook (testing purpose only)
@@ -265,12 +293,10 @@ class StorageManagerTest extends DatabaseTest
 
 		self::assertInstanceOf(SampleStorageBackend::class, $storageManager->getBackend());
 
-		self::assertTrue($storageManager->unregister(SampleStorageBackend::class));
+		self::expectException(Storage\StorageException::class);
-		self::assertEquals(StorageManager::DEFAULT_BACKENDS, $this->config->get('storage', 'backends'));
+		self::expectExceptionMessage('Cannot unregister Sample Storage, because it\'s currently active.');
-		self::assertEquals(StorageManager::DEFAULT_BACKENDS, $storageManager->listBackends());
 
-		self::assertNull($storageManager->getBackend());
+		$storageManager->unregister(SampleStorageBackend::class);
-		self::assertNull($this->config->get('storage', 'name'));
 	}
 
 	/**