From 9bb58916458a3e90533ed757318f0212daa1d0cb Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Fri, 17 Jan 2020 19:31:34 +0100 Subject: [PATCH 1/8] Fix loading SystemResource files --- src/Core/StorageManager.php | 13 +++++++------ tests/src/Core/StorageManagerTest.php | 6 +++++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index 6a8fac5b80..7dd4c67371 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -62,7 +62,8 @@ class StorageManager $currentName = $this->config->get('storage', 'name', ''); - $this->currentBackend = $this->getByName($currentName); + // you can only use user backends as a "default" backend, so the second parameter is true + $this->currentBackend = $this->getByName($currentName, true); } /** @@ -79,13 +80,13 @@ class StorageManager * @brief Return storage backend class by registered name * * @param string|null $name Backend name - * @param boolean $userBackend Just return instances in case it's a user backend (e.g. not SystemResource) + * @param boolean $userBackend True, if just user specific instances should be returrned (e.g. not SystemResource) * * @return Storage\IStorage|null null if no backend registered at $name * * @throws \Friendica\Network\HTTPException\InternalServerErrorException */ - public function getByName(string $name = null, $userBackend = true) + public function getByName(string $name = null, $userBackend = false) { // If there's no cached instance create a new instance if (!isset($this->backendInstances[$name])) { @@ -133,7 +134,7 @@ class StorageManager * * @return boolean True, if the backend is a valid backend */ - public function isValidBackend(string $name = null, bool $userBackend = true) + public function isValidBackend(string $name = null, bool $userBackend = false) { return array_key_exists($name, $this->backends) || (!$userBackend && $name === Storage\SystemResource::getName()); @@ -148,12 +149,12 @@ class StorageManager */ public function setBackend(string $name = null) { - if (!$this->isValidBackend($name)) { + if (!$this->isValidBackend($name, false)) { return false; } if ($this->config->set('storage', 'name', $name)) { - $this->currentBackend = $this->getByName($name); + $this->currentBackend = $this->getByName($name, false); return true; } else { return false; diff --git a/tests/src/Core/StorageManagerTest.php b/tests/src/Core/StorageManagerTest.php index 443f8e786b..7cef2dc281 100644 --- a/tests/src/Core/StorageManagerTest.php +++ b/tests/src/Core/StorageManagerTest.php @@ -139,7 +139,11 @@ class StorageManagerTest extends DatabaseTest { $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n); - $this->assertEquals($userBackend, $storageManager->isValidBackend($name)); + // true in every of the backends + $this->assertEquals(!empty($assertName), $storageManager->isValidBackend($name)); + + // if userBackend is set to true, filter out e.g. SystemRessource + $this->assertEquals($userBackend, $storageManager->isValidBackend($name, true)); } /** From 0af83e6f7ca3dd5049cc40c51fc555ccd930f800 Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Fri, 17 Jan 2020 20:23:30 +0100 Subject: [PATCH 2/8] Rename bool flag for user backend --- src/Core/StorageManager.php | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index 7dd4c67371..8b5b73a788 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -79,19 +79,19 @@ class StorageManager /** * @brief Return storage backend class by registered name * - * @param string|null $name Backend name - * @param boolean $userBackend True, if just user specific instances should be returrned (e.g. not SystemResource) + * @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 * * @throws \Friendica\Network\HTTPException\InternalServerErrorException */ - public function getByName(string $name = null, $userBackend = false) + public function getByName(string $name = null, $onlyUserBackend = false) { // 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, $userBackend)) { + if ($this->isValidBackend($name, $onlyUserBackend)) { switch ($name) { // Try the filesystem backend case Storage\Filesystem::getName(): @@ -103,11 +103,11 @@ class StorageManager break; // at least, try if there's an addon for the backend case Storage\SystemResource::getName(): - $this->backendInstances[$name] = new Storage\SystemResource(); + $this->backendInstances[$name] = new Storage\SystemResource(); break; default: $data = [ - 'name' => $name, + 'name' => $name, 'storage' => null, ]; Hook::callAll('storage_instance', $data); @@ -129,15 +129,15 @@ class StorageManager /** * Checks, if the storage is a valid backend * - * @param string|null $name The name or class of the backend - * @param boolean $userBackend True, if just user backend should get returned (e.g. not SystemResource) + * @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) * * @return boolean True, if the backend is a valid backend */ - public function isValidBackend(string $name = null, bool $userBackend = false) + public function isValidBackend(string $name = null, bool $onlyUserBackend = false) { return array_key_exists($name, $this->backends) || - (!$userBackend && $name === Storage\SystemResource::getName()); + (!$onlyUserBackend && $name === Storage\SystemResource::getName()); } /** @@ -185,7 +185,7 @@ class StorageManager if (is_subclass_of($class, Storage\IStorage::class)) { /** @var Storage\IStorage $class */ - $backends = $this->backends; + $backends = $this->backends; $backends[$class::getName()] = $class; if ($this->config->set('storage', 'backends', $backends)) { @@ -214,7 +214,7 @@ class StorageManager unset($this->backends[$class::getName()]); if ($this->currentBackend instanceof $class) { - $this->config->set('storage', 'name', null); + $this->config->set('storage', 'name', null); $this->currentBackend = null; } From 14c97f7b62648ea0d1732c569031460c19b6e747 Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Fri, 17 Jan 2020 20:45:21 +0100 Subject: [PATCH 3/8] avoid exception throwing because of false-like return --- src/Model/Storage/Filesystem.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Model/Storage/Filesystem.php b/src/Model/Storage/Filesystem.php index 9c429cfb3c..f45e1db449 100644 --- a/src/Model/Storage/Filesystem.php +++ b/src/Model/Storage/Filesystem.php @@ -125,7 +125,10 @@ class Filesystem extends AbstractStorage $this->createFoldersForFile($file); - if (!file_put_contents($file, $data)) { + $result = file_put_contents($file, $data); + + // just in case the result is REALLY false, not zero or empty or anything else, throw the exception + if ($result === false) { $this->logger->warning('Failed to write data.', ['file' => $file]); throw new StorageException($this->l10n->t('Filesystem storage failed to save data to "%s". Check your write permissions', $file)); } From 732992a296e4de14ce1c75dfda952d2768158a4c Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Fri, 17 Jan 2020 21:01:37 +0100 Subject: [PATCH 4/8] Improve impossible exception-handler for storage move --- src/Core/StorageManager.php | 4 ++-- tests/src/Core/StorageManagerTest.php | 13 +++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index 8b5b73a788..d1a943a227 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -240,8 +240,8 @@ class StorageManager */ public function move(Storage\IStorage $destination, array $tables = self::TABLES, int $limit = 5000) { - if ($destination === null) { - throw new Storage\StorageException('Can\'t move to NULL storage backend'); + if (!$this->isValidBackend($destination, true)) { + throw new Storage\StorageException(sprintf("Can't move to storage backend '%s'", $destination::getName())); } $moved = 0; diff --git a/tests/src/Core/StorageManagerTest.php b/tests/src/Core/StorageManagerTest.php index 7cef2dc281..fd72eb7fb2 100644 --- a/tests/src/Core/StorageManagerTest.php +++ b/tests/src/Core/StorageManagerTest.php @@ -265,4 +265,17 @@ class StorageManagerTest extends DatabaseTest $this->assertNotEmpty($data); } } + + /** + * Test moving data to a WRONG storage + * + * @expectedException \Friendica\Model\Storage\StorageException + * @expectedExceptionMessageRegExp /Can't move to storage backend '.*'/ + */ + public function testMoveStorageWrong() + { + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n); + $storage = $storageManager->getByName(Storage\SystemResource::getName()); + $storageManager->move($storage); + } } From 3e72e8015bb72a2fd3918b5b1b9ae589739f1093 Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Fri, 17 Jan 2020 21:02:18 +0100 Subject: [PATCH 5/8] Improve impossible exception-handler for storage move --- tests/src/Core/StorageManagerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Core/StorageManagerTest.php b/tests/src/Core/StorageManagerTest.php index fd72eb7fb2..084caa68ab 100644 --- a/tests/src/Core/StorageManagerTest.php +++ b/tests/src/Core/StorageManagerTest.php @@ -270,7 +270,7 @@ class StorageManagerTest extends DatabaseTest * Test moving data to a WRONG storage * * @expectedException \Friendica\Model\Storage\StorageException - * @expectedExceptionMessageRegExp /Can't move to storage backend '.*'/ + * @expectedExceptionMessage Can't move to storage backend 'SystemResource' */ public function testMoveStorageWrong() { From ca8ca05051964dc5ffd881b83d8b1d04a56df776 Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Fri, 17 Jan 2020 23:25:11 +0100 Subject: [PATCH 6/8] Add legacy backend storage functionality --- src/Core/StorageManager.php | 22 +++++++++++++++++ tests/src/Core/StorageManagerTest.php | 34 ++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index d1a943a227..8fd0d1a93f 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -88,6 +88,9 @@ class StorageManager */ public function getByName(string $name = null, $onlyUserBackend = false) { + // @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 @@ -140,6 +143,25 @@ class StorageManager (!$onlyUserBackend && $name === Storage\SystemResource::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; + } + /** * @brief Set current storage backend class * diff --git a/tests/src/Core/StorageManagerTest.php b/tests/src/Core/StorageManagerTest.php index 084caa68ab..b7e5b8c3dd 100644 --- a/tests/src/Core/StorageManagerTest.php +++ b/tests/src/Core/StorageManagerTest.php @@ -109,10 +109,42 @@ 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) { @@ -123,7 +155,6 @@ class StorageManagerTest extends DatabaseTest if (!empty($assert)) { $this->assertInstanceOf(Storage\IStorage::class, $storage); $this->assertInstanceOf($assert, $storage); - $this->assertEquals($name, $storage::getName()); } else { $this->assertNull($storage); } @@ -178,6 +209,7 @@ class StorageManagerTest extends DatabaseTest * Test the method getBackend() with a pre-configured backend * * @dataProvider dataStorages + * @dataProvider dataLegacyBackends */ public function testPresetBackend($name, $assert, $assertName, $userBackend) { From 3ea5177c42eb1b958a3eee899d6d9ea7a4ae68af Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Fri, 17 Jan 2020 23:47:26 +0100 Subject: [PATCH 7/8] Fix update script --- update.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/update.php b/update.php index 924fac5458..55fd82b2ed 100644 --- a/update.php +++ b/update.php @@ -425,8 +425,8 @@ function update_1330() } // Update attachments and photos - if (!DBA::p("UPDATE `photo` SET `photo`.`backend-class` = SUBSTR(`photo`.`backend-class`, 22) WHERE `photo`.`backend-class` LIKE 'Friendica\\Model\\Storage\\%'") || - !DBA::p("UPDATE `attach` SET `attach`.`backend-class` = SUBSTR(`attach`.`backend-class`, 22) WHERE `attach`.`backend-class` LIKE 'Friendica\\Model\\Storage\\%'")) { + if (!DBA::p("UPDATE `photo` SET `photo`.`backend-class` = SUBSTR(`photo`.`backend-class`, 25) WHERE `photo`.`backend-class` LIKE 'Friendica\\\Model\\\Storage\\\%' ESCAPE '|'") || + !DBA::p("UPDATE `attach` SET `attach`.`backend-class` = SUBSTR(`attach`.`backend-class`, 25) WHERE `attach`.`backend-class` LIKE 'Friendica\\\Model\\\Storage\\\%' ESCAPE '|'")) { return Update::FAILED; }; From e5eeaf78f207f55e007e0a858a3d1638ce506122 Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Sat, 18 Jan 2020 00:08:48 +0100 Subject: [PATCH 8/8] Fix Storage move command --- src/Console/Storage.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Console/Storage.php b/src/Console/Storage.php index f4b4de562e..fcf55a9444 100644 --- a/src/Console/Storage.php +++ b/src/Console/Storage.php @@ -130,7 +130,6 @@ HELP; protected function doMove() { - $tables = null; if (count($this->args) < 1 || count($this->args) > 2) { throw new CommandArgsException('Invalid arguments'); } @@ -141,6 +140,8 @@ HELP; throw new CommandArgsException('Invalid table'); } $tables = [$table]; + } else { + $tables = StorageManager::TABLES; } $current = $this->storageManager->getBackend();