From ab797062533bfac412eb9300c2ae01e380609b45 Mon Sep 17 00:00:00 2001 From: Faraz Samapoor Date: Thu, 3 Aug 2023 16:35:09 +0330 Subject: [PATCH 1/3] Refactors files_version app commands. To improve code readability. Signed-off-by: Faraz Samapoor --- apps/files_versions/lib/Command/CleanUp.php | 29 ++++---------- apps/files_versions/lib/Command/Expire.php | 21 +++------- .../lib/Command/ExpireVersions.php | 38 +++++-------------- 3 files changed, 24 insertions(+), 64 deletions(-) diff --git a/apps/files_versions/lib/Command/CleanUp.php b/apps/files_versions/lib/Command/CleanUp.php index d7bb4caa483ff..26a1c57ec74c7 100644 --- a/apps/files_versions/lib/Command/CleanUp.php +++ b/apps/files_versions/lib/Command/CleanUp.php @@ -34,24 +34,14 @@ use Symfony\Component\Console\Output\OutputInterface; class CleanUp extends Command { - - /** @var IUserManager */ - protected $userManager; - - /** @var IRootFolder */ - protected $rootFolder; - - /** - * @param IRootFolder $rootFolder - * @param IUserManager $userManager - */ - public function __construct(IRootFolder $rootFolder, IUserManager $userManager) { + public function __construct( + protected IRootFolder $rootFolder, + protected IUserManager $userManager, + ) { parent::__construct(); - $this->userManager = $userManager; - $this->rootFolder = $rootFolder; } - protected function configure() { + protected function configure(): void { $this ->setName('versions:cleanup') ->setDescription('Delete versions') @@ -76,7 +66,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int if ($path) { if (!preg_match('#^/([^/]+)/files(/.*)?$#', $path, $pathMatches)) { $output->writeln("Invalid path given"); - return 1; + return self::FAILURE; } $users = [ $pathMatches[1] ]; @@ -90,7 +80,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->deleteVersions($user, $path); } else { $output->writeln("Unknown user $user"); - return 1; + return self::FAILURE; } } } else { @@ -116,15 +106,12 @@ protected function execute(InputInterface $input, OutputInterface $output): int } while (count($users) >= $limit); } } - return 0; + return self::SUCCESS; } /** * delete versions for the given user - * - * @param string $user - * @param string|null $path */ protected function deleteVersions(string $user, string $path = null): void { \OC_Util::tearDownFS(); diff --git a/apps/files_versions/lib/Command/Expire.php b/apps/files_versions/lib/Command/Expire.php index 62b2343a5e013..3744df10093b6 100644 --- a/apps/files_versions/lib/Command/Expire.php +++ b/apps/files_versions/lib/Command/Expire.php @@ -33,22 +33,13 @@ class Expire implements ICommand { use FileAccess; - /** - * @var string - */ - private $fileName; - - /** - * @var string - */ - private $user; - - public function __construct(string $user, string $fileName) { - $this->user = $user; - $this->fileName = $fileName; + public function __construct( + private string $user, + private string $fileName, + ) { } - public function handle() { + public function handle(): void { /** @var IUserManager $userManager */ $userManager = \OC::$server->get(IUserManager::class); if (!$userManager->userExists($this->user)) { @@ -62,7 +53,7 @@ public function handle() { // In case of external storage and session credentials, the expiration // fails because the command does not have those credentials - /** @var LoggerInterface */ + /** @var LoggerInterface $logger */ $logger = \OC::$server->get(LoggerInterface::class); $logger->warning($e->getMessage(), [ 'exception' => $e, diff --git a/apps/files_versions/lib/Command/ExpireVersions.php b/apps/files_versions/lib/Command/ExpireVersions.php index 43068e2145119..38c7929ff5bb1 100644 --- a/apps/files_versions/lib/Command/ExpireVersions.php +++ b/apps/files_versions/lib/Command/ExpireVersions.php @@ -36,30 +36,14 @@ use Symfony\Component\Console\Output\OutputInterface; class ExpireVersions extends Command { - - /** - * @var Expiration - */ - private $expiration; - - /** - * @var IUserManager - */ - private $userManager; - - /** - * @param IUserManager $userManager - * @param Expiration $expiration - */ - public function __construct(IUserManager $userManager, - Expiration $expiration) { + public function __construct( + private IUserManager $userManager, + private Expiration $expiration, + ) { parent::__construct(); - - $this->userManager = $userManager; - $this->expiration = $expiration; } - protected function configure() { + protected function configure(): void { $this ->setName('versions:expire') ->setDescription('Expires the users file versions') @@ -74,7 +58,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $maxAge = $this->expiration->getMaxAgeAsTimestamp(); if (!$maxAge) { $output->writeln("Auto expiration is configured - expiration will be handled automatically according to the expiration patterns detailed at the following link https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/file_versioning.html."); - return 1; + return self::FAILURE; } $users = $input->getArgument('user_id'); @@ -86,7 +70,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->expireVersionsForUser($userObject); } else { $output->writeln("Unknown user $user"); - return 1; + return self::FAILURE; } } } else { @@ -99,10 +83,10 @@ protected function execute(InputInterface $input, OutputInterface $output): int $p->finish(); $output->writeln(''); } - return 0; + return self::SUCCESS; } - public function expireVersionsForUser(IUser $user) { + public function expireVersionsForUser(IUser $user): void { $uid = $user->getUID(); if (!$this->setupFS($uid)) { return; @@ -112,10 +96,8 @@ public function expireVersionsForUser(IUser $user) { /** * Act on behalf on versions item owner - * @param string $user - * @return boolean */ - protected function setupFS($user) { + protected function setupFS(string $user): bool { \OC_Util::tearDownFS(); \OC_Util::setupFS($user); From 90652fd30fcbe620d1b1bf87899eeae6f6c7c657 Mon Sep 17 00:00:00 2001 From: Faraz Samapoor Date: Thu, 3 Aug 2023 16:42:56 +0330 Subject: [PATCH 2/3] Uses early returns. To improve code readability. Signed-off-by: Faraz Samapoor --- apps/files_versions/lib/Command/CleanUp.php | 48 ++++++++++--------- .../lib/Command/ExpireVersions.php | 29 +++++------ 2 files changed, 40 insertions(+), 37 deletions(-) diff --git a/apps/files_versions/lib/Command/CleanUp.php b/apps/files_versions/lib/Command/CleanUp.php index 26a1c57ec74c7..2a19c57710ed8 100644 --- a/apps/files_versions/lib/Command/CleanUp.php +++ b/apps/files_versions/lib/Command/CleanUp.php @@ -75,37 +75,39 @@ protected function execute(InputInterface $input, OutputInterface $output): int if (!empty($users)) { foreach ($users as $user) { - if ($this->userManager->userExists($user)) { - $output->writeln("Delete versions of $user"); - $this->deleteVersions($user, $path); - } else { + if (!$this->userManager->userExists($user)) { $output->writeln("Unknown user $user"); return self::FAILURE; } + + $output->writeln("Delete versions of $user"); + $this->deleteVersions($user, $path); } - } else { - $output->writeln('Delete all versions'); - foreach ($this->userManager->getBackends() as $backend) { - $name = get_class($backend); + return self::SUCCESS; + } - if ($backend instanceof IUserBackend) { - $name = $backend->getBackendName(); - } + $output->writeln('Delete all versions'); + foreach ($this->userManager->getBackends() as $backend) { + $name = get_class($backend); - $output->writeln("Delete versions for users on backend $name"); - - $limit = 500; - $offset = 0; - do { - $users = $backend->getUsers('', $limit, $offset); - foreach ($users as $user) { - $output->writeln(" $user"); - $this->deleteVersions($user); - } - $offset += $limit; - } while (count($users) >= $limit); + if ($backend instanceof IUserBackend) { + $name = $backend->getBackendName(); } + + $output->writeln("Delete versions for users on backend $name"); + + $limit = 500; + $offset = 0; + do { + $users = $backend->getUsers('', $limit, $offset); + foreach ($users as $user) { + $output->writeln(" $user"); + $this->deleteVersions($user); + } + $offset += $limit; + } while (count($users) >= $limit); } + return self::SUCCESS; } diff --git a/apps/files_versions/lib/Command/ExpireVersions.php b/apps/files_versions/lib/Command/ExpireVersions.php index 38c7929ff5bb1..d8e59b40dbab2 100644 --- a/apps/files_versions/lib/Command/ExpireVersions.php +++ b/apps/files_versions/lib/Command/ExpireVersions.php @@ -64,25 +64,26 @@ protected function execute(InputInterface $input, OutputInterface $output): int $users = $input->getArgument('user_id'); if (!empty($users)) { foreach ($users as $user) { - if ($this->userManager->userExists($user)) { - $output->writeln("Remove deleted files of $user"); - $userObject = $this->userManager->get($user); - $this->expireVersionsForUser($userObject); - } else { + if (!$this->userManager->userExists($user)) { $output->writeln("Unknown user $user"); return self::FAILURE; } + + $output->writeln("Remove deleted files of $user"); + $userObject = $this->userManager->get($user); + $this->expireVersionsForUser($userObject); } - } else { - $p = new ProgressBar($output); - $p->start(); - $this->userManager->callForSeenUsers(function (IUser $user) use ($p) { - $p->advance(); - $this->expireVersionsForUser($user); - }); - $p->finish(); - $output->writeln(''); + return self::SUCCESS; } + + $p = new ProgressBar($output); + $p->start(); + $this->userManager->callForSeenUsers(function (IUser $user) use ($p) { + $p->advance(); + $this->expireVersionsForUser($user); + }); + $p->finish(); + $output->writeln(''); return self::SUCCESS; } From 0273b96eef8111def6bb3d5773a46f543802cb02 Mon Sep 17 00:00:00 2001 From: Faraz Samapoor Date: Thu, 21 Sep 2023 11:40:18 +0330 Subject: [PATCH 3/3] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com> Signed-off-by: Faraz Samapoor --- apps/files_versions/lib/Command/CleanUp.php | 2 +- apps/files_versions/lib/Command/Expire.php | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/files_versions/lib/Command/CleanUp.php b/apps/files_versions/lib/Command/CleanUp.php index 2a19c57710ed8..519b3689a58da 100644 --- a/apps/files_versions/lib/Command/CleanUp.php +++ b/apps/files_versions/lib/Command/CleanUp.php @@ -115,7 +115,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int /** * delete versions for the given user */ - protected function deleteVersions(string $user, string $path = null): void { + protected function deleteVersions(string $user, ?string $path = null): void { \OC_Util::tearDownFS(); \OC_Util::setupFS($user); diff --git a/apps/files_versions/lib/Command/Expire.php b/apps/files_versions/lib/Command/Expire.php index 3744df10093b6..688a6e869e945 100644 --- a/apps/files_versions/lib/Command/Expire.php +++ b/apps/files_versions/lib/Command/Expire.php @@ -53,7 +53,6 @@ public function handle(): void { // In case of external storage and session credentials, the expiration // fails because the command does not have those credentials - /** @var LoggerInterface $logger */ $logger = \OC::$server->get(LoggerInterface::class); $logger->warning($e->getMessage(), [ 'exception' => $e,