From 1901716479a3adcbc778e2a1da640c9c6aa4d3cd Mon Sep 17 00:00:00 2001 From: Philipp Date: Mon, 16 Aug 2021 00:08:06 +0200 Subject: [PATCH] Lower complexity for valid backends (replace hashmap with a "simple" name array) --- src/Core/StorageManager.php | 67 +++++++++++++++++---------- tests/src/Core/StorageManagerTest.php | 40 +++++++++++++--- 2 files changed, 76 insertions(+), 31 deletions(-) diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index 132cd64538..6e0e0796bd 100644 --- a/src/Core/StorageManager.php +++ b/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\Database::NAME => Storage\Database::class, + Storage\Filesystem::NAME, + 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 * @@ -178,19 +180,19 @@ 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 $name The name or class of the backend + * @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\ExternalResource::getName() => '' + Storage\SystemResource::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; - $backends[$class::getName()] = $class; + if (array_search($class::getName(), $this->validBackends, true) !== false) { + 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 instanceof $class) { - $this->config->set('storage', 'name', null); - $this->currentBackend = null; + if ($this->currentBackend::getName() == $class::getName()) { + throw new Storage\StorageException(sprintf('Cannot unregister %s, because it\'s currently active.', $class::getName())); } - 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())); } diff --git a/tests/src/Core/StorageManagerTest.php b/tests/src/Core/StorageManagerTest.php index c4ba9368cf..9e8e3aa2c2 100644 --- a/tests/src/Core/StorageManagerTest.php +++ b/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::assertEquals(StorageManager::DEFAULT_BACKENDS, $this->config->get('storage', 'backends')); - self::assertEquals(StorageManager::DEFAULT_BACKENDS, $storageManager->listBackends()); + self::expectException(Storage\StorageException::class); + self::expectExceptionMessage('Cannot unregister Sample Storage, because it\'s currently active.'); - self::assertNull($storageManager->getBackend()); - self::assertNull($this->config->get('storage', 'name')); + $storageManager->unregister(SampleStorageBackend::class); } /**