Skip to content

Let exceptions fly while bootstrapping #3

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.

Sign up for GitHub

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

Closed
clxmstaab wants to merge 6 commits into from
Closed

Let exceptions fly while bootstrapping #3

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
854f2b2
Let exceptions fly while bootstrapping
clxmstaab Nov 6, 2019
f68f38c
Adding the exception-cause to bootstrapping errors
clxmstaab Nov 6, 2019
252c458
Revert "Let exceptions fly while bootstrapping"
clxmstaab Nov 6, 2019
eb02a32
Revert "Adding the exception-cause to bootstrapping errors"
clxmstaab Nov 6, 2019
baa885c
use Symfony\Component\Console\Application::renderException()
clxmstaab Nov 6, 2019
84e8154
fix missing arg
clxmstaab Nov 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions src/Command/AnalyseCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,21 +77,17 @@ protected function execute(InputInterface $input, OutputInterface $output): int
throw new \PHPStan\ShouldNotHappenException();
}

try {
$inceptionResult = CommandHelper::begin(
$input,
$output,
$paths,
$pathsFile,
$memoryLimit,
$autoloadFile,
$configuration,
$level,
$allowXdebug
);
} catch (\PHPStan\Command\InceptionNotSuccessfulException $e) {
Copy link
Contributor Author

@clxmstaab clxmstaab Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

a Exception thrown within a command will automatically lead to a exit-code of 1 by symfony-console

Copy link
Member

@ondrejmirtes ondrejmirtes Nov 6, 2019

Choose a reason for hiding this comment

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

Hi, I disagree with this change. CommandHelper outputs the error message, InceptionNotSuccessfulException signals the AnalyseCommand to return the exit code 1.

Copy link
Member

@ondrejmirtes ondrejmirtes Nov 6, 2019

Choose a reason for hiding this comment

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

This change would lead to a really chaotic output in most cases which isn't needed.

Copy link
Contributor

@staabm staabm Nov 6, 2019

Choose a reason for hiding this comment

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

InceptionNotSuccessfulException signals the AnalyseCommand to return the exit code 1.

symfony will automatically use a exit-code of 1 when a un-handled exception occurs.
no need to do this here and interrupt the exception handling

Copy link
Contributor

@staabm staabm Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This change would lead to a really chaotic output in most cases which isn't needed.

IMO the cause of chaotic output is that the output is scattered accross CommandHelper and the actual commands. output should be written only in a few places in the upper stack

Copy link
Member

@ondrejmirtes ondrejmirtes Nov 6, 2019

Choose a reason for hiding this comment

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

But the problem is that I don't always want to output the thrown exception with stack trace. I want to present something more friendly to the user.

Copy link
Contributor Author

@clxmstaab clxmstaab Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

when letting the exception fly through we would get a error like (no stacktrace)

image

no stacktrace or similar involved.

you get the stacktrace etc. only when adding -v to the command:

image

return 1;
}
$inceptionResult = CommandHelper::begin(
$input,
$output,
$paths,
$pathsFile,
$memoryLimit,
$autoloadFile,
$configuration,
$level,
$allowXdebug
);

$errorOutput = $inceptionResult->getErrorOutput();
$errorFormat = $input->getOption('error-format');
Expand Down
2 changes: 1 addition & 1 deletion src/Command/CommandHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ public static function begin(
})($bootstrapFile);
} catch (\Throwable $e) {
$errorOutput->writeln($e->getMessage());
Copy link
Contributor Author

@clxmstaab clxmstaab Nov 6, 2019

Choose a reason for hiding this comment

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

IMO, in cases where we throw InceptionNotSuccessfulException with a root-cause exception provided, we should not print our the message manually but let symfony-console do its magic.

(this would apply to a few more places within this file)

Copy link
Member

@ondrejmirtes ondrejmirtes Nov 6, 2019

Choose a reason for hiding this comment

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

We could print out the stack trace in this place manually. The same way Symfony Console does. Can you investigate what function should we call to do that?

Copy link
Contributor

@staabm staabm Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

there is a Symfony\Component\Console\Application::renderException().. I will have a look

IMO having this kind of error handling logik on a command-level is wrong though

throw new \PHPStan\Command\InceptionNotSuccessfulException();
throw new \PHPStan\Command\InceptionNotSuccessfulException($e->getMessage(), 0, $e);
}
}

Expand Down