Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@clxmstaab clxmstaab commented Nov 6, 2019 •

edited

Loading

with this change we get a proper exception stacktrace on the CLI.

without this change a Exception thrown while bootstrapping will just emit its message, e.g.

$ php ../phpstan-src/bin/phpstan analyse -c app/partner/phpstan.neon.dist
Required package "complex/rocket" is not installed: cannot detect its version

after this changes we get a proper stacktrace, like one would expect who is used to symfony-console applications, which means depending on the use of -v parameter you get more details about the actual error:

$ php ../phpstan-src/bin/phpstan analyse -c app/partner/phpstan.neon.dist
Required package "complex/rocket" is not installed: cannot detect its version
In CommandHelper.php line 306:
 [PHPStan\Command\InceptionNotSuccessfulException]
analyse [--paths-file PATHS-FILE] [-c|--configuration CONFIGURATION] [-l|--level LEVEL] [--no-progress] [--debug] [-a|--autoload-file AUTOLOAD-FILE] [--error-format ERROR-FORMAT] [--memory-limit MEMORY-LIMIT] [
--xdebug] [--] [<paths>...]

adding -v:

$ php ../phpstan-src/bin/phpstan analyse -v -c app/partner/phpstan.neon.dist
Required package "complex/rocket" is not installed: cannot detect its version
In CommandHelper.php line 306:
 [PHPStan\Command\InceptionNotSuccessfulException]
Exception trace:
 at C:\dvl\Zend\DefaultWorkspace13\phpstan-src\src\Command\CommandHelper.php:306
 PHPStan\Command\CommandHelper::begin() at C:\dvl\Zend\DefaultWorkspace13\phpstan-src\src\Command\AnalyseCommand.php:89
 PHPStan\Command\AnalyseCommand->execute() at C:\dvl\Zend\DefaultWorkspace13\phpstan-src\vendor\symfony\console\Command\Command.php:255
 Symfony\Component\Console\Command\Command->run() at C:\dvl\Zend\DefaultWorkspace13\phpstan-src\vendor\symfony\console\Application.php:934
 Symfony\Component\Console\Application->doRunCommand() at C:\dvl\Zend\DefaultWorkspace13\phpstan-src\vendor\symfony\console\Application.php:273
 Symfony\Component\Console\Application->doRun() at C:\dvl\Zend\DefaultWorkspace13\phpstan-src\vendor\symfony\console\Application.php:149
 Symfony\Component\Console\Application->run() at C:\dvl\Zend\DefaultWorkspace13\phpstan-src\bin\phpstan:60
 {closure}() at C:\dvl\Zend\DefaultWorkspace13\phpstan-src\bin\phpstan:61
analyse [--paths-file PATHS-FILE] [-c|--configuration CONFIGURATION] [-l|--level LEVEL] [--no-progress] [--debug] [-a|--autoload-file AUTOLOAD-FILE] [--error-format ERROR-FORMAT] [--memory-limit MEMORY-LIMIT] [
--xdebug] [--] [<paths>...]

DX wise this is a major improvement and saves a lot of time while debugging the cause of a error (without the need to install/activate xdebug)

clxmstaab added 2 commits

November 6, 2019 09:51


 Let exceptions fly while bootstrapping

854f2b2

that way we get a proper exception stacktrace on the CLI


 Adding the exception-cause to bootstrapping errors

f68f38c

Copy link

Contributor Author

clxmstaab commented Nov 6, 2019 •

edited

Loading

with the 2nd commit we actually give the exception-hanlder the information to tell us about the cause of the exception:

$ php ../phpstan-src/bin/phpstan analyse -c app/partner/phpstan.neon.dist
Required package "complex/rocket" is not installed: cannot detect its version
In CommandHelper.php line 306:
 Required package "complex/rocket" is not installed: cannot detect its version
In Versions.php line 103:
 Required package "complex/rocket" is not installed: cannot detect its version
analyse [--paths-file PATHS-FILE] [-c|--configuration CONFIGURATION] [-l|--level LEVEL] [--no-progress] [--debug] [-a|--autoload-file AUTOLOAD-FILE] [--error-format ERROR-FORMAT] [--memory-limit MEMORY-LIMIT] [
--xdebug] [--] [<paths>...]

clxmstaab

clxmstaab commented

src/Command/AnalyseCommand.php

$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

clxmstaab

clxmstaab commented

src/Command/CommandHelper.php Outdated

require_once $file;

})($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

@staabm staabm mentioned this pull request

provide stacktrace on fatal errors phpstan/phpstan#2210

Closed

ondrejmirtes

ondrejmirtes requested changes

src/Command/AnalyseCommand.php

$level,

$allowXdebug

);

} catch (\PHPStan\Command\InceptionNotSuccessfulException $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.

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

src/Command/CommandHelper.php Outdated

require_once $file;

})($bootstrapFile);

} catch (\Throwable $e) {

$errorOutput->writeln($e->getMessage());

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?

src/Command/AnalyseCommand.php

$level,

$allowXdebug

);

} catch (\PHPStan\Command\InceptionNotSuccessfulException $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.

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

clxmstaab added 4 commits

November 6, 2019 12:04


 Revert "Let exceptions fly while bootstrapping"

252c458

This reverts commit 854f2b2.


 Revert "Adding the exception-cause to bootstrapping errors"

eb02a32

This reverts commit f68f38c.


 use Symfony\Component\Console\Application::renderException()

baa885c


 fix missing arg

84e8154

ondrejmirtes

ondrejmirtes requested changes