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

Do not stop at the first PHP error/warning in files:scan #37944

Merged
merged 3 commits into from
May 9, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Apr 27, 2023

Summary

The files:scan command sets a custom error handler to show PHP errors. But it stops at the first one, failing the whole scan for one malformed PNG file.
This PR changes that to instead show errors but continue.

Output:
Screenshot_20230427_104813

Checklist

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc added enhancement 3. to review Waiting for reviews labels Apr 27, 2023
@come-nc come-nc added this to the Nextcloud 27 milestone Apr 27, 2023
@come-nc come-nc self-assigned this Apr 27, 2023
@come-nc come-nc requested review from a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team April 27, 2023 08:52
@@ -236,31 +236,35 @@
/**
* Initialises some useful tools for the Command
*/
protected function initTools() {
protected function initTools(OutputInterface $output) {

Check notice

Code scanning / Psalm

MissingReturnType

Method OCA\Files\Command\Scan::initTools does not have a return type, expecting void
@solracsf
Copy link
Member

Is the full stack trace needed on screen?
Could it be logged (on file) and instead just display the exeption message?

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented May 2, 2023

Is the full stack trace needed on screen? Could it be logged (on file) and instead just display the exeption message?

The full stack trace was there on screen already, I did not change that.
But I now changed it so that the stacktrace only shows in very verbose mode, it makes more sense 👍

@szaimen
Copy link
Contributor

szaimen commented May 2, 2023

/backport to stable26

@szaimen
Copy link
Contributor

szaimen commented May 2, 2023

/backport to stable25

@icewind1991
Copy link
Member

icewind1991 commented May 2, 2023

Putting the full exception trace in the output isn't great imo. Probably better to only show the message in the output and log the rest.

Edit: missed the previous discussion

Additionally, summarizing the number of failures somewhere at the end would prevent errors from being unseen from scrolling of the screen with large verbose outputs.

@skjnldsv skjnldsv mentioned this pull request May 3, 2023
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented May 4, 2023

Additionally, summarizing the number of failures somewhere at the end would prevent errors from being unseen from scrolling of the screen with large verbose outputs.

Added an error counter.

This was referenced May 9, 2023
@come-nc come-nc merged commit fc07627 into master May 9, 2023
@come-nc come-nc deleted the enh/allow-filescan-to-continue-on-error branch May 9, 2023 15:52
@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin/stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: unhandled exception in ExifProvider crashes scan run
5 participants