[stable9] Log files:scan exception #27136

Merged
merged 1 commit into from Feb 21, 2017

Conversation

Projects
None yet
3 participants
@PVince81
Member

PVince81 commented Feb 10, 2017

Description

Log files:scan exception

Related Issue

Motivation and Context

Because else it looks like all was fine but the scanner actually aborted and the summary was still displayed.
The exception could be useful to debug the actual issue.

How Has This Been Tested?

Manually threw an exception from within the scanner to see that it's displayed properly.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Forward ports

  • stable9.1
  • master

@jvillafanez

@PVince81 PVince81 added this to the 9.0.9 milestone Feb 10, 2017

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 10, 2017

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mmattel, @bartv2 and @DeepDiver1975 to be potential reviewers.

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mmattel, @bartv2 and @DeepDiver1975 to be potential reviewers.

apps/files/command/scan.php
@@ -136,6 +136,9 @@ protected function scanFiles($user, $path, $verbose, OutputInterface $output) {
$output->writeln("<error>Home storage for user $user not writable</error>");
$output->writeln("Make sure you're running the scan command only as the user the web server runs as");
} catch (\Exception $e) {
+ if ($e->getMessage() !== 'ctrl-c') {

This comment has been minimized.

@jvillafanez

jvillafanez Feb 16, 2017

Member

Any other better method? This is ugly... It's unlikely that someone throws any exception with that exact message, but if it happens it will be messy to debug because this is the typical things that everyone forgets.

@DeepDiver1975 is it acceptable to create a custom exception in this file that will be thrown and catch only here? That should provide the isolation we want for this case.

@jvillafanez

jvillafanez Feb 16, 2017

Member

Any other better method? This is ugly... It's unlikely that someone throws any exception with that exact message, but if it happens it will be messy to debug because this is the typical things that everyone forgets.

@DeepDiver1975 is it acceptable to create a custom exception in this file that will be thrown and catch only here? That should provide the isolation we want for this case.

This comment has been minimized.

@PVince81

PVince81 Feb 16, 2017

Member

We could introduce an InterruptedException for "ctrl-c" and catch it separately.
Would that be acceptable ?

@PVince81

PVince81 Feb 16, 2017

Member

We could introduce an InterruptedException for "ctrl-c" and catch it separately.
Would that be acceptable ?

This comment has been minimized.

@jvillafanez

jvillafanez Feb 16, 2017

Member

Looks like nested classes aren't supported (they would fit here well), and anonymous classes are supported only in PHP7+ (they might fit)

The proposed option would add the custom exception here, in the same namespace. The scan.php file would be something like

namespace OCA\Files\Command;

class CustomException extends \Exception {}
class Scan extends Base {
...
}

The scan class would throw and catch the CustomException at any moment.

The autoloader won't fetch the CustomException, which should remain hidden for the rest of the system unless it's explicitly fetched. In the case another CustomException is available (in a CustomException.php file), the rest of the system will use the "public" one instead of this one.
There is still a possible name collision inside the file in this case if the Scan class wants to use the "public" CustomException, and I'm not sure if a simple alias would resolve the problem, but if this ever happens we could change the name of the internal one.

@jvillafanez

jvillafanez Feb 16, 2017

Member

Looks like nested classes aren't supported (they would fit here well), and anonymous classes are supported only in PHP7+ (they might fit)

The proposed option would add the custom exception here, in the same namespace. The scan.php file would be something like

namespace OCA\Files\Command;

class CustomException extends \Exception {}
class Scan extends Base {
...
}

The scan class would throw and catch the CustomException at any moment.

The autoloader won't fetch the CustomException, which should remain hidden for the rest of the system unless it's explicitly fetched. In the case another CustomException is available (in a CustomException.php file), the rest of the system will use the "public" one instead of this one.
There is still a possible name collision inside the file in this case if the Scan class wants to use the "public" CustomException, and I'm not sure if a simple alias would resolve the problem, but if this ever happens we could change the name of the internal one.

This comment has been minimized.

@PVince81

PVince81 Feb 16, 2017

Member

I'd rather introduce a new proper InterruptedException in a more general namespace. We could use this for any kind of commands in the future, not only the scanner.

@PVince81

PVince81 Feb 16, 2017

Member

I'd rather introduce a new proper InterruptedException in a more general namespace. We could use this for any kind of commands in the future, not only the scanner.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 21, 2017

Member

Added InterruptedException, @jvillafanez does this look better ?

Member

PVince81 commented Feb 21, 2017

Added InterruptedException, @jvillafanez does this look better ?

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Feb 21, 2017

Member

It's looks better, but I don't find the OC\Core\Command\InterruptedException

Member

jvillafanez commented Feb 21, 2017

It's looks better, but I don't find the OC\Core\Command\InterruptedException

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 21, 2017

Member

It's right here in my git status, forgot to add it...

Added now

Member

PVince81 commented Feb 21, 2017

It's right here in my git status, forgot to add it...

Added now

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
Member

jvillafanez commented Feb 21, 2017

👍

@PVince81 PVince81 merged commit a95aaf1 into stable9 Feb 21, 2017

2 of 3 checks passed

continuous-integration/jenkins/pr-head This commit is being built
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the stable9-logscanexception branch Feb 21, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment