- Fixing SystemResource

- Adding tests for StorageManager
- Updating doc

doc/AddonStorageBackend.md

@@ -215,6 +215,11 @@ class SampleStorageBackend implements IStorage
 	{
 		return self::NAME;
 	}
+
+	public static function getName()
+	{
+		return self::NAME;
+	}
 }
 ```
 
@@ -239,13 +244,13 @@ function samplestorage_install()
 	// on addon install, we register our class with name "Sample Storage".
 	// note: we use `::class` property, which returns full class name as string
 	// this save us the problem of correctly escape backslashes in class name
-	DI::facStorage()->register("Sample Storage", SampleStorageBackend::class);
+	DI::facStorage()->register(SampleStorageBackend::class);
 }
 
 function samplestorage_unistall()
 {
 	// when the plugin is uninstalled, we unregister the backend.
-	DI::facStorage()->unregister("Sample Storage");
+	DI::facStorage()->unregister(SampleStorageBackend::class);
 }
 ```
 

src/Core/StorageManager.php

@@ -45,6 +45,7 @@ class StorageManager
 	 * @param Database        $dba
 	 * @param IConfiguration  $config
 	 * @param LoggerInterface $logger
+	 * @param Dice            $dice
 	 */
 	public function __construct(Database $dba, IConfiguration $config, LoggerInterface $logger, Dice $dice)
 	{
@@ -76,27 +77,39 @@ class StorageManager
 	/**
 	 * @brief Return storage backend class by registered name
 	 *
-	 * @param string $name Backend name
+	 * @param string|null $name Backend name
 	 *
 	 * @return Storage\IStorage|null null if no backend registered at $name
 	 */
-	public function getByName(string $name)
+	public function getByName(string $name = null)
 	{
-		if (!$this->isValidBackend($name)) {
+		if (!$this->isValidBackend($name) &&
+		    $name !== Storage\SystemResource::getName()) {
 			return null;
 		}
 
-		return $this->dice->create($this->backends[$name]);
+		/** @var Storage\IStorage $storage */
+		$storage = null;
+
+		// If the storage of the file is a system resource,
+		// create it directly since it isn't listed in the registered backends
+		if ($name === Storage\SystemResource::getName()) {
+			$storage = $this->dice->create(Storage\SystemResource::class);
+		} else {
+			$storage = $this->dice->create($this->backends[$name]);
+		}
+
+		return $storage;
 	}
 
 	/**
 	 * Checks, if the storage is a valid backend
 	 *
-	 * @param string $name The name or class of the backend
+	 * @param string|null $name The name or class of the backend
 	 *
 	 * @return boolean True, if the backend is a valid backend
 	 */
-	public function isValidBackend(string $name)
+	public function isValidBackend(string $name = null)
 	{
 		return array_key_exists($name, $this->backends);
 	}
@@ -108,7 +121,7 @@ class StorageManager
 	 *
 	 * @return boolean True, if the set was successful
 	 */
-	public function setBackend(string $name)
+	public function setBackend(string $name = null)
 	{
 		if (!$this->isValidBackend($name)) {
 			return false;
@@ -135,39 +148,52 @@ class StorageManager
 	/**
 	 * @brief Register a storage backend class
 	 *
-	 * @param string $name  User readable backend name
 	 * @param string $class Backend class name
 	 *
 	 * @return boolean True, if the registration was successful
 	 */
-	public function register(string $name, string $class)
+	public function register(string $class)
 	{
-		if (!is_subclass_of($class, Storage\IStorage::class)) {
-			return false;
-		}
+		if (is_subclass_of($class, Storage\IStorage::class)) {
+			/** @var Storage\IStorage $class */
 
 			$backends        = $this->backends;
-		$backends[$name] = $class;
+			$backends[$class::getName()] = $class;
 
-		if ($this->config->set('storage', 'backends', $this->backends)) {
+			if ($this->config->set('storage', 'backends', $backends)) {
 				$this->backends = $backends;
 				return true;
 			} else {
 				return false;
 			}
+		} else {
+			return false;
+		}
 	}
 
 	/**
 	 * @brief Unregister a storage backend class
 	 *
-	 * @param string $name User readable backend name
+	 * @param string $class Backend class name
 	 *
 	 * @return boolean True, if unregistering was successful
 	 */
-	public function unregister(string $name)
+	public function unregister(string $class)
 	{
-		unset($this->backends[$name]);
+		if (is_subclass_of($class, Storage\IStorage::class)) {
+			/** @var Storage\IStorage $class */
+
+			unset($this->backends[$class::getName()]);
+
+			if ($this->currentBackend instanceof $class) {
+			    $this->config->set('storage', 'name', null);
+				$this->currentBackend = null;
+			}
+
 			return $this->config->set('storage', 'backends', $this->backends);
+		} else {
+			return false;
+		}
 	}
 
 	/**
@@ -196,7 +222,7 @@ class StorageManager
 			$resources = $this->dba->select(
 				$table,
 				['id', 'data', 'backend-class', 'backend-ref'],
-				['`backend-class` IS NULL or `backend-class` != ?', $destination],
+				['`backend-class` IS NULL or `backend-class` != ?', $destination::getName()],
 				['limit' => $limit]
 			);
 

src/Model/Photo.php

@@ -16,6 +16,7 @@ use Friendica\Database\DBA;
 use Friendica\Database\DBStructure;
 use Friendica\DI;
 use Friendica\Model\Storage\IStorage;
+use Friendica\Model\Storage\SystemResource;
 use Friendica\Object\Image;
 use Friendica\Util\DateTimeFormat;
 use Friendica\Util\Images;
@@ -223,7 +224,7 @@ class Photo
 		$values = array_fill(0, count($fields), "");
 
 		$photo = array_combine($fields, $values);
-		$photo["backend-class"] = Storage\SystemResource::class;
+		$photo["backend-class"] = SystemResource::NAME;
 		$photo["backend-ref"] = $filename;
 		$photo["type"] = $mimetype;
 		$photo["cacheable"] = false;
@@ -275,7 +276,7 @@ class Photo
 
 		if (DBA::isResult($existing_photo)) {
 			$backend_ref = (string)$existing_photo["backend-ref"];
-			$storage = DI::facStorage()->getByName((string)$existing_photo["backend-class"]);
+			$storage = DI::facStorage()->getByName($existing_photo["backend-class"] ?? '');
 		} else {
 			$storage = DI::storage();
 		}

src/Model/Storage/AbstractStorage.php

@@ -0,0 +1,32 @@
+<?php
+
+namespace Friendica\Model\Storage;
+
+use Friendica\Core\L10n\L10n;
+use Psr\Log\LoggerInterface;
+
+/**
+ * A general storage class which loads common dependencies and implements common methods
+ */
+abstract class AbstractStorage implements IStorage
+{
+	/** @var L10n */
+	protected $l10n;
+	/** @var LoggerInterface */
+	protected $logger;
+
+	/**
+	 * @param L10n            $l10n
+	 * @param LoggerInterface $logger
+	 */
+	public function __construct(L10n $l10n, LoggerInterface $logger)
+	{
+		$this->l10n   = $l10n;
+		$this->logger = $logger;
+	}
+
+	public function __toString()
+	{
+		return static::getName();
+	}
+}

src/Model/Storage/Database.php

@@ -6,35 +6,32 @@
 
 namespace Friendica\Model\Storage;
 
-use Friendica\Core\L10n;
+use Friendica\Core\L10n\L10n;
 use Psr\Log\LoggerInterface;
+use Friendica\Database\Database as DBA;
 
 /**
  * @brief Database based storage system
  *
  * This class manage data stored in database table.
  */
-class Database implements IStorage
+class Database extends AbstractStorage
 {
 	const NAME = 'Database';
 
-	/** @var \Friendica\Database\Database */
+	/** @var DBA */
 	private $dba;
-	/** @var LoggerInterface */
-	private $logger;
-	/** @var L10n\L10n */
-	private $l10n;
 
 	/**
-	 * @param \Friendica\Database\Database $dba
+	 * @param DBA             $dba
 	 * @param LoggerInterface $logger
-	 * @param L10n\L10n                    $l10n
+	 * @param L10n            $l10n
 	 */
-	public function __construct(\Friendica\Database\Database $dba, LoggerInterface $logger, L10n\L10n $l10n)
+	public function __construct(DBA $dba, LoggerInterface $logger, L10n $l10n)
 	{
+		parent::__construct($l10n, $logger);
+
 		$this->dba = $dba;
-		$this->logger = $logger;
-		$this->l10n   = $l10n;
 	}
 
 	/**
@@ -101,7 +98,7 @@ class Database implements IStorage
 	/**
 	 * @inheritDoc
 	 */
-	public function __toString()
+	public static function getName()
 	{
 		return self::NAME;
 	}

src/Model/Storage/Filesystem.php

@@ -21,7 +21,7 @@ use Psr\Log\LoggerInterface;
  * Each new resource gets a value as reference and is saved in a
  * folder tree stucture created from that value.
  */
-class Filesystem implements IStorage
+class Filesystem extends AbstractStorage
 {
 	const NAME = 'Filesystem';
 
@@ -30,10 +30,6 @@ class Filesystem implements IStorage
 
 	/** @var IConfiguration */
 	private $config;
-	/** @var LoggerInterface */
-	private $logger;
-	/** @var L10n */
-	private $l10n;
 
 	/** @var string */
 	private $basePath;
@@ -47,9 +43,9 @@ class Filesystem implements IStorage
 	 */
 	public function __construct(IConfiguration $config, LoggerInterface $logger, L10n $l10n)
 	{
+		parent::__construct($l10n, $logger);
+
 		$this->config = $config;
-		$this->logger = $logger;
-		$this->l10n   = $l10n;
 
 		$path           = $this->config->get('storage', 'filesystem_path', self::DEFAULT_BASE_FOLDER);
 		$this->basePath = rtrim($path, '/');
@@ -185,7 +181,7 @@ class Filesystem implements IStorage
 	/**
 	 * @inheritDoc
 	 */
-	public function __toString()
+	public static function getName()
 	{
 		return self::NAME;
 	}

src/Model/Storage/IStorage.php

@@ -96,4 +96,11 @@ interface IStorage
 	 * @return string
 	 */
 	public function __toString();
+
+	/**
+	 * The name of the backend
+	 *
+	 * @return string
+	 */
+	public static function getName();
 }

src/Model/Storage/SystemResource.php

@@ -16,10 +16,15 @@ use \BadMethodCallException;
  */
 class SystemResource implements IStorage
 {
+	const NAME = 'SystemResource';
+
 	// Valid folders to look for resources
 	const VALID_FOLDERS = ["images"];
 
-	public static function get($filename)
+	/**
+	 * @inheritDoc
+	 */
+	public function get(string $filename)
 	{
 		$folder = dirname($filename);
 		if (!in_array($folder, self::VALID_FOLDERS)) {
@@ -31,25 +36,48 @@ class SystemResource implements IStorage
 		return file_get_contents($filename);
 	}
 
-
-	public static function put($data, $filename = "")
+	/**
+	 * @inheritDoc
+	 */
+	public function put(string $data, string $filename = '')
 	{
 		throw new BadMethodCallException();
 	}
 
-	public static function delete($filename)
+	public function delete(string $filename)
 	{
 		throw new BadMethodCallException();
 	}
 
-	public static function getOptions()
+	/**
+	 * @inheritDoc
+	 */
+	public function getOptions()
 	{
 		return [];
 	}
 
-	public static function saveOptions($data)
+	/**
+	 * @inheritDoc
+	 */
+	public function saveOptions(array $data)
 	{
 		return [];
 	}
+
+	/**
+	 * @inheritDoc
+	 */
+	public function __toString()
+	{
+		return self::NAME;
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	public static function getName()
+	{
+		return self::NAME;
+	}
 }
-

tests/Util/SampleStorageBackend.php

@@ -0,0 +1,94 @@
+<?php
+
+namespace Friendica\Test\Util;
+
+use Friendica\Model\Storage\IStorage;
+
+use Friendica\Core\L10n\L10n;
+
+/**
+ * A backend storage example class
+ */
+class SampleStorageBackend implements IStorage
+{
+	const NAME = 'Sample Storage';
+
+	/** @var L10n */
+	private $l10n;
+
+	/** @var array */
+	private $options = [
+		'filename' => [
+			'input',    // will use a simple text input
+			'The file to return',    // the label
+			'sample',    // the current value
+			'Enter the path to a file', // the help text
+			// no extra data for 'input' type..
+		],
+	];
+	/** @var array Just save the data in memory */
+	private $data = [];
+
+	/**
+	 * SampleStorageBackend constructor.
+	 *
+	 * @param L10n $l10n The configuration of Friendica
+	 *
+	 * You can add here every dynamic class as dependency you like and add them to a private field
+	 * Friendica automatically creates these classes and passes them as argument to the constructor
+	 */
+	public function __construct(L10n $l10n)
+	{
+		$this->l10n = $l10n;
+	}
+
+	public function get(string $reference)
+	{
+		// we return always the same image data. Which file we load is defined by
+		// a config key
+		return $this->data[$reference] ?? null;
+	}
+
+	public function put(string $data, string $reference = '')
+	{
+		if ($reference === '') {
+			$reference = 'sample';
+		}
+
+		$this->data[$reference] = $data;
+
+		return $reference;
+	}
+
+	public function delete(string $reference)
+	{
+		if (isset($this->data[$reference])) {
+			unset($this->data[$reference]);
+		}
+
+		return true;
+	}
+
+	public function getOptions()
+	{
+		return $this->options;
+	}
+
+	public function saveOptions(array $data)
+	{
+		$this->options = $data;
+
+		// no errors, return empty array
+		return $this->options;
+	}
+
+	public function __toString()
+	{
+		return self::NAME;
+	}
+
+	public static function getName()
+	{
+		return self::NAME;
+	}
+}

tests/datasets/storage/database.fixture.php

@@ -0,0 +1,40 @@
+<?php
+
+return [
+	'photo'   => [
+		// move from data-attribute to storage backend
+		[
+			'id'            => 1,
+			'backend-class' => null,
+			'backend-ref'   => 'f0c0d0i2',
+			'data'          => 'without class',
+		],
+		// move from storage-backend to maybe filesystem backend, skip at database backend
+		[
+			'id'            => 2,
+			'backend-class' => 'Database',
+			'backend-ref'   => 1,
+			'data'          => '',
+		],
+		// move data if invalid storage
+		[
+			'id'            => 3,
+			'backend-class' => 'invalid!',
+			'backend-ref'   => 'unimported',
+			'data'          => 'invalid data moved',
+		],
+		// skip everytime because of invalid storage and no data
+		[
+			'id'            => 3,
+			'backend-class' => 'invalid!',
+			'backend-ref'   => 'unimported',
+			'data'          => '',
+		],
+	],
+	'storage' => [
+		[
+			'id'   => 1,
+			'data' => 'inside database',
+		],
+	],
+];

tests/src/Core/StorageManagerTest.php

@@ -0,0 +1,252 @@
+<?php
+
+namespace Friendica\Test\src\Core;
+
+use Dice\Dice;
+use Friendica\Core\Config\IConfiguration;
+use Friendica\Core\Config\PreloadConfiguration;
+use Friendica\Core\Session\ISession;
+use Friendica\Core\StorageManager;
+use Friendica\Database\Database;
+use Friendica\Factory\ConfigFactory;
+use Friendica\Model\Config\Config;
+use Friendica\Model\Storage;
+use Friendica\Core\Session;
+use Friendica\Test\DatabaseTest;
+use Friendica\Test\Util\Database\StaticDatabase;
+use Friendica\Test\Util\VFSTrait;
+use Friendica\Util\ConfigFileLoader;
+use Friendica\Util\Profiler;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
+use Friendica\Test\Util\SampleStorageBackend;
+
+class StorageManagerTest extends DatabaseTest
+{
+	/** @var Database */
+	private $dba;
+	/** @var IConfiguration */
+	private $config;
+	/** @var LoggerInterface */
+	private $logger;
+	/** @var Dice */
+	private $dice;
+
+	use VFSTrait;
+
+	public function setUp()
+	{
+		parent::setUp();
+
+		$this->setUpVfsDir();
+
+		$this->logger = new NullLogger();
+		$this->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]);
+
+		$profiler = \Mockery::mock(Profiler::class);
+		$profiler->shouldReceive('saveTimestamp')->withAnyArgs()->andReturn(true);
+
+		// load real config to avoid mocking every config-entry which is related to the Database class
+		$configFactory = new ConfigFactory();
+		$loader        = new ConfigFileLoader($this->root->url());
+		$configCache   = $configFactory->createCache($loader);
+
+		$this->dba = new StaticDatabase($configCache, $profiler, $this->logger);
+
+		$configModel  = new Config($this->dba);
+		$this->config = new PreloadConfiguration($configCache, $configModel);
+	}
+
+	/**
+	 * Test plain instancing first
+	 */
+	public function testInstance()
+	{
+		$storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice);
+
+		$this->assertInstanceOf(StorageManager::class, $storageManager);
+	}
+
+	public function dataStorages()
+	{
+		return [
+			'empty'          => [
+				'name'        => '',
+				'assert'      => null,
+				'assertName'  => '',
+				'userBackend' => false,
+			],
+			'database'       => [
+				'name'        => Storage\Database::NAME,
+				'assert'      => Storage\Database::class,
+				'assertName'  => Storage\Database::NAME,
+				'userBackend' => true,
+			],
+			'filesystem'     => [
+				'name'        => Storage\Filesystem::NAME,
+				'assert'      => Storage\Filesystem::class,
+				'assertName'  => Storage\Filesystem::NAME,
+				'userBackend' => true,
+			],
+			'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,
+			],
+			'invalid'        => [
+				'name'        => 'invalid',
+				'assert'      => null,
+				'assertName'  => '',
+				'userBackend' => false,
+			],
+		];
+	}
+
+	/**
+	 * Test the getByName() method
+	 *
+	 * @dataProvider dataStorages
+	 */
+	public function testGetByName($name, $assert, $assertName)
+	{
+		$storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice);
+
+		$storage = $storageManager->getByName($name);
+
+		if (!empty($assert)) {
+			$this->assertInstanceOf(Storage\IStorage::class, $storage);
+			$this->assertInstanceOf($assert, $storage);
+			$this->assertEquals($name, $storage::getName());
+		} else {
+			$this->assertNull($storage);
+		}
+		$this->assertEquals($assertName, $storage);
+	}
+
+	/**
+	 * Test the isValidBackend() method
+	 *
+	 * @dataProvider dataStorages
+	 */
+	public function testIsValidBackend($name, $assert, $assertName, $userBackend)
+	{
+		$storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice);
+
+		$this->assertEquals($userBackend, $storageManager->isValidBackend($name));
+	}
+
+	/**
+	 * Test the method listBackends() with default setting
+	 */
+	public function testListBackends()
+	{
+		$storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice);
+
+		$this->assertEquals(StorageManager::DEFAULT_BACKENDS, $storageManager->listBackends());
+	}
+
+	/**
+	 * Test the method getBackend()
+	 *
+	 * @dataProvider dataStorages
+	 */
+	public function testGetBackend($name, $assert, $assertName, $userBackend)
+	{
+		$storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice);
+
+		$this->assertNull($storageManager->getBackend());
+
+		if ($userBackend) {
+			$storageManager->setBackend($name);
+
+			$this->assertInstanceOf($assert, $storageManager->getBackend());
+		}
+	}
+
+	/**
+	 * Test the method getBackend() with a pre-configured backend
+	 *
+	 * @dataProvider dataStorages
+	 */
+	public function testPresetBackend($name, $assert, $assertName, $userBackend)
+	{
+		$this->config->set('storage', 'name', $name);
+
+		$storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice);
+
+		if ($userBackend) {
+			$this->assertInstanceOf($assert, $storageManager->getBackend());
+		} else {
+			$this->assertNull($storageManager->getBackend());
+		}
+	}
+
+	/**
+	 * Tests the register and unregister methods for a new backend storage class
+	 *
+	 * Uses a sample storage for testing
+	 *
+	 * @see SampleStorageBackend
+	 */
+	public function testRegisterUnregisterBackends()
+	{
+		$storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice);
+
+		$this->assertTrue($storageManager->register(SampleStorageBackend::class));
+
+		$this->assertEquals(array_merge(StorageManager::DEFAULT_BACKENDS, [
+			'Sample Storage' => SampleStorageBackend::class,
+		]), $storageManager->listBackends());
+		$this->assertEquals(array_merge(StorageManager::DEFAULT_BACKENDS, [
+			'Sample Storage' => SampleStorageBackend::class,
+		]), $this->config->get('storage', 'backends'));
+
+		$this->assertTrue($storageManager->setBackend(SampleStorageBackend::NAME));
+		$this->assertEquals(SampleStorageBackend::NAME, $this->config->get('storage', 'name'));
+
+		$this->assertInstanceOf(SampleStorageBackend::class, $storageManager->getBackend());
+
+		$this->assertTrue($storageManager->unregister(SampleStorageBackend::class));
+		$this->assertEquals(StorageManager::DEFAULT_BACKENDS, $this->config->get('storage', 'backends'));
+		$this->assertEquals(StorageManager::DEFAULT_BACKENDS, $storageManager->listBackends());
+
+		$this->assertNull($storageManager->getBackend());
+		$this->assertNull($this->config->get('storage', 'name'));
+	}
+
+	/**
+	 * Test moving data to a new storage (currently testing db & filesystem)
+	 *
+	 * @dataProvider dataStorages
+	 */
+	public function testMoveStorage($name, $assert, $assertName, $userBackend)
+	{
+		if (!$userBackend) {
+			return;
+		}
+
+		$this->loadFixture(__DIR__ . '/../../datasets/storage/database.fixture.php', $this->dba);
+
+		$storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice);
+		$storage = $storageManager->getByName($name);
+		$storageManager->move($storage);
+
+		$photos = $this->dba->select('photo', ['backend-ref', 'backend-class', 'id', 'data']);
+
+		while ($photo = $this->dba->fetch($photos)) {
+
+			$this->assertEmpty($photo['data']);
+
+			$storage = $storageManager->getByName($photo['backend-class']);
+			$data = $storage->get($photo['backend-ref']);
+
+			$this->assertNotEmpty($data);
+		}
+	}
+}

update.php

@@ -418,5 +418,15 @@ function update_1329()
 		Config::delete('storage', 'class');
 	}
 
+	$photos = DBA::select('photos', ['backend-class', 'id'], ['backend-class IS NOT NULL']);
+	foreach ($photos as $photo) {
+		DBA::update('photos', ['backend-class' => $photo['backend-class']::NAME], ['id' => $photo['id']]);
+	}
+
+	$attachs = DBA::select('attach', ['backend-class', 'id'], ['backend-class IS NOT NULL']);
+	foreach ($attachs as $attach) {
+		DBA::update('photos', ['backend-class' => $attach['backend-class']::NAME], ['id' => $attach['id']]);
+	}
+
 	return Update::SUCCESS;
 }