From 098c357edb492322371c1d87daebdb844bd1f508 Mon Sep 17 00:00:00 2001 From: Faraz Samapoor Date: Tue, 15 Aug 2023 10:46:16 +0330 Subject: [PATCH 1/2] Refactors files_trashbin app commands. To improve code readability. Signed-off-by: Faraz Samapoor --- apps/files_trashbin/lib/Command/CleanUp.php | 30 ++++----------- apps/files_trashbin/lib/Command/Expire.php | 15 ++------ .../lib/Command/ExpireTrash.php | 38 +++++-------------- .../lib/Command/RestoreAllFiles.php | 37 +++++------------- apps/files_trashbin/lib/Command/Size.php | 22 ++++------- 5 files changed, 39 insertions(+), 103 deletions(-) diff --git a/apps/files_trashbin/lib/Command/CleanUp.php b/apps/files_trashbin/lib/Command/CleanUp.php index a00a96d5ee210..db0e528fdea3b 100644 --- a/apps/files_trashbin/lib/Command/CleanUp.php +++ b/apps/files_trashbin/lib/Command/CleanUp.php @@ -37,29 +37,15 @@ use Symfony\Component\Console\Output\OutputInterface; class CleanUp extends Command { - - /** @var IUserManager */ - protected $userManager; - - /** @var IRootFolder */ - protected $rootFolder; - - /** @var \OCP\IDBConnection */ - protected $dbConnection; - - /** - * @param IRootFolder $rootFolder - * @param IUserManager $userManager - * @param IDBConnection $dbConnection - */ - public function __construct(IRootFolder $rootFolder, IUserManager $userManager, IDBConnection $dbConnection) { + public function __construct( + protected IRootFolder $rootFolder, + protected IUserManager $userManager, + protected IDBConnection $dbConnection, + ) { parent::__construct(); - $this->userManager = $userManager; - $this->rootFolder = $rootFolder; - $this->dbConnection = $dbConnection; } - protected function configure() { + protected function configure(): void { $this ->setName('trashbin:cleanup') ->setDescription('Remove deleted files') @@ -88,7 +74,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->removeDeletedFiles($user, $output, $verbose); } else { $output->writeln("Unknown user $user"); - return 1; + return self::FAILURE; } } } elseif ($input->getOption('all-users')) { @@ -113,7 +99,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int } else { throw new InvalidOptionException('Either specify a user_id or --all-users'); } - return 0; + return self::SUCCESS; } /** diff --git a/apps/files_trashbin/lib/Command/Expire.php b/apps/files_trashbin/lib/Command/Expire.php index 568506737ebc3..6ccf3c7fdac40 100644 --- a/apps/files_trashbin/lib/Command/Expire.php +++ b/apps/files_trashbin/lib/Command/Expire.php @@ -33,19 +33,12 @@ class Expire implements ICommand { use FileAccess; - /** - * @var string - */ - private $user; - - /** - * @param string $user - */ - public function __construct($user) { - $this->user = $user; + public function __construct( + private string $user, + ) { } - public function handle() { + public function handle(): void { $userManager = \OC::$server->getUserManager(); if (!$userManager->userExists($this->user)) { // User has been deleted already diff --git a/apps/files_trashbin/lib/Command/ExpireTrash.php b/apps/files_trashbin/lib/Command/ExpireTrash.php index e64ef2973f76a..6794dc54627fe 100644 --- a/apps/files_trashbin/lib/Command/ExpireTrash.php +++ b/apps/files_trashbin/lib/Command/ExpireTrash.php @@ -37,30 +37,14 @@ use Symfony\Component\Console\Output\OutputInterface; class ExpireTrash extends Command { - - /** - * @var Expiration - */ - private $expiration; - - /** - * @var IUserManager - */ - private $userManager; - - /** - * @param IUserManager|null $userManager - * @param Expiration|null $expiration - */ - public function __construct(IUserManager $userManager = null, - Expiration $expiration = null) { + public function __construct( + private ?IUserManager $userManager = null, + private ?Expiration $expiration = null, + ) { parent::__construct(); - - $this->userManager = $userManager; - $this->expiration = $expiration; } - protected function configure() { + protected function configure(): void { $this ->setName('trashbin:expire') ->setDescription('Expires the users trashbin') @@ -75,7 +59,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $maxAge = $this->expiration->getMaxAgeAsTimestamp(); if (!$maxAge) { $output->writeln("Auto expiration is configured - keeps files and folders in the trash bin for 30 days and automatically deletes anytime after that if space is needed (note: files may not be deleted if space is not needed)"); - return 1; + return self::FAILURE; } $users = $input->getArgument('user_id'); @@ -87,7 +71,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->expireTrashForUser($userObject); } else { $output->writeln("Unknown user $user"); - return 1; + return self::FAILURE; } } } else { @@ -100,10 +84,10 @@ protected function execute(InputInterface $input, OutputInterface $output): int $p->finish(); $output->writeln(''); } - return 0; + return self::SUCCESS; } - public function expireTrashForUser(IUser $user) { + public function expireTrashForUser(IUser $user): void { $uid = $user->getUID(); if (!$this->setupFS($uid)) { return; @@ -114,10 +98,8 @@ public function expireTrashForUser(IUser $user) { /** * Act on behalf on trash item owner - * @param string $user - * @return boolean */ - protected function setupFS($user) { + protected function setupFS(string $user): bool { \OC_Util::tearDownFS(); \OC_Util::setupFS($user); diff --git a/apps/files_trashbin/lib/Command/RestoreAllFiles.php b/apps/files_trashbin/lib/Command/RestoreAllFiles.php index cd79f1e8def68..35b06f3d69143 100644 --- a/apps/files_trashbin/lib/Command/RestoreAllFiles.php +++ b/apps/files_trashbin/lib/Command/RestoreAllFiles.php @@ -44,33 +44,16 @@ class RestoreAllFiles extends Base { 'all' => self::SCOPE_ALL ]; - /** @var IUserManager */ - protected $userManager; - - /** @var IRootFolder */ - protected $rootFolder; - - /** @var \OCP\IDBConnection */ - protected $dbConnection; - - protected ITrashManager $trashManager; - - /** @var IL10N */ - protected $l10n; - - /** - * @param IRootFolder $rootFolder - * @param IUserManager $userManager - * @param IDBConnection $dbConnection - * @param ITrashManager $trashManager - * @param IFactory $l10nFactory - */ - public function __construct(IRootFolder $rootFolder, IUserManager $userManager, IDBConnection $dbConnection, ITrashManager $trashManager, IFactory $l10nFactory) { + protected IL10N $l10n; + + public function __construct( + protected IRootFolder $rootFolder, + protected IUserManager $userManager, + protected IDBConnection $dbConnection, + protected ITrashManager $trashManager, + IFactory $l10nFactory, + ) { parent::__construct(); - $this->userManager = $userManager; - $this->rootFolder = $rootFolder; - $this->dbConnection = $dbConnection; - $this->trashManager = $trashManager; $this->l10n = $l10nFactory->get('files_trashbin'); } @@ -153,7 +136,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int } else { throw new InvalidOptionException('Either specify a user_id or --all-users'); } - return 0; + return self::SUCCESS; } /** diff --git a/apps/files_trashbin/lib/Command/Size.php b/apps/files_trashbin/lib/Command/Size.php index 30ac474199ed5..511caa98bbc98 100644 --- a/apps/files_trashbin/lib/Command/Size.php +++ b/apps/files_trashbin/lib/Command/Size.php @@ -36,23 +36,15 @@ use Symfony\Component\Console\Output\OutputInterface; class Size extends Base { - private $config; - private $userManager; - private $commandBus; - public function __construct( - IConfig $config, - IUserManager $userManager, - IBus $commandBus + private IConfig $config, + private IUserManager $userManager, + private IBus $commandBus ) { parent::__construct(); - - $this->config = $config; - $this->userManager = $userManager; - $this->commandBus = $commandBus; } - protected function configure() { + protected function configure(): void { parent::configure(); $this ->setName('trashbin:size') @@ -73,7 +65,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $parsedSize = \OC_Helper::computerFileSize($size); if ($parsedSize === false) { $output->writeln("Failed to parse input size"); - return -1; + return self::FAILURE; } if ($user) { $this->config->setUserValue($user, 'files_trashbin', 'trashbin_size', (string)$parsedSize); @@ -87,10 +79,10 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->printTrashbinSize($input, $output, $user); } - return 0; + return self::SUCCESS; } - private function printTrashbinSize(InputInterface $input, OutputInterface $output, ?string $user) { + private function printTrashbinSize(InputInterface $input, OutputInterface $output, ?string $user): void { $globalSize = (int)$this->config->getAppValue('files_trashbin', 'trashbin_size', '-1'); if ($globalSize < 0) { $globalHumanSize = "default (50% of available space)"; From 754b425f059fe6631b0a110fb18f9b69b4cddd18 Mon Sep 17 00:00:00 2001 From: Faraz Samapoor Date: Tue, 15 Aug 2023 13:19:49 +0330 Subject: [PATCH 2/2] Uses early returns. To improve code readability. Signed-off-by: Faraz Samapoor --- apps/files_trashbin/lib/Command/CleanUp.php | 93 +++++++++------- .../lib/Command/ExpireTrash.php | 31 +++--- .../lib/Command/RestoreAllFiles.php | 1 + apps/files_trashbin/lib/Command/Size.php | 100 ++++++++++-------- 4 files changed, 124 insertions(+), 101 deletions(-) diff --git a/apps/files_trashbin/lib/Command/CleanUp.php b/apps/files_trashbin/lib/Command/CleanUp.php index db0e528fdea3b..50e51e70ac924 100644 --- a/apps/files_trashbin/lib/Command/CleanUp.php +++ b/apps/files_trashbin/lib/Command/CleanUp.php @@ -65,40 +65,48 @@ protected function configure(): void { protected function execute(InputInterface $input, OutputInterface $output): int { $users = $input->getArgument('user_id'); $verbose = $input->getOption('verbose'); + + if ((empty($users)) and (!$input->getOption('all-users'))) { + throw new InvalidOptionException('Either specify a user_id or --all-users'); + } + if ((!empty($users)) and ($input->getOption('all-users'))) { throw new InvalidOptionException('Either specify a user_id or --all-users'); - } elseif (!empty($users)) { + } + + if (!empty($users)) { foreach ($users as $user) { - if ($this->userManager->userExists($user)) { - $output->writeln("Remove deleted files of $user"); - $this->removeDeletedFiles($user, $output, $verbose); - } else { + if (!$this->userManager->userExists($user)) { $output->writeln("Unknown user $user"); return self::FAILURE; } + + $output->writeln("Remove deleted files of $user"); + $this->removeDeletedFiles($user, $output, $verbose); } - } elseif ($input->getOption('all-users')) { - $output->writeln('Remove deleted files for all users'); - foreach ($this->userManager->getBackends() as $backend) { - $name = get_class($backend); - if ($backend instanceof IUserBackend) { - $name = $backend->getBackendName(); - } - $output->writeln("Remove deleted files for users on backend $name"); - $limit = 500; - $offset = 0; - do { - $users = $backend->getUsers('', $limit, $offset); - foreach ($users as $user) { - $output->writeln(" $user"); - $this->removeDeletedFiles($user, $output, $verbose); - } - $offset += $limit; - } while (count($users) >= $limit); + + return self::SUCCESS; + } + + $output->writeln('Remove deleted files for all users'); + foreach ($this->userManager->getBackends() as $backend) { + $name = get_class($backend); + if ($backend instanceof IUserBackend) { + $name = $backend->getBackendName(); } - } else { - throw new InvalidOptionException('Either specify a user_id or --all-users'); + $output->writeln("Remove deleted files for users on backend $name"); + $limit = 500; + $offset = 0; + do { + $users = $backend->getUsers('', $limit, $offset); + foreach ($users as $user) { + $output->writeln(" $user"); + $this->removeDeletedFiles($user, $output, $verbose); + } + $offset += $limit; + } while (count($users) >= $limit); } + return self::SUCCESS; } @@ -109,26 +117,29 @@ protected function removeDeletedFiles(string $uid, OutputInterface $output, bool \OC_Util::tearDownFS(); \OC_Util::setupFS($uid); $path = '/' . $uid . '/files_trashbin'; - if ($this->rootFolder->nodeExists($path)) { - $node = $this->rootFolder->get($path); - if ($verbose) { - $output->writeln("Deleting " . \OC_Helper::humanFileSize($node->getSize()) . " in trash for $uid."); - } - $node->delete(); - if ($this->rootFolder->nodeExists($path)) { - $output->writeln("Trash folder sill exists after attempting to delete it"); - return; - } - $query = $this->dbConnection->getQueryBuilder(); - $query->delete('files_trash') - ->where($query->expr()->eq('user', $query->createParameter('uid'))) - ->setParameter('uid', $uid); - $query->execute(); - } else { + if (!$this->rootFolder->nodeExists($path)) { if ($verbose) { $output->writeln("No trash found for $uid"); } + + return; + } + + $node = $this->rootFolder->get($path); + + if ($verbose) { + $output->writeln("Deleting " . \OC_Helper::humanFileSize($node->getSize()) . " in trash for $uid."); + } + $node->delete(); + if ($this->rootFolder->nodeExists($path)) { + $output->writeln("Trash folder sill exists after attempting to delete it"); + return; } + $query = $this->dbConnection->getQueryBuilder(); + $query->delete('files_trash') + ->where($query->expr()->eq('user', $query->createParameter('uid'))) + ->setParameter('uid', $uid); + $query->execute(); } } diff --git a/apps/files_trashbin/lib/Command/ExpireTrash.php b/apps/files_trashbin/lib/Command/ExpireTrash.php index 6794dc54627fe..a64e1e9b837fd 100644 --- a/apps/files_trashbin/lib/Command/ExpireTrash.php +++ b/apps/files_trashbin/lib/Command/ExpireTrash.php @@ -65,25 +65,28 @@ 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->expireTrashForUser($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->expireTrashForUser($userObject); } - } else { - $p = new ProgressBar($output); - $p->start(); - $this->userManager->callForSeenUsers(function (IUser $user) use ($p) { - $p->advance(); - $this->expireTrashForUser($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->expireTrashForUser($user); + }); + $p->finish(); + $output->writeln(''); + return self::SUCCESS; } diff --git a/apps/files_trashbin/lib/Command/RestoreAllFiles.php b/apps/files_trashbin/lib/Command/RestoreAllFiles.php index 35b06f3d69143..2dd1555a36ca5 100644 --- a/apps/files_trashbin/lib/Command/RestoreAllFiles.php +++ b/apps/files_trashbin/lib/Command/RestoreAllFiles.php @@ -136,6 +136,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int } else { throw new InvalidOptionException('Either specify a user_id or --all-users'); } + return self::SUCCESS; } diff --git a/apps/files_trashbin/lib/Command/Size.php b/apps/files_trashbin/lib/Command/Size.php index 511caa98bbc98..af682fa2892aa 100644 --- a/apps/files_trashbin/lib/Command/Size.php +++ b/apps/files_trashbin/lib/Command/Size.php @@ -61,24 +61,27 @@ protected function execute(InputInterface $input, OutputInterface $output): int $user = $input->getOption('user'); $size = $input->getArgument('size'); - if ($size) { - $parsedSize = \OC_Helper::computerFileSize($size); - if ($parsedSize === false) { - $output->writeln("Failed to parse input size"); - return self::FAILURE; - } - if ($user) { - $this->config->setUserValue($user, 'files_trashbin', 'trashbin_size', (string)$parsedSize); - $this->commandBus->push(new Expire($user)); - } else { - $this->config->setAppValue('files_trashbin', 'trashbin_size', (string)$parsedSize); - $output->writeln("Warning: changing the default trashbin size will automatically trigger cleanup of existing trashbins,"); - $output->writeln("a users trashbin can exceed the configured size until they move a new file to the trashbin."); - } - } else { + if (!$size) { $this->printTrashbinSize($input, $output, $user); + return self::SUCCESS; + } + + $parsedSize = \OC_Helper::computerFileSize($size); + if ($parsedSize === false) { + $output->writeln("Failed to parse input size"); + return self::FAILURE; } + if ($user) { + $this->config->setUserValue($user, 'files_trashbin', 'trashbin_size', (string)$parsedSize); + $this->commandBus->push(new Expire($user)); + return self::SUCCESS; + } + + $this->config->setAppValue('files_trashbin', 'trashbin_size', (string)$parsedSize); + $output->writeln("Warning: changing the default trashbin size will automatically trigger cleanup of existing trashbins,"); + $output->writeln("a users trashbin can exceed the configured size until they move a new file to the trashbin."); + return self::SUCCESS; } @@ -101,40 +104,45 @@ private function printTrashbinSize(InputInterface $input, OutputInterface $outpu if ($input->getOption('output') == self::OUTPUT_FORMAT_PLAIN) { $output->writeln($userHumanSize); - } else { - $userValue = ($userSize < 0) ? 'default' : $userSize; - $globalValue = ($globalSize < 0) ? 'default' : $globalSize; - $this->writeArrayInOutputFormat($input, $output, [ - 'user_size' => $userValue, - 'global_size' => $globalValue, - 'effective_size' => ($userSize < 0) ? $globalValue : $userValue, - ]); + return; } - } else { - $users = []; - $this->userManager->callForSeenUsers(function (IUser $user) use (&$users) { - $users[] = $user->getUID(); - }); - $userValues = $this->config->getUserValueForUsers('files_trashbin', 'trashbin_size', $users); - if ($input->getOption('output') == self::OUTPUT_FORMAT_PLAIN) { - $output->writeln("Default size: $globalHumanSize"); - $output->writeln(""); - if (count($userValues)) { - $output->writeln("Per-user sizes:"); - $this->writeArrayInOutputFormat($input, $output, array_map(function ($size) { - return \OC_Helper::humanFileSize($size); - }, $userValues)); - } else { - $output->writeln("No per-user sizes configured"); - } - } else { - $globalValue = ($globalSize < 0) ? 'default' : $globalSize; - $this->writeArrayInOutputFormat($input, $output, [ - 'global_size' => $globalValue, - 'user_sizes' => $userValues, - ]); + $userValue = ($userSize < 0) ? 'default' : $userSize; + $globalValue = ($globalSize < 0) ? 'default' : $globalSize; + $this->writeArrayInOutputFormat($input, $output, [ + 'user_size' => $userValue, + 'global_size' => $globalValue, + 'effective_size' => ($userSize < 0) ? $globalValue : $userValue, + ]); + return; + } + + $users = []; + $this->userManager->callForSeenUsers(function (IUser $user) use (&$users) { + $users[] = $user->getUID(); + }); + $userValues = $this->config->getUserValueForUsers('files_trashbin', 'trashbin_size', $users); + + if ($input->getOption('output') == self::OUTPUT_FORMAT_PLAIN) { + $output->writeln("Default size: $globalHumanSize"); + $output->writeln(""); + + if (!count($userValues)) { + $output->writeln("No per-user sizes configured"); + return; } + + $output->writeln("Per-user sizes:"); + $this->writeArrayInOutputFormat($input, $output, array_map(function ($size) { + return \OC_Helper::humanFileSize($size); + }, $userValues)); + return; } + + $globalValue = ($globalSize < 0) ? 'default' : $globalSize; + $this->writeArrayInOutputFormat($input, $output, [ + 'global_size' => $globalValue, + 'user_sizes' => $userValues, + ]); } }