From 4eada2d804894dad5f0ce717945b7490864ba3bc Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 16:14:59 +0100 Subject: [PATCH] refactor(Files): Modernize Wrapper Signed-off-by: provokateurin --- apps/files_sharing/lib/SharedStorage.php | 6 -- build/rector-strict.php | 1 + lib/private/Files/Storage/Wrapper/Jail.php | 8 ++ lib/private/Files/Storage/Wrapper/Wrapper.php | 95 +++++++++++-------- psalm-strict.xml | 1 + .../lib/Encryption/EncryptionWrapperTest.php | 13 +-- .../Files/Storage/Wrapper/EncryptionTest.php | 2 +- 7 files changed, 71 insertions(+), 55 deletions(-) diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index aeaaa54f370..136008a7b5c 100644 --- a/apps/files_sharing/lib/SharedStorage.php +++ b/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); diff --git a/build/rector-strict.php b/build/rector-strict.php index 232630ab521..768fe3cebe5 100644 --- a/build/rector-strict.php +++ b/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, diff --git a/lib/private/Files/Storage/Wrapper/Jail.php b/lib/private/Files/Storage/Wrapper/Jail.php index ebde93328d9..1d5dfc7153a 100644 --- a/lib/private/Files/Storage/Wrapper/Jail.php +++ b/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; } diff --git a/lib/private/Files/Storage/Wrapper/Wrapper.php b/lib/private/Files/Storage/Wrapper/Wrapper.php index 3a95c754747..3c8d4d63d10 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -1,5 +1,7 @@ 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; } diff --git a/psalm-strict.xml b/psalm-strict.xml index 32b6550d799..07be9fddeea 100644 --- a/psalm-strict.xml +++ b/psalm-strict.xml @@ -24,6 +24,7 @@ + diff --git a/tests/lib/Encryption/EncryptionWrapperTest.php b/tests/lib/Encryption/EncryptionWrapperTest.php index 58bf5aff005..8d1deacf3fd 100644 --- a/tests/lib/Encryption/EncryptionWrapperTest.php +++ b/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() diff --git a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php index cd82d200de8..8308d64c5a5 100644 --- a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php +++ b/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,