From bf71461bee0ef2bc21054585ae46f92c43795640 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 12:01:58 +0100 Subject: [PATCH 01/10] fix(psalm-strict): Supress errors related to internal classes, methods and properties Signed-off-by: provokateurin --- psalm-strict.xml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/psalm-strict.xml b/psalm-strict.xml index 900470bcbab17..01e6e3a579e51 100644 --- a/psalm-strict.xml +++ b/psalm-strict.xml @@ -29,4 +29,21 @@ + + + + + + + + + + + + + + + + + From e40d6b1c6b76e345ec9500cb23cf5475ffbb2773 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 12:05:11 +0100 Subject: [PATCH 02/10] refactor(dav): Modernize Node Signed-off-by: provokateurin --- apps/dav/lib/Connector/Sabre/FilesPlugin.php | 5 - apps/dav/lib/Connector/Sabre/Node.php | 178 +++++++++--------- apps/dav/lib/Files/FilesHome.php | 4 +- .../Sabre/CommentsPropertiesPluginTest.php | 10 +- .../unit/Connector/Sabre/DirectoryTest.php | 23 ++- .../tests/unit/Connector/Sabre/FileTest.php | 16 ++ .../unit/Connector/Sabre/FilesPluginTest.php | 48 +---- .../Connector/Sabre/FilesReportPluginTest.php | 8 +- .../tests/unit/Connector/Sabre/NodeTest.php | 25 +++ .../unit/Connector/Sabre/ObjectTreeTest.php | 12 ++ build/psalm-baseline.xml | 9 - build/rector-strict.php | 1 + psalm-strict.xml | 6 + 13 files changed, 182 insertions(+), 163 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index 7b2f144dfa1d8..83ec30e089301 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -166,11 +166,6 @@ public function checkCopy($source, $target): void { return; } - // Ensure source exists - $sourceNodeFileInfo = $sourceNode->getFileInfo(); - if ($sourceNodeFileInfo === null) { - throw new NotFound($source . ' does not exist'); - } // Ensure the target name is valid try { [$targetPath, $targetName] = \Sabre\Uri\split($target); diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php index 14ac7063ace4e..1feac9839df79 100644 --- a/apps/dav/lib/Connector/Sabre/Node.php +++ b/apps/dav/lib/Connector/Sabre/Node.php @@ -1,5 +1,7 @@ path = $this->fileView->getRelativePath($info->getPath()); - $this->info = $info; - if ($shareManager) { - $this->shareManager = $shareManager; - } else { - $this->shareManager = Server::get(\OCP\Share\IManager::class); + $relativePath = $this->fileView->getRelativePath($info->getPath()); + if ($relativePath === null) { + throw new RuntimeException('Failed to get relative path for ' . $info->getPath()); } + + $this->path = $relativePath; + $this->info = $info; + $this->shareManager = $shareManager instanceof IManager ? $shareManager : Server::get(IManager::class); + if ($info instanceof Folder || $info instanceof File) { $this->node = $info; } else { @@ -70,11 +78,16 @@ public function __construct( } } + /** + * @throws Exception + * @throws PreConditionNotMetException + */ protected function refreshInfo(): void { $info = $this->fileView->getFileInfo($this->path); if ($info === false) { - throw new \Sabre\DAV\Exception('Failed to get fileinfo for ' . $this->path); + throw new Exception('Failed to get fileinfo for ' . $this->path); } + $this->info = $info; $root = Server::get(IRootFolder::class); $rootView = Server::get(View::class); @@ -87,19 +100,15 @@ protected function refreshInfo(): void { /** * Returns the name of the node - * - * @return string */ - public function getName() { + public function getName(): string { return $this->info->getName(); } /** * Returns the full path - * - * @return string */ - public function getPath() { + public function getPath(): string { return $this->path; } @@ -107,25 +116,30 @@ public function getPath() { * Renames the node * * @param string $name The new name - * @throws \Sabre\DAV\Exception\BadRequest - * @throws \Sabre\DAV\Exception\Forbidden + * @throws Exception + * @throws Forbidden + * @throws InvalidPath + * @throws PreConditionNotMetException + * @throws LockedException */ - public function setName($name) { + public function setName($name): void { // rename is only allowed if the delete privilege is granted // (basically rename is a copy with delete of the original node) - if (!($this->info->isDeletable() || ($this->info->getMountPoint() instanceof MoveableMount && $this->info->getInternalPath() === ''))) { - throw new \Sabre\DAV\Exception\Forbidden(); + if (!$this->info->isDeletable() && !($this->info->getMountPoint() instanceof MoveableMount && $this->info->getInternalPath() === '')) { + throw new Forbidden(); } + /** @var string $parentPath */ [$parentPath,] = \Sabre\Uri\split($this->path); + /** @var string $newName */ [, $newName] = \Sabre\Uri\split($name); $newPath = $parentPath . '/' . $newName; // verify path of the target $this->verifyPath($newPath); - if (!$this->fileView->rename($this->path, $newPath)) { - throw new \Sabre\DAV\Exception('Failed to rename ' . $this->path . ' to ' . $newPath); + if ($this->fileView->rename($this->path, $newPath) === false) { + throw new Exception('Failed to rename ' . $this->path . ' to ' . $newPath); } $this->path = $newPath; @@ -138,12 +152,8 @@ public function setName($name) { * * @return int timestamp as integer */ - public function getLastModified() { - $timestamp = $this->info->getMtime(); - if (!empty($timestamp)) { - return (int)$timestamp; - } - return $timestamp; + public function getLastModified(): int { + return $this->info->getMtime(); } /** @@ -151,7 +161,7 @@ public function getLastModified() { * in the second parameter or to now if the second param is empty. * Even if the modification time is set to a custom value the access time is set to now. */ - public function touch($mtime) { + public function touch(string $mtime): void { $mtime = $this->sanitizeMtime($mtime); $this->fileView->touch($this->path, $mtime); $this->refreshInfo(); @@ -165,37 +175,29 @@ public function touch($mtime) { * arbitrary string, but MUST be surrounded by double-quotes. * * Return null if the ETag can not effectively be determined - * - * @return string */ - public function getETag() { + public function getETag(): string { return '"' . $this->info->getEtag() . '"'; } /** * Sets the ETag * - * @param string $etag - * * @return int file id of updated file or -1 on failure */ - public function setETag($etag) { + public function setETag(string $etag): int { return $this->fileView->putFileInfo($this->path, ['etag' => $etag]); } - public function setCreationTime(int $time) { + public function setCreationTime(int $time): int { return $this->fileView->putFileInfo($this->path, ['creation_time' => $time]); } - public function setUploadTime(int $time) { - return $this->fileView->putFileInfo($this->path, ['upload_time' => $time]); - } - /** * Returns the size of the node, in bytes * + * @psalm-suppress UnusedPsalmSuppress psalm:strict actually thinks there is no mismatch, idk lol * @psalm-suppress ImplementedReturnTypeMismatch \Sabre\DAV\IFile::getSize signature does not support 32bit - * @return int|float */ public function getSize(): int|float { return $this->info->getSize(); @@ -203,28 +205,21 @@ public function getSize(): int|float { /** * Returns the cache's file id - * - * @return int */ - public function getId() { + public function getId(): ?int { return $this->info->getId(); } - /** - * @return string|null - */ - public function getFileId() { - if ($id = $this->info->getId()) { + public function getFileId(): ?string { + $id = $this->info->getId(); + if ($id !== null) { return DavUtil::getDavFileId($id); } return null; } - /** - * @return integer - */ - public function getInternalFileId() { + public function getInternalFileId(): ?int { return $this->info->getId(); } @@ -232,30 +227,24 @@ public function getInternalPath(): string { return $this->info->getInternalPath(); } - /** - * @param string $user - * @return int - */ - public function getSharePermissions($user) { + public function getSharePermissions(?string $user): int { // check of we access a federated share if ($user !== null) { try { - $share = $this->shareManager->getShareByToken($user); - return $share->getPermissions(); - } catch (ShareNotFound $e) { + return $this->shareManager->getShareByToken($user)->getPermissions(); + } catch (ShareNotFound) { // ignore } } try { $storage = $this->info->getStorage(); - } catch (StorageNotAvailableException $e) { + } catch (StorageNotAvailableException) { $storage = null; } if ($storage && $storage->instanceOfStorage(ISharedStorage::class)) { - /** @var ISharedStorage $storage */ - $permissions = (int)$storage->getShare()->getPermissions(); + $permissions = $storage->getShare()->getPermissions(); } else { $permissions = $this->info->getPermissions(); } @@ -266,6 +255,10 @@ public function getSharePermissions($user) { */ $mountpoint = $this->info->getMountPoint(); if (!($mountpoint instanceof MoveableMount)) { + /** + * @psalm-suppress UnnecessaryVarAnnotation Rector doesn't trust the return type annotation + * @var string $mountpointpath + */ $mountpointpath = $mountpoint->getMountPoint(); if (str_ends_with($mountpointpath, '/')) { $mountpointpath = substr($mountpointpath, 0, -1); @@ -286,25 +279,21 @@ public function getSharePermissions($user) { return $permissions; } - /** - * @return array - */ public function getShareAttributes(): array { try { $storage = $this->node->getStorage(); - } catch (NotFoundException $e) { + } catch (NotFoundException) { return []; } $attributes = []; if ($storage->instanceOfStorage(ISharedStorage::class)) { - /** @var ISharedStorage $storage */ $attributes = $storage->getShare()->getAttributes(); if ($attributes === null) { return []; - } else { - return $attributes->toArray(); } + + return $attributes->toArray(); } return $attributes; @@ -318,63 +307,66 @@ public function getNoteFromShare(?string $user): ?string { } if ($storage->instanceOfStorage(ISharedStorage::class)) { - /** @var ISharedStorage $storage */ $share = $storage->getShare(); if ($user === $share->getShareOwner()) { // Note is only for recipient not the owner return null; } + return $share->getNote(); } return null; } - /** - * @return string - */ - public function getDavPermissions() { + public function getDavPermissions(): string { return DavUtil::getDavPermissions($this->info); } - public function getOwner() { + public function getOwner(): ?IUser { return $this->info->getOwner(); } + /** + * @throws InvalidPath + */ protected function verifyPath(?string $path = null): void { try { - $path = $path ?? $this->info->getPath(); + $path ??= $this->info->getPath(); $this->fileView->verifyPath( dirname($path), basename($path), ); - } catch (InvalidPathException $ex) { - throw new InvalidPath($ex->getMessage()); + } catch (InvalidPathException $invalidPathException) { + throw new InvalidPath($invalidPathException->getMessage(), false, $invalidPathException); } } /** - * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param ILockingProvider::LOCK_* $type + * @throws LockedException */ - public function acquireLock($type) { + public function acquireLock($type): void { $this->fileView->lockFile($this->path, $type); } /** - * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param ILockingProvider::LOCK_* $type + * @throws LockedException */ - public function releaseLock($type) { + public function releaseLock($type): void { $this->fileView->unlockFile($this->path, $type); } /** - * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param ILockingProvider::LOCK_* $type + * @throws LockedException */ - public function changeLock($type) { + public function changeLock($type): void { $this->fileView->changeLock($this->path, $type); } - public function getFileInfo() { + public function getFileInfo(): FileInfo { return $this->info; } diff --git a/apps/dav/lib/Files/FilesHome.php b/apps/dav/lib/Files/FilesHome.php index f8aa82cdcc9a4..ab3a9b68c13db 100644 --- a/apps/dav/lib/Files/FilesHome.php +++ b/apps/dav/lib/Files/FilesHome.php @@ -32,12 +32,12 @@ public function delete() { throw new Forbidden('Permission denied to delete home folder'); } - public function getName() { + public function getName(): string { [,$name] = \Sabre\Uri\split($this->principalInfo['uri']); return $name; } - public function setName($name) { + public function setName($name): void { throw new Forbidden('Permission denied to rename this folder'); } } diff --git a/apps/dav/tests/unit/Connector/Sabre/CommentsPropertiesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/CommentsPropertiesPluginTest.php index f89e8e065425c..e3f296353e353 100644 --- a/apps/dav/tests/unit/Connector/Sabre/CommentsPropertiesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/CommentsPropertiesPluginTest.php @@ -61,14 +61,14 @@ public function testHandleGetProperties(string $class, bool $expectedSuccessful) public static function baseUriProvider(): array { return [ - ['owncloud/remote.php/webdav/', '4567', 'owncloud/remote.php/dav/comments/files/4567'], - ['owncloud/remote.php/files/', '4567', 'owncloud/remote.php/dav/comments/files/4567'], - ['owncloud/wicked.php/files/', '4567', null] + ['owncloud/remote.php/webdav/', 4567, 'owncloud/remote.php/dav/comments/files/4567'], + ['owncloud/remote.php/files/', 4567, 'owncloud/remote.php/dav/comments/files/4567'], + ['owncloud/wicked.php/files/', 4567, null] ]; } #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'baseUriProvider')] - public function testGetCommentsLink(string $baseUri, string $fid, ?string $expectedHref): void { + public function testGetCommentsLink(string $baseUri, int $fid, ?string $expectedHref): void { $node = $this->createMock(File::class); $node->expects($this->any()) ->method('getId') @@ -94,7 +94,7 @@ public function testGetUnreadCount(?string $user): void { $node = $this->createMock(File::class); $node->expects($this->any()) ->method('getId') - ->willReturn('4567'); + ->willReturn(4567); if ($user !== null) { $user = $this->createMock($user); diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index 4144c79821ef5..4fe6dd88ad909 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -230,6 +230,10 @@ public function testGetChildrenNoPermission(): void { $info->expects($this->any()) ->method('isReadable') ->willReturn(false); + $this->view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $dir = new Directory($this->view, $info); $dir->getChildren(); @@ -242,6 +246,10 @@ public function testGetChildNoPermission(): void { $this->info->expects($this->any()) ->method('isReadable') ->willReturn(false); + $this->view + ->method('getRelativePath') + ->with('/admin/files/folder') + ->willReturn(''); $dir = new Directory($this->view, $this->info); $dir->getChild('test'); @@ -254,6 +262,10 @@ public function testGetChildThrowStorageNotAvailableException(): void { $this->view->expects($this->once()) ->method('getFileInfo') ->willThrowException(new StorageNotAvailableException()); + $this->view + ->method('getRelativePath') + ->with('/admin/files/folder') + ->willReturn(''); $dir = new Directory($this->view, $this->info); $dir->getChild('.'); @@ -268,6 +280,10 @@ public function testGetChildThrowInvalidPath(): void { ->willThrowException(new InvalidPathException()); $this->view->expects($this->never()) ->method('getFileInfo'); + $this->view + ->method('getRelativePath') + ->with('/admin/files/folder') + ->willReturn(''); $dir = new Directory($this->view, $this->info); $dir->getChild('.'); @@ -376,6 +392,11 @@ public function testGetNodeForPathFailsWithNoReadPermissionsForParent(): void { } public function testGetNodeForPathFailsWithNoReadPermissionsForPath(): void { + $this->view + ->method('getRelativePath') + ->with('/admin/files/') + ->willReturn(''); + $directoryNode = $this->createMock(Folder::class); $pathNode = $this->createMock(Folder::class); $storage = $this->createMock(IStorage::class); @@ -396,7 +417,7 @@ public function testGetNodeForPathFailsWithNoReadPermissionsForPath(): void { 2 => false, }; }); - $directoryNode->expects($this->once()) + $directoryNode ->method('getPath') ->willReturn('/admin/files/'); $directoryNode->expects($this->once()) diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 8c5b04486ddb4..a5ea180e873e4 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -673,6 +673,10 @@ public function testDeleteWhenAllowed(): void { /** @var View&MockObject */ $view = $this->getMockBuilder(View::class) ->getMock(); + $view + ->method('getRelativePath') + ->with('/test.txt') + ->willReturn(''); $view->expects($this->once()) ->method('unlink') @@ -697,6 +701,10 @@ public function testDeleteThrowsWhenDeletionNotAllowed(): void { /** @var View&MockObject */ $view = $this->getMockBuilder(View::class) ->getMock(); + $view + ->method('getRelativePath') + ->with('/test.txt') + ->willReturn(''); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ 'permissions' => 0, @@ -717,6 +725,10 @@ public function testDeleteThrowsWhenDeletionFailed(): void { /** @var View&MockObject */ $view = $this->getMockBuilder(View::class) ->getMock(); + $view + ->method('getRelativePath') + ->with('/test.txt') + ->willReturn(''); // but fails $view->expects($this->once()) @@ -742,6 +754,10 @@ public function testDeleteThrowsWhenDeletionThrows(): void { /** @var View&MockObject */ $view = $this->getMockBuilder(View::class) ->getMock(); + $view + ->method('getRelativePath') + ->with('/test.txt') + ->willReturn(''); // but fails $view->expects($this->once()) diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php index 6e2d555762653..a5a8358d61faa 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php @@ -101,7 +101,7 @@ private function createTestNode(string $class, string $path = '/dummypath'): INo ->willReturn('00000123instanceid'); $node->expects($this->any()) ->method('getInternalFileId') - ->willReturn('123'); + ->willReturn(123); $node->expects($this->any()) ->method('getEtag') ->willReturn('"abc"'); @@ -455,7 +455,7 @@ public function testUpdateProps(): void { $node->expects($this->once()) ->method('setEtag') ->with('newetag') - ->willReturn(true); + ->willReturn(123); $node->expects($this->once()) ->method('setCreationTime') @@ -562,35 +562,11 @@ public function testMoveSrcDeletable(): void { $this->plugin->checkMove('FolderA/test.txt', 'test.txt'); } - public function testMoveSrcNotExist(): void { - $this->expectException(\Sabre\DAV\Exception\NotFound::class); - $this->expectExceptionMessage('FolderA/test.txt does not exist'); - - $node = $this->createMock(Node::class); - $node->expects($this->atLeastOnce()) - ->method('getFileInfo') - ->willReturn(null); - - $this->tree->expects($this->atLeastOnce()) - ->method('getNodeForPath') - ->willReturn($node); - - $this->plugin->checkMove('FolderA/test.txt', 'test.txt'); - } - public function testMoveDestinationInvalid(): void { $this->expectException(InvalidPath::class); $this->expectExceptionMessage('Mocked exception'); - $fileInfoFolderATestTXT = $this->createMock(FileInfo::class); - $fileInfoFolderATestTXT->expects(self::any()) - ->method('isDeletable') - ->willReturn(true); - $node = $this->createMock(Node::class); - $node->expects($this->atLeastOnce()) - ->method('getFileInfo') - ->willReturn($fileInfoFolderATestTXT); $this->tree->expects($this->atLeastOnce()) ->method('getNodeForPath') @@ -604,31 +580,11 @@ public function testMoveDestinationInvalid(): void { $this->plugin->checkMove('FolderA/test.txt', 'invalid\\path.txt'); } - public function testCopySrcNotExist(): void { - $this->expectException(\Sabre\DAV\Exception\NotFound::class); - $this->expectExceptionMessage('FolderA/test.txt does not exist'); - - $node = $this->createMock(Node::class); - $node->expects($this->atLeastOnce()) - ->method('getFileInfo') - ->willReturn(null); - - $this->tree->expects($this->atLeastOnce()) - ->method('getNodeForPath') - ->willReturn($node); - - $this->plugin->checkCopy('FolderA/test.txt', 'test.txt'); - } - public function testCopyDestinationInvalid(): void { $this->expectException(InvalidPath::class); $this->expectExceptionMessage('Mocked exception'); - $fileInfoFolderATestTXT = $this->createMock(FileInfo::class); $node = $this->createMock(Node::class); - $node->expects($this->atLeastOnce()) - ->method('getFileInfo') - ->willReturn($fileInfoFolderATestTXT); $this->tree->expects($this->atLeastOnce()) ->method('getNodeForPath') diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index 9c408c55e28e5..717cecda7588d 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -57,6 +57,10 @@ protected function setUp(): void { $this->tree = $this->createMock(Tree::class); $this->view = $this->createMock(View::class); + $this->view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $this->server = $this->getMockBuilder(Server::class) ->setConstructorArgs([$this->tree]) @@ -315,14 +319,14 @@ public function testPrepareResponses(): void { $node1->expects($this->once()) ->method('getInternalFileId') - ->willReturn('111'); + ->willReturn(111); $node1->expects($this->any()) ->method('getPath') ->willReturn('/node1'); $node1->method('getFileInfo')->willReturn($fileInfo); $node2->expects($this->once()) ->method('getInternalFileId') - ->willReturn('222'); + ->willReturn(222); $node2->expects($this->once()) ->method('getSize') ->willReturn(1024); diff --git a/apps/dav/tests/unit/Connector/Sabre/NodeTest.php b/apps/dav/tests/unit/Connector/Sabre/NodeTest.php index 896f36f12536d..80e30d29d94e9 100644 --- a/apps/dav/tests/unit/Connector/Sabre/NodeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/NodeTest.php @@ -94,6 +94,10 @@ public function testDavPermissions(int $permissions, string $type, bool $shared, $info->method('getStorage') ->willReturn($storage); $view = $this->createMock(View::class); + $view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $node = new File($view, $info); $this->assertEquals($expected, $node->getDavPermissions()); @@ -169,6 +173,10 @@ public function testSharePermissions(string $type, ?string $user, int $permissio $info->method('getPermissions')->willReturn($permissions); $view = $this->createMock(View::class); + $view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $node = new File($view, $info); $this->invokePrivate($node, 'shareManager', [$shareManager]); @@ -204,6 +212,10 @@ public function testShareAttributes(): void { /** @var View&MockObject $view */ $view = $this->createMock(View::class); + $view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $node = new File($view, $info); $this->invokePrivate($node, 'shareManager', [$shareManager]); @@ -225,6 +237,10 @@ public function testShareAttributesNonShare(): void { /** @var View&MockObject */ $view = $this->createMock(View::class); + $view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $node = new File($view, $info); $this->invokePrivate($node, 'shareManager', [$shareManager]); @@ -243,6 +259,10 @@ public function testSanitizeMtime(string|int $mtime, int $expected): void { $view = $this->getMockBuilder(View::class) ->disableOriginalConstructor() ->getMock(); + $view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $info = $this->getMockBuilder(FileInfo::class) ->disableOriginalConstructor() ->getMock(); @@ -263,6 +283,11 @@ public function testInvalidSanitizeMtime(int|string $mtime): void { $this->expectException(\InvalidArgumentException::class); $view = $this->createMock(View::class); + $view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); + $info = $this->createMock(FileInfo::class); $node = new File($view, $info); diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php index fbbda7deffbd5..d46232e34adc4 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -63,6 +63,10 @@ public function testCopy(string $sourcePath, string $targetPath, string $targetP ->method('getFileInfo') ->with($targetParent === '' ? '.' : $targetParent) ->willReturn($info); + $view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $rootDir = new Directory($view, $info); $objectTree = $this->getMockBuilder(ObjectTree::class) @@ -104,6 +108,10 @@ public function testCopyFailNotCreatable($sourcePath, $targetPath, $targetParent ->method('getFileInfo') ->with($targetParent === '' ? '.' : $targetParent) ->willReturn($info); + $view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $rootDir = new Directory($view, $info); $objectTree = $this->getMockBuilder(ObjectTree::class) @@ -141,6 +149,10 @@ public function testGetNodeForPath( $view->method('getFileInfo') ->with($fileInfoQueryPath) ->willReturn($fileInfo); + $view + ->method('getRelativePath') + ->with(null) + ->willReturn(''); $tree = new ObjectTree(); $tree->init($rootNode, $view, $mountManager); diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 1163db1d93015..4faa60b4a1b3b 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -616,20 +616,11 @@ - - - - - - - info->getId()]]> - info->getId()]]> - diff --git a/build/rector-strict.php b/build/rector-strict.php index 529eace3b6743..10d3f34a5aaba 100644 --- a/build/rector-strict.php +++ b/build/rector-strict.php @@ -12,6 +12,7 @@ $nextcloudDir . '/build/rector-strict.php', $nextcloudDir . '/core/BackgroundJobs/ExpirePreviewsJob.php', $nextcloudDir . '/lib/public/IContainer.php', + $nextcloudDir . '/apps/dav/lib/Connector/Sabre/Node.php', ]) ->withPreparedSets( deadCode: true, diff --git a/psalm-strict.xml b/psalm-strict.xml index 01e6e3a579e51..5e57d683b09eb 100644 --- a/psalm-strict.xml +++ b/psalm-strict.xml @@ -18,6 +18,7 @@ + @@ -27,8 +28,13 @@ + + + + + From 3693cbeaf9700e67e780cd8a46703633f6c07f16 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 12:09:19 +0100 Subject: [PATCH 03/10] fix(IMetadataVersion): Use correct return type for getMetadata Signed-off-by: provokateurin --- apps/files_versions/lib/Versions/IMetadataVersion.php | 2 +- build/rector-strict.php | 1 + psalm-strict.xml | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/files_versions/lib/Versions/IMetadataVersion.php b/apps/files_versions/lib/Versions/IMetadataVersion.php index bc4cd77138b49..30b421d4174d1 100644 --- a/apps/files_versions/lib/Versions/IMetadataVersion.php +++ b/apps/files_versions/lib/Versions/IMetadataVersion.php @@ -16,7 +16,7 @@ interface IMetadataVersion { /** * retrieves the all the metadata * - * @return string[] + * @return array * @since 29.0.0 */ public function getMetadata(): array; diff --git a/build/rector-strict.php b/build/rector-strict.php index 10d3f34a5aaba..fa26a99a6cb93 100644 --- a/build/rector-strict.php +++ b/build/rector-strict.php @@ -13,6 +13,7 @@ $nextcloudDir . '/core/BackgroundJobs/ExpirePreviewsJob.php', $nextcloudDir . '/lib/public/IContainer.php', $nextcloudDir . '/apps/dav/lib/Connector/Sabre/Node.php', + $nextcloudDir . '/apps/files_versions/lib/Versions/IMetadataVersion.php', ]) ->withPreparedSets( deadCode: true, diff --git a/psalm-strict.xml b/psalm-strict.xml index 5e57d683b09eb..9a39616578620 100644 --- a/psalm-strict.xml +++ b/psalm-strict.xml @@ -19,6 +19,7 @@ + From e454cc676568314a004dc667ad9b4faa6ba6dd21 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 12:24:33 +0100 Subject: [PATCH 04/10] refactor(settings): Modernize authorized group classes Signed-off-by: provokateurin --- .../lib/Service/AuthorizedGroupService.php | 53 +++++++++++-------- build/rector-strict.php | 3 ++ lib/private/Settings/AuthorizedGroup.php | 18 ++++--- .../Settings/AuthorizedGroupMapper.php | 42 +++++++-------- psalm-strict.xml | 4 ++ 5 files changed, 68 insertions(+), 52 deletions(-) diff --git a/apps/settings/lib/Service/AuthorizedGroupService.php b/apps/settings/lib/Service/AuthorizedGroupService.php index 3189c504de24b..223bf8ee19e62 100644 --- a/apps/settings/lib/Service/AuthorizedGroupService.php +++ b/apps/settings/lib/Service/AuthorizedGroupService.php @@ -1,5 +1,7 @@ mapper->findAll(); @@ -30,43 +33,40 @@ public function findAll(): array { /** * Find AuthorizedGroup by id. - * - * @param int $id + * @throws DoesNotExistException + * @throws Exception + * @throws MultipleObjectsReturnedException */ public function find(int $id): ?AuthorizedGroup { return $this->mapper->find($id); } /** - * @param $e * @throws NotFoundException + * @throws Throwable */ - private function handleException(\Exception $e): void { + private function handleException(Throwable $e): void { if ($e instanceof DoesNotExistException || $e instanceof MultipleObjectsReturnedException) { throw new NotFoundException('AuthorizedGroup not found'); - } else { - throw $e; } + + throw $e; } /** * Create a new AuthorizedGroup * - * @param string $groupId - * @param string $class - * @return AuthorizedGroup * @throws Exception * @throws ConflictException + * @throws MultipleObjectsReturnedException */ public function create(string $groupId, string $class): AuthorizedGroup { // Check if the group is already assigned to this class try { - $existing = $this->mapper->findByGroupIdAndClass($groupId, $class); - if ($existing) { - throw new ConflictException('Group is already assigned to this class'); - } - } catch (DoesNotExistException $e) { + $this->mapper->findByGroupIdAndClass($groupId, $class); + throw new ConflictException('Group is already assigned to this class'); + } catch (DoesNotExistException) { // This is expected when no duplicate exists, continue with creation } @@ -78,30 +78,37 @@ public function create(string $groupId, string $class): AuthorizedGroup { /** * @throws NotFoundException + * @throws Throwable */ public function delete(int $id): void { try { $authorizedGroup = $this->mapper->find($id); $this->mapper->delete($authorizedGroup); - } catch (\Exception $e) { - $this->handleException($e); + } catch (\Exception $exception) { + $this->handleException($exception); } } + /** + * @return list + */ public function findExistingGroupsForClass(string $class): array { try { - $authorizedGroup = $this->mapper->findExistingGroupsForClass($class); - return $authorizedGroup; - } catch (\Exception $e) { + return $this->mapper->findExistingGroupsForClass($class); + } catch (\Exception) { return []; } } + /** + * @throws Throwable + * @throws NotFoundException + */ public function removeAuthorizationAssociatedTo(IGroup $group): void { try { $this->mapper->removeGroup($group->getGID()); - } catch (\Exception $e) { - $this->handleException($e); + } catch (\Exception $exception) { + $this->handleException($exception); } } } diff --git a/build/rector-strict.php b/build/rector-strict.php index fa26a99a6cb93..6490cf73d41a4 100644 --- a/build/rector-strict.php +++ b/build/rector-strict.php @@ -14,6 +14,9 @@ $nextcloudDir . '/lib/public/IContainer.php', $nextcloudDir . '/apps/dav/lib/Connector/Sabre/Node.php', $nextcloudDir . '/apps/files_versions/lib/Versions/IMetadataVersion.php', + $nextcloudDir . '/lib/private/Settings/AuthorizedGroup.php', + $nextcloudDir . '/lib/private/Settings/AuthorizedGroupMapper.php', + $nextcloudDir . '/apps/settings/lib/Service/AuthorizedGroupService.php', ]) ->withPreparedSets( deadCode: true, diff --git a/lib/private/Settings/AuthorizedGroup.php b/lib/private/Settings/AuthorizedGroup.php index 6949c0649aa7f..4ad9b228b5c0e 100644 --- a/lib/private/Settings/AuthorizedGroup.php +++ b/lib/private/Settings/AuthorizedGroup.php @@ -8,21 +8,25 @@ */ namespace OC\Settings; +use JsonSerializable; use OCP\AppFramework\Db\Entity; /** * @method setGroupId(string $groupId) * @method setClass(string $class) - * @method getGroupId(): string - * @method getClass(): string + * @method string getGroupId() + * @method string getClass() */ -class AuthorizedGroup extends Entity implements \JsonSerializable { - /** @var string $group_id */ - protected $groupId; +class AuthorizedGroup extends Entity implements JsonSerializable { + public $id; - /** @var string $class */ - protected $class; + protected ?string $groupId = null; + protected ?string $class = null; + + /** + * @return array + */ public function jsonSerialize(): array { return [ 'id' => $this->getId(), diff --git a/lib/private/Settings/AuthorizedGroupMapper.php b/lib/private/Settings/AuthorizedGroupMapper.php index f59d3fd7fd9cd..800dc5b80770c 100644 --- a/lib/private/Settings/AuthorizedGroupMapper.php +++ b/lib/private/Settings/AuthorizedGroupMapper.php @@ -1,5 +1,7 @@ db->getQueryBuilder(); - /** @var IGroupManager $groupManager */ $groupManager = Server::get(IGroupManager::class); $groups = $groupManager->getUserGroups($user); if (count($groups) === 0) { return []; } - $result = $qb->select('class') + /** @var list $rows */ + $rows = $qb->select('class') ->from($this->getTableName(), 'auth') - ->where($qb->expr()->in('group_id', array_map(function (IGroup $group) use ($qb) { - return $qb->createNamedParameter($group->getGID()); - }, $groups), IQueryBuilder::PARAM_STR)) - ->executeQuery(); + ->where($qb->expr()->in('group_id', array_map(static fn (IGroup $group) => $qb->createNamedParameter($group->getGID()), $groups), IQueryBuilder::PARAM_STR)) + ->executeQuery() + ->fetchFirstColumn(); - $classes = []; - while ($row = $result->fetch()) { - $classes[] = $row['class']; - } - $result->closeCursor(); - return $classes; + return $rows; } /** * @throws DoesNotExistException * @throws MultipleObjectsReturnedException - * @throws \OCP\DB\Exception + * @throws Exception */ public function find(int $id): AuthorizedGroup { $queryBuilder = $this->db->getQueryBuilder(); $queryBuilder->select('*') ->from($this->getTableName()) ->where($queryBuilder->expr()->eq('id', $queryBuilder->createNamedParameter($id))); - /** @var AuthorizedGroup $authorizedGroup */ - $authorizedGroup = $this->findEntity($queryBuilder); - return $authorizedGroup; + return $this->findEntity($queryBuilder); } /** * Get all the authorizations stored in the database. * * @return AuthorizedGroup[] - * @throws \OCP\DB\Exception + * @throws Exception */ public function findAll(): array { $qb = $this->db->getQueryBuilder(); @@ -81,7 +74,12 @@ public function findAll(): array { return $this->findEntities($qb); } - public function findByGroupIdAndClass(string $groupId, string $class) { + /** + * @throws DoesNotExistException + * @throws Exception + * @throws MultipleObjectsReturnedException + */ + public function findByGroupIdAndClass(string $groupId, string $class): AuthorizedGroup { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) @@ -91,8 +89,8 @@ public function findByGroupIdAndClass(string $groupId, string $class) { } /** - * @return Entity[] - * @throws \OCP\DB\Exception + * @return list + * @throws Exception */ public function findExistingGroupsForClass(string $class): array { $qb = $this->db->getQueryBuilder(); @@ -105,7 +103,7 @@ public function findExistingGroupsForClass(string $class): array { /** * @throws Exception */ - public function removeGroup(string $gid) { + public function removeGroup(string $gid): void { $qb = $this->db->getQueryBuilder(); $qb->delete($this->getTableName()) ->where($qb->expr()->eq('group_id', $qb->createNamedParameter($gid))) diff --git a/psalm-strict.xml b/psalm-strict.xml index 9a39616578620..13e1aae88d5b7 100644 --- a/psalm-strict.xml +++ b/psalm-strict.xml @@ -20,6 +20,9 @@ + + + @@ -30,6 +33,7 @@ + From 2d486c5629fc0fcaa85cbd11a969c1f42f427efe Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 12:28:23 +0100 Subject: [PATCH 05/10] fix(IGroupManager): Use correct return type for displayNamesInGroup Signed-off-by: provokateurin --- lib/private/Group/Manager.php | 9 --------- lib/public/IGroupManager.php | 2 +- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index e3097e3d7cac0..a060576742866 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -391,15 +391,6 @@ public function getUserGroupNames(IUser $user) { }, $this->getUserGroups($user)); } - /** - * get a list of all display names in a group - * - * @param string $gid - * @param string $search - * @param int $limit - * @param int $offset - * @return array an array of display names (value) and user ids (key) - */ public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0) { $group = $this->get($gid); if (is_null($group)) { diff --git a/lib/public/IGroupManager.php b/lib/public/IGroupManager.php index 7e0575ad4be9d..93013e72891b0 100644 --- a/lib/public/IGroupManager.php +++ b/lib/public/IGroupManager.php @@ -101,7 +101,7 @@ public function getUserGroupIds(IUser $user): array; * @param string $search * @param int $limit * @param int $offset - * @return array an array of display names (value) and user ids (key) + * @return array ['user id' => 'display name'] * @since 8.0.0 */ public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0); From 7ceddb3ffb2837f670e4becb9b6f464cb044f04e Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 15:11:46 +0100 Subject: [PATCH 06/10] fix(IStorage): Use correct return type for stat Signed-off-by: provokateurin --- lib/public/Files/Storage/IStorage.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/public/Files/Storage/IStorage.php b/lib/public/Files/Storage/IStorage.php index f09f899d4f278..ab88404b70c94 100644 --- a/lib/public/Files/Storage/IStorage.php +++ b/lib/public/Files/Storage/IStorage.php @@ -81,7 +81,7 @@ public function is_file(string $path); * see https://www.php.net/manual/en/function.stat.php * only the following keys are required in the result: size and mtime * - * @return array|false + * @return array|false * @since 9.0.0 */ public function stat(string $path); From c953ad9bcdaf0da3fadceb29bdd1232329fe8091 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 15:14:33 +0100 Subject: [PATCH 07/10] fix(Storage): Use correct return types for getMetaData and getDirectoryContent Signed-off-by: provokateurin --- build/rector-strict.php | 1 + lib/private/Files/Storage/Storage.php | 5 +++++ psalm-strict.xml | 1 + 3 files changed, 7 insertions(+) diff --git a/build/rector-strict.php b/build/rector-strict.php index 6490cf73d41a4..232630ab5219c 100644 --- a/build/rector-strict.php +++ b/build/rector-strict.php @@ -17,6 +17,7 @@ $nextcloudDir . '/lib/private/Settings/AuthorizedGroup.php', $nextcloudDir . '/lib/private/Settings/AuthorizedGroupMapper.php', $nextcloudDir . '/apps/settings/lib/Service/AuthorizedGroupService.php', + $nextcloudDir . '/lib/private/Files/Storage/Storage.php', ]) ->withPreparedSets( deadCode: true, diff --git a/lib/private/Files/Storage/Storage.php b/lib/private/Files/Storage/Storage.php index e2beb5a8ac981..938d676471b5d 100644 --- a/lib/private/Files/Storage/Storage.php +++ b/lib/private/Files/Storage/Storage.php @@ -35,6 +35,9 @@ public function getUpdater(?IStorage $storage = null): IUpdater; public function getStorageCache(): \OC\Files\Cache\Storage; + /** + * @return ?array + */ public function getMetaData(string $path): ?array; /** @@ -49,6 +52,8 @@ public function getMetaData(string $path): ?array; * - etag * - storage_mtime * - permissions + * + * @return \Traversable> */ public function getDirectoryContent(string $directory): \Traversable; } diff --git a/psalm-strict.xml b/psalm-strict.xml index 13e1aae88d5b7..32b6550d799df 100644 --- a/psalm-strict.xml +++ b/psalm-strict.xml @@ -23,6 +23,7 @@ + From 5f4a5d8c6796c3915dcd8b20668bb2f6e0b64e79 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 16:44:03 +0100 Subject: [PATCH 08/10] refactor(Files): Remove deprecated streamCopy method Signed-off-by: provokateurin --- apps/dav/lib/Connector/Sabre/File.php | 8 +++- apps/files_versions/lib/Storage.php | 5 ++- build/psalm-baseline.xml | 8 ---- lib/private/Files/Storage/Common.php | 9 ++-- .../Files/Storage/LocalTempFileTrait.php | 7 ++- .../Files/Storage/Wrapper/Encryption.php | 15 +++++-- lib/private/Files/Storage/Wrapper/Jail.php | 8 +++- lib/private/Files/Storage/Wrapper/Wrapper.php | 8 +++- lib/private/Files/View.php | 5 ++- lib/public/Files.php | 43 ------------------- tests/lib/Archive/TestBase.php | 3 +- tests/lib/Files/Storage/Wrapper/QuotaTest.php | 8 ++-- tests/lib/FilesTest.php | 32 -------------- 13 files changed, 55 insertions(+), 104 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 64e8faa5c368b..109440eea4c7b 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -236,7 +236,13 @@ public function put($data) { // because we have no clue about the cause we can only throw back a 500/Internal Server Error throw new Exception($this->l10n->t('Could not write file contents')); } - [$count, $result] = Files::streamCopy($data, $target, true); + $count = stream_copy_to_stream($data, $target); + if ($count === false) { + $result = false; + $count = 0; + } else { + $result = true; + } fclose($target); } if ($result === false && $expected !== null) { diff --git a/apps/files_versions/lib/Storage.php b/apps/files_versions/lib/Storage.php index ca3a130d9b394..fe9f91534fa01 100644 --- a/apps/files_versions/lib/Storage.php +++ b/apps/files_versions/lib/Storage.php @@ -433,7 +433,10 @@ private static function copyFileContents($view, $path1, $path2) { $target = $storage2->fopen($internalPath2, 'w'); $result = $target !== false; if ($result) { - [, $result] = Files::streamCopy($source, $target, true); + $result = stream_copy_to_stream($source, $target); + if ($result !== false) { + $result = true; + } } // explicit check as S3 library closes streams already if (is_resource($target)) { diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 4faa60b4a1b3b..634b83f8731e8 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -557,12 +557,6 @@ - - - - - - @@ -1982,14 +1976,12 @@ - - diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index e19f8812e9ae4..d58042b042c39 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -212,7 +212,10 @@ public function copy(string $source, string $target): bool { } else { $sourceStream = $this->fopen($source, 'r'); $targetStream = $this->fopen($target, 'w'); - [, $result] = Files::streamCopy($sourceStream, $targetStream, true); + $result = stream_copy_to_stream($sourceStream, $targetStream); + if ($result !== false) { + $result = true; + } if (!$result) { Server::get(LoggerInterface::class)->warning("Failed to write data while copying $source to $target"); } @@ -743,8 +746,8 @@ public function writeStream(string $path, $stream, ?int $size = null): int { throw new GenericFileException("Failed to open $path for writing"); } try { - [$count, $result] = Files::streamCopy($stream, $target, true); - if (!$result) { + $count = stream_copy_to_stream($stream, $target); + if ($count === false) { throw new GenericFileException('Failed to copy stream'); } } finally { diff --git a/lib/private/Files/Storage/LocalTempFileTrait.php b/lib/private/Files/Storage/LocalTempFileTrait.php index 5824189e3d8fc..805a67084ccaa 100644 --- a/lib/private/Files/Storage/LocalTempFileTrait.php +++ b/lib/private/Files/Storage/LocalTempFileTrait.php @@ -7,7 +7,6 @@ */ namespace OC\Files\Storage; -use OCP\Files; use OCP\ITempManager; use OCP\Server; @@ -49,8 +48,12 @@ protected function toTmpFile(string $path): string|false { //no longer in the st } $tmpFile = Server::get(ITempManager::class)->getTemporaryFile($extension); $target = fopen($tmpFile, 'w'); - Files::streamCopy($source, $target); + $result = stream_copy_to_stream($source, $target); fclose($target); + if ($result === false) { + return false; + } + return $tmpFile; } } diff --git a/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index 9673ba9575a81..0af619cc23261 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -693,7 +693,10 @@ private function copyBetweenStorage( if ($source === false || $target === false) { $result = false; } else { - [, $result] = Files::streamCopy($source, $target, true); + $result = stream_copy_to_stream($source, $target); + if ($result !== false) { + $result = true; + } } } finally { if ($source !== false) { @@ -715,7 +718,7 @@ private function copyBetweenStorage( $this->getCache()->remove($targetInternalPath); } } - return (bool)$result; + return $result; } public function getLocalFile(string $path): string|false { @@ -915,7 +918,13 @@ public function writeStream(string $path, $stream, ?int $size = null): int { if ($target === false) { throw new GenericFileException("Failed to open $path for writing"); } - [$count, $result] = Files::streamCopy($stream, $target, true); + $count = stream_copy_to_stream($stream, $target); + if ($count === false) { + $result = false; + $count = 0; + } else { + $result = true; + } fclose($stream); fclose($target); diff --git a/lib/private/Files/Storage/Wrapper/Jail.php b/lib/private/Files/Storage/Wrapper/Jail.php index 125f5469f926e..ebde93328d98f 100644 --- a/lib/private/Files/Storage/Wrapper/Jail.php +++ b/lib/private/Files/Storage/Wrapper/Jail.php @@ -11,10 +11,10 @@ use OC\Files\Cache\Wrapper\JailPropagator; use OC\Files\Cache\Wrapper\JailWatcher; use OC\Files\Filesystem; -use OCP\Files; use OCP\Files\Cache\ICache; use OCP\Files\Cache\IPropagator; use OCP\Files\Cache\IWatcher; +use OCP\Files\GenericFileException; use OCP\Files\Storage\IStorage; use OCP\Files\Storage\IWriteStreamStorage; use OCP\IDBConnection; @@ -258,9 +258,13 @@ public function writeStream(string $path, $stream, ?int $size = null): int { return $storage->writeStream($this->getUnjailedPath($path), $stream, $size); } else { $target = $this->fopen($path, 'w'); - $count = Files::streamCopy($stream, $target); + $count = stream_copy_to_stream($stream, $target); fclose($stream); fclose($target); + if ($count === false) { + throw new GenericFileException('Failed to copy stream.'); + } + return $count; } } diff --git a/lib/private/Files/Storage/Wrapper/Wrapper.php b/lib/private/Files/Storage/Wrapper/Wrapper.php index 19fe9a6621e2d..3a95c7547472f 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -9,12 +9,12 @@ use OC\Files\Storage\FailedStorage; use OC\Files\Storage\Storage; -use OCP\Files; use OCP\Files\Cache\ICache; use OCP\Files\Cache\IPropagator; use OCP\Files\Cache\IScanner; use OCP\Files\Cache\IUpdater; use OCP\Files\Cache\IWatcher; +use OCP\Files\GenericFileException; use OCP\Files\Storage\ILockingStorage; use OCP\Files\Storage\IStorage; use OCP\Files\Storage\IWriteStreamStorage; @@ -330,9 +330,13 @@ public function writeStream(string $path, $stream, ?int $size = null): int { return $storage->writeStream($path, $stream, $size); } else { $target = $this->fopen($path, 'w'); - $count = Files::streamCopy($stream, $target); + $count = stream_copy_to_stream($stream, $target); fclose($stream); fclose($target); + if ($count === false) { + throw new GenericFileException('Failed to copy stream.'); + } + return $count; } } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index e7bf6d7aea97f..0d20ab75067a3 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -639,7 +639,10 @@ public function file_put_contents($path, $data) { [$storage, $internalPath] = $this->resolvePath($path); $target = $storage->fopen($internalPath, 'w'); if ($target) { - [, $result] = Files::streamCopy($data, $target, true); + $result = stream_copy_to_stream($data, $target); + if ($result !== false) { + $result = true; + } fclose($target); fclose($data); diff --git a/lib/public/Files.php b/lib/public/Files.php index 77657c41a912a..c7918c4cee67c 100644 --- a/lib/public/Files.php +++ b/lib/public/Files.php @@ -83,47 +83,4 @@ public static function getMimeType($path) { public static function searchByMime($mimetype) { return \OC\Files\Filesystem::searchByMime($mimetype); } - - /** - * Copy the contents of one stream to another - * - * @template T of null|true - * @param resource $source - * @param resource $target - * @param T $includeResult - * @return int|array - * @psalm-return (T is true ? array{0: int, 1: bool} : int) - * @since 5.0.0 - * @since 32.0.0 added $includeResult parameter - * @deprecated 14.0.0 - */ - public static function streamCopy($source, $target, ?bool $includeResult = null) { - if (!$source || !$target) { - return $includeResult ? [0, false] : 0; - } - - $bufSize = 8192; - $count = 0; - $result = true; - while (!feof($source)) { - $buf = fread($source, $bufSize); - if ($buf === false) { - $result = false; - break; - } - - $bytesWritten = fwrite($target, $buf); - if ($bytesWritten !== false) { - $count += $bytesWritten; - } - - if ($bytesWritten === false - || ($bytesWritten < $bufSize && $bytesWritten < strlen($buf)) - ) { - $result = false; - break; - } - } - return $includeResult ? [$count, $result] : $count; - } } diff --git a/tests/lib/Archive/TestBase.php b/tests/lib/Archive/TestBase.php index f3728dacbade6..24b518104d53a 100644 --- a/tests/lib/Archive/TestBase.php +++ b/tests/lib/Archive/TestBase.php @@ -96,7 +96,8 @@ public function testWriteStream(): void { $this->instance = $this->getNew(); $fh = $this->instance->getStream('lorem.txt', 'w'); $source = fopen($dir . '/lorem.txt', 'r'); - Files::streamCopy($source, $fh); + $result = stream_copy_to_stream($source, $fh); + $this->assertNotFalse($result); fclose($source); fclose($fh); $this->assertTrue($this->instance->fileExists('lorem.txt')); diff --git a/tests/lib/Files/Storage/Wrapper/QuotaTest.php b/tests/lib/Files/Storage/Wrapper/QuotaTest.php index 3d313666a93b0..ce43aa2736e09 100644 --- a/tests/lib/Files/Storage/Wrapper/QuotaTest.php +++ b/tests/lib/Files/Storage/Wrapper/QuotaTest.php @@ -115,9 +115,8 @@ public function testStreamCopyWithEnoughSpace(): void { $instance = $this->getLimitedStorage(16); $inputStream = fopen('data://text/plain,foobarqwerty', 'r'); $outputStream = $instance->fopen('files/foo', 'w+'); - [$count, $result] = Files::streamCopy($inputStream, $outputStream, true); + $count = stream_copy_to_stream($inputStream, $outputStream); $this->assertEquals(12, $count); - $this->assertTrue($result); fclose($inputStream); fclose($outputStream); } @@ -126,9 +125,8 @@ public function testStreamCopyNotEnoughSpace(): void { $instance = $this->getLimitedStorage(9); $inputStream = fopen('data://text/plain,foobarqwerty', 'r'); $outputStream = $instance->fopen('files/foo', 'w+'); - [$count, $result] = Files::streamCopy($inputStream, $outputStream, true); - $this->assertEquals(9, $count); - $this->assertFalse($result); + $count = stream_copy_to_stream($inputStream, $outputStream); + $this->assertFalse($count); fclose($inputStream); fclose($outputStream); } diff --git a/tests/lib/FilesTest.php b/tests/lib/FilesTest.php index 5b1bc59d2d98e..2ba6ce2666bba 100644 --- a/tests/lib/FilesTest.php +++ b/tests/lib/FilesTest.php @@ -39,36 +39,4 @@ public function testRecursiveFolderDeletion(): void { Files::rmdirr($baseDir); $this->assertFalse(file_exists($baseDir)); } - - #[\PHPUnit\Framework\Attributes\DataProvider('streamCopyDataProvider')] - public function testStreamCopy($expectedCount, $expectedResult, $source, $target): void { - if (is_string($source)) { - $source = fopen($source, 'r'); - } - if (is_string($target)) { - $target = fopen($target, 'w'); - } - - [$count, $result] = Files::streamCopy($source, $target, true); - - if (is_resource($source)) { - fclose($source); - } - if (is_resource($target)) { - fclose($target); - } - - $this->assertSame($expectedCount, $count); - $this->assertSame($expectedResult, $result); - } - - - public static function streamCopyDataProvider(): array { - return [ - [0, false, false, false], - [0, false, \OC::$SERVERROOT . '/tests/data/lorem.txt', false], - [filesize(\OC::$SERVERROOT . '/tests/data/lorem.txt'), true, \OC::$SERVERROOT . '/tests/data/lorem.txt', \OC::$SERVERROOT . '/tests/data/lorem-copy.txt'], - [3670, true, \OC::$SERVERROOT . '/tests/data/testimage.png', \OC::$SERVERROOT . '/tests/data/testimage-copy.png'], - ]; - } } From 672a29aa0c10affd97e9dcb70b417b1073faaa04 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 16:44:44 +0100 Subject: [PATCH 09/10] refactor(Files): Remove deprecated and unused getMimeType and searchByMime methods Signed-off-by: provokateurin --- lib/public/Files.php | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/lib/public/Files.php b/lib/public/Files.php index c7918c4cee67c..cdfb469dafa74 100644 --- a/lib/public/Files.php +++ b/lib/public/Files.php @@ -10,8 +10,6 @@ namespace OCP; -use OCP\Files\IMimeTypeDetector; - /** * This class provides access to the internal filesystem abstraction layer. Use * this class exclusively if you want to access files @@ -60,27 +58,4 @@ public static function rmdirr($dir, bool $deleteSelf = true) { return !file_exists($dir); } - - /** - * Get the mimetype form a local file - * @param string $path - * @return string - * does NOT work for ownClouds filesystem, use OC_FileSystem::getMimeType instead - * @since 5.0.0 - * @deprecated 14.0.0 - */ - public static function getMimeType($path) { - return Server::get(IMimeTypeDetector::class)->detect($path); - } - - /** - * Search for files by mimetype - * @param string $mimetype - * @return array - * @since 6.0.0 - * @deprecated 14.0.0 - */ - public static function searchByMime($mimetype) { - return \OC\Files\Filesystem::searchByMime($mimetype); - } } From 4eada2d804894dad5f0ce717945b7490864ba3bc Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 16:14:59 +0100 Subject: [PATCH 10/10] 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 | 93 +++++++++++-------- psalm-strict.xml | 1 + .../lib/Encryption/EncryptionWrapperTest.php | 13 +-- .../Files/Storage/Wrapper/EncryptionTest.php | 2 +- 7 files changed, 70 insertions(+), 54 deletions(-) diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index aeaaa54f3708d..136008a7b5c13 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 232630ab5219c..768fe3cebe50e 100644 --- a/build/rector-strict.php +++ b/build/rector-strict.php @@ -18,6 +18,7 @@ $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 ebde93328d98f..1d5dfc7153afb 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\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\IDBConnection; use OCP\Lock\ILockingProvider; use OCP\Server; +use Psr\Log\LoggerInterface; /** * Jail to a subdirectory of the wrapped storage @@ -51,6 +53,12 @@ public function getUnjailedPath(string $path): string { * 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 3a95c7547472f..3c8d4d63d1061 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 @@ public function hasUpdated(string $path, int $time): bool { } 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 @@ public function getOwner(string $path): string|false { } 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 @@ public function isLocal(): bool { } 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 @@ public function getInstanceOfStorage(string $class): ?IStorage { if ($storage instanceof $class) { break; } + $storage = $storage->getWrapperStorage(); } + if (!($storage instanceof $class)) { return null; } + return $storage; } @@ -261,6 +268,7 @@ public function __call(string $method, array $args) { #[Override] public function getDirectDownload(string $path): array|false { + /** @psalm-suppress DeprecatedMethod */ return $this->getWrapperStorage()->getDirectDownload($path); } @@ -302,20 +310,23 @@ public function getMetaData(string $path): ?array { } 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 @@ public function writeStream(string $path, $stream, ?int $size = null): int { 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.'); - } + } + + $target = $this->fopen($path, 'w'); + if ($target === false) { + throw new GenericFileException('Failed to open ' . $path); + } - return $count; + $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 @@ public function isWrapperOf(IStorage $storage): bool { 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 32b6550d799df..07be9fddeeafe 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 58bf5aff005fb..8d1deacf3fda5 100644 --- a/tests/lib/Encryption/EncryptionWrapperTest.php +++ b/tests/lib/Encryption/EncryptionWrapperTest.php @@ -15,7 +15,6 @@ 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 @@ protected function setUp(): void { #[\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 cd82d200de8cc..8308d64c5a548 100644 --- a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php +++ b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php @@ -958,7 +958,7 @@ public function testShouldEncrypt( $wrapper = $this->getMockBuilder(Encryption::class) ->setConstructorArgs( [ - ['mountPoint' => '', 'mount' => $mount, 'storage' => ''], + ['mountPoint' => '', 'mount' => $mount, 'storage' => null], $encryptionManager, $util, $this->logger,