Skip to content

[Console] Allow Application to handle PHP 7 Errors #20808

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 2 commits into from
Closed

[Console] Allow Application to handle PHP 7 Errors #20808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64f9856
[Console] Allow Application to handle PHP 7 Errors
ogizanagi Dec 7, 2016
11bd596
Use FatalThrowableError
ogizanagi Dec 14, 2016
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
28 changes: 24 additions & 4 deletions src/Symfony/Component/Console/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class Application
private $name;
private $version;
private $catchExceptions = true;
private $catchErrors = false;
private $autoExit = true;
private $definition;
private $helperSet;
Expand Down Expand Up @@ -100,8 +101,6 @@ public function setDispatcher(EventDispatcherInterface $dispatcher)
* @param OutputInterface $output An Output instance
*
* @return int 0 if everything went fine, or an error code
*
* @throws \Exception When doRun returns Exception
*/
public function run(InputInterface $input = null, OutputInterface $output = null)
{
Expand All @@ -124,7 +123,14 @@ public function run(InputInterface $input = null, OutputInterface $output = null
if (!$this->catchExceptions) {
throw $e;
}
} catch (\Error $e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn’t this be \Throwable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using Throwable would make the catchErrors flag catch both exceptions and errors, where there already is a dedicated flag for exceptions.
So, if we keep both flags (and I still think we need it currently, as I tried to explain in my previous comments, unless we find a better alternative :/ ), it should probably remains \Error here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed!

if (!$this->catchErrors) {
throw $e;
}
$e = new FatalThrowableError($e);
}

if (isset($e)) {
if ($output instanceof ConsoleOutputInterface) {
$this->renderException($e, $output->getErrorOutput());
} else {
Expand Down Expand Up @@ -271,6 +277,22 @@ public function setCatchExceptions($boolean)
$this->catchExceptions = (bool) $boolean;
}

/**
* @return bool Whether errors are caught or not during commands execution
*/
public function areErrorsCaught()
{
return $this->catchErrors;
}

/**
* @param bool $boolean Whether to catch errors or not during commands execution
*/
public function setCatchErrors($boolean)
{
$this->catchErrors = (bool) $boolean;
}
Copy link
Member

Choose a reason for hiding this comment

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

The addition of these 2 methods does not seem related to this PR and should be removed.

Copy link
Contributor Author

@ogizanagi ogizanagi Dec 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

  • I doubt about the usefulness of areErrorsCaught right now, but this echoes areExceptionsCaught.
  • setCatchErrors is the way to tell the application instance to catch php 7 errors and cannot be removed (reasons below).

IMHO, we still have to differentiate exceptions from errors, as the last ones are currently handled in the full stack framework by the handler registered through the Debug component and the DebugHandlersListener from HttpKernel, whereas classic exceptions are handled by the Console component itself. Thus, relying on catchExceptions would change the behavior using the full stack, which can be annoying and considered a BC break. The handler used in full stack does extra work on the exception. For instance:

Using full stack:

[Symfony\Component\Debug\Exception\UndefinedMethodException]
Attempted to call an undefined method named "Option" of class "Symfony\Component\Console\Input\ArgvInput".
Did you mean to call e.g. "getOption", "getOptions", "getParameterOption", "hasOption", "hasParameterOption" or "setOption"?

versus the console component standalone, with the catchErrors flag:

[Error]
Call to undefined method Symfony\Component\Console\Input\ArgvInput::Option()

(stack traces are the same)

Thus the second catchErrors flag, otherwise you'll get the last sample using the full stack when an error occurred.

Also see #19813 (comment)


/**
* Gets whether to automatically exit after a command execution or not.
*
Expand Down Expand Up @@ -814,8 +836,6 @@ protected function configureIO(InputInterface $input, OutputInterface $output)
* @param OutputInterface $output An Output instance
*
* @return int 0 if everything went fine, or an error code
*
* @throws \Exception when the command being run threw an exception
*/
protected function doRunCommand(Command $command, InputInterface $input, OutputInterface $output)
{
Expand Down
33 changes: 33 additions & 0 deletions src/Symfony/Component/Console/Tests/ApplicationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,6 +1000,39 @@ public function testRunWithErrorFailingStatusCode()
$this->assertSame(1, $tester->getStatusCode(), 'Status code should be 1');
}

public function testErrorsAreNotCaughtByDefault()
{
$application = new Application();

$this->assertFalse($application->areErrorsCaught());
}

/**
* @requires PHP 7
*/
public function testCatchesErrors()
{
$application = new Application();

$application->setCatchErrors(true);
$application->setAutoExit(false);

$this->assertTrue($application->areErrorsCaught());

$application->register('foo')->setCode(function () {
throw new \Error('This error should be caught by Application::run');
});

$tester = new ApplicationTester($application);
$tester->run(array('command' => 'foo'));
$this->assertSame(1, $tester->getStatusCode(), 'Status code should be 1');
$this->assertContains(<<<'EOTXT'
[Symfony\Component\Debug\Exception\FatalThrowableError]
This error should be caught by Application::run
EOTXT
, $tester->getDisplay(true), 'The PHP error should be caught when catchErrors is true.');
}

public function testRunWithDispatcherSkippingCommand()
{
$application = new Application();
Expand Down