From bbe3728fb4174aa88c7033617f1842bc407e1288 Mon Sep 17 00:00:00 2001 From: fenn-cs Date: Mon, 15 Apr 2024 20:38:26 +0100 Subject: [PATCH] refactor(shareApiController): use contrusctor property promotion & DI logger Signed-off-by: fenn-cs --- .../lib/Controller/ShareAPIController.php | 72 ++++---------- apps/files_sharing/tests/ApiTest.php | 9 +- .../Controller/ShareAPIControllerTest.php | 96 ++++++++----------- 3 files changed, 62 insertions(+), 115 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 1e7ddc2ab0136..28a87196f8e4f 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -71,7 +71,6 @@ use OCP\IL10N; use OCP\IPreview; use OCP\IRequest; -use OCP\IServerContainer; use OCP\IURLGenerator; use OCP\IUserManager; use OCP\Lock\ILockingProvider; @@ -83,6 +82,7 @@ use OCP\Share\IShare; use OCP\UserStatus\IManager as IUserStatusManager; use Psr\Container\ContainerExceptionInterface; +use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; /** @@ -92,32 +92,8 @@ */ class ShareAPIController extends OCSController { - /** @var IManager */ - private $shareManager; - /** @var IGroupManager */ - private $groupManager; - /** @var IUserManager */ - private $userManager; - /** @var IRootFolder */ - private $rootFolder; - /** @var IURLGenerator */ - private $urlGenerator; - /** @var string */ - private $currentUser; - /** @var IL10N */ - private $l; - /** @var \OCP\Files\Node */ - private $lockedNode; - /** @var IConfig */ - private $config; - /** @var IAppManager */ - private $appManager; - /** @var IServerContainer */ - private $serverContainer; - /** @var IUserStatusManager */ - private $userStatusManager; - /** @var IPreview */ - private $previewManager; + private ?Node $lockedNode = null; + private string $currentUser; /** * Share20OCS constructor. @@ -125,35 +101,23 @@ class ShareAPIController extends OCSController { public function __construct( string $appName, IRequest $request, - IManager $shareManager, - IGroupManager $groupManager, - IUserManager $userManager, - IRootFolder $rootFolder, - IURLGenerator $urlGenerator, - string $userId = null, - IL10N $l10n, - IConfig $config, - IAppManager $appManager, - IServerContainer $serverContainer, - IUserStatusManager $userStatusManager, - IPreview $previewManager, + private IManager $shareManager, + private IGroupManager $groupManager, + private IUserManager $userManager, + private IRootFolder $rootFolder, + private IURLGenerator $urlGenerator, + private IL10N $l, + private IConfig $config, + private IAppManager $appManager, + private ContainerInterface $serverContainer, + private IUserStatusManager $userStatusManager, + private IPreview $previewManager, private IDateTimeZone $dateTimeZone, + private LoggerInterface $logger, + ?string $userId = null ) { parent::__construct($appName, $request); - - $this->shareManager = $shareManager; - $this->userManager = $userManager; - $this->groupManager = $groupManager; - $this->request = $request; - $this->rootFolder = $rootFolder; - $this->urlGenerator = $urlGenerator; $this->currentUser = $userId; - $this->l = $l10n; - $this->config = $config; - $this->appManager = $appManager; - $this->serverContainer = $serverContainer; - $this->userStatusManager = $userStatusManager; - $this->previewManager = $previewManager; } /** @@ -355,7 +319,7 @@ private function getDisplayNameFromAddressBook(string $query, string $property): 'strict_search' => true, ]); } catch (Exception $e) { - Server::get(LoggerInterface::class)->error( + $this->logger->error( $e->getMessage(), ['exception' => $e] ); @@ -437,7 +401,7 @@ private function retrieveFederatedDisplayName(array $userIds, bool $cacheOnly = try { $slaveService = Server::get(\OCA\GlobalSiteSelector\Service\SlaveService::class); } catch (\Throwable $e) { - Server::get(LoggerInterface::class)->error( + $this->logger->error( $e->getMessage(), ['exception' => $e] ); diff --git a/apps/files_sharing/tests/ApiTest.php b/apps/files_sharing/tests/ApiTest.php index 66c8432c1c90f..4214d83827e0f 100644 --- a/apps/files_sharing/tests/ApiTest.php +++ b/apps/files_sharing/tests/ApiTest.php @@ -49,9 +49,10 @@ use OCP\IL10N; use OCP\IPreview; use OCP\IRequest; -use OCP\IServerContainer; use OCP\Share\IShare; use OCP\UserStatus\IManager as IUserStatusManager; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; /** * Class ApiTest @@ -121,10 +122,11 @@ private function createOCS($userId) { }); $config = $this->createMock(IConfig::class); $appManager = $this->createMock(IAppManager::class); - $serverContainer = $this->createMock(IServerContainer::class); + $serverContainer = $this->createMock(ContainerInterface::class); $userStatusManager = $this->createMock(IUserStatusManager::class); $previewManager = $this->createMock(IPreview::class); $dateTimeZone = $this->createMock(IDateTimeZone::class); + $logger = $this->createMock(LoggerInterface::class); $dateTimeZone->method('getTimeZone')->willReturn(new \DateTimeZone(date_default_timezone_get())); return new ShareAPIController( @@ -135,7 +137,6 @@ private function createOCS($userId) { \OC::$server->getUserManager(), \OC::$server->getRootFolder(), \OC::$server->getURLGenerator(), - $userId, $l, $config, $appManager, @@ -143,6 +144,8 @@ private function createOCS($userId) { $userStatusManager, $previewManager, $dateTimeZone, + $logger, + $userId, ); } diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 0f62c625c6257..d202d6e836148 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -54,7 +54,6 @@ use OCP\IL10N; use OCP\IPreview; use OCP\IRequest; -use OCP\IServerContainer; use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; @@ -74,53 +73,23 @@ */ class ShareAPIControllerTest extends TestCase { - /** @var string */ - private $appName = 'files_sharing'; - - /** @var \OC\Share20\Manager|\PHPUnit\Framework\MockObject\MockObject */ - private $shareManager; - - /** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */ - private $groupManager; - - /** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */ - private $userManager; - - /** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */ - private $request; - - /** @var IRootFolder|\PHPUnit\Framework\MockObject\MockObject */ - private $rootFolder; - - /** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */ - private $urlGenerator; - - /** @var string|\PHPUnit\Framework\MockObject\MockObject */ - private $currentUser; - - /** @var ShareAPIController */ - private $ocs; - - /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ - private $l; - - /** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */ - private $config; - - /** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject */ - private $appManager; - - /** @var IServerContainer|\PHPUnit\Framework\MockObject\MockObject */ - private $serverContainer; - - /** @var IUserStatusManager|\PHPUnit\Framework\MockObject\MockObject */ - private $userStatusManager; - - /** @var IPreview|\PHPUnit\Framework\MockObject\MockObject */ - private $previewManager; - - /** @var IDateTimeZone|\PHPUnit\Framework\MockObject\MockObject */ - private $dateTimeZone; + private string $appName = 'files_sharing'; + private \OC\Share20\Manager|\PHPUnit\Framework\MockObject\MockObject $shareManager; + private IGroupManager|\PHPUnit\Framework\MockObject\MockObject $groupManager; + private IUserManager|\PHPUnit\Framework\MockObject\MockObject $userManager; + private IRequest|\PHPUnit\Framework\MockObject\MockObject $request; + private IRootFolder|\PHPUnit\Framework\MockObject\MockObject $rootFolder; + private IURLGenerator|\PHPUnit\Framework\MockObject\MockObject $urlGenerator; + private string|\PHPUnit\Framework\MockObject\MockObject $currentUser; + private ShareAPIController $ocs; + private IL10N|\PHPUnit\Framework\MockObject\MockObject $l; + private IConfig|\PHPUnit\Framework\MockObject\MockObject $config; + private IAppManager|\PHPUnit\Framework\MockObject\MockObject $appManager; + private IServerContainer|\PHPUnit\Framework\MockObject\MockObject $serverContainer; + private IUserStatusManager|\PHPUnit\Framework\MockObject\MockObject $userStatusManager; + private IPreview|\PHPUnit\Framework\MockObject\MockObject $previewManager; + private IDateTimeZone|\PHPUnit\Framework\MockObject\MockObject $dateTimeZone; + private LoggerInterface $logger; protected function setUp(): void { $this->shareManager = $this->createMock(IManager::class); @@ -145,7 +114,7 @@ protected function setUp(): void { }); $this->config = $this->createMock(IConfig::class); $this->appManager = $this->createMock(IAppManager::class); - $this->serverContainer = $this->createMock(IServerContainer::class); + $this->serverContainer = $this->createMock(ContainerInterface::class); $this->userStatusManager = $this->createMock(IUserStatusManager::class); $this->previewManager = $this->createMock(IPreview::class); $this->previewManager->method('isAvailable') @@ -153,6 +122,7 @@ protected function setUp(): void { return $fileInfo->getMimeType() === 'mimeWithPreview'; }); $this->dateTimeZone = $this->createMock(IDateTimeZone::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->ocs = new ShareAPIController( $this->appName, @@ -162,7 +132,6 @@ protected function setUp(): void { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, $this->l, $this->config, $this->appManager, @@ -170,6 +139,8 @@ protected function setUp(): void { $this->userStatusManager, $this->previewManager, $this->dateTimeZone, + $this->logger, + $this->currentUser, ); } @@ -186,7 +157,6 @@ private function mockFormatShare() { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, $this->l, $this->config, $this->appManager, @@ -194,6 +164,8 @@ private function mockFormatShare() { $this->userStatusManager, $this->previewManager, $this->dateTimeZone, + $this->logger, + $this->currentUser, ])->setMethods(['formatShare']) ->getMock(); } @@ -775,7 +747,6 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, $this->l, $this->config, $this->appManager, @@ -783,6 +754,9 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) { $this->userStatusManager, $this->previewManager, $this->dateTimeZone, + $this->logger, + $this->currentUser, + ])->setMethods(['canAccessShare']) ->getMock(); @@ -1401,7 +1375,6 @@ public function testGetShares(array $getSharesParameters, array $shares, array $ $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, $this->l, $this->config, $this->appManager, @@ -1409,6 +1382,8 @@ public function testGetShares(array $getSharesParameters, array $shares, array $ $this->userStatusManager, $this->previewManager, $this->dateTimeZone, + $this->logger, + $this->currentUser, ])->setMethods(['formatShare']) ->getMock(); @@ -1741,7 +1716,6 @@ public function testCreateShareUser() { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, $this->l, $this->config, $this->appManager, @@ -1749,6 +1723,8 @@ public function testCreateShareUser() { $this->userStatusManager, $this->previewManager, $this->dateTimeZone, + $this->logger, + $this->currentUser, ])->setMethods(['formatShare']) ->getMock(); @@ -1836,7 +1812,6 @@ public function testCreateShareGroup() { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, $this->l, $this->config, $this->appManager, @@ -1844,6 +1819,8 @@ public function testCreateShareGroup() { $this->userStatusManager, $this->previewManager, $this->dateTimeZone, + $this->logger, + $this->currentUser, ])->setMethods(['formatShare']) ->getMock(); @@ -2246,7 +2223,6 @@ public function testCreateShareRemote() { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, $this->l, $this->config, $this->appManager, @@ -2254,6 +2230,8 @@ public function testCreateShareRemote() { $this->userStatusManager, $this->previewManager, $this->dateTimeZone, + $this->logger, + $this->currentUser, ])->setMethods(['formatShare']) ->getMock(); @@ -2313,7 +2291,6 @@ public function testCreateShareRemoteGroup() { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, $this->l, $this->config, $this->appManager, @@ -2321,6 +2298,8 @@ public function testCreateShareRemoteGroup() { $this->userStatusManager, $this->previewManager, $this->dateTimeZone, + $this->logger, + $this->currentUser, ])->setMethods(['formatShare']) ->getMock(); @@ -2553,7 +2532,6 @@ public function testCreateReshareOfFederatedMountNoDeletePermissions() { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, $this->l, $this->config, $this->appManager, @@ -2561,6 +2539,8 @@ public function testCreateReshareOfFederatedMountNoDeletePermissions() { $this->userStatusManager, $this->previewManager, $this->dateTimeZone, + $this->logger, + $this->currentUser, ])->setMethods(['formatShare']) ->getMock();