From 29c7552df57c31d81e8ce2458e2873f4d2ece35e Mon Sep 17 00:00:00 2001 From: Philipp Date: Sun, 1 Aug 2021 14:00:48 +0200 Subject: [PATCH] Refactor IStorage --- src/Console/Storage.php | 6 +- src/Core/StorageManager.php | 90 +++++++++++++++++++------- src/DI.php | 4 +- src/Model/Attach.php | 18 ++++-- src/Model/Photo.php | 25 +++++-- src/Model/Storage/ExternalResource.php | 4 +- src/Model/Storage/Filesystem.php | 2 + src/Module/Admin/Storage.php | 17 +++-- src/Worker/MoveStorage.php | 2 +- static/dependencies.config.php | 4 +- tests/src/Core/StorageManagerTest.php | 4 +- 11 files changed, 121 insertions(+), 55 deletions(-) diff --git a/src/Console/Storage.php b/src/Console/Storage.php index 93bbb2657..70e8e2630 100644 --- a/src/Console/Storage.php +++ b/src/Console/Storage.php @@ -131,10 +131,10 @@ HELP; throw new CommandArgsException('Invalid arguments'); } - $name = $this->args[1]; - $class = $this->storageManager->getByName($name); + $name = $this->args[1]; + $class = $this->storageManager->getSelectableStorageByName($name); - if ($class === '') { + if (is_null($class)) { $this->out($name . ' is not a registered backend.'); return -1; } diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index 64e53c10b..03f89f312 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -61,8 +61,6 @@ class StorageManager private $logger; /** @var L10n */ private $l10n; - /** @var IHTTPRequest */ - private $httpRequest; /** @var Storage\IStorage */ private $currentBackend; @@ -73,25 +71,24 @@ class StorageManager * @param LoggerInterface $logger * @param L10n $l10n */ - public function __construct(Database $dba, IConfig $config, LoggerInterface $logger, L10n $l10n, IHTTPRequest $httpRequest) + public function __construct(Database $dba, IConfig $config, LoggerInterface $logger, L10n $l10n) { $this->dba = $dba; $this->config = $config; $this->logger = $logger; $this->l10n = $l10n; - $this->httpRequest = $httpRequest; $this->backends = $config->get('storage', 'backends', self::DEFAULT_BACKENDS); $currentName = $this->config->get('storage', 'name', ''); // you can only use user backends as a "default" backend, so the second parameter is true - $this->currentBackend = $this->getByName($currentName, true); + $this->currentBackend = $this->getSelectableStorageByName($currentName); } /** * Return current storage backend class * - * @return Storage\IStorage|null + * @return Storage\ISelectableStorage|null */ public function getBackend() { @@ -99,16 +96,15 @@ class StorageManager } /** - * Return storage backend class by registered name + * Returns a selectable storage backend class by registered name * * @param string|null $name Backend name - * @param boolean $onlyUserBackend True, if just user specific instances should be returrned (e.g. not SystemResource) * - * @return Storage\IStorage|null null if no backend registered at $name + * @return Storage\ISelectableStorage|null null if no backend registered at $name * * @throws \Friendica\Network\HTTPException\InternalServerErrorException */ - public function getByName(string $name = null, $onlyUserBackend = false) + public function getSelectableStorageByName(string $name = null) { // @todo 2020.09 Remove this call after 2 releases $name = $this->checkLegacyBackend($name); @@ -116,22 +112,70 @@ class StorageManager // If there's no cached instance create a new instance if (!isset($this->backendInstances[$name])) { // If the current name isn't a valid backend (or the SystemResource instance) create it - if ($this->isValidBackend($name, $onlyUserBackend)) { + if ($this->isValidBackend($name, true)) { switch ($name) { // Try the filesystem backend case Storage\Filesystem::getName(): - $this->backendInstances[$name] = new Storage\Filesystem($this->config, $this->logger, $this->l10n); + $this->backendInstances[$name] = new Storage\Filesystem($this->config, $this->l10n); break; // try the database backend case Storage\Database::getName(): - $this->backendInstances[$name] = new Storage\Database($this->dba, $this->logger, $this->l10n); + $this->backendInstances[$name] = new Storage\Database($this->dba); + break; + default: + $data = [ + 'name' => $name, + 'storage' => null, + ]; + Hook::callAll('storage_instance', $data); + if (($data['storage'] ?? null) instanceof Storage\ISelectableStorage) { + $this->backendInstances[$data['name'] ?? $name] = $data['storage']; + } else { + return null; + } + break; + } + } else { + return null; + } + } + + return $this->backendInstances[$name]; + } + + /** + * Return storage backend class by registered name + * + * @param string|null $name Backend name + * + * @return Storage\IStorage|null null if no backend registered at $name + * + * @throws \Friendica\Network\HTTPException\InternalServerErrorException + */ + public function getByName(string $name = null) + { + // @todo 2020.09 Remove this call after 2 releases + $name = $this->checkLegacyBackend($name); + + // If there's no cached instance create a new instance + if (!isset($this->backendInstances[$name])) { + // If the current name isn't a valid backend (or the SystemResource instance) create it + if ($this->isValidBackend($name, false)) { + switch ($name) { + // Try the filesystem backend + case Storage\Filesystem::getName(): + $this->backendInstances[$name] = new Storage\Filesystem($this->config, $this->l10n); + break; + // try the database backend + case Storage\Database::getName(): + $this->backendInstances[$name] = new Storage\Database($this->dba); break; // at least, try if there's an addon for the backend case Storage\SystemResource::getName(): $this->backendInstances[$name] = new Storage\SystemResource(); break; case Storage\ExternalResource::getName(): - $this->backendInstances[$name] = new Storage\ExternalResource($this->httpRequest); + $this->backendInstances[$name] = new Storage\ExternalResource(); break; default: $data = [ @@ -190,18 +234,14 @@ class StorageManager /** * Set current storage backend class * - * @param string $name Backend class name + * @param Storage\ISelectableStorage $storage The storage class * * @return boolean True, if the set was successful */ - public function setBackend(string $name = null) + public function setBackend(Storage\ISelectableStorage $storage) { - if (!$this->isValidBackend($name, false)) { - return false; - } - - if ($this->config->set('storage', 'name', $name)) { - $this->currentBackend = $this->getByName($name, false); + if ($this->config->set('storage', 'name', $storage::getName())) { + $this->currentBackend = $storage; return true; } else { return false; @@ -277,7 +317,7 @@ class StorageManager * Copy existing data to destination storage and delete from source. * This method cannot move to legacy in-table `data` field. * - * @param Storage\IStorage $destination Destination storage class name + * @param Storage\ISelectableStorage $destination Destination storage class name * @param array $tables Tables to look in for resources. Optional, defaults to ['photo', 'attach'] * @param int $limit Limit of the process batch size, defaults to 5000 * @@ -285,7 +325,7 @@ class StorageManager * @throws Storage\StorageException * @throws Exception */ - public function move(Storage\IStorage $destination, array $tables = self::TABLES, int $limit = 5000) + public function move(Storage\ISelectableStorage $destination, array $tables = self::TABLES, int $limit = 5000) { if (!$this->isValidBackend($destination, true)) { throw new Storage\StorageException(sprintf("Can't move to storage backend '%s'", $destination::getName())); @@ -304,7 +344,7 @@ class StorageManager while ($resource = $this->dba->fetch($resources)) { $id = $resource['id']; $data = $resource['data']; - $source = $this->getByName($resource['backend-class']); + $source = $this->getSelectableStorageByName($resource['backend-class']); $sourceRef = $resource['backend-ref']; if (!empty($source)) { diff --git a/src/DI.php b/src/DI.php index 02620ea11..9e96ddc01 100644 --- a/src/DI.php +++ b/src/DI.php @@ -387,11 +387,11 @@ abstract class DI } /** - * @return Model\Storage\IStorage + * @return Model\Storage\ISelectableStorage */ public static function storage() { - return self::$dice->create(Model\Storage\IStorage::class); + return self::$dice->create(Model\Storage\ISelectableStorage::class); } // diff --git a/src/Model/Attach.php b/src/Model/Attach.php index 6182727c9..d3ff5462c 100644 --- a/src/Model/Attach.php +++ b/src/Model/Attach.php @@ -25,6 +25,7 @@ use Friendica\Core\System; use Friendica\Database\DBA; use Friendica\Database\DBStructure; use Friendica\DI; +use Friendica\Model\Storage\ReferenceStorageException; use Friendica\Object\Image; use Friendica\Util\DateTimeFormat; use Friendica\Util\Mimetype; @@ -173,7 +174,12 @@ class Attach return $i['data']; } else { $backendRef = $item['backend-ref']; - return $backendClass->get($backendRef); + try { + return $backendClass->get($backendRef); + } catch (ReferenceStorageException $referenceStorageException) { + DI::logger()->debug('No data found for item', ['item' => $item, 'exception' => $referenceStorageException]); + return ''; + } } } @@ -278,7 +284,7 @@ class Attach $items = self::selectToArray(['backend-class','backend-ref'], $conditions); foreach($items as $item) { - $backend_class = DI::storageManager()->getByName($item['backend-class'] ?? ''); + $backend_class = DI::storageManager()->getSelectableStorageByName($item['backend-class'] ?? ''); if (!empty($backend_class)) { $fields['backend-ref'] = $backend_class->put($img->asString(), $item['backend-ref'] ?? ''); } else { @@ -310,9 +316,13 @@ class Attach $items = self::selectToArray(['backend-class','backend-ref'], $conditions); foreach($items as $item) { - $backend_class = DI::storageManager()->getByName($item['backend-class'] ?? ''); + $backend_class = DI::storageManager()->getSelectableStorageByName($item['backend-class'] ?? ''); if (!empty($backend_class)) { - $backend_class->delete($item['backend-ref'] ?? ''); + try { + $backend_class->delete($item['backend-ref'] ?? ''); + } catch (ReferenceStorageException $referenceStorageException) { + DI::logger()->debug('Item doesn\'t exist.', ['conditions' => $conditions, 'exception' => $referenceStorageException]); + } } } diff --git a/src/Model/Photo.php b/src/Model/Photo.php index 26369a354..f74c68da5 100644 --- a/src/Model/Photo.php +++ b/src/Model/Photo.php @@ -28,6 +28,8 @@ use Friendica\Database\DBA; use Friendica\Database\DBStructure; use Friendica\DI; use Friendica\Model\Storage\ExternalResource; +use Friendica\Model\Storage\ReferenceStorageException; +use Friendica\Model\Storage\StorageException; use Friendica\Model\Storage\SystemResource; use Friendica\Object\Image; use Friendica\Util\DateTimeFormat; @@ -183,9 +185,10 @@ class Photo * * @param array $photo Photo data. Needs at least 'id', 'type', 'backend-class', 'backend-ref' * - * @return \Friendica\Object\Image + * @return \Friendica\Object\Image|string * @throws \Friendica\Network\HTTPException\InternalServerErrorException * @throws \ImagickException + * @throws StorageException */ public static function getImageDataForPhoto(array $photo) { @@ -198,12 +201,17 @@ class Photo // legacy data storage in "data" column $i = self::selectFirst(['data'], ['id' => $photo['id']]); if ($i === false) { - return null; + return ''; } $data = $i['data']; } else { $backendRef = $photo['backend-ref'] ?? ''; - $data = $backendClass->get($backendRef); + try { + $data = $backendClass->get($backendRef); + } catch (ReferenceStorageException $referenceStorageException) { + DI::logger()->debug('No data found for photo', ['photo' => $photo, 'exception' => $referenceStorageException]); + return ''; + } } return $data; } @@ -339,7 +347,7 @@ class Photo if (DBA::isResult($existing_photo)) { $backend_ref = (string)$existing_photo["backend-ref"]; - $storage = DI::storageManager()->getByName($existing_photo["backend-class"] ?? ''); + $storage = DI::storageManager()->getSelectableStorageByName($existing_photo["backend-class"] ?? ''); } else { $storage = DI::storage(); } @@ -403,11 +411,14 @@ class Photo $photos = DBA::select('photo', ['id', 'backend-class', 'backend-ref'], $conditions); while ($photo = DBA::fetch($photos)) { - $backend_class = DI::storageManager()->getByName($photo['backend-class'] ?? ''); + $backend_class = DI::storageManager()->getSelectableStorageByName($photo['backend-class'] ?? ''); if (!empty($backend_class)) { - if ($backend_class->delete($photo["backend-ref"] ?? '')) { + try { + $backend_class->delete($item['backend-ref'] ?? ''); // Delete the photos after they had been deleted successfully DBA::delete("photo", ['id' => $photo['id']]); + } catch (ReferenceStorageException $referenceStorageException) { + DI::logger()->debug('phot doesn\'t exist.', ['conditions' => $conditions, 'exception' => $referenceStorageException]); } } } @@ -437,7 +448,7 @@ class Photo $photos = self::selectToArray(['backend-class', 'backend-ref'], $conditions); foreach($photos as $photo) { - $backend_class = DI::storageManager()->getByName($photo['backend-class'] ?? ''); + $backend_class = DI::storageManager()->getSelectableStorageByName($photo['backend-class'] ?? ''); if (!empty($backend_class)) { $fields["backend-ref"] = $backend_class->put($img->asString(), $photo['backend-ref']); } else { diff --git a/src/Model/Storage/ExternalResource.php b/src/Model/Storage/ExternalResource.php index 402619e67..918bcf8ac 100644 --- a/src/Model/Storage/ExternalResource.php +++ b/src/Model/Storage/ExternalResource.php @@ -52,12 +52,12 @@ class ExternalResource implements IStorage try { $fetchResult = HTTPSignature::fetchRaw($data->url, $data->uid, ['accept_content' => '']); } catch (Exception $exception) { - throw new StorageException(sprintf('External resource failed to get %s', $reference), $exception->getCode(), $exception); + throw new ReferenceStorageException(sprintf('External resource failed to get %s', $reference), $exception->getCode(), $exception); } if ($fetchResult->isSuccess()) { return $fetchResult->getBody(); } else { - throw new StorageException(sprintf('External resource failed to get %s', $reference), $fetchResult->getReturnCode(), new Exception($fetchResult->getBody())); + throw new ReferenceStorageException(sprintf('External resource failed to get %s', $reference), $fetchResult->getReturnCode(), new Exception($fetchResult->getBody())); } } diff --git a/src/Model/Storage/Filesystem.php b/src/Model/Storage/Filesystem.php index 7760b4a53..fe5230049 100644 --- a/src/Model/Storage/Filesystem.php +++ b/src/Model/Storage/Filesystem.php @@ -131,6 +131,8 @@ class Filesystem implements ISelectableStorage if ($result === false) { throw new StorageException(sprintf('Filesystem storage failed to get data to "%s". Check your write permissions', $file)); } + + return $result; } /** diff --git a/src/Module/Admin/Storage.php b/src/Module/Admin/Storage.php index 101070712..374d692bf 100644 --- a/src/Module/Admin/Storage.php +++ b/src/Module/Admin/Storage.php @@ -23,7 +23,7 @@ namespace Friendica\Module\Admin; use Friendica\Core\Renderer; use Friendica\DI; -use Friendica\Model\Storage\IStorage; +use Friendica\Model\Storage\ISelectableStorage; use Friendica\Module\BaseAdmin; use Friendica\Util\Strings; @@ -37,8 +37,8 @@ class Storage extends BaseAdmin $storagebackend = Strings::escapeTags(trim($parameters['name'] ?? '')); - /** @var IStorage $newstorage */ - $newstorage = DI::storageManager()->getByName($storagebackend); + /** @var ISelectableStorage $newstorage */ + $newstorage = DI::storageManager()->getSelectableStorageByName($storagebackend); // save storage backend form $storage_opts = $newstorage->getOptions(); @@ -68,7 +68,10 @@ class Storage extends BaseAdmin } if (!empty($_POST['submit_save_set'])) { - if (empty($storagebackend) || !DI::storageManager()->setBackend($storagebackend)) { + /** @var ISelectableStorage $newstorage */ + $newstorage = DI::storageManager()->getSelectableStorageByName($storagebackend); + + if (!DI::storageManager()->setBackend($newstorage)) { notice(DI::l10n()->t('Invalid storage backend setting value.')); } } @@ -89,7 +92,7 @@ class Storage extends BaseAdmin $storage_form_prefix = preg_replace('|[^a-zA-Z0-9]|', '', $name); $storage_form = []; - foreach (DI::storageManager()->getByName($name)->getOptions() as $option => $info) { + foreach (DI::storageManager()->getSelectableStorageByName($name)->getOptions() as $option => $info) { $type = $info[0]; // Backward compatibilty with yesno field description if ($type == 'yesno') { @@ -108,7 +111,7 @@ class Storage extends BaseAdmin 'name' => $name, 'prefix' => $storage_form_prefix, 'form' => $storage_form, - 'active' => $current_storage_backend instanceof IStorage && $name === $current_storage_backend::getName(), + 'active' => $current_storage_backend instanceof ISelectableStorage && $name === $current_storage_backend::getName(), ]; } @@ -124,7 +127,7 @@ class Storage extends BaseAdmin '$noconfig' => DI::l10n()->t('This backend doesn\'t have custom settings'), '$baseurl' => DI::baseUrl()->get(true), '$form_security_token' => self::getFormSecurityToken("admin_storage"), - '$storagebackend' => $current_storage_backend instanceof IStorage ? $current_storage_backend::getName() : DI::l10n()->t('Database (legacy)'), + '$storagebackend' => $current_storage_backend instanceof ISelectableStorage ? $current_storage_backend::getName() : DI::l10n()->t('Database (legacy)'), '$availablestorageforms' => $available_storage_forms, ]); } diff --git a/src/Worker/MoveStorage.php b/src/Worker/MoveStorage.php index ac70c6354..34ecb5c14 100644 --- a/src/Worker/MoveStorage.php +++ b/src/Worker/MoveStorage.php @@ -34,7 +34,7 @@ class MoveStorage public static function execute() { $current = DI::storage(); - $moved = DI::storageManager()->move($current); + $moved = DI::storageManager()->move($current); if ($moved) { Worker::add(PRIORITY_LOW, 'MoveStorage'); diff --git a/static/dependencies.config.php b/static/dependencies.config.php index 90fa13684..ea57efca5 100644 --- a/static/dependencies.config.php +++ b/static/dependencies.config.php @@ -44,7 +44,7 @@ use Friendica\Core\Session\ISession; use Friendica\Core\StorageManager; use Friendica\Database\Database; use Friendica\Factory; -use Friendica\Model\Storage\IStorage; +use Friendica\Model\Storage\ISelectableStorage; use Friendica\Model\User\Cookie; use Friendica\Network; use Friendica\Util; @@ -213,7 +213,7 @@ return [ $_SERVER, $_COOKIE ], ], - IStorage::class => [ + ISelectableStorage::class => [ 'instanceOf' => StorageManager::class, 'call' => [ ['getBackend', [], Dice::CHAIN_CALL], diff --git a/tests/src/Core/StorageManagerTest.php b/tests/src/Core/StorageManagerTest.php index deb9c4b11..537fd841e 100644 --- a/tests/src/Core/StorageManagerTest.php +++ b/tests/src/Core/StorageManagerTest.php @@ -309,7 +309,7 @@ class StorageManagerTest extends DatabaseTest $this->loadFixture(__DIR__ . '/../../datasets/storage/database.fixture.php', $this->dba); $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n, $this->httpRequest); - $storage = $storageManager->getByName($name); + $storage = $storageManager->getSelectableStorageByName($name); $storageManager->move($storage); $photos = $this->dba->select('photo', ['backend-ref', 'backend-class', 'id', 'data']); @@ -334,7 +334,7 @@ class StorageManagerTest extends DatabaseTest $this->expectException(StorageException::class); $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n, $this->httpRequest); - $storage = $storageManager->getByName(Storage\SystemResource::getName()); + $storage = $storageManager->getSelectableStorageByName(Storage\SystemResource::getName()); $storageManager->move($storage); } }