Skip to content
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

feat: add occ command files:list #46352

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

yemkareems
Copy link
Contributor

@yemkareems yemkareems commented Jul 8, 2024

Duplicate pr of #43342 since CI CD issues are there in the original PR
Possible usage example with params
occ files:list admin --path abc --sort name --order ASC --minSize 100 --maxSize 1008400 --type application

Sample usage

image
occ files:list admin

if path is not mentioned list all files in the root directory of the user

image

occ files:list admin --path abc

if path is mentioned list all files in the path mentioned alone

image

occ files:list admin --sort name --order ASC

image

occ files:list alice --path Media --sort size --order DESC

image

occ files:list admin --type image

image

occ files:list admin --type application --minSize 2750 --maxSize 148415

  • Resolves: #

Summary

TODO

  • ...

Checklist

@yemkareems yemkareems added the 3. to review Waiting for reviews label Jul 8, 2024
@yemkareems yemkareems added this to the Nextcloud 30 milestone Jul 8, 2024
@yemkareems yemkareems self-assigned this Jul 8, 2024
apps/files/lib/Command/ListFiles.php Fixed Show fixed Hide fixed
apps/files/lib/Command/ListFiles.php Fixed Show fixed Hide fixed
apps/files/lib/Command/ListFiles.php Fixed Show fixed Hide fixed
apps/files/lib/Command/ListFiles.php Fixed Show fixed Hide fixed
@artonge artonge mentioned this pull request Jul 8, 2024
5 tasks
apps/files/lib/Command/ListFiles.php Outdated Show resolved Hide resolved
apps/files/lib/Command/ListFiles.php Outdated Show resolved Hide resolved
apps/files/lib/Command/ListFiles.php Show resolved Hide resolved
apps/files/lib/Command/ListFiles.php Outdated Show resolved Hide resolved
@yemkareems yemkareems force-pushed the feature/files-list-occ-command branch from 60c5d0c to 86dfbfc Compare July 8, 2024 11:07
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure I understand the reasoning behind the path handling.

Would it not be better to have a user argument and a path argument inside this user folder?
Here it forces a path starting with a username, that feels unexpected. Did you try it with groupfolders and such?

apps/files/lib/Command/ListFiles.php Show resolved Hide resolved
apps/files/lib/Command/ListFiles.php Outdated Show resolved Hide resolved
apps/files/lib/Command/ListFiles.php Outdated Show resolved Hide resolved
apps/files/lib/Command/ListFiles.php Outdated Show resolved Hide resolved
apps/files/lib/Command/ListFiles.php Outdated Show resolved Hide resolved
apps/files/lib/Command/ListFiles.php Outdated Show resolved Hide resolved
@yemkareems
Copy link
Contributor Author

I’m not sure I understand the reasoning behind the path handling.

Would it not be better to have a user argument and a path argument inside this user folder? Here it forces a path starting with a username, that feels unexpected. Did you try it with groupfolders and such?

@come-nc this was implemented similar to files:scan which had both user_id and path. Since user was extracted from the path it was omitted and path was made a required argument

@szaimen szaimen removed their request for review July 9, 2024 10:23
@come-nc
Copy link
Contributor

come-nc commented Jul 9, 2024

I’m not sure I understand the reasoning behind the path handling.
Would it not be better to have a user argument and a path argument inside this user folder? Here it forces a path starting with a username, that feels unexpected. Did you try it with groupfolders and such?

@come-nc this was implemented similar to files:scan which had both user_id and path. Since user was extracted from the path it was omitted and path was made a required argument

This is not the same thing, files:scan scans the disk folders so it’s limited to paths on the disk, but I expect files:list is expected to list files as in the UI, no? Otherwise people can use ls.

After discussing with the team:
I think we should go with having two arguments, userid and path.
Then you call getUserFolder($userId) on the root folder, and you can use get($path) on the userfolder instance.

@yemkareems
Copy link
Contributor Author

path

I’m not sure I understand the reasoning behind the path handling.
Would it not be better to have a user argument and a path argument inside this user folder? Here it forces a path starting with a username, that feels unexpected. Did you try it with groupfolders and such?

@come-nc this was implemented similar to files:scan which had both user_id and path. Since user was extracted from the path it was omitted and path was made a required argument

This is not the same thing, files:scan scans the disk folders so it’s limited to paths on the disk, but I expect files:list is expected to list files as in the UI, no? Otherwise people can use ls.

