-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
refactor: Commands and background jobs for the trashbin #54876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use strong type in the constructor? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,14 @@ | |
| namespace OCA\Files_Trashbin\Command; | ||
|
|
||
| use OC\Command\FileAccess; | ||
| use OC\Files\SetupManager; | ||
| use OCA\Files_Trashbin\Trashbin; | ||
| use OCP\Command\ICommand; | ||
| use OCP\Files\Folder; | ||
| use OCP\Files\IRootFolder; | ||
| use OCP\Files\NotFoundException; | ||
| use OCP\Files\NotPermittedException; | ||
| use OCP\IUser; | ||
| use OCP\IUserManager; | ||
| use OCP\Server; | ||
|
|
||
|
|
@@ -26,14 +32,35 @@ public function __construct( | |
|
|
||
| public function handle() { | ||
| $userManager = Server::get(IUserManager::class); | ||
| if (!$userManager->userExists($this->user)) { | ||
| $userObject = $userManager->get($this->user); | ||
| if (!$userObject) { | ||
| // User has been deleted already | ||
| return; | ||
| } | ||
|
|
||
| \OC_Util::tearDownFS(); | ||
| \OC_Util::setupFS($this->user); | ||
| Trashbin::expire($this->user); | ||
| \OC_Util::tearDownFS(); | ||
| $rootFolder = $this->getTrashRoot($userObject); | ||
| if (!$rootFolder) { | ||
| return; | ||
| } | ||
|
|
||
| Trashbin::expire($rootFolder, $userObject); | ||
| $setupManager = Server::get(SetupManager::class); | ||
| $setupManager->tearDown(); | ||
| } | ||
|
|
||
| protected function getTrashRoot(IUser $user): ?Folder { | ||
| $setupManager = Server::get(SetupManager::class); | ||
| $rootFolder = Server::get(IRootFolder::class); | ||
|
Comment on lines
+52
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we use DI? |
||
| $setupManager->tearDown(); | ||
| $setupManager->setupForUser($user); | ||
|
|
||
| try { | ||
| /** @var Folder $folder */ | ||
| $folder = $rootFolder->getUserFolder($user->getUID())->getParent()->get('files_trashbin'); | ||
| return $folder; | ||
| } catch (NotFoundException|NotPermittedException) { | ||
| $setupManager->tearDown(); | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,33 +7,36 @@ | |||||
| */ | ||||||
| namespace OCA\Files_Trashbin\Command; | ||||||
|
|
||||||
| use OC\Files\View; | ||||||
| use OC\Core\Command\Base; | ||||||
| use OC\Files\SetupManager; | ||||||
| use OCA\Files_Trashbin\Expiration; | ||||||
| use OCA\Files_Trashbin\Trashbin; | ||||||
| use OCP\Files\Folder; | ||||||
| use OCP\Files\IRootFolder; | ||||||
| use OCP\Files\NotFoundException; | ||||||
| use OCP\Files\NotPermittedException; | ||||||
| use OCP\IUser; | ||||||
| use OCP\IUserManager; | ||||||
| use Psr\Log\LoggerInterface; | ||||||
| use Symfony\Component\Console\Command\Command; | ||||||
| use Symfony\Component\Console\Helper\ProgressBar; | ||||||
| use Symfony\Component\Console\Input\InputArgument; | ||||||
| use Symfony\Component\Console\Input\InputInterface; | ||||||
| use Symfony\Component\Console\Output\OutputInterface; | ||||||
|
|
||||||
| class ExpireTrash extends Command { | ||||||
| class ExpireTrash extends Base { | ||||||
|
|
||||||
| /** | ||||||
| * @param IUserManager|null $userManager | ||||||
| * @param Expiration|null $expiration | ||||||
| */ | ||||||
| public function __construct( | ||||||
| private LoggerInterface $logger, | ||||||
| private ?IUserManager $userManager = null, | ||||||
| private ?Expiration $expiration = null, | ||||||
| readonly private LoggerInterface $logger, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Same for the others |
||||||
| readonly private ?IUserManager $userManager, | ||||||
| readonly private ?Expiration $expiration, | ||||||
| readonly private SetupManager $setupManager, | ||||||
| readonly private IRootFolder $rootFolder, | ||||||
| ) { | ||||||
| parent::__construct(); | ||||||
| } | ||||||
|
|
||||||
| protected function configure() { | ||||||
| parent::configure(); | ||||||
| $this | ||||||
| ->setName('trashbin:expire') | ||||||
| ->setDescription('Expires the users trashbin') | ||||||
|
|
@@ -81,31 +84,26 @@ protected function execute(InputInterface $input, OutputInterface $output): int | |||||
|
|
||||||
| public function expireTrashForUser(IUser $user) { | ||||||
| try { | ||||||
| $uid = $user->getUID(); | ||||||
| if (!$this->setupFS($uid)) { | ||||||
| $trashRoot = $this->getTrashRoot($user); | ||||||
| if (!$trashRoot) { | ||||||
| return; | ||||||
| } | ||||||
| Trashbin::expire($uid); | ||||||
| Trashbin::expire($trashRoot, $user); | ||||||
| } catch (\Throwable $e) { | ||||||
| $this->logger->error('Error while expiring trashbin for user ' . $user->getUID(), ['exception' => $e]); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Act on behalf on trash item owner | ||||||
| * @param string $user | ||||||
| * @return boolean | ||||||
| */ | ||||||
| protected function setupFS($user) { | ||||||
| \OC_Util::tearDownFS(); | ||||||
| \OC_Util::setupFS($user); | ||||||
| protected function getTrashRoot(IUser $user): ?Folder { | ||||||
| $this->setupManager->tearDown(); | ||||||
| $this->setupManager->setupForUser($user); | ||||||
|
|
||||||
| // Check if this user has a trashbin directory | ||||||
| $view = new View('/' . $user); | ||||||
| if (!$view->is_dir('/files_trashbin/files')) { | ||||||
| return false; | ||||||
| try { | ||||||
| /** @var Folder $folder */ | ||||||
| $folder = $this->rootFolder->getUserFolder($user->getUID())->getParent()->get('files_trashbin'); | ||||||
| return $folder; | ||||||
| } catch (NotFoundException|NotPermittedException) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it be caught? If you let it bubble up it will get logged. But I’m not sure whether it should get logged. NotFound would just mean the user trashbin was not used yes. |
||||||
| return null; | ||||||
| } | ||||||
|
|
||||||
| return true; | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather rename
$usersand$userto$userIdsand$userIdas it would be even less confusing. But that's a nitpick.