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
wants to merge 6 commits into from
Closed

Let exceptions fly while bootstrapping #3

Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion src/Command/AnalyseCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$autoloadFile,
$configuration,
$level,
$allowXdebug
$allowXdebug,
$this->getApplication()
);
} 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;
Expand Down
9 changes: 6 additions & 3 deletions src/Command/CommandHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use PHPStan\DependencyInjection\NeonAdapter;
use PHPStan\File\FileFinder;
use PHPStan\File\FileHelper;
use Symfony\Component\Console\Application;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\ConsoleOutputInterface;
use Symfony\Component\Console\Output\OutputInterface;
Expand All @@ -36,7 +37,8 @@ public static function begin(
?string $autoloadFile,
?string $projectConfigFile,
?string $level,
bool $allowXdebug
bool $allowXdebug,
Application $application
): InceptionResult
{
if (!$allowXdebug) {
Expand Down Expand Up @@ -129,7 +131,7 @@ public static function begin(
try {
$projectConfig = $loader->load($projectConfigFile, null);
} catch (\Nette\InvalidStateException | \Nette\FileNotFoundException $e) {
$errorOutput->writeln($e->getMessage());
$application->renderException($e);
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.

Why this change? The message is usually about which file wasn't found which is sufficient.

throw new \PHPStan\Command\InceptionNotSuccessfulException();
}
$defaultParameters = [
Expand Down Expand Up @@ -302,7 +304,8 @@ public static function begin(
require_once $file;
})($bootstrapFile);
} catch (\Throwable $e) {
$errorOutput->writeln($e->getMessage());
$application->renderException($e);

throw new \PHPStan\Command\InceptionNotSuccessfulException();
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/Command/DumpDependenciesCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$autoloadFile,
$configurationFile,
'0', // irrelevant but prevents an error when a config file is passed
$allowXdebug
$allowXdebug,
$this->getApplication()
);
} catch (\PHPStan\Command\InceptionNotSuccessfulException $e) {
return 1;
Expand Down