After discussing with the team: I think we should go with having two arguments, userid and path. Then you call getUserFolder($userId) on the root folder, and you can use get($path) on the userfolder instance.

Did it as suggested. Now user is a mandatory argument and path may or may not be present. Used getUserFolder in place of get and then get is used on top of getUserFolder instance

@yemkareems yemkareems force-pushed the feature/files-list-occ-command branch 2 times, most recently from b934eac to be1f059 Compare July 29, 2024 07:57
try {
$userFolder = $this->rootFolder->getUserFolder($user);
/** @var Folder $pathList **/
$pathList = $userFolder->get('/' . $path);

Check notice

Code scanning / Psalm

PossiblyNullOperand Note

Cannot concatenate with a possibly null null|string
@yemkareems yemkareems force-pushed the feature/files-list-occ-command branch from be1f059 to 363c391 Compare July 29, 2024 08:11
@yemkareems yemkareems requested a review from come-nc July 29, 2024 09:34
@yemkareems yemkareems force-pushed the feature/files-list-occ-command branch 3 times, most recently from 8267c6d to 8921fb8 Compare July 29, 2024 13:41
… converted, user check removed

Signed-off-by: yemkareems <[email protected]>
Signed-off-by: yemkareems <[email protected]>
… breaking the listing, inputPath check removed as path is mandatory, sorting done only when sort param is there, writeTableInOutputFormat done

Signed-off-by: yemkareems <[email protected]>
…directory. Add path to get the list based on the path or else list the user folder.

Signed-off-by: yemkareems <[email protected]>
Signed-off-by: yemkareems <[email protected]>
Signed-off-by: yemkareems <[email protected]>
@yemkareems yemkareems force-pushed the feature/files-list-occ-command branch from 8921fb8 to 2dd5854 Compare July 30, 2024 04:31
@blizzz blizzz mentioned this pull request Jul 30, 2024
}
} catch (ForbiddenException $e) {
$output->writeln(
"<error>Home storage for user $user not writable or 'files' subdirectory missing</error>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"<error>Home storage for user $user not writable or 'files' subdirectory missing</error>"
"<error>Home storage for user $user not readable or 'files' subdirectory missing</error>"

protected function listFiles(
string $user,
OutputInterface $output,
?string $path = "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
?string $path = "",
string $path = "",

Comment on lines +144 to +149
} catch (\Exception $e) {
$output->writeln(
"<error>Exception during list: " . $e->getMessage() . "</error>"
);
$output->writeln("<error>" . $e->getTraceAsString() . "</error>");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} catch (\Exception $e) {
$output->writeln(
"<error>Exception during list: " . $e->getMessage() . "</error>"
);
$output->writeln("<error>" . $e->getTraceAsString() . "</error>");
}
}

You can simply let unexpected exceptions bubble up, parent class takes care of showing the trace depending on verbosity if I recall correctly.

Comment on lines +183 to +235
/**
* Initialises some useful tools for the Command
*/
protected function initTools(OutputInterface $output): void {
// Convert PHP errors to exceptions
set_error_handler(
fn (
int $severity,
string $message,
string $file,
int $line
): bool => $this->exceptionErrorHandler(
$output,
$severity,
$message,
$file,
$line
),
E_ALL
);
}

/**
* Processes PHP errors in order to be able to show them in the output
*
* @see https://www.php.net/manual/en/function.set-error-handler.php
*
* @param int $severity the level of the error raised
* @param string $message
* @param string $file the filename that the error was raised in
* @param int $line the line number the error was raised
*/
public function exceptionErrorHandler(
OutputInterface $output,
int $severity,
string $message,
string $file,
int $line
): bool {
if ($severity === E_DEPRECATED || $severity === E_USER_DEPRECATED) {
// Do not show deprecation warnings
return false;
}
$e = new \ErrorException($message, 0, $severity, $file, $line);
$output->writeln(
"<error>Error during list: " . $e->getMessage() . "</error>"
);
$output->writeln(
"<error>" . $e->getTraceAsString() . "</error>",
OutputInterface::VERBOSITY_VERY_VERBOSE
);
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why all this?
This is the only command that do that to my knowledge.
Nextcloud already has an exception handler which logs them I think.

I suggest to remove this.

@blizzz blizzz mentioned this pull request Aug 1, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
@skjnldsv skjnldsv marked this pull request as draft November 15, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants