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(); diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index 6a8fac5b80..8fd0d1a93f 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); } /** @@ -78,19 +79,22 @@ 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 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 = true) + 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 - if ($this->isValidBackend($name, $userBackend)) { + if ($this->isValidBackend($name, $onlyUserBackend)) { switch ($name) { // Try the filesystem backend case Storage\Filesystem::getName(): @@ -102,11 +106,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); @@ -128,15 +132,34 @@ 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 = true) + 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()); + } + + /** + * 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; } /** @@ -148,12 +171,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; @@ -184,7 +207,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)) { @@ -213,7 +236,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; } @@ -239,8 +262,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/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)); } diff --git a/tests/src/Core/StorageManagerTest.php b/tests/src/Core/StorageManagerTest.php index 443f8e786b..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); } @@ -139,7 +170,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)); } /** @@ -174,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) { @@ -261,4 +297,17 @@ class StorageManagerTest extends DatabaseTest $this->assertNotEmpty($data); } } + + /** + * Test moving data to a WRONG storage + * + * @expectedException \Friendica\Model\Storage\StorageException + * @expectedExceptionMessage Can't move to storage backend 'SystemResource' + */ + public function testMoveStorageWrong() + { + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->l10n); + $storage = $storageManager->getByName(Storage\SystemResource::getName()); + $storageManager->move($storage); + } } 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; };