From c17bc55158c63201203a3027f515d505a20529d9 Mon Sep 17 00:00:00 2001 From: Philipp Date: Tue, 10 Aug 2021 23:56:30 +0200 Subject: [PATCH] Introduce InvalidClassStorageException and adapt the code for it --- src/Console/Storage.php | 19 ++- src/Core/StorageManager.php | 140 ++++++--------- src/Model/Attach.php | 43 ++--- src/Model/Photo.php | 99 +++++------ .../Storage/InvalidClassStorageException.php | 29 ++++ src/Module/Admin/Storage.php | 22 ++- src/Module/Photo.php | 8 + src/Module/Settings/Profile/Photo/Crop.php | 7 + tests/src/Core/StorageManagerTest.php | 159 ++++++++---------- 9 files changed, 259 insertions(+), 267 deletions(-) create mode 100644 src/Model/Storage/InvalidClassStorageException.php diff --git a/src/Console/Storage.php b/src/Console/Storage.php index 34419c48cd..e2aa185c92 100644 --- a/src/Console/Storage.php +++ b/src/Console/Storage.php @@ -23,6 +23,7 @@ namespace Friendica\Console; use Asika\SimpleConsole\CommandArgsException; use Friendica\Core\StorageManager; +use Friendica\Model\Storage\ReferenceStorageException; use Friendica\Model\Storage\StorageException; /** @@ -127,23 +128,23 @@ HELP; protected function doSet() { - if (count($this->args) !== 2) { + if (count($this->args) !== 2 || empty($this->args[1])) { throw new CommandArgsException('Invalid arguments'); } - $name = $this->args[1]; - $class = $this->storageManager->getWritableStorageByName($name); + $name = $this->args[1]; + try { + $class = $this->storageManager->getWritableStorageByName($name); - if (is_null($class)) { + if (!$this->storageManager->setBackend($class)) { + $this->out($class . ' is not a valid backend storage class.'); + return -1; + } + } catch (ReferenceStorageException $exception) { $this->out($name . ' is not a registered backend.'); return -1; } - if (!$this->storageManager->setBackend($class)) { - $this->out($class . ' is not a valid backend storage class.'); - return -1; - } - return 0; } diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index 9e75f83058..132cd64538 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -28,7 +28,6 @@ use Friendica\Model\Storage; use Friendica\Network\HTTPException\InternalServerErrorException; use Psr\Log\LoggerInterface; - /** * Manage storage backends * @@ -46,7 +45,7 @@ class StorageManager Storage\Database::NAME => Storage\Database::class, ]; - private $backends = []; + private $backends; /** * @var Storage\IStorage[] A local cache for storage instances @@ -62,7 +61,7 @@ class StorageManager /** @var L10n */ private $l10n; - /** @var Storage\IStorage */ + /** @var Storage\IWritableStorage */ private $currentBackend; /** @@ -70,16 +69,19 @@ class StorageManager * @param IConfig $config * @param LoggerInterface $logger * @param L10n $l10n + * + * @throws Storage\InvalidClassStorageException in case the active backend class is invalid + * @throws Storage\StorageException in case of unexpected errors during the active backend class loading */ public function __construct(Database $dba, IConfig $config, LoggerInterface $logger, L10n $l10n) { - $this->dba = $dba; - $this->config = $config; - $this->logger = $logger; - $this->l10n = $l10n; + $this->dba = $dba; + $this->config = $config; + $this->logger = $logger; + $this->l10n = $l10n; $this->backends = $config->get('storage', 'backends', self::DEFAULT_BACKENDS); - $currentName = $this->config->get('storage', 'name', ''); + $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->getWritableStorageByName($currentName); @@ -88,7 +90,7 @@ class StorageManager /** * Return current storage backend class * - * @return Storage\IWritableStorage|null + * @return Storage\IWritableStorage */ public function getBackend() { @@ -102,71 +104,36 @@ class StorageManager * * @return Storage\IWritableStorage * - * @throws Storage\ReferenceStorageException in case there's no backend class for the name + * @throws Storage\InvalidClassStorageException in case there's no backend class for the name * @throws Storage\StorageException in case of an unexpected failure during the hook call */ - public function getWritableStorageByName(string $name = null) + public function getWritableStorageByName(string $name): Storage\IWritableStorage { - // @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, true)) { - 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; - default: - $data = [ - 'name' => $name, - 'storage' => null, - ]; - try { - Hook::callAll('storage_instance', $data); - if (($data['storage'] ?? null) instanceof Storage\IWritableStorage) { - $this->backendInstances[$data['name'] ?? $name] = $data['storage']; - } else { - throw new Storage\ReferenceStorageException(sprintf('Backend %s was not found', $name)); - } - } catch (InternalServerErrorException $exception) { - throw new Storage\StorageException(sprintf('Failed calling hook::storage_instance for backend %s', $name), $exception); - } - break; - } - } else { - throw new Storage\ReferenceStorageException(sprintf('Backend %s is not valid', $name)); - } + $storage = $this->getByName($name, $this->backends); + if ($storage instanceof Storage\IWritableStorage) { + return $storage; + } else { + throw new Storage\InvalidClassStorageException(sprintf('Backend %s is not writable', $name)); } - - return $this->backendInstances[$name]; } /** * Return storage backend class by registered name * - * @param string $name Backend name + * @param string $name Backend name + * @param array|null $validBackends possible, manual override of the valid backends * * @return Storage\IStorage * - * @throws Storage\ReferenceStorageException in case there's no backend class for the name + * @throws Storage\InvalidClassStorageException in case there's no backend class for the name * @throws Storage\StorageException in case of an unexpected failure during the hook call */ - public function getByName(string $name) + public function getByName(string $name, array $validBackends = null): Storage\IStorage { - // @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)) { + if ($this->isValidBackend($name, $validBackends)) { switch ($name) { // Try the filesystem backend case Storage\Filesystem::getName(): @@ -193,7 +160,7 @@ class StorageManager if (($data['storage'] ?? null) instanceof Storage\IStorage) { $this->backendInstances[$data['name'] ?? $name] = $data['storage']; } else { - throw new Storage\ReferenceStorageException(sprintf('Backend %s was not found', $name)); + throw new Storage\InvalidClassStorageException(sprintf('Backend %s was not found', $name)); } } catch (InternalServerErrorException $exception) { throw new Storage\StorageException(sprintf('Failed calling hook::storage_instance for backend %s', $name), $exception); @@ -201,7 +168,7 @@ class StorageManager break; } } else { - throw new Storage\ReferenceStorageException(sprintf('Backend %s is not valid', $name)); + throw new Storage\InvalidClassStorageException(sprintf('Backend %s is not valid', $name)); } } @@ -211,34 +178,19 @@ class StorageManager /** * Checks, if the storage is a valid backend * - * @param string|null $name The name or class of the backend - * @param boolean $onlyUserBackend True, if just user backend should get returned (e.g. not SystemResource) + * @param string|null $name The name or class of the backend + * @param array|null $validBackends Possible, valid backends to check * * @return boolean True, if the backend is a valid backend */ - public function isValidBackend(string $name = null, bool $onlyUserBackend = false) + public function isValidBackend(string $name = null, array $validBackends = null): bool { - return array_key_exists($name, $this->backends) || - (!$onlyUserBackend && in_array($name, [Storage\SystemResource::getName(), Storage\ExternalResource::getName()])); - } - - /** - * Check for legacy backend storage class names (= full model class name) - * - * @todo 2020.09 Remove this function after 2 releases, because there shouldn't be any legacy backend classes left - * - * @param string|null $name a potential, legacy storage name ("Friendica\Model\Storage\...") - * - * @return string|null The current storage name - */ - private function checkLegacyBackend(string $name = null) - { - if (stristr($name, 'Friendica\Model\Storage\\')) { - $this->logger->notice('Using deprecated storage class value', ['name' => $name]); - return substr($name, 24); - } - - return $name; + $validBackends = $validBackends ?? array_merge($this->backends, + [ + Storage\SystemResource::getName() => '', + Storage\ExternalResource::getName() => '' + ]); + return array_key_exists($name, $validBackends); } /** @@ -248,7 +200,7 @@ class StorageManager * * @return boolean True, if the set was successful */ - public function setBackend(Storage\IWritableStorage $storage) + public function setBackend(Storage\IWritableStorage $storage): bool { if ($this->config->set('storage', 'name', $storage::getName())) { $this->currentBackend = $storage; @@ -263,7 +215,7 @@ class StorageManager * * @return array */ - public function listBackends() + public function listBackends(): array { return $this->backends; } @@ -277,7 +229,7 @@ class StorageManager * * @return boolean True, if the registration was successful */ - public function register(string $class) + public function register(string $class): bool { if (is_subclass_of($class, Storage\IStorage::class)) { /** @var Storage\IStorage $class */ @@ -303,7 +255,7 @@ class StorageManager * * @return boolean True, if unregistering was successful */ - public function unregister(string $class) + public function unregister(string $class): bool { if (is_subclass_of($class, Storage\IStorage::class)) { /** @var Storage\IStorage $class */ @@ -335,9 +287,9 @@ class StorageManager * @throws Storage\StorageException * @throws Exception */ - public function move(Storage\IWritableStorage $destination, array $tables = self::TABLES, int $limit = 5000) + public function move(Storage\IWritableStorage $destination, array $tables = self::TABLES, int $limit = 5000): int { - if (!$this->isValidBackend($destination, true)) { + if (!$this->isValidBackend($destination, $this->backends)) { throw new Storage\StorageException(sprintf("Can't move to storage backend '%s'", $destination::getName())); } @@ -353,13 +305,19 @@ class StorageManager while ($resource = $this->dba->fetch($resources)) { $id = $resource['id']; - $data = $resource['data']; - $source = $this->getWritableStorageByName($resource['backend-class']); $sourceRef = $resource['backend-ref']; + $source = null; - if (!empty($source)) { + try { + $source = $this->getWritableStorageByName($resource['backend-class'] ?? ''); $this->logger->info('Get data from old backend.', ['oldBackend' => $source, 'oldReference' => $sourceRef]); $data = $source->get($sourceRef); + } catch (Storage\InvalidClassStorageException $exception) { + $this->logger->info('Get data from DB resource field.', ['oldReference' => $sourceRef]); + $data = $resource['data']; + } catch (Storage\ReferenceStorageException $exception) { + $this->logger->info('Invalid source reference.', ['oldBackend' => $source, 'oldReference' => $sourceRef]); + continue; } $this->logger->info('Save data to new backend.', ['newBackend' => $destination::getName()]); diff --git a/src/Model/Attach.php b/src/Model/Attach.php index ac8bca07f1..e11fd01bc3 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\InvalidClassStorageException; use Friendica\Model\Storage\ReferenceStorageException; use Friendica\Object\Image; use Friendica\Util\DateTimeFormat; @@ -164,22 +165,20 @@ class Attach return $item['data']; } - $backendClass = DI::storageManager()->getByName($item['backend-class'] ?? ''); - if (empty($backendClass)) { + try { + $backendClass = DI::storageManager()->getByName($item['backend-class'] ?? ''); + $backendRef = $item['backend-ref']; + return $backendClass->get($backendRef); + } catch (InvalidClassStorageException $storageException) { // legacy data storage in 'data' column $i = self::selectFirst(['data'], ['id' => $item['id']]); if ($i === false) { return null; } return $i['data']; - } else { - $backendRef = $item['backend-ref']; - try { - return $backendClass->get($backendRef); - } catch (ReferenceStorageException $referenceStorageException) { - DI::logger()->debug('No data found for item', ['item' => $item, 'exception' => $referenceStorageException]); - return ''; - } + } catch (ReferenceStorageException $referenceStorageException) { + DI::logger()->debug('No data found for item', ['item' => $item, 'exception' => $referenceStorageException]); + return ''; } } @@ -284,11 +283,13 @@ class Attach $items = self::selectToArray(['backend-class','backend-ref'], $conditions); foreach($items as $item) { - $backend_class = DI::storageManager()->getWritableStorageByName($item['backend-class'] ?? ''); - if (!empty($backend_class)) { + try { + $backend_class = DI::storageManager()->getWritableStorageByName($item['backend-class'] ?? ''); $fields['backend-ref'] = $backend_class->put($img->asString(), $item['backend-ref'] ?? ''); - } else { - $fields['data'] = $img->asString(); + } catch (InvalidClassStorageException $storageException) { + DI::logger()->debug('Storage class not found.', ['conditions' => $conditions, 'exception' => $storageException]); + } catch (ReferenceStorageException $referenceStorageException) { + DI::logger()->debug('Item doesn\'t exist.', ['conditions' => $conditions, 'exception' => $referenceStorageException]); } } } @@ -316,13 +317,13 @@ class Attach $items = self::selectToArray(['backend-class','backend-ref'], $conditions); foreach($items as $item) { - $backend_class = DI::storageManager()->getWritableStorageByName($item['backend-class'] ?? ''); - if (!empty($backend_class)) { - try { - $backend_class->delete($item['backend-ref'] ?? ''); - } catch (ReferenceStorageException $referenceStorageException) { - DI::logger()->debug('Item doesn\'t exist.', ['conditions' => $conditions, 'exception' => $referenceStorageException]); - } + try { + $backend_class = DI::storageManager()->getWritableStorageByName($item['backend-class'] ?? ''); + $backend_class->delete($item['backend-ref'] ?? ''); + } catch (InvalidClassStorageException $storageException) { + DI::logger()->debug('Storage class not found.', ['conditions' => $conditions, 'exception' => $storageException]); + } 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 28eb6f3611..8df9b92f2d 100644 --- a/src/Model/Photo.php +++ b/src/Model/Photo.php @@ -28,6 +28,7 @@ use Friendica\Database\DBA; use Friendica\Database\DBStructure; use Friendica\DI; use Friendica\Model\Storage\ExternalResource; +use Friendica\Model\Storage\InvalidClassStorageException; use Friendica\Model\Storage\ReferenceStorageException; use Friendica\Model\Storage\StorageException; use Friendica\Model\Storage\SystemResource; @@ -186,9 +187,6 @@ class Photo * @param array $photo Photo data. Needs at least 'id', 'type', 'backend-class', 'backend-ref' * * @return \Friendica\Object\Image - * @throws \Friendica\Network\HTTPException\InternalServerErrorException - * @throws \ImagickException - * @throws StorageException */ public static function getImageDataForPhoto(array $photo) { @@ -196,24 +194,36 @@ class Photo return $photo['data']; } - $backendClass = DI::storageManager()->getByName($photo['backend-class'] ?? ''); - if (empty($backendClass)) { - // legacy data storage in "data" column - $i = self::selectFirst(['data'], ['id' => $photo['id']]); - if ($i === false) { - return null; + try { + $backendClass = DI::storageManager()->getByName($photo['backend-class'] ?? ''); + $image = $backendClass->get($photo['backend-ref'] ?? ''); + + if ($image instanceof Image) { + return $image; + } else { + DI::logger()->info('Stored data is not an image', ['photo' => $photo]); } - $data = $i['data']; - } else { - $backendRef = $photo['backend-ref'] ?? ''; + } catch (InvalidClassStorageException $storageException) { try { - $data = $backendClass->get($backendRef); - } catch (ReferenceStorageException $referenceStorageException) { - DI::logger()->debug('No data found for photo', ['photo' => $photo, 'exception' => $referenceStorageException]); - return null; + // legacy data storage in "data" column + $i = self::selectFirst(['data'], ['id' => $photo['id']]); + if ($i !== false) { + return $i['data']; + } else { + DI::logger()->info('Stored legacy data is empty', ['photo' => $photo]); + } + } catch (\Exception $exception) { + DI::logger()->info('Unexpected database exception', ['photo' => $photo, 'exception' => $exception]); } + } catch (ReferenceStorageException $referenceStorageException) { + DI::logger()->debug('Invalid reference for photo', ['photo' => $photo, 'exception' => $referenceStorageException]); + } catch (StorageException $storageException) { + DI::logger()->info('Unexpected storage exception', ['photo' => $photo, 'exception' => $storageException]); + } catch (\ImagickException $imagickException) { + DI::logger()->info('Unexpected imagick exception', ['photo' => $photo, 'exception' => $imagickException]); } - return $data; + + return null; } /** @@ -225,14 +235,9 @@ class Photo * @throws \Friendica\Network\HTTPException\InternalServerErrorException * @throws \ImagickException */ - public static function getImageForPhoto(array $photo) + public static function getImageForPhoto(array $photo): Image { - $data = self::getImageDataForPhoto($photo); - if (empty($data)) { - return null; - } - - return new Image($data, $photo['type']); + return new Image(self::getImageDataForPhoto($photo), $photo['type']); } /** @@ -342,20 +347,20 @@ class Photo // Get defined storage backend. // if no storage backend, we use old "data" column in photo table. // if is an existing photo, reuse same backend - $data = ""; + $data = ""; $backend_ref = ""; + $storage = ""; - if (DBA::isResult($existing_photo)) { - $backend_ref = (string)$existing_photo["backend-ref"]; - $storage = DI::storageManager()->getWritableStorageByName($existing_photo["backend-class"] ?? ''); - } else { - $storage = DI::storage(); - } - - if (empty($storage)) { - $data = $Image->asString(); - } else { + try { + if (DBA::isResult($existing_photo)) { + $backend_ref = (string)$existing_photo["backend-ref"]; + $storage = DI::storageManager()->getWritableStorageByName($existing_photo["backend-class"] ?? ''); + } else { + $storage = DI::storage(); + } $backend_ref = $storage->put($Image->asString(), $backend_ref); + } catch (InvalidClassStorageException $storageException) { + $data = $Image->asString(); } $fields = [ @@ -411,15 +416,15 @@ class Photo $photos = DBA::select('photo', ['id', 'backend-class', 'backend-ref'], $conditions); while ($photo = DBA::fetch($photos)) { - $backend_class = DI::storageManager()->getWritableStorageByName($photo['backend-class'] ?? ''); - if (!empty($backend_class)) { - 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]); - } + try { + $backend_class = DI::storageManager()->getWritableStorageByName($photo['backend-class'] ?? ''); + $backend_class->delete($item['backend-ref'] ?? ''); + // Delete the photos after they had been deleted successfully + DBA::delete("photo", ['id' => $photo['id']]); + } catch (InvalidClassStorageException $storageException) { + DI::logger()->debug('Storage class not found.', ['conditions' => $conditions, 'exception' => $storageException]); + } catch (ReferenceStorageException $referenceStorageException) { + DI::logger()->debug('Photo doesn\'t exist.', ['conditions' => $conditions, 'exception' => $referenceStorageException]); } } @@ -448,10 +453,10 @@ class Photo $photos = self::selectToArray(['backend-class', 'backend-ref'], $conditions); foreach($photos as $photo) { - $backend_class = DI::storageManager()->getWritableStorageByName($photo['backend-class'] ?? ''); - if (!empty($backend_class)) { + try { + $backend_class = DI::storageManager()->getWritableStorageByName($photo['backend-class'] ?? ''); $fields["backend-ref"] = $backend_class->put($img->asString(), $photo['backend-ref']); - } else { + } catch (InvalidClassStorageException $storageException) { $fields["data"] = $img->asString(); } } diff --git a/src/Model/Storage/InvalidClassStorageException.php b/src/Model/Storage/InvalidClassStorageException.php new file mode 100644 index 0000000000..9c39b3a60c --- /dev/null +++ b/src/Model/Storage/InvalidClassStorageException.php @@ -0,0 +1,29 @@ +. + * + */ + +namespace Friendica\Model\Storage; + +/** + * Storage Exception in case of invalid storage class + */ +class InvalidClassStorageException extends StorageException +{ +} diff --git a/src/Module/Admin/Storage.php b/src/Module/Admin/Storage.php index e12cecd031..6aac6aace7 100644 --- a/src/Module/Admin/Storage.php +++ b/src/Module/Admin/Storage.php @@ -23,6 +23,7 @@ namespace Friendica\Module\Admin; use Friendica\Core\Renderer; use Friendica\DI; +use Friendica\Model\Storage\InvalidClassStorageException; use Friendica\Model\Storage\IWritableStorage; use Friendica\Module\BaseAdmin; use Friendica\Util\Strings; @@ -37,8 +38,13 @@ class Storage extends BaseAdmin $storagebackend = Strings::escapeTags(trim($parameters['name'] ?? '')); - /** @var IWritableStorage $newstorage */ - $newstorage = DI::storageManager()->getWritableStorageByName($storagebackend); + try { + /** @var IWritableStorage $newstorage */ + $newstorage = DI::storageManager()->getWritableStorageByName($storagebackend); + } catch (InvalidClassStorageException $storageException) { + notice(DI::l10n()->t('Storage backend, %s is invalid.', $storagebackend)); + DI::baseUrl()->redirect('admin/storage'); + } // save storage backend form $storage_opts = $newstorage->getOptions(); @@ -62,16 +68,20 @@ class Storage extends BaseAdmin $storage_form_errors = $newstorage->saveOptions($storage_opts_data); if (count($storage_form_errors)) { foreach ($storage_form_errors as $name => $err) { - notice('Storage backend, ' . $storage_opts[$name][1] . ': ' . $err); + notice(DI::l10n()->t('Storage backend %s error: %s', $storage_opts[$name][1], $err)); } DI::baseUrl()->redirect('admin/storage'); } if (!empty($_POST['submit_save_set'])) { - /** @var IWritableStorage $newstorage */ - $newstorage = DI::storageManager()->getWritableStorageByName($storagebackend); + try { + /** @var IWritableStorage $newstorage */ + $newstorage = DI::storageManager()->getWritableStorageByName($storagebackend); - if (!DI::storageManager()->setBackend($newstorage)) { + if (!DI::storageManager()->setBackend($newstorage)) { + notice(DI::l10n()->t('Invalid storage backend setting value.')); + } + } catch (InvalidClassStorageException $storageException) { notice(DI::l10n()->t('Invalid storage backend setting value.')); } } diff --git a/src/Module/Photo.php b/src/Module/Photo.php index 06a369df77..9d2d31b45d 100644 --- a/src/Module/Photo.php +++ b/src/Module/Photo.php @@ -30,7 +30,11 @@ use Friendica\Model\Photo as MPhoto; use Friendica\Model\Post; use Friendica\Model\Profile; use Friendica\Model\Storage\ExternalResource; +use Friendica\Model\Storage\ReferenceStorageException; +use Friendica\Model\Storage\StorageException; use Friendica\Model\Storage\SystemResource; +use Friendica\Network\HTTPException\InternalServerErrorException; +use Friendica\Network\HTTPException\NotFoundException; use Friendica\Util\Proxy; use Friendica\Object\Image; use Friendica\Util\Images; @@ -105,7 +109,11 @@ class Photo extends BaseModule $cacheable = ($photo["allow_cid"] . $photo["allow_gid"] . $photo["deny_cid"] . $photo["deny_gid"] === "") && (isset($photo["cacheable"]) ? $photo["cacheable"] : true); $stamp = microtime(true); + $imgdata = MPhoto::getImageDataForPhoto($photo); + if (empty($imgdata)) { + throw new NotFoundException(); + } // The mimetype for an external or system resource can only be known reliably after it had been fetched if (in_array($photo['backend-class'], [ExternalResource::NAME, SystemResource::NAME])) { diff --git a/src/Module/Settings/Profile/Photo/Crop.php b/src/Module/Settings/Profile/Photo/Crop.php index 83722c8661..ba96e0032f 100644 --- a/src/Module/Settings/Profile/Photo/Crop.php +++ b/src/Module/Settings/Profile/Photo/Crop.php @@ -61,6 +61,10 @@ class Crop extends BaseSettings $base_image = Photo::selectFirst([], ['resource-id' => $resource_id, 'uid' => local_user(), 'scale' => $scale]); if (DBA::isResult($base_image)) { $Image = Photo::getImageForPhoto($base_image); + if (empty($Image)) { + throw new HTTPException\InternalServerErrorException(); + } + if ($Image->isValid()) { // If setting for the default profile, unset the profile photo flag from any other photos I own DBA::update('photo', ['profile' => 0], ['uid' => local_user()]); @@ -188,6 +192,9 @@ class Crop extends BaseSettings } $Image = Photo::getImageForPhoto($photos[0]); + if (empty($Image)) { + throw new HTTPException\InternalServerErrorException(); + } $imagecrop = [ 'resource-id' => $resource_id, diff --git a/tests/src/Core/StorageManagerTest.php b/tests/src/Core/StorageManagerTest.php index 25ae44b1bc..c4ba9368cf 100644 --- a/tests/src/Core/StorageManagerTest.php +++ b/tests/src/Core/StorageManagerTest.php @@ -34,7 +34,6 @@ use Friendica\Factory\ConfigFactory; use Friendica\Model\Config\Config; use Friendica\Model\Storage; use Friendica\Core\Session; -use Friendica\Model\Storage\StorageException; use Friendica\Network\HTTPRequest; use Friendica\Test\DatabaseTest; use Friendica\Test\Util\Database\StaticDatabase; @@ -47,6 +46,7 @@ use Friendica\Test\Util\SampleStorageBackend; class StorageManagerTest extends DatabaseTest { + use VFSTrait; /** @var Database */ private $dba; /** @var IConfig */ @@ -58,8 +58,6 @@ class StorageManagerTest extends DatabaseTest /** @var HTTPRequest */ private $httpRequest; - use VFSTrait; - protected function setUp(): void { parent::setUp(); @@ -82,6 +80,7 @@ class StorageManagerTest extends DatabaseTest $configModel = new Config($this->dba); $this->config = new PreloadConfig($configCache, $configModel); + $this->config->set('storage', 'name', 'Database'); $this->l10n = \Mockery::mock(L10n::class); @@ -101,34 +100,38 @@ class StorageManagerTest extends DatabaseTest public function dataStorages() { return [ - 'empty' => [ - 'name' => '', - 'assert' => null, - 'assertName' => '', - 'userBackend' => false, + 'empty' => [ + 'name' => '', + 'valid' => false, + 'interface' => Storage\IStorage::class, + 'assert' => null, + 'assertName' => '', ], - 'database' => [ - 'name' => Storage\Database::NAME, - 'assert' => Storage\Database::class, - 'assertName' => Storage\Database::NAME, - 'userBackend' => true, + 'database' => [ + 'name' => Storage\Database::NAME, + 'valid' => true, + 'interface' => Storage\IWritableStorage::class, + 'assert' => Storage\Database::class, + 'assertName' => Storage\Database::NAME, ], - 'filesystem' => [ - 'name' => Storage\Filesystem::NAME, - 'assert' => Storage\Filesystem::class, - 'assertName' => Storage\Filesystem::NAME, - 'userBackend' => true, + 'filesystem' => [ + 'name' => Storage\Filesystem::NAME, + 'valid' => true, + 'interface' => Storage\IWritableStorage::class, + 'assert' => Storage\Filesystem::class, + 'assertName' => Storage\Filesystem::NAME, ], 'systemresource' => [ - 'name' => Storage\SystemResource::NAME, - 'assert' => Storage\SystemResource::class, - 'assertName' => Storage\SystemResource::NAME, - // false here, because SystemResource isn't meant to be a user backend, - // it's for system resources only - 'userBackend' => false, + 'name' => Storage\SystemResource::NAME, + 'valid' => true, + 'interface' => Storage\IStorage::class, + 'assert' => Storage\SystemResource::class, + 'assertName' => Storage\SystemResource::NAME, ], - 'invalid' => [ + 'invalid' => [ 'name' => 'invalid', + 'valid' => false, + 'interface' => null, 'assert' => null, 'assertName' => '', 'userBackend' => false, @@ -136,59 +139,31 @@ class StorageManagerTest extends DatabaseTest ]; } - /** - * Data array for legacy backends - * - * @todo 2020.09 After 2 releases, remove the legacy functionality and these data array with it - * - * @return array - */ - public function dataLegacyBackends() - { - return [ - 'legacyDatabase' => [ - 'name' => 'Friendica\Model\Storage\Database', - 'assert' => Storage\Database::class, - 'assertName' => Storage\Database::NAME, - 'userBackend' => true, - ], - 'legacyFilesystem' => [ - 'name' => 'Friendica\Model\Storage\Filesystem', - 'assert' => Storage\Filesystem::class, - 'assertName' => Storage\Filesystem::NAME, - 'userBackend' => true, - ], - 'legacySystemResource' => [ - 'name' => 'Friendica\Model\Storage\SystemResource', - 'assert' => Storage\SystemResource::class, - 'assertName' => Storage\SystemResource::NAME, - 'userBackend' => false, - ], - ]; - } - /** * Test the getByName() method * * @dataProvider dataStorages - * @dataProvider dataLegacyBackends */ - public function testGetByName($name, $assert, $assertName, $userBackend) + public function testGetByName($name, $valid, $interface, $assert, $assertName) { - $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n, $this->httpRequest); + if (!$valid) { + $this->expectException(Storage\InvalidClassStorageException::class); + } - if ($userBackend) { + if ($interface === Storage\IWritableStorage::class) { + $this->config->set('storage', 'name', $name); + } + + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n); + + if ($interface === Storage\IWritableStorage::class) { $storage = $storageManager->getWritableStorageByName($name); } else { $storage = $storageManager->getByName($name); } - if (!empty($assert)) { - self::assertInstanceOf(Storage\IStorage::class, $storage); - self::assertInstanceOf($assert, $storage); - } else { - self::assertNull($storage); - } + self::assertInstanceOf($interface, $storage); + self::assertInstanceOf($assert, $storage); self::assertEquals($assertName, $storage); } @@ -197,15 +172,15 @@ class StorageManagerTest extends DatabaseTest * * @dataProvider dataStorages */ - public function testIsValidBackend($name, $assert, $assertName, $userBackend) + public function testIsValidBackend($name, $valid, $interface, $assert, $assertName) { $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n); // true in every of the backends self::assertEquals(!empty($assertName), $storageManager->isValidBackend($name)); - // if userBackend is set to true, filter out e.g. SystemRessource - self::assertEquals($userBackend, $storageManager->isValidBackend($name, true)); + // if it's a IWritableStorage, the valid backend should return true, otherwise false + self::assertEquals($interface === Storage\IWritableStorage::class, $storageManager->isValidBackend($name, StorageManager::DEFAULT_BACKENDS)); } /** @@ -223,37 +198,35 @@ class StorageManagerTest extends DatabaseTest * * @dataProvider dataStorages */ - public function testGetBackend($name, $assert, $assertName, $userBackend) + public function testGetBackend($name, $valid, $interface, $assert, $assertName) { + if ($interface !== Storage\IWritableStorage::class) { + static::markTestSkipped('only works for IWritableStorage'); + } + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n); - self::assertNull($storageManager->getBackend()); + $selBackend = $storageManager->getWritableStorageByName($name); + $storageManager->setBackend($selBackend); - if ($userBackend) { - $selBackend = $storageManager->getWritableStorageByName($name); - $storageManager->setBackend($selBackend); - - self::assertInstanceOf($assert, $storageManager->getBackend()); - } + self::assertInstanceOf($assert, $storageManager->getBackend()); } /** * Test the method getBackend() with a pre-configured backend * * @dataProvider dataStorages - * @dataProvider dataLegacyBackends */ - public function testPresetBackend($name, $assert, $assertName, $userBackend) + public function testPresetBackend($name, $valid, $interface, $assert, $assertName) { $this->config->set('storage', 'name', $name); + if ($interface !== Storage\IWritableStorage::class) { + $this->expectException(Storage\InvalidClassStorageException::class); + } $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n); - if ($userBackend) { - self::assertInstanceOf($assert, $storageManager->getBackend()); - } else { - self::assertNull($storageManager->getBackend()); - } + self::assertInstanceOf($assert, $storageManager->getBackend()); } /** @@ -266,7 +239,7 @@ class StorageManagerTest extends DatabaseTest public function testRegisterUnregisterBackends() { /// @todo Remove dice once "Hook" is dynamic and mockable - $dice = (new Dice()) + $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]); @@ -287,7 +260,7 @@ class StorageManagerTest extends DatabaseTest SampleStorageBackend::registerHook(); Hook::loadHooks(); - self::assertTrue($storageManager->setBackend( $storageManager->getWritableStorageByName(SampleStorageBackend::NAME))); + self::assertTrue($storageManager->setBackend($storageManager->getWritableStorageByName(SampleStorageBackend::NAME))); self::assertEquals(SampleStorageBackend::NAME, $this->config->get('storage', 'name')); self::assertInstanceOf(SampleStorageBackend::class, $storageManager->getBackend()); @@ -305,26 +278,25 @@ class StorageManagerTest extends DatabaseTest * * @dataProvider dataStorages */ - public function testMoveStorage($name, $assert, $assertName, $userBackend) + public function testMoveStorage($name, $valid, $interface, $assert, $assertName) { - if (!$userBackend) { + if ($interface !== Storage\IWritableStorage::class) { self::markTestSkipped("No user backend"); } $this->loadFixture(__DIR__ . '/../../datasets/storage/database.fixture.php', $this->dba); $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n); - $storage = $storageManager->getWritableStorageByName($name); + $storage = $storageManager->getWritableStorageByName($name); $storageManager->move($storage); $photos = $this->dba->select('photo', ['backend-ref', 'backend-class', 'id', 'data']); while ($photo = $this->dba->fetch($photos)) { - self::assertEmpty($photo['data']); $storage = $storageManager->getByName($photo['backend-class']); - $data = $storage->get($photo['backend-ref']); + $data = $storage->get($photo['backend-ref']); self::assertNotEmpty($data); } @@ -335,10 +307,11 @@ class StorageManagerTest extends DatabaseTest */ public function testWrongWritableStorage() { - $this->expectException(\TypeError::class); + $this->expectException(Storage\InvalidClassStorageException::class); + $this->expectExceptionMessage('Backend SystemResource is not valid'); $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n); - $storage = $storageManager->getWritableStorageByName(Storage\SystemResource::getName()); + $storage = $storageManager->getWritableStorageByName(Storage\SystemResource::getName()); $storageManager->move($storage); } }