From 966a3e696335d9dad2cc8dfa2ec44d44298626ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 13 Mar 2023 18:33:12 +0100 Subject: [PATCH 1/8] Tidy up typing in OC\Files\View MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../unit/Connector/Sabre/DirectoryTest.php | 3 +- lib/private/Files/Filesystem.php | 65 +++------ lib/private/Files/View.php | 131 +++++++----------- tests/lib/Files/ViewTest.php | 2 +- 4 files changed, 76 insertions(+), 125 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index a74cb13996607..c6365cf316851 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -63,7 +63,7 @@ public function rename($path1, $path2) { return $this->canRename; } - public function getRelativePath($path) { + public function getRelativePath($path): ?string { return $path; } } @@ -73,7 +73,6 @@ public function getRelativePath($path) { * @group DB */ class DirectoryTest extends \Test\TestCase { - use UserTrait; /** @var \OC\Files\View | \PHPUnit\Framework\MockObject\MockObject */ diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index 500a13b1f9d8b..7067ddf4a9593 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -42,6 +42,7 @@ use OC\User\NoUserException; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Events\Node\FilesystemTornDownEvent; +use OCP\Files\Mount\IMountManager; use OCP\Files\NotFoundException; use OCP\Files\Storage\IStorageFactory; use OCP\IUser; @@ -49,25 +50,16 @@ use OCP\IUserSession; class Filesystem { - /** - * @var Mount\Manager $mounts - */ - private static $mounts; - - public static $loaded = false; - /** - * @var \OC\Files\View $defaultInstance - */ - private static $defaultInstance; + private static ?Mount\Manager $mounts = null; - private static $usersSetup = []; + public static bool $loaded = false; - private static $normalizedPathCache = null; + private static ?View $defaultInstance = null; - private static $listeningForProviders = false; + private static ?CappedMemoryCache $normalizedPathCache = null; /** @var string[]|null */ - private static $blacklist = null; + private static ?array $blacklist = null; /** * classname which used for hooks handling @@ -186,22 +178,18 @@ class Filesystem { public const signal_param_mount_type = 'mounttype'; public const signal_param_users = 'users'; - /** - * @var \OC\Files\Storage\StorageFactory $loader - */ - private static $loader; + private static ?\OC\Files\Storage\StorageFactory $loader = null; - /** @var bool */ - private static $logWarningWhenAddingStorageWrapper = true; + private static bool $logWarningWhenAddingStorageWrapper = true; /** * @param bool $shouldLog * @return bool previous value * @internal */ - public static function logWarningWhenAddingStorageWrapper($shouldLog) { + public static function logWarningWhenAddingStorageWrapper(bool $shouldLog): bool { $previousValue = self::$logWarningWhenAddingStorageWrapper; - self::$logWarningWhenAddingStorageWrapper = (bool) $shouldLog; + self::$logWarningWhenAddingStorageWrapper = $shouldLog; return $previousValue; } @@ -232,18 +220,17 @@ public static function addStorageWrapper($wrapperName, $wrapper, $priority = 50) */ public static function getLoader() { if (!self::$loader) { - self::$loader = \OC::$server->query(IStorageFactory::class); + self::$loader = \OC::$server->get(IStorageFactory::class); } return self::$loader; } /** * Returns the mount manager - * - * @return \OC\Files\Mount\Manager */ - public static function getMountManager($user = '') { + public static function getMountManager(): Mount\Manager { self::initMountManager(); + assert(self::$mounts !== null); return self::$mounts; } @@ -313,14 +300,14 @@ public static function getMountByNumericId($id) { * resolve a path to a storage and internal path * * @param string $path - * @return array an array consisting of the storage and the internal path + * @return array{?\OCP\Files\Storage\IStorage, string} an array consisting of the storage and the internal path */ - public static function resolvePath($path) { + public static function resolvePath($path): array { $mount = self::getMountManager()->find($path); return [$mount->getStorage(), rtrim($mount->getInternalPath($path), '/')]; } - public static function init($user, $root) { + public static function init(string|IUser|null $user, string $root): bool { if (self::$defaultInstance) { return false; } @@ -332,7 +319,7 @@ public static function init($user, $root) { return true; } - public static function initInternal($root) { + public static function initInternal(string $root): bool { if (self::$defaultInstance) { return false; } @@ -342,32 +329,28 @@ public static function initInternal($root) { $eventDispatcher = \OC::$server->get(IEventDispatcher::class); $eventDispatcher->addListener(FilesystemTornDownEvent::class, function () { self::$defaultInstance = null; - self::$usersSetup = []; self::$loaded = false; }); - if (!self::$mounts) { - self::$mounts = \OC::$server->getMountManager(); - } + self::initMountManager(); self::$loaded = true; return true; } - public static function initMountManager() { + public static function initMountManager(): void { if (!self::$mounts) { - self::$mounts = \OC::$server->getMountManager(); + self::$mounts = \OC::$server->get(IMountManager::class); } } /** * Initialize system and personal mount points for a user * - * @param string|IUser|null $user * @throws \OC\User\NoUserException if the user is not available */ - public static function initMountPoints($user = '') { + public static function initMountPoints(string|IUser|null $user = ''): void { /** @var IUserManager $userManager */ $userManager = \OC::$server->get(IUserManager::class); @@ -382,11 +365,9 @@ public static function initMountPoints($user = '') { } /** - * get the default filesystem view - * - * @return View + * Get the default filesystem view */ - public static function getView() { + public static function getView(): ?View { if (!self::$defaultInstance) { /** @var IUserSession $session */ $session = \OC::$server->get(IUserSession::class); diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 1bd131303e37e..2a6b34f2de615 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -52,6 +52,7 @@ use OC\User\LazyUser; use OC\Share\Share; use OC\User\User; +use OC\User\Manager as UserManager; use OCA\Files_Sharing\SharedMount; use OCP\Constants; use OCP\Files\Cache\ICacheEntry; @@ -86,31 +87,17 @@ * \OC\Files\Storage\Storage object */ class View { - /** @var string */ - private $fakeRoot = ''; - - /** - * @var \OCP\Lock\ILockingProvider - */ - protected $lockingProvider; - - private $lockingEnabled; - - private $updaterEnabled = true; - - /** @var \OC\User\Manager */ - private $userManager; - + private string $fakeRoot = ''; + private ILockingProvider $lockingProvider; + private bool $lockingEnabled; + private bool $updaterEnabled = true; + private UserManager $userManager; private LoggerInterface $logger; /** - * @param string $root * @throws \Exception If $root contains an invalid path */ - public function __construct($root = '') { - if (is_null($root)) { - throw new \InvalidArgumentException('Root can\'t be null'); - } + public function __construct(string $root = '') { if (!Filesystem::isValidPath($root)) { throw new \Exception(); } @@ -122,7 +109,13 @@ public function __construct($root = '') { $this->logger = \OC::$server->get(LoggerInterface::class); } - public function getAbsolutePath($path = '/') { + /** + * @param ?string $path + * @psalm-template S as string|null + * @psalm-param S $path + * @psalm-return (S is string ? string : null) + */ + public function getAbsolutePath($path = '/'): ?string { if ($path === null) { return null; } @@ -137,12 +130,11 @@ public function getAbsolutePath($path = '/') { } /** - * change the root to a fake root + * Change the root to a fake root * * @param string $fakeRoot - * @return boolean|null */ - public function chroot($fakeRoot) { + public function chroot($fakeRoot): void { if (!$fakeRoot == '') { if ($fakeRoot[0] !== '/') { $fakeRoot = '/' . $fakeRoot; @@ -152,11 +144,9 @@ public function chroot($fakeRoot) { } /** - * get the fake root - * - * @return string + * Get the fake root */ - public function getRoot() { + public function getRoot(): string { return $this->fakeRoot; } @@ -164,9 +154,8 @@ public function getRoot() { * get path relative to the root of the view * * @param string $path - * @return ?string */ - public function getRelativePath($path) { + public function getRelativePath($path): ?string { $this->assertPathLength($path); if ($this->fakeRoot == '') { return $path; @@ -192,74 +181,70 @@ public function getRelativePath($path) { } /** - * get the mountpoint of the storage object for a path + * Get the mountpoint of the storage object for a path * ( note: because a storage is not always mounted inside the fakeroot, the * returned mountpoint is relative to the absolute root of the filesystem * and does not take the chroot into account ) * * @param string $path - * @return string */ - public function getMountPoint($path) { + public function getMountPoint($path): string { return Filesystem::getMountPoint($this->getAbsolutePath($path)); } /** - * get the mountpoint of the storage object for a path + * Get the mountpoint of the storage object for a path * ( note: because a storage is not always mounted inside the fakeroot, the * returned mountpoint is relative to the absolute root of the filesystem * and does not take the chroot into account ) * * @param string $path - * @return \OCP\Files\Mount\IMountPoint */ - public function getMount($path) { + public function getMount($path): IMountPoint { return Filesystem::getMountManager()->find($this->getAbsolutePath($path)); } /** - * resolve a path to a storage and internal path + * Resolve a path to a storage and internal path * * @param string $path - * @return array an array consisting of the storage and the internal path + * @return array{?\OCP\Files\Storage\IStorage, string} an array consisting of the storage and the internal path */ - public function resolvePath($path) { + public function resolvePath($path): array { $a = $this->getAbsolutePath($path); $p = Filesystem::normalizePath($a); return Filesystem::resolvePath($p); } /** - * return the path to a local version of the file + * Return the path to a local version of the file * we need this because we can't know if a file is stored local or not from * outside the filestorage and for some purposes a local file is needed * * @param string $path - * @return string */ - public function getLocalFile($path) { - $parent = substr($path, 0, strrpos($path, '/')); + public function getLocalFile($path): string|bool { + $parent = substr($path, 0, strrpos($path, '/') ?: 0); $path = $this->getAbsolutePath($path); [$storage, $internalPath] = Filesystem::resolvePath($path); if (Filesystem::isValidPath($parent) and $storage) { return $storage->getLocalFile($internalPath); } else { - return null; + return false; } } /** * @param string $path - * @return string */ - public function getLocalFolder($path) { - $parent = substr($path, 0, strrpos($path, '/')); + public function getLocalFolder($path): string|bool { + $parent = substr($path, 0, strrpos($path, '/') ?: 0); $path = $this->getAbsolutePath($path); [$storage, $internalPath] = Filesystem::resolvePath($path); if (Filesystem::isValidPath($parent) and $storage) { return $storage->getLocalFolder($internalPath); } else { - return null; + return false; } } @@ -277,9 +262,8 @@ public function mkdir($path) { * * @param IMountPoint $mount * @param string $path relative to data/ - * @return boolean */ - protected function removeMount($mount, $path) { + protected function removeMount($mount, $path): bool { if ($mount instanceof MoveableMount) { // cut of /user/files to get the relative path to data/user/files $pathParts = explode('/', $path, 4); @@ -308,15 +292,15 @@ protected function removeMount($mount, $path) { } } - public function disableCacheUpdate() { + public function disableCacheUpdate(): void { $this->updaterEnabled = false; } - public function enableCacheUpdate() { + public function enableCacheUpdate(): void { $this->updaterEnabled = true; } - protected function writeUpdate(Storage $storage, $internalPath, $time = null) { + protected function writeUpdate(Storage $storage, string $internalPath, ?int $time = null): void { if ($this->updaterEnabled) { if (is_null($time)) { $time = time(); @@ -325,13 +309,13 @@ protected function writeUpdate(Storage $storage, $internalPath, $time = null) { } } - protected function removeUpdate(Storage $storage, $internalPath) { + protected function removeUpdate(Storage $storage, string $internalPath): void { if ($this->updaterEnabled) { $storage->getUpdater()->remove($internalPath); } } - protected function renameUpdate(Storage $sourceStorage, Storage $targetStorage, $sourceInternalPath, $targetInternalPath) { + protected function renameUpdate(Storage $sourceStorage, Storage $targetStorage, string $sourceInternalPath, string $targetInternalPath): void { if ($this->updaterEnabled) { $targetStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); } @@ -559,9 +543,8 @@ public function filemtime($path) { /** * @param string $path * @param int|string $mtime - * @return bool */ - public function touch($path, $mtime = null) { + public function touch($path, $mtime = null): bool { if (!is_null($mtime) and !is_numeric($mtime)) { $mtime = strtotime($mtime); } @@ -602,12 +585,7 @@ public function file_get_contents($path) { return $this->basicOperation('file_get_contents', $path, ['read']); } - /** - * @param bool $exists - * @param string $path - * @param bool $run - */ - protected function emit_file_hooks_pre($exists, $path, &$run) { + protected function emit_file_hooks_pre(bool $exists, string $path, bool &$run): void { if (!$exists) { \OC_Hook::emit(Filesystem::CLASSNAME, Filesystem::signal_create, [ Filesystem::signal_param_path => $this->getHookPath($path), @@ -625,11 +603,7 @@ protected function emit_file_hooks_pre($exists, $path, &$run) { ]); } - /** - * @param bool $exists - * @param string $path - */ - protected function emit_file_hooks_post($exists, $path) { + protected function emit_file_hooks_post(bool $exists, string $path): void { if (!$exists) { \OC_Hook::emit(Filesystem::CLASSNAME, Filesystem::signal_post_create, [ Filesystem::signal_param_path => $this->getHookPath($path), @@ -973,7 +947,7 @@ public function copy($source, $target, $preserveMtime = false) { /** * @param string $path * @param string $mode 'r' or 'w' - * @return resource + * @return resource|false * @throws LockedException */ public function fopen($path, $mode) { @@ -1018,10 +992,9 @@ public function fopen($path, $mode) { /** * @param string $path - * @return bool|string * @throws \OCP\Files\InvalidPathException */ - public function toTmpFile($path) { + public function toTmpFile($path): string|false { $this->assertPathLength($path); if (Filesystem::isValidPath($path)) { $source = $this->fopen($path, 'r'); @@ -1061,9 +1034,12 @@ public function fromTmpFile($tmpFile, $path) { $source = fopen($tmpFile, 'r'); if ($source) { $result = $this->file_put_contents($path, $source); - // $this->file_put_contents() might have already closed - // the resource, so we check it, before trying to close it - // to avoid messages in the error log. + /** + * $this->file_put_contents() might have already closed + * the resource, so we check it, before trying to close it + * to avoid messages in the error log. + * @psalm-suppress RedundantCondition false-positive + */ if (is_resource($source)) { fclose($source); } @@ -1092,9 +1068,8 @@ public function getMimeType($path) { * @param string $type * @param string $path * @param bool $raw - * @return bool|string */ - public function hash($type, $path, $raw = false) { + public function hash($type, $path, $raw = false): string|bool { $postFix = (substr($path, -1) === '/') ? '/' : ''; $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); if (Filesystem::isValidPath($path)) { @@ -1722,10 +1697,6 @@ public function getOwner($path) { * @return string */ public function getETag($path) { - /** - * @var Storage\Storage $storage - * @var string $internalPath - */ [$storage, $internalPath] = $this->resolvePath($path); if ($storage) { return $storage->getETag($internalPath); diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index 7a449c4893bea..b17eeea83c03a 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -1296,7 +1296,7 @@ public function testGetRelativePathOnNull() { public function testNullAsRoot() { - $this->expectException(\InvalidArgumentException::class); + $this->expectException(\TypeError::class); new View(null); } From 4393b96542be6b0495d93cf9ed87e9b51b36121f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 28 Feb 2023 14:14:20 +0100 Subject: [PATCH 2/8] Remove unused method getLocalFolder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s not used and not in any OCP interface/class. Signed-off-by: Côme Chilliet --- .../tests/Command/FixEncryptedVersionTest.php | 2 +- lib/private/Files/Filesystem.php | 8 -------- lib/private/Files/Storage/FailedStorage.php | 4 ---- lib/private/Files/Storage/Local.php | 4 ---- lib/private/Files/View.php | 14 -------------- tests/lib/Files/ViewTest.php | 1 - 6 files changed, 1 insertion(+), 32 deletions(-) diff --git a/apps/encryption/tests/Command/FixEncryptedVersionTest.php b/apps/encryption/tests/Command/FixEncryptedVersionTest.php index 5c938b4350d6a..2a6c86ef5b23c 100644 --- a/apps/encryption/tests/Command/FixEncryptedVersionTest.php +++ b/apps/encryption/tests/Command/FixEncryptedVersionTest.php @@ -263,7 +263,7 @@ public function testRepairUnencryptedFileWhenVersionIsSet() { $cacheInfo = ['encryptedVersion' => 1, 'encrypted' => 1]; $cache1->put($fileCache1->getPath(), $cacheInfo); - $absPath = $view->getLocalFolder(''). '/hello.txt'; + $absPath = $storage1->getSourcePath('').$fileInfo1->getInternalPath(); // create unencrypted file on disk, the version stays file_put_contents($absPath, 'hello contents'); diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index 7067ddf4a9593..ee8db5b4353fe 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -428,14 +428,6 @@ public static function getLocalFile($path) { return self::$defaultInstance->getLocalFile($path); } - /** - * @param string $path - * @return string - */ - public static function getLocalFolder($path) { - return self::$defaultInstance->getLocalFolder($path); - } - /** * return path to file which reflects one visible in browser * diff --git a/lib/private/Files/Storage/FailedStorage.php b/lib/private/Files/Storage/FailedStorage.php index ceb802570bf7e..07b3b21d965d5 100644 --- a/lib/private/Files/Storage/FailedStorage.php +++ b/lib/private/Files/Storage/FailedStorage.php @@ -164,10 +164,6 @@ public function getLocalFile($path) { throw new StorageNotAvailableException($this->e->getMessage(), $this->e->getCode(), $this->e); } - public function getLocalFolder($path) { - throw new StorageNotAvailableException($this->e->getMessage(), $this->e->getCode(), $this->e); - } - public function hasUpdated($path, $time) { throw new StorageNotAvailableException($this->e->getMessage(), $this->e->getCode(), $this->e); } diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php index 1ff733e0d9251..448346e562251 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -433,10 +433,6 @@ public function getLocalFile($path) { return $this->getSourcePath($path); } - public function getLocalFolder($path) { - return $this->getSourcePath($path); - } - /** * @param string $query * @param string $dir diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 2a6b34f2de615..4ee44b995b333 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -234,20 +234,6 @@ public function getLocalFile($path): string|bool { } } - /** - * @param string $path - */ - public function getLocalFolder($path): string|bool { - $parent = substr($path, 0, strrpos($path, '/') ?: 0); - $path = $this->getAbsolutePath($path); - [$storage, $internalPath] = Filesystem::resolvePath($path); - if (Filesystem::isValidPath($parent) and $storage) { - return $storage->getLocalFolder($internalPath); - } else { - return false; - } - } - /** * the following functions operate with arguments and return values identical * to those of their PHP built-in equivalents. Mostly they are merely wrappers diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index b17eeea83c03a..18a6fca05b0ad 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -1096,7 +1096,6 @@ public function tooLongPathDataProvider() { ['getMountPoint'], ['resolvePath'], ['getLocalFile'], - ['getLocalFolder'], ['mkdir'], ['rmdir'], ['opendir'], From f974281ac9a086004c7e971f3585ed8aa21d194d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 28 Feb 2023 16:51:44 +0100 Subject: [PATCH 3/8] Improve typing for fopen/toTmpFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Files/Filesystem.php | 7 ++++--- lib/private/Files/Node/File.php | 2 +- lib/public/Files/File.php | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index ee8db5b4353fe..74205351e60ca 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -390,7 +390,7 @@ public static function tearDown() { /** * get the relative path of the root data directory for the current user * - * @return string + * @return ?string * * Returns path like /admin/files */ @@ -584,9 +584,10 @@ public static function fopen($path, $mode) { } /** - * @return string + * @param string $path + * @throws \OCP\Files\InvalidPathException */ - public static function toTmpFile($path) { + public static function toTmpFile($path): string|false { return self::$defaultInstance->toTmpFile($path); } diff --git a/lib/private/Files/Node/File.php b/lib/private/Files/Node/File.php index 475930b361a48..27056efef72b4 100644 --- a/lib/private/Files/Node/File.php +++ b/lib/private/Files/Node/File.php @@ -80,7 +80,7 @@ public function putContent($data) { /** * @param string $mode - * @return resource + * @return resource|false * @throws NotPermittedException * @throws LockedException */ diff --git a/lib/public/Files/File.php b/lib/public/Files/File.php index cdb868241b6b4..93d0ab860c1e2 100644 --- a/lib/public/Files/File.php +++ b/lib/public/Files/File.php @@ -68,7 +68,7 @@ public function getMimeType(); * Open the file as stream, resulting resource can be operated as stream like the result from php's own fopen * * @param string $mode - * @return resource + * @return resource|false * @throws NotPermittedException * @throws LockedException * @since 6.0.0 From ea05544213b6d472338684e9c7bae66f68453c58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 13 Mar 2023 18:59:16 +0100 Subject: [PATCH 4/8] Fix return type of methods returning false on error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../lib/Versions/IVersionBackend.php | 2 +- lib/private/Files/Filesystem.php | 10 ++----- lib/private/Files/SimpleFS/NewSimpleFile.php | 2 +- lib/private/Files/SimpleFS/SimpleFile.php | 2 +- .../Files/Storage/LocalTempFileTrait.php | 16 +++------- .../Files/Storage/Wrapper/Encoding.php | 6 ++-- .../Wrapper/EncodingDirectoryWrapper.php | 2 +- lib/private/Files/Storage/Wrapper/Jail.php | 6 ++-- lib/private/Files/Storage/Wrapper/Wrapper.php | 6 ++-- lib/private/Files/View.php | 8 ++--- lib/private/Security/CertificateManager.php | 30 +++++++------------ lib/private/Server.php | 2 +- lib/public/Files/SimpleFS/ISimpleFile.php | 2 +- lib/public/Files/Storage.php | 6 ++-- lib/public/Files/Storage/IStorage.php | 6 ++-- 15 files changed, 41 insertions(+), 65 deletions(-) diff --git a/apps/files_versions/lib/Versions/IVersionBackend.php b/apps/files_versions/lib/Versions/IVersionBackend.php index c06e395e4c12d..39e7567226632 100644 --- a/apps/files_versions/lib/Versions/IVersionBackend.php +++ b/apps/files_versions/lib/Versions/IVersionBackend.php @@ -78,7 +78,7 @@ public function rollback(IVersion $version); * Open the file for reading * * @param IVersion $version - * @return resource + * @return resource|false * @throws NotFoundException * @since 15.0.0 */ diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index 74205351e60ca..367982eed72d8 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -420,11 +420,8 @@ public static function mount($class, $arguments, $mountpoint) { * return the path to a local version of the file * we need this because we can't know if a file is stored local or not from * outside the filestorage and for some purposes a local file is needed - * - * @param string $path - * @return string */ - public static function getLocalFile($path) { + public static function getLocalFile(string $path): string|false { return self::$defaultInstance->getLocalFile($path); } @@ -761,11 +758,8 @@ public static function getOwner($path) { /** * get the ETag for a file or folder - * - * @param string $path - * @return string */ - public static function getETag($path) { + public static function getETag(string $path): string|false { return self::$defaultInstance->getETag($path); } } diff --git a/lib/private/Files/SimpleFS/NewSimpleFile.php b/lib/private/Files/SimpleFS/NewSimpleFile.php index 1d0972bcc54f4..50511b5a5f927 100644 --- a/lib/private/Files/SimpleFS/NewSimpleFile.php +++ b/lib/private/Files/SimpleFS/NewSimpleFile.php @@ -196,7 +196,7 @@ public function getExtension(): string { /** * Open the file as stream for reading, resulting resource can be operated as stream like the result from php's own fopen * - * @return resource + * @return resource|false * @throws \OCP\Files\NotPermittedException * @since 14.0.0 */ diff --git a/lib/private/Files/SimpleFS/SimpleFile.php b/lib/private/Files/SimpleFS/SimpleFile.php index 95ef332e2f52a..76ab06feaab38 100644 --- a/lib/private/Files/SimpleFS/SimpleFile.php +++ b/lib/private/Files/SimpleFS/SimpleFile.php @@ -150,7 +150,7 @@ public function getExtension(): string { /** * Open the file as stream for reading, resulting resource can be operated as stream like the result from php's own fopen * - * @return resource + * @return resource|false * @throws \OCP\Files\NotPermittedException * @since 14.0.0 */ diff --git a/lib/private/Files/Storage/LocalTempFileTrait.php b/lib/private/Files/Storage/LocalTempFileTrait.php index 2ac0a74b6928a..314e38cb27704 100644 --- a/lib/private/Files/Storage/LocalTempFileTrait.php +++ b/lib/private/Files/Storage/LocalTempFileTrait.php @@ -35,14 +35,10 @@ * in classes which extend it, e.g. $this->stat() . */ trait LocalTempFileTrait { - /** @var string[] */ - protected $cachedFiles = []; + /** @var array */ + protected array $cachedFiles = []; - /** - * @param string $path - * @return string - */ - protected function getCachedFile($path) { + protected function getCachedFile(string $path): string|false { if (!isset($this->cachedFiles[$path])) { $this->cachedFiles[$path] = $this->toTmpFile($path); } @@ -56,11 +52,7 @@ protected function removeCachedFile($path) { unset($this->cachedFiles[$path]); } - /** - * @param string $path - * @return string - */ - protected function toTmpFile($path) { //no longer in the storage api, still useful here + protected function toTmpFile(string $path): string|false { //no longer in the storage api, still useful here $source = $this->fopen($path, 'r'); if (!$source) { return false; diff --git a/lib/private/Files/Storage/Wrapper/Encoding.php b/lib/private/Files/Storage/Wrapper/Encoding.php index d6006f746ad4e..dd699993e841b 100644 --- a/lib/private/Files/Storage/Wrapper/Encoding.php +++ b/lib/private/Files/Storage/Wrapper/Encoding.php @@ -159,7 +159,7 @@ public function rmdir($path) { * see https://www.php.net/manual/en/function.opendir.php * * @param string $path - * @return resource|bool + * @return resource|false */ public function opendir($path) { $handle = $this->storage->opendir($this->findPathToUse($path)); @@ -429,7 +429,7 @@ public function touch($path, $mtime = null) { * The local version of the file can be temporary and doesn't have to be persistent across requests * * @param string $path - * @return string|bool + * @return string|false */ public function getLocalFile($path) { return $this->storage->getLocalFile($this->findPathToUse($path)); @@ -481,7 +481,7 @@ public function getScanner($path = '', $storage = null) { * get the ETag for a file or folder * * @param string $path - * @return string|bool + * @return string|false */ public function getETag($path) { return $this->storage->getETag($this->findPathToUse($path)); diff --git a/lib/private/Files/Storage/Wrapper/EncodingDirectoryWrapper.php b/lib/private/Files/Storage/Wrapper/EncodingDirectoryWrapper.php index fd0d2707f8d12..3cda399f22d6c 100644 --- a/lib/private/Files/Storage/Wrapper/EncodingDirectoryWrapper.php +++ b/lib/private/Files/Storage/Wrapper/EncodingDirectoryWrapper.php @@ -46,7 +46,7 @@ public function dir_readdir() { /** * @param resource $source * @param callable $filter - * @return resource|bool + * @return resource|false */ public static function wrap($source) { return self::wrapSource($source, [ diff --git a/lib/private/Files/Storage/Wrapper/Jail.php b/lib/private/Files/Storage/Wrapper/Jail.php index e21a2cba244a8..619d8b07137c8 100644 --- a/lib/private/Files/Storage/Wrapper/Jail.php +++ b/lib/private/Files/Storage/Wrapper/Jail.php @@ -108,7 +108,7 @@ public function rmdir($path) { * see https://www.php.net/manual/en/function.opendir.php * * @param string $path - * @return resource|bool + * @return resource|false */ public function opendir($path) { return $this->getWrapperStorage()->opendir($this->getUnjailedPath($path)); @@ -368,7 +368,7 @@ public function touch($path, $mtime = null) { * The local version of the file can be temporary and doesn't have to be persistent across requests * * @param string $path - * @return string|bool + * @return string|false */ public function getLocalFile($path) { return $this->getWrapperStorage()->getLocalFile($this->getUnjailedPath($path)); @@ -431,7 +431,7 @@ public function getWatcher($path = '', $storage = null) { * get the ETag for a file or folder * * @param string $path - * @return string|bool + * @return string|false */ public function getETag($path) { return $this->getWrapperStorage()->getETag($this->getUnjailedPath($path)); diff --git a/lib/private/Files/Storage/Wrapper/Wrapper.php b/lib/private/Files/Storage/Wrapper/Wrapper.php index c2d382f5dfc52..28a1e0b1e4da5 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -98,7 +98,7 @@ public function rmdir($path) { * see https://www.php.net/manual/en/function.opendir.php * * @param string $path - * @return resource|bool + * @return resource|false */ public function opendir($path) { return $this->getWrapperStorage()->opendir($path); @@ -358,7 +358,7 @@ public function touch($path, $mtime = null) { * The local version of the file can be temporary and doesn't have to be persistent across requests * * @param string $path - * @return string|bool + * @return string|false */ public function getLocalFile($path) { return $this->getWrapperStorage()->getLocalFile($path); @@ -456,7 +456,7 @@ public function getStorageCache() { * get the ETag for a file or folder * * @param string $path - * @return string|bool + * @return string|false */ public function getETag($path) { return $this->getWrapperStorage()->getETag($path); diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 4ee44b995b333..fd264a77cbaf7 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -223,7 +223,7 @@ public function resolvePath($path): array { * * @param string $path */ - public function getLocalFile($path): string|bool { + public function getLocalFile($path): string|false { $parent = substr($path, 0, strrpos($path, '/') ?: 0); $path = $this->getAbsolutePath($path); [$storage, $internalPath] = Filesystem::resolvePath($path); @@ -333,7 +333,7 @@ public function rmdir($path) { /** * @param string $path - * @return resource + * @return resource|false */ public function opendir($path) { return $this->basicOperation('opendir', $path, ['read']); @@ -1680,14 +1680,14 @@ public function getOwner($path) { * get the ETag for a file or folder * * @param string $path - * @return string + * @return string|false */ public function getETag($path) { [$storage, $internalPath] = $this->resolvePath($path); if ($storage) { return $storage->getETag($internalPath); } else { - return null; + return false; } } diff --git a/lib/private/Security/CertificateManager.php b/lib/private/Security/CertificateManager.php index fa26c19ceae84..be884654bd0bd 100644 --- a/lib/private/Security/CertificateManager.php +++ b/lib/private/Security/CertificateManager.php @@ -33,6 +33,7 @@ namespace OC\Security; use OC\Files\Filesystem; +use OC\Files\View; use OCP\ICertificate; use OCP\ICertificateManager; use OCP\IConfig; @@ -43,24 +44,14 @@ * Manage trusted certificates for users */ class CertificateManager implements ICertificateManager { - /** - * @var \OC\Files\View - */ - protected $view; - - /** - * @var IConfig - */ - protected $config; - + protected View $view; + protected IConfig $config; protected LoggerInterface $logger; - - /** @var ISecureRandom */ - protected $random; + protected ISecureRandom $random; private ?string $bundlePath = null; - public function __construct(\OC\Files\View $view, + public function __construct(View $view, IConfig $config, LoggerInterface $logger, ISecureRandom $random) { @@ -233,8 +224,7 @@ public function getCertificateBundle(): string { /** * Get the full local path to the certificate bundle - * - * @return string + * @throws \Exception when getting bundle path fails */ public function getAbsoluteBundlePath(): string { try { @@ -247,7 +237,10 @@ public function getAbsoluteBundlePath(): string { $this->createCertificateBundle(); } - $this->bundlePath = $this->view->getLocalFile($this->getCertificateBundle()); + $this->bundlePath = $this->view->getLocalFile($this->getCertificateBundle()) ?: null; + } + if ($this->bundlePath === null) { + throw new \Exception('Failed to get absolute bundle path'); } return $this->bundlePath; } catch (\Exception $e) { @@ -255,9 +248,6 @@ public function getAbsoluteBundlePath(): string { } } - /** - * @return string - */ private function getPathToCertificates(): string { return '/files_external/'; } diff --git a/lib/private/Server.php b/lib/private/Server.php index f1e9617088662..97762e4b29b93 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -439,7 +439,7 @@ public function __construct($webRoot, \OC\Config $config) { return $c->get('SystemTagManagerFactory')->getObjectMapper(); }); $this->registerService('RootFolder', function (ContainerInterface $c) { - $manager = \OC\Files\Filesystem::getMountManager(null); + $manager = \OC\Files\Filesystem::getMountManager(); $view = new View(); $root = new Root( $manager, diff --git a/lib/public/Files/SimpleFS/ISimpleFile.php b/lib/public/Files/SimpleFS/ISimpleFile.php index e55261497be2c..8afc310883660 100644 --- a/lib/public/Files/SimpleFS/ISimpleFile.php +++ b/lib/public/Files/SimpleFS/ISimpleFile.php @@ -107,7 +107,7 @@ public function getExtension(): string; /** * Open the file as stream for reading, resulting resource can be operated as stream like the result from php's own fopen * - * @return resource + * @return resource|false * @throws \OCP\Files\NotPermittedException * @since 14.0.0 */ diff --git a/lib/public/Files/Storage.php b/lib/public/Files/Storage.php index c91d47bdcdfec..f5b0775849d37 100644 --- a/lib/public/Files/Storage.php +++ b/lib/public/Files/Storage.php @@ -88,7 +88,7 @@ public function rmdir($path); * see https://www.php.net/manual/en/function.opendir.php * * @param string $path - * @return resource|bool + * @return resource|false * @since 6.0.0 */ public function opendir($path); @@ -326,7 +326,7 @@ public function touch($path, $mtime = null); * The local version of the file can be temporary and doesn't have to be persistent across requests * * @param string $path - * @return string|bool + * @return string|false * @since 6.0.0 */ public function getLocalFile($path); @@ -348,7 +348,7 @@ public function hasUpdated($path, $time); * get the ETag for a file or folder * * @param string $path - * @return string|bool + * @return string|false * @since 6.0.0 */ public function getETag($path); diff --git a/lib/public/Files/Storage/IStorage.php b/lib/public/Files/Storage/IStorage.php index 7b3b9eb484e82..2f38165830bbe 100644 --- a/lib/public/Files/Storage/IStorage.php +++ b/lib/public/Files/Storage/IStorage.php @@ -85,7 +85,7 @@ public function rmdir($path); * see https://www.php.net/manual/en/function.opendir.php * * @param string $path - * @return resource|bool + * @return resource|false * @since 9.0.0 */ public function opendir($path); @@ -314,7 +314,7 @@ public function touch($path, $mtime = null); * The local version of the file can be temporary and doesn't have to be persistent across requests * * @param string $path - * @return string|bool + * @return string|false * @since 9.0.0 */ public function getLocalFile($path); @@ -336,7 +336,7 @@ public function hasUpdated($path, $time); * get the ETag for a file or folder * * @param string $path - * @return string|bool + * @return string|false * @since 9.0.0 */ public function getETag($path); From 0b3dad895fcd87dadb3861bb3842ea8b67b28448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 28 Mar 2023 10:34:01 +0200 Subject: [PATCH 5/8] More type cleanup in View and FileInfo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Files/FileInfo.php | 32 ++++---- lib/private/Files/View.php | 145 ++++++++++++++++----------------- lib/public/Files/FileInfo.php | 2 +- lib/public/Files/Storage.php | 2 +- 4 files changed, 84 insertions(+), 97 deletions(-) diff --git a/lib/private/Files/FileInfo.php b/lib/private/Files/FileInfo.php index b94be4c5497be..3016080d6f73b 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -37,13 +37,9 @@ use OCP\IUser; class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { + private array|ICacheEntry $data; /** - * @var array $data - */ - private $data; - - /** - * @var string $path + * @var string */ private $path; @@ -53,7 +49,7 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { private $storage; /** - * @var string $internalPath + * @var string */ private $internalPath; @@ -62,22 +58,19 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { */ private $mount; - /** - * @var IUser - */ - private $owner; + private ?IUser $owner; /** * @var string[] */ - private $childEtags = []; + private array $childEtags = []; /** * @var IMountPoint[] */ - private $subMounts = []; + private array $subMounts = []; - private $subMountsUsed = false; + private bool $subMountsUsed = false; /** * The size of the file/folder without any sub mount @@ -89,8 +82,8 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { * @param Storage\Storage $storage * @param string $internalPath * @param array|ICacheEntry $data - * @param \OCP\Files\Mount\IMountPoint $mount - * @param \OCP\IUser|null $owner + * @param IMountPoint $mount + * @param ?IUser $owner */ public function __construct($path, $storage, $internalPath, $data, $mount, $owner = null) { $this->path = $path; @@ -107,6 +100,9 @@ public function __construct($path, $storage, $internalPath, $data, $mount, $owne } public function offsetSet($offset, $value): void { + if (is_null($offset)) { + throw new \TypeError('Null offset not supported'); + } $this->data[$offset] = $value; } @@ -357,7 +353,7 @@ public function getMountPoint() { /** * Get the owner of the file * - * @return \OCP\IUser + * @return ?IUser */ public function getOwner() { return $this->owner; @@ -370,7 +366,7 @@ public function setSubMounts(array $mounts) { $this->subMounts = $mounts; } - private function updateEntryfromSubMounts() { + private function updateEntryfromSubMounts(): void { if ($this->subMountsUsed) { return; } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index fd264a77cbaf7..8f324a60ad61f 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -388,7 +388,7 @@ public function filesize(string $path) { /** * @param string $path * @return bool|mixed - * @throws \OCP\Files\InvalidPathException + * @throws InvalidPathException */ public function readfile($path) { $this->assertPathLength($path); @@ -413,7 +413,7 @@ public function readfile($path) { * @param int $from * @param int $to * @return bool|mixed - * @throws \OCP\Files\InvalidPathException + * @throws InvalidPathException * @throws \OCP\Files\UnseekableException */ public function readfilePart($path, $from, $to) { @@ -617,6 +617,9 @@ public function file_put_contents($path, $data) { and !Filesystem::isFileBlacklisted($path) ) { $path = $this->getRelativePath($absolutePath); + if ($path === null) { + throw new InvalidPathException("Path $absolutePath is not in the expected root"); + } $this->lockFile($path, ILockingProvider::LOCK_SHARED); @@ -978,7 +981,7 @@ public function fopen($path, $mode) { /** * @param string $path - * @throws \OCP\Files\InvalidPathException + * @throws InvalidPathException */ public function toTmpFile($path): string|false { $this->assertPathLength($path); @@ -1001,7 +1004,7 @@ public function toTmpFile($path): string|false { * @param string $tmpFile * @param string $path * @return bool|mixed - * @throws \OCP\Files\InvalidPathException + * @throws InvalidPathException */ public function fromTmpFile($tmpFile, $path) { $this->assertPathLength($path); @@ -1043,7 +1046,7 @@ public function fromTmpFile($tmpFile, $path) { /** * @param string $path * @return mixed - * @throws \OCP\Files\InvalidPathException + * @throws InvalidPathException */ public function getMimeType($path) { $this->assertPathLength($path); @@ -1082,7 +1085,7 @@ public function hash($type, $path, $raw = false): string|bool { /** * @param string $path * @return mixed - * @throws \OCP\Files\InvalidPathException + * @throws InvalidPathException */ public function free_space($path = '/') { $this->assertPathLength($path); @@ -1096,9 +1099,6 @@ public function free_space($path = '/') { /** * abstraction layer for basic filesystem functions: wrapper for \OC\Files\Storage\Storage * - * @param string $operation - * @param string $path - * @param array $hooks (optional) * @param mixed $extraParam (optional) * @return mixed * @throws LockedException @@ -1107,7 +1107,7 @@ public function free_space($path = '/') { * files), processes hooks and proxies, sanitises paths, and finally passes them on to * \OC\Files\Storage\Storage for delegation to a storage backend for execution */ - private function basicOperation($operation, $path, $hooks = [], $extraParam = null) { + private function basicOperation(string $operation, string $path, array $hooks = [], $extraParam = null) { $postFix = (substr($path, -1) === '/') ? '/' : ''; $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); if (Filesystem::isValidPath($path) @@ -1124,9 +1124,9 @@ private function basicOperation($operation, $path, $hooks = [], $extraParam = nu } $run = $this->runHooks($hooks, $path); - /** @var \OC\Files\Storage\Storage $storage */ [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); if ($run and $storage) { + /** @var \OC\Files\Storage\Storage $storage */ if (in_array('write', $hooks) || in_array('delete', $hooks)) { try { $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); @@ -1204,14 +1204,15 @@ private function basicOperation($operation, $path, $hooks = [], $extraParam = nu * @param string $path * @return ?string */ - private function getHookPath($path) { - if (!Filesystem::getView()) { + private function getHookPath($path): ?string { + $view = Filesystem::getView(); + if (!$view) { return $path; } - return Filesystem::getView()->getRelativePath($this->getAbsolutePath($path)); + return $view->getRelativePath($this->getAbsolutePath($path)); } - private function shouldEmitHooks($path = '') { + private function shouldEmitHooks(string $path = ''): bool { if ($path && Cache\Scanner::isPartialFile($path)) { return false; } @@ -1335,7 +1336,7 @@ private function getCacheEntry($storage, $internalPath, $relativePath) { * @param string $path * @param bool|string $includeMountPoints true to add mountpoint sizes, * 'ext' to add only ext storage mount point sizes. Defaults to true. - * @return \OC\Files\FileInfo|false False if file does not exist + * @return \OCP\Files\FileInfo|false False if file does not exist */ public function getFileInfo($path, $includeMountPoints = true) { $this->assertPathLength($path); @@ -1745,13 +1746,13 @@ public function getPath($id, int $storageId = null) { * @param string $path * @throws InvalidPathException */ - private function assertPathLength($path) { + private function assertPathLength($path): void { $maxLen = min(PHP_MAXPATHLEN, 4000); // Check for the string length - performed using isset() instead of strlen() // because isset() is about 5x-40x faster. if (isset($path[$maxLen])) { $pathLen = strlen($path); - throw new \OCP\Files\InvalidPathException("Path length($pathLen) exceeds max path length($maxLen): $path"); + throw new InvalidPathException("Path length($pathLen) exceeds max path length($maxLen): $path"); } } @@ -1759,23 +1760,19 @@ private function assertPathLength($path) { * check if it is allowed to move a mount point to a given target. * It is not allowed to move a mount point into a different mount point or * into an already shared folder - * - * @param IStorage $targetStorage - * @param string $targetInternalPath - * @return boolean */ - private function targetIsNotShared(IStorage $targetStorage, string $targetInternalPath) { + private function targetIsNotShared(IStorage $targetStorage, string $targetInternalPath): bool { // note: cannot use the view because the target is already locked - $fileId = (int)$targetStorage->getCache()->getId($targetInternalPath); + $fileId = $targetStorage->getCache()->getId($targetInternalPath); if ($fileId === -1) { // target might not exist, need to check parent instead - $fileId = (int)$targetStorage->getCache()->getId(dirname($targetInternalPath)); + $fileId = $targetStorage->getCache()->getId(dirname($targetInternalPath)); } // check if any of the parents were shared by the current owner (include collections) $shares = Share::getItemShared( 'folder', - $fileId, + (string)$fileId, \OC\Share\Constants::FORMAT_NONE, null, true @@ -1826,7 +1823,7 @@ private function getPartFileInfo($path) { * @param string $fileName * @throws InvalidPathException */ - public function verifyPath($path, $fileName) { + public function verifyPath($path, $fileName): void { try { /** @type \OCP\Files\Storage $storage */ [$storage, $internalPath] = $this->resolvePath($path); @@ -1884,7 +1881,7 @@ private function getParents($path) { * is mounted directly on the given path, false otherwise * @return IMountPoint mount point for which to apply locks */ - private function getMountForLock($absolutePath, $useParentMount = false) { + private function getMountForLock(string $absolutePath, bool $useParentMount = false): IMountPoint { $mount = Filesystem::getMountManager()->find($absolutePath); if ($useParentMount) { @@ -1917,24 +1914,22 @@ private function lockPath($path, $type, $lockMountPoint = false) { } $mount = $this->getMountForLock($absolutePath, $lockMountPoint); - if ($mount) { - try { - $storage = $mount->getStorage(); - if ($storage && $storage->instanceOfStorage('\OCP\Files\Storage\ILockingStorage')) { - $storage->acquireLock( - $mount->getInternalPath($absolutePath), - $type, - $this->lockingProvider - ); - } - } catch (LockedException $e) { - // rethrow with the a human-readable path - throw new LockedException( - $this->getPathRelativeToFiles($absolutePath), - $e, - $e->getExistingLock() + try { + $storage = $mount->getStorage(); + if ($storage && $storage->instanceOfStorage('\OCP\Files\Storage\ILockingStorage')) { + $storage->acquireLock( + $mount->getInternalPath($absolutePath), + $type, + $this->lockingProvider ); } + } catch (LockedException $e) { + // rethrow with the a human-readable path + throw new LockedException( + $this->getPathRelativeToFiles($absolutePath), + $e, + $e->getExistingLock() + ); } return true; @@ -1959,31 +1954,29 @@ public function changeLock($path, $type, $lockMountPoint = false) { } $mount = $this->getMountForLock($absolutePath, $lockMountPoint); - if ($mount) { + try { + $storage = $mount->getStorage(); + if ($storage && $storage->instanceOfStorage('\OCP\Files\Storage\ILockingStorage')) { + $storage->changeLock( + $mount->getInternalPath($absolutePath), + $type, + $this->lockingProvider + ); + } + } catch (LockedException $e) { try { - $storage = $mount->getStorage(); - if ($storage && $storage->instanceOfStorage('\OCP\Files\Storage\ILockingStorage')) { - $storage->changeLock( - $mount->getInternalPath($absolutePath), - $type, - $this->lockingProvider - ); - } - } catch (LockedException $e) { - try { - // rethrow with the a human-readable path - throw new LockedException( - $this->getPathRelativeToFiles($absolutePath), - $e, - $e->getExistingLock() - ); - } catch (\InvalidArgumentException $ex) { - throw new LockedException( - $absolutePath, - $ex, - $e->getExistingLock() - ); - } + // rethrow with the a human-readable path + throw new LockedException( + $this->getPathRelativeToFiles($absolutePath), + $e, + $e->getExistingLock() + ); + } catch (\InvalidArgumentException $ex) { + throw new LockedException( + $absolutePath, + $ex, + $e->getExistingLock() + ); } } @@ -2008,15 +2001,13 @@ private function unlockPath($path, $type, $lockMountPoint = false) { } $mount = $this->getMountForLock($absolutePath, $lockMountPoint); - if ($mount) { - $storage = $mount->getStorage(); - if ($storage && $storage->instanceOfStorage('\OCP\Files\Storage\ILockingStorage')) { - $storage->releaseLock( - $mount->getInternalPath($absolutePath), - $type, - $this->lockingProvider - ); - } + $storage = $mount->getStorage(); + if ($storage && $storage->instanceOfStorage('\OCP\Files\Storage\ILockingStorage')) { + $storage->releaseLock( + $mount->getInternalPath($absolutePath), + $type, + $this->lockingProvider + ); } return true; diff --git a/lib/public/Files/FileInfo.php b/lib/public/Files/FileInfo.php index 0e521cea65c85..83ae4adef9204 100644 --- a/lib/public/Files/FileInfo.php +++ b/lib/public/Files/FileInfo.php @@ -250,7 +250,7 @@ public function getMountPoint(); /** * Get the owner of the file * - * @return \OCP\IUser + * @return ?\OCP\IUser * @since 9.0.0 */ public function getOwner(); diff --git a/lib/public/Files/Storage.php b/lib/public/Files/Storage.php index f5b0775849d37..c165266a1b72a 100644 --- a/lib/public/Files/Storage.php +++ b/lib/public/Files/Storage.php @@ -254,7 +254,7 @@ public function rename($source, $target); /** * see https://www.php.net/manual/en/function.copy.php * - * @param string $soruce + * @param string $source * @param string $target * @return bool * @since 6.0.0 From 5ad045619c0d4010ecf30c1c4fe1e1275e9ab094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 28 Mar 2023 11:13:49 +0200 Subject: [PATCH 6/8] View needs to return an instance of OC\Files\FileInfo explicitely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applications are calling methods from the class which are not from the public interface, and which cannot be added easily to public interface because Node interface extends FileInfo. Signed-off-by: Côme Chilliet --- lib/private/Files/FileInfo.php | 4 +--- lib/private/Files/View.php | 7 ++----- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/private/Files/FileInfo.php b/lib/private/Files/FileInfo.php index 3016080d6f73b..d9b773cc2a660 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -235,10 +235,8 @@ public function isEncrypted() { /** * Return the currently version used for the HMAC in the encryption app - * - * @return int */ - public function getEncryptedVersion() { + public function getEncryptedVersion(): int { return isset($this->data['encryptedVersion']) ? (int) $this->data['encryptedVersion'] : 1; } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 8f324a60ad61f..e864ddef84db3 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1336,7 +1336,7 @@ private function getCacheEntry($storage, $internalPath, $relativePath) { * @param string $path * @param bool|string $includeMountPoints true to add mountpoint sizes, * 'ext' to add only ext storage mount point sizes. Defaults to true. - * @return \OCP\Files\FileInfo|false False if file does not exist + * @return \OC\Files\FileInfo|false False if file does not exist */ public function getFileInfo($path, $includeMountPoints = true) { $this->assertPathLength($path); @@ -1790,11 +1790,8 @@ private function targetIsNotShared(IStorage $targetStorage, string $targetIntern /** * Get a fileinfo object for files that are ignored in the cache (part files) - * - * @param string $path - * @return \OCP\Files\FileInfo */ - private function getPartFileInfo($path) { + private function getPartFileInfo(string $path): \OC\Files\FileInfo { $mount = $this->getMount($path); $storage = $mount->getStorage(); $internalPath = $mount->getInternalPath($this->getAbsolutePath($path)); From 8f26a5d766afb5b95f522e059ac9327a8d8eddcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 30 Mar 2023 14:50:09 +0200 Subject: [PATCH 7/8] Fix dav application tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php index d219888ef155a..8d6bfc1764b35 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -62,8 +62,7 @@ public function testCopy($sourcePath, $targetPath, $targetParent): void { $view = $this->createMock(View::class); $view->expects($this->once()) ->method('verifyPath') - ->with($targetParent) - ->willReturn(true); + ->with($targetParent); $view->expects($this->once()) ->method('file_exists') ->with($targetPath) From 6633b4ced65f97495b1c123a9283c6f187debf41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Wed, 5 Apr 2023 09:13:39 +0200 Subject: [PATCH 8/8] Remove unecessary fully qualified namespaces from phpdoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Files/View.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index e864ddef84db3..91e6cc2d60e0c 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -641,7 +641,7 @@ public function file_put_contents($path, $data) { throw $e; } - /** @var \OC\Files\Storage\Storage $storage */ + /** @var Storage $storage */ [$storage, $internalPath] = $this->resolvePath($path); $target = $storage->fopen($internalPath, 'w'); if ($target) { @@ -1126,7 +1126,7 @@ private function basicOperation(string $operation, string $path, array $hooks = $run = $this->runHooks($hooks, $path); [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); if ($run and $storage) { - /** @var \OC\Files\Storage\Storage $storage */ + /** @var Storage $storage */ if (in_array('write', $hooks) || in_array('delete', $hooks)) { try { $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); @@ -1296,7 +1296,7 @@ private function getUserObjectForOwner(string $ownerId) { * If the file is not in cached it will be scanned * If the file has changed on storage the cache will be updated * - * @param \OC\Files\Storage\Storage $storage + * @param Storage $storage * @param string $internalPath * @param string $relativePath * @return ICacheEntry|bool @@ -1547,7 +1547,7 @@ public function putFileInfo($path, $data) { } $path = Filesystem::normalizePath($this->fakeRoot . '/' . $path); /** - * @var \OC\Files\Storage\Storage $storage + * @var Storage $storage * @var string $internalPath */ [$storage, $internalPath] = Filesystem::resolvePath($path);