From 065b46c72156a52b595b054be30fdd7b6796e76b Mon Sep 17 00:00:00 2001 From: Philipp Date: Mon, 4 Oct 2021 10:25:29 +0200 Subject: [PATCH] Split Storage usage and Storage configuration --- src/Core/StorageManager.php | 40 +++++++- src/Model/Storage/Database.php | 16 --- src/Model/Storage/Filesystem.php | 52 +--------- src/Model/Storage/FilesystemConfig.php | 99 +++++++++++++++++++ src/Model/Storage/IStorageConfiguration.php | 78 +++++++++++++++ src/Model/Storage/IWritableStorage.php | 50 ---------- src/Module/Admin/Storage.php | 82 ++++++++------- .../src/Model/Storage/DatabaseStorageTest.php | 13 +-- .../Storage/FilesystemStorageConfigTest.php | 67 +++++++++++++ .../Model/Storage/FilesystemStorageTest.php | 35 +------ tests/src/Model/Storage/StorageConfigTest.php | 43 ++++++++ tests/src/Model/Storage/StorageTest.php | 12 --- 12 files changed, 379 insertions(+), 208 deletions(-) create mode 100644 src/Model/Storage/FilesystemConfig.php create mode 100644 src/Model/Storage/IStorageConfiguration.php create mode 100644 tests/src/Model/Storage/FilesystemStorageConfigTest.php create mode 100644 tests/src/Model/Storage/StorageConfigTest.php diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index f206868885..61dd8d375d 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -119,6 +119,43 @@ class StorageManager return $storage; } + /** + * Return storage backend configuration by registered name + * + * @param string $name Backend name + * + * @return Storage\IStorageConfiguration|false + * + * @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 getConfigurationByName(string $name) + { + switch ($name) { + // Try the filesystem backend + case Storage\Filesystem::getName(): + return new Storage\FilesystemConfig($this->config, $this->l10n); + // try the database backend + case Storage\Database::getName(): + return false; + default: + $data = [ + 'name' => $name, + 'storage_config' => null, + ]; + try { + Hook::callAll('storage_config', $data); + if (!($data['storage_config'] ?? null) instanceof Storage\IStorageConfiguration) { + throw new Storage\InvalidClassStorageException(sprintf('Configuration for backend %s was not found', $name)); + } + + return $data['storage_config']; + } catch (InternalServerErrorException $exception) { + throw new Storage\StorageException(sprintf('Failed calling hook::storage_config for backend %s', $name), $exception); + } + } + } + /** * Return storage backend class by registered name * @@ -142,7 +179,8 @@ class StorageManager switch ($name) { // Try the filesystem backend case Storage\Filesystem::getName(): - $this->backendInstances[$name] = new Storage\Filesystem($this->config, $this->l10n); + $storageConfig = new Storage\FilesystemConfig($this->config, $this->l10n); + $this->backendInstances[$name] = new Storage\Filesystem($storageConfig->getStoragePath()); break; // try the database backend case Storage\Database::getName(): diff --git a/src/Model/Storage/Database.php b/src/Model/Storage/Database.php index 3457fa4a30..7b90d87890 100644 --- a/src/Model/Storage/Database.php +++ b/src/Model/Storage/Database.php @@ -113,22 +113,6 @@ class Database implements IWritableStorage } } - /** - * @inheritDoc - */ - public function getOptions(): array - { - return []; - } - - /** - * @inheritDoc - */ - public function saveOptions(array $data): array - { - return []; - } - /** * @inheritDoc */ diff --git a/src/Model/Storage/Filesystem.php b/src/Model/Storage/Filesystem.php index c6c939bd46..5a4ea1f0d3 100644 --- a/src/Model/Storage/Filesystem.php +++ b/src/Model/Storage/Filesystem.php @@ -22,8 +22,6 @@ namespace Friendica\Model\Storage; use Exception; -use Friendica\Core\Config\IConfig; -use Friendica\Core\L10n; use Friendica\Util\Strings; /** @@ -40,30 +38,17 @@ class Filesystem implements IWritableStorage { const NAME = 'Filesystem'; - // Default base folder - const DEFAULT_BASE_FOLDER = 'storage'; - - /** @var IConfig */ - private $config; - /** @var string */ private $basePath; - /** @var L10n */ - private $l10n; - /** * Filesystem constructor. * - * @param IConfig $config - * @param L10n $l10n + * @param string $filesystemPath */ - public function __construct(IConfig $config, L10n $l10n) + public function __construct(string $filesystemPath = FilesystemConfig::DEFAULT_BASE_FOLDER) { - $this->config = $config; - $this->l10n = $l10n; - - $path = $this->config->get('storage', 'filesystem_path', self::DEFAULT_BASE_FOLDER); + $path = $filesystemPath; $this->basePath = rtrim($path, '/'); } @@ -176,37 +161,6 @@ class Filesystem implements IWritableStorage } } - /** - * @inheritDoc - */ - public function getOptions(): array - { - return [ - 'storagepath' => [ - 'input', - $this->l10n->t('Storage base path'), - $this->basePath, - $this->l10n->t('Folder where uploaded files are saved. For maximum security, This should be a path outside web server folder tree') - ] - ]; - } - - /** - * @inheritDoc - */ - public function saveOptions(array $data): array - { - $storagePath = $data['storagepath'] ?? ''; - if ($storagePath === '' || !is_dir($storagePath)) { - return [ - 'storagepath' => $this->l10n->t('Enter a valid existing folder') - ]; - }; - $this->config->set('storage', 'filesystem_path', $storagePath); - $this->basePath = $storagePath; - return []; - } - /** * @inheritDoc */ diff --git a/src/Model/Storage/FilesystemConfig.php b/src/Model/Storage/FilesystemConfig.php new file mode 100644 index 0000000000..735808e822 --- /dev/null +++ b/src/Model/Storage/FilesystemConfig.php @@ -0,0 +1,99 @@ +. + * + */ + +namespace Friendica\Model\Storage; + +use Friendica\Core\Config\IConfig; +use Friendica\Core\L10n; + +/** + * Filesystem based storage backend configuration + */ +class FilesystemConfig implements IStorageConfiguration +{ + // Default base folder + const DEFAULT_BASE_FOLDER = 'storage'; + + /** @var IConfig */ + private $config; + + /** @var string */ + private $storagePath; + + /** @var L10n */ + private $l10n; + + /** + * Returns the current storage path + * + * @return string + */ + public function getStoragePath(): string + { + return $this->storagePath; + } + + /** + * Filesystem constructor. + * + * @param IConfig $config + * @param L10n $l10n + */ + public function __construct(IConfig $config, L10n $l10n) + { + $this->config = $config; + $this->l10n = $l10n; + + $path = $this->config->get('storage', 'filesystem_path', self::DEFAULT_BASE_FOLDER); + $this->storagePath = rtrim($path, '/'); + } + + /** + * @inheritDoc + */ + public function getOptions(): array + { + return [ + 'storagepath' => [ + 'input', + $this->l10n->t('Storage base path'), + $this->storagePath, + $this->l10n->t('Folder where uploaded files are saved. For maximum security, This should be a path outside web server folder tree') + ] + ]; + } + + /** + * @inheritDoc + */ + public function saveOptions(array $data): array + { + $storagePath = $data['storagepath'] ?? ''; + if ($storagePath === '' || !is_dir($storagePath)) { + return [ + 'storagepath' => $this->l10n->t('Enter a valid existing folder') + ]; + }; + $this->config->set('storage', 'filesystem_path', $storagePath); + $this->storagePath = $storagePath; + return []; + } +} diff --git a/src/Model/Storage/IStorageConfiguration.php b/src/Model/Storage/IStorageConfiguration.php new file mode 100644 index 0000000000..c589f5ed5c --- /dev/null +++ b/src/Model/Storage/IStorageConfiguration.php @@ -0,0 +1,78 @@ +. + * + */ + +namespace Friendica\Model\Storage; + +/** + * The interface to use for configurable storage backends + */ +interface IStorageConfiguration +{ + /** + * Get info about storage options + * + * @return array + * + * This method return an array with information about storage options + * from which the form presented to the user is build. + * + * The returned array is: + * + * [ + * 'option1name' => [ ..info.. ], + * 'option2name' => [ ..info.. ], + * ... + * ] + * + * An empty array can be returned if backend doesn't have any options + * + * The info array for each option MUST be as follows: + * + * [ + * 'type', // define the field used in form, and the type of data. + * // one of 'checkbox', 'combobox', 'custom', 'datetime', + * // 'input', 'intcheckbox', 'password', 'radio', 'richtext' + * // 'select', 'select_raw', 'textarea' + * + * 'label', // Translatable label of the field + * 'value', // Current value + * 'help text', // Translatable description for the field + * extra data // Optional. Depends on 'type': + * // select: array [ value => label ] of choices + * // intcheckbox: value of input element + * // select_raw: prebuild html string of < option > tags + * ] + * + * See https://github.com/friendica/friendica/wiki/Quick-Template-Guide + */ + public function getOptions(): array; + + /** + * Validate and save options + * + * @param array $data Array [optionname => value] to be saved + * + * @return array Validation errors: [optionname => error message] + * + * Return array must be empty if no error. + */ + public function saveOptions(array $data): array; +} diff --git a/src/Model/Storage/IWritableStorage.php b/src/Model/Storage/IWritableStorage.php index ee0001a669..118f4b2561 100644 --- a/src/Model/Storage/IWritableStorage.php +++ b/src/Model/Storage/IWritableStorage.php @@ -50,54 +50,4 @@ interface IWritableStorage extends IStorage * @throws ReferenceStorageException in case the reference doesn't exist */ public function delete(string $reference); - - /** - * Get info about storage options - * - * @return array - * - * This method return an array with informations about storage options - * from which the form presented to the user is build. - * - * The returned array is: - * - * [ - * 'option1name' => [ ..info.. ], - * 'option2name' => [ ..info.. ], - * ... - * ] - * - * An empty array can be returned if backend doesn't have any options - * - * The info array for each option MUST be as follows: - * - * [ - * 'type', // define the field used in form, and the type of data. - * // one of 'checkbox', 'combobox', 'custom', 'datetime', - * // 'input', 'intcheckbox', 'password', 'radio', 'richtext' - * // 'select', 'select_raw', 'textarea' - * - * 'label', // Translatable label of the field - * 'value', // Current value - * 'help text', // Translatable description for the field - * extra data // Optional. Depends on 'type': - * // select: array [ value => label ] of choices - * // intcheckbox: value of input element - * // select_raw: prebuild html string of < option > tags - * ] - * - * See https://github.com/friendica/friendica/wiki/Quick-Template-Guide - */ - public function getOptions(): array; - - /** - * Validate and save options - * - * @param array $data Array [optionname => value] to be saved - * - * @return array Validation errors: [optionname => error message] - * - * Return array must be empty if no error. - */ - public function saveOptions(array $data): array; } diff --git a/src/Module/Admin/Storage.php b/src/Module/Admin/Storage.php index c6bc3878ef..6b22d905cf 100644 --- a/src/Module/Admin/Storage.php +++ b/src/Module/Admin/Storage.php @@ -24,6 +24,7 @@ namespace Friendica\Module\Admin; use Friendica\Core\Renderer; use Friendica\DI; use Friendica\Model\Storage\InvalidClassStorageException; +use Friendica\Model\Storage\IStorageConfiguration; use Friendica\Model\Storage\IWritableStorage; use Friendica\Module\BaseAdmin; use Friendica\Util\Strings; @@ -39,38 +40,40 @@ class Storage extends BaseAdmin $storagebackend = Strings::escapeTags(trim($parameters['name'] ?? '')); try { - /** @var IWritableStorage $newstorage */ - $newstorage = DI::storageManager()->getWritableStorageByName($storagebackend); + /** @var IStorageConfiguration|false $newStorageConfig */ + $newStorageConfig = DI::storageManager()->getConfigurationByName($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(); - $storage_form_prefix = preg_replace('|[^a-zA-Z0-9]|', '', $storagebackend); - $storage_opts_data = []; - foreach ($storage_opts as $name => $info) { - $fieldname = $storage_form_prefix . '_' . $name; - switch ($info[0]) { // type - case 'checkbox': - case 'yesno': - $value = !empty($_POST[$fieldname]); - break; - default: - $value = $_POST[$fieldname] ?? ''; + if ($newStorageConfig !== false) { + // save storage backend form + $storage_opts = $newStorageConfig->getOptions(); + $storage_form_prefix = preg_replace('|[^a-zA-Z0-9]|', '', $storagebackend); + $storage_opts_data = []; + foreach ($storage_opts as $name => $info) { + $fieldname = $storage_form_prefix . '_' . $name; + switch ($info[0]) { // type + case 'checkbox': + case 'yesno': + $value = !empty($_POST[$fieldname]); + break; + default: + $value = $_POST[$fieldname] ?? ''; + } + $storage_opts_data[$name] = $value; } - $storage_opts_data[$name] = $value; - } - unset($name); - unset($info); + unset($name); + unset($info); - $storage_form_errors = $newstorage->saveOptions($storage_opts_data); - if (count($storage_form_errors)) { - foreach ($storage_form_errors as $name => $err) { - notice(DI::l10n()->t('Storage backend %s error: %s', $storage_opts[$name][1], $err)); + $storage_form_errors = $newStorageConfig->saveOptions($storage_opts_data); + if (count($storage_form_errors)) { + foreach ($storage_form_errors as $name => $err) { + notice(DI::l10n()->t('Storage backend %s error: %s', $storage_opts[$name][1], $err)); + } + DI::baseUrl()->redirect('admin/storage'); } - DI::baseUrl()->redirect('admin/storage'); } if (!empty($_POST['submit_save_set'])) { @@ -101,20 +104,25 @@ class Storage extends BaseAdmin // build storage config form, $storage_form_prefix = preg_replace('|[^a-zA-Z0-9]|', '', $name); - $storage_form = []; - foreach (DI::storageManager()->getWritableStorageByName($name)->getOptions() as $option => $info) { - $type = $info[0]; - // Backward compatibilty with yesno field description - if ($type == 'yesno') { - $type = 'checkbox'; - // Remove translated labels Yes No from field info - unset($info[4]); - } + $storage_form = []; + $storageConfig = DI::storageManager()->getConfigurationByName($name); - $info[0] = $storage_form_prefix . '_' . $option; - $info['type'] = $type; - $info['field'] = 'field_' . $type . '.tpl'; - $storage_form[$option] = $info; + if ($storageConfig !== false) { + foreach ($storageConfig->getOptions() as $option => $info) { + + $type = $info[0]; + // Backward compatibilty with yesno field description + if ($type == 'yesno') { + $type = 'checkbox'; + // Remove translated labels Yes No from field info + unset($info[4]); + } + + $info[0] = $storage_form_prefix . '_' . $option; + $info['type'] = $type; + $info['field'] = 'field_' . $type . '.tpl'; + $storage_form[$option] = $info; + } } $available_storage_forms[] = [ diff --git a/tests/src/Model/Storage/DatabaseStorageTest.php b/tests/src/Model/Storage/DatabaseStorageTest.php index aa25d1cc7f..e1dfef9a18 100644 --- a/tests/src/Model/Storage/DatabaseStorageTest.php +++ b/tests/src/Model/Storage/DatabaseStorageTest.php @@ -23,11 +23,9 @@ namespace Friendica\Test\src\Model\Storage; use Friendica\Factory\ConfigFactory; use Friendica\Model\Storage\Database; -use Friendica\Model\Storage\IWritableStorage; use Friendica\Test\DatabaseTestTrait; use Friendica\Test\Util\Database\StaticDatabase; use Friendica\Test\Util\VFSTrait; -use Friendica\Util\ConfigFileLoader; use Friendica\Util\Profiler; use Psr\Log\NullLogger; @@ -47,7 +45,7 @@ class DatabaseStorageTest extends StorageTest protected function getInstance() { - $logger = new NullLogger(); + $logger = new NullLogger(); $profiler = \Mockery::mock(Profiler::class); $profiler->shouldReceive('startRecording'); $profiler->shouldReceive('stopRecording'); @@ -55,19 +53,14 @@ class DatabaseStorageTest extends StorageTest // load real config to avoid mocking every config-entry which is related to the Database class $configFactory = new ConfigFactory(); - $loader = (new ConfigFactory())->createConfigFileLoader($this->root->url(), []); - $configCache = $configFactory->createCache($loader); + $loader = (new ConfigFactory())->createConfigFileLoader($this->root->url(), []); + $configCache = $configFactory->createCache($loader); $dba = new StaticDatabase($configCache, $profiler, $logger); return new Database($dba); } - protected function assertOption(IWritableStorage $storage) - { - self::assertEmpty($storage->getOptions()); - } - protected function tearDown(): void { $this->tearDownDb(); diff --git a/tests/src/Model/Storage/FilesystemStorageConfigTest.php b/tests/src/Model/Storage/FilesystemStorageConfigTest.php new file mode 100644 index 0000000000..5821309386 --- /dev/null +++ b/tests/src/Model/Storage/FilesystemStorageConfigTest.php @@ -0,0 +1,67 @@ +. + * + */ + +namespace Friendica\Test\src\Model\Storage; + +use Friendica\Core\Config\IConfig; +use Friendica\Core\L10n; +use Friendica\Model\Storage\FilesystemConfig; +use Friendica\Model\Storage\IStorageConfiguration; +use Friendica\Test\Util\VFSTrait; +use Mockery\MockInterface; +use org\bovigo\vfs\vfsStream; + +class FilesystemStorageConfigTest extends StorageConfigTest +{ + use VFSTrait; + + protected function setUp(): void + { + $this->setUpVfsDir(); + + vfsStream::create(['storage' => []], $this->root); + + parent::setUp(); + } + + protected function getInstance() + { + /** @var MockInterface|L10n $l10n */ + $l10n = \Mockery::mock(L10n::class)->makePartial(); + $config = \Mockery::mock(IConfig::class); + $config->shouldReceive('get') + ->with('storage', 'filesystem_path', FilesystemConfig::DEFAULT_BASE_FOLDER) + ->andReturn($this->root->getChild('storage')->url()); + + return new FilesystemConfig($config, $l10n); + } + + protected function assertOption(IStorageConfiguration $storage) + { + self::assertEquals([ + 'storagepath' => [ + 'input', 'Storage base path', + $this->root->getChild('storage')->url(), + 'Folder where uploaded files are saved. For maximum security, This should be a path outside web server folder tree' + ] + ], $storage->getOptions()); + } +} diff --git a/tests/src/Model/Storage/FilesystemStorageTest.php b/tests/src/Model/Storage/FilesystemStorageTest.php index 45a7264163..d3204c6829 100644 --- a/tests/src/Model/Storage/FilesystemStorageTest.php +++ b/tests/src/Model/Storage/FilesystemStorageTest.php @@ -21,23 +21,15 @@ namespace Friendica\Test\src\Model\Storage; -use Friendica\Core\Config\IConfig; -use Friendica\Core\L10n; use Friendica\Model\Storage\Filesystem; -use Friendica\Model\Storage\IWritableStorage; use Friendica\Model\Storage\StorageException; use Friendica\Test\Util\VFSTrait; -use Friendica\Util\Profiler; -use Mockery\MockInterface; use org\bovigo\vfs\vfsStream; class FilesystemStorageTest extends StorageTest { use VFSTrait; - /** @var MockInterface|IConfig */ - protected $config; - protected function setUp(): void { $this->setUpVfsDir(); @@ -49,30 +41,7 @@ class FilesystemStorageTest extends StorageTest protected function getInstance() { - $profiler = \Mockery::mock(Profiler::class); - $profiler->shouldReceive('startRecording'); - $profiler->shouldReceive('stopRecording'); - $profiler->shouldReceive('saveTimestamp')->withAnyArgs()->andReturn(true); - - /** @var MockInterface|L10n $l10n */ - $l10n = \Mockery::mock(L10n::class)->makePartial(); - $this->config = \Mockery::mock(IConfig::class); - $this->config->shouldReceive('get') - ->with('storage', 'filesystem_path', Filesystem::DEFAULT_BASE_FOLDER) - ->andReturn($this->root->getChild('storage')->url()); - - return new Filesystem($this->config, $l10n); - } - - protected function assertOption(IWritableStorage $storage) - { - self::assertEquals([ - 'storagepath' => [ - 'input', 'Storage base path', - $this->root->getChild('storage')->url(), - 'Folder where uploaded files are saved. For maximum security, This should be a path outside web server folder tree' - ] - ], $storage->getOptions()); + return new Filesystem($this->root->getChild('storage')->url()); } /** @@ -116,7 +85,7 @@ class FilesystemStorageTest extends StorageTest $instance->put('test', 'f0c0d0i0'); - $dir = $this->root->getChild('storage/f0/c0')->url(); + $dir = $this->root->getChild('storage/f0/c0')->url(); $file = $this->root->getChild('storage/f0/c0/d0i0')->url(); self::assertDirectoryExists($dir); diff --git a/tests/src/Model/Storage/StorageConfigTest.php b/tests/src/Model/Storage/StorageConfigTest.php new file mode 100644 index 0000000000..80cbbaeb0a --- /dev/null +++ b/tests/src/Model/Storage/StorageConfigTest.php @@ -0,0 +1,43 @@ +. + * + */ + +namespace Friendica\Test\src\Model\Storage; + +use Friendica\Model\Storage\IStorageConfiguration; +use Friendica\Test\MockedTest; + +abstract class StorageConfigTest extends MockedTest +{ + /** @return IStorageConfiguration */ + abstract protected function getInstance(); + + abstract protected function assertOption(IStorageConfiguration $storage); + + /** + * Test if the "getOption" is asserted + */ + public function testGetOptions() + { + $instance = $this->getInstance(); + + $this->assertOption($instance); + } +} diff --git a/tests/src/Model/Storage/StorageTest.php b/tests/src/Model/Storage/StorageTest.php index 340aee8bfd..7433747575 100644 --- a/tests/src/Model/Storage/StorageTest.php +++ b/tests/src/Model/Storage/StorageTest.php @@ -31,8 +31,6 @@ abstract class StorageTest extends MockedTest /** @return IWritableStorage */ abstract protected function getInstance(); - abstract protected function assertOption(IWritableStorage $storage); - /** * Test if the instance is "really" implementing the interface */ @@ -42,16 +40,6 @@ abstract class StorageTest extends MockedTest self::assertInstanceOf(IStorage::class, $instance); } - /** - * Test if the "getOption" is asserted - */ - public function testGetOptions() - { - $instance = $this->getInstance(); - - $this->assertOption($instance); - } - /** * Test basic put, get and delete operations */