refactor(Files): Modernize Wrapper

Signed-off-by: provokateurin <kate@provokateurin.de>

apps/files_sharing/lib/SharedStorage.php

@@ -85,12 +85,6 @@ class SharedStorage extends Jail implements LegacyISharedStorage, ISharedStorage
 
 	private static int $initDepth = 0;
 
-	/**
-	 * @psalm-suppress NonInvariantDocblockPropertyType
-	 * @var ?Storage $storage
-	 */
-	protected $storage;
-
 	public function __construct(array $parameters) {
 		$this->ownerView = $parameters['ownerView'];
 		$this->logger = Server::get(LoggerInterface::class);

build/rector-strict.php

@@ -18,6 +18,7 @@ return (require __DIR__ . '/rector-shared.php')
 		$nextcloudDir . '/lib/private/Settings/AuthorizedGroupMapper.php',
 		$nextcloudDir . '/apps/settings/lib/Service/AuthorizedGroupService.php',
 		$nextcloudDir . '/lib/private/Files/Storage/Storage.php',
+		$nextcloudDir . '/lib/private/Files/Storage/Wrapper/Wrapper.php',
 	])
 	->withPreparedSets(
 		deadCode: true,

lib/private/Files/Storage/Wrapper/Jail.php

@@ -11,6 +11,7 @@ use OC\Files\Cache\Wrapper\CacheJail;
 use OC\Files\Cache\Wrapper\JailPropagator;
 use OC\Files\Cache\Wrapper\JailWatcher;
 use OC\Files\Filesystem;
+use OC\Files\Storage\FailedStorage;
 use OCP\Files\Cache\ICache;
 use OCP\Files\Cache\IPropagator;
 use OCP\Files\Cache\IWatcher;
@@ -20,6 +21,7 @@ use OCP\Files\Storage\IWriteStreamStorage;
 use OCP\IDBConnection;
 use OCP\Lock\ILockingProvider;
 use OCP\Server;
+use Psr\Log\LoggerInterface;
 
 /**
  * Jail to a subdirectory of the wrapped storage
@@ -51,6 +53,12 @@ class Jail extends Wrapper {
 	 * This is separate from Wrapper::getWrapperStorage so we can get the jailed storage consistently even if the jail is inside another wrapper
 	 */
 	public function getUnjailedStorage(): IStorage {
+		if ($this->storage === null) {
+			$message = 'storage jail ' . get_class($this) . " doesn't have a wrapped storage set";
+			Server::get(LoggerInterface::class)->error($message);
+			$this->storage = new FailedStorage(['exception' => new \Exception($message)]);
+		}
+
 		return $this->storage;
 	}
 

lib/private/Files/Storage/Wrapper/Wrapper.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
@@ -24,31 +26,32 @@ use Override;
 use Psr\Log\LoggerInterface;
 
 class Wrapper implements Storage, ILockingStorage, IWriteStreamStorage {
-	/**
-	 * @var Storage $storage
-	 */
-	protected $storage;
+	protected ?Storage $storage = null;
 
-	public $cache;
-	public $scanner;
-	public $watcher;
-	public $propagator;
-	public $updater;
+	public ?ICache $cache = null;
+
+	public ?IScanner $scanner = null;
+
+	public ?IWatcher $watcher = null;
+
+	public ?IPropagator $propagator = null;
+
+	public ?IUpdater $updater = null;
 
 	/**
-	 * @param array $parameters
+	 * @param array{storage: Storage} $parameters
 	 */
 	public function __construct(array $parameters) {
 		$this->storage = $parameters['storage'];
 	}
 
 	public function getWrapperStorage(): Storage {
-		if (!$this->storage) {
-			$message = 'storage wrapper ' . get_class($this) . " doesn't have a wrapped storage set";
-			$logger = Server::get(LoggerInterface::class);
-			$logger->error($message);
+		if (!$this->storage instanceof Storage) {
+			$message = 'storage wrapper ' . static::class . " doesn't have a wrapped storage set";
+			Server::get(LoggerInterface::class)->error($message);
 			$this->storage = new FailedStorage(['exception' => new \Exception($message)]);
 		}
+
 		return $this->storage;
 	}
 
@@ -169,16 +172,18 @@ class Wrapper implements Storage, ILockingStorage, IWriteStreamStorage {
 	}
 
 	public function getCache(string $path = '', ?IStorage $storage = null): ICache {
-		if (!$storage) {
+		if (!$storage instanceof IStorage) {
 			$storage = $this;
 		}
+
 		return $this->getWrapperStorage()->getCache($path, $storage);
 	}
 
 	public function getScanner(string $path = '', ?IStorage $storage = null): IScanner {
-		if (!$storage) {
+		if (!$storage instanceof IStorage) {
 			$storage = $this;
 		}
+
 		return $this->getWrapperStorage()->getScanner($path, $storage);
 	}
 
@@ -187,23 +192,26 @@ class Wrapper implements Storage, ILockingStorage, IWriteStreamStorage {
 	}
 
 	public function getWatcher(string $path = '', ?IStorage $storage = null): IWatcher {
-		if (!$storage) {
+		if (!$storage instanceof IStorage) {
 			$storage = $this;
 		}
+
 		return $this->getWrapperStorage()->getWatcher($path, $storage);
 	}
 
 	public function getPropagator(?IStorage $storage = null): IPropagator {
-		if (!$storage) {
+		if (!$storage instanceof IStorage) {
 			$storage = $this;
 		}
+
 		return $this->getWrapperStorage()->getPropagator($storage);
 	}
 
 	public function getUpdater(?IStorage $storage = null): IUpdater {
-		if (!$storage) {
+		if (!$storage instanceof IStorage) {
 			$storage = $this;
 		}
+
 		return $this->getWrapperStorage()->getUpdater($storage);
 	}
 
@@ -224,10 +232,6 @@ class Wrapper implements Storage, ILockingStorage, IWriteStreamStorage {
 	}
 
 	public function instanceOfStorage(string $class): bool {
-		if (ltrim($class, '\\') === 'OC\Files\Storage\Shared') {
-			// FIXME Temporary fix to keep existing checks working
-			$class = '\OCA\Files_Sharing\SharedStorage';
-		}
 		return is_a($this, $class) || $this->getWrapperStorage()->instanceOfStorage($class);
 	}
 
@@ -242,11 +246,14 @@ class Wrapper implements Storage, ILockingStorage, IWriteStreamStorage {
 			if ($storage instanceof $class) {
 				break;
 			}
+
 			$storage = $storage->getWrapperStorage();
 		}
+
 		if (!($storage instanceof $class)) {
 			return null;
 		}
+
 		return $storage;
 	}
 
@@ -261,6 +268,7 @@ class Wrapper implements Storage, ILockingStorage, IWriteStreamStorage {
 
 	#[Override]
 	public function getDirectDownload(string $path): array|false {
+		/** @psalm-suppress DeprecatedMethod */
 		return $this->getWrapperStorage()->getDirectDownload($path);
 	}
 
@@ -302,20 +310,23 @@ class Wrapper implements Storage, ILockingStorage, IWriteStreamStorage {
 	}
 
 	public function acquireLock(string $path, int $type, ILockingProvider $provider): void {
-		if ($this->getWrapperStorage()->instanceOfStorage('\OCP\Files\Storage\ILockingStorage')) {
-			$this->getWrapperStorage()->acquireLock($path, $type, $provider);
+		$storage = $this->getWrapperStorage();
+		if ($storage->instanceOfStorage(ILockingStorage::class)) {
+			$storage->acquireLock($path, $type, $provider);
 		}
 	}
 
 	public function releaseLock(string $path, int $type, ILockingProvider $provider): void {
-		if ($this->getWrapperStorage()->instanceOfStorage('\OCP\Files\Storage\ILockingStorage')) {
-			$this->getWrapperStorage()->releaseLock($path, $type, $provider);
+		$storage = $this->getWrapperStorage();
+		if ($storage->instanceOfStorage(ILockingStorage::class)) {
+			$storage->releaseLock($path, $type, $provider);
 		}
 	}
 
 	public function changeLock(string $path, int $type, ILockingProvider $provider): void {
-		if ($this->getWrapperStorage()->instanceOfStorage('\OCP\Files\Storage\ILockingStorage')) {
-			$this->getWrapperStorage()->changeLock($path, $type, $provider);
+		$storage = $this->getWrapperStorage();
+		if ($storage->instanceOfStorage(ILockingStorage::class)) {
+			$storage->changeLock($path, $type, $provider);
 		}
 	}
 
@@ -328,17 +339,21 @@ class Wrapper implements Storage, ILockingStorage, IWriteStreamStorage {
 		if ($storage->instanceOfStorage(IWriteStreamStorage::class)) {
 			/** @var IWriteStreamStorage $storage */
 			return $storage->writeStream($path, $stream, $size);
-		} else {
-			$target = $this->fopen($path, 'w');
-			$count = stream_copy_to_stream($stream, $target);
-			fclose($stream);
-			fclose($target);
-			if ($count === false) {
-				throw new GenericFileException('Failed to copy stream.');
-			}
-
-			return $count;
 		}
+
+		$target = $this->fopen($path, 'w');
+		if ($target === false) {
+			throw new GenericFileException('Failed to open ' . $path);
+		}
+
+		$count = stream_copy_to_stream($stream, $target);
+		fclose($stream);
+		fclose($target);
+		if ($count === false) {
+			throw new GenericFileException('Failed to copy stream.');
+		}
+
+		return $count;
 	}
 
 	public function getDirectoryContent(string $directory): \Traversable {
@@ -350,9 +365,11 @@ class Wrapper implements Storage, ILockingStorage, IWriteStreamStorage {
 		if ($wrapped === $storage) {
 			return true;
 		}
+
 		if ($wrapped instanceof Wrapper) {
 			return $wrapped->isWrapperOf($storage);
 		}
+
 		return false;
 	}
 

psalm-strict.xml

@@ -24,6 +24,7 @@
 		<file name="lib/private/Settings/AuthorizedGroupMapper.php"/>
 		<file name="apps/settings/lib/Service/AuthorizedGroupService.php"/>
 		<file name="lib/private/Files/Storage/Storage.php"/>
+		<file name="lib/private/Files/Storage/Wrapper/Wrapper.php"/>
 		<ignoreFiles>
 			<directory name="apps/**/composer"/>
 			<directory name="apps/**/tests"/>

tests/lib/Encryption/EncryptionWrapperTest.php

@@ -15,7 +15,6 @@ use OC\Memcache\ArrayCache;
 use OCA\Files_Trashbin\Storage;
 use OCP\Files\Mount\IMountPoint;
 use OCP\Files\Storage\IDisableEncryptionStorage;
-use OCP\Files\Storage\IStorage;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
@@ -45,17 +44,13 @@ class EncryptionWrapperTest extends TestCase {
 
 	#[\PHPUnit\Framework\Attributes\DataProvider('provideWrapStorage')]
 	public function testWrapStorage($expectedWrapped, $wrappedStorages): void {
-		$storage = $this->getMockBuilder(IStorage::class)
+		$storage = $this->getMockBuilder(Storage::class)
 			->disableOriginalConstructor()
 			->getMock();
 
-		foreach ($wrappedStorages as $wrapper) {
-			$storage->expects($this->any())
-				->method('instanceOfStorage')
-				->willReturnMap([
-					[$wrapper, true],
-				]);
-		}
+		$storage->expects($this->any())
+			->method('instanceOfStorage')
+			->willReturnCallback(fn (string $storage): bool => in_array($storage, $wrappedStorages, true));
 
 		$mount = $this->getMockBuilder(IMountPoint::class)
 			->disableOriginalConstructor()

tests/lib/Files/Storage/Wrapper/EncryptionTest.php

@@ -958,7 +958,7 @@ class EncryptionTest extends Storage {
 		$wrapper = $this->getMockBuilder(Encryption::class)
 			->setConstructorArgs(
 				[
-					['mountPoint' => '', 'mount' => $mount, 'storage' => ''],
+					['mountPoint' => '', 'mount' => $mount, 'storage' => null],
 					$encryptionManager,
 					$util,
 					$this->logger,