Skip to content

[Console] Fix dispatching throwables from ConsoleEvents::COMMAND #22435

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

Merged
merged 1 commit into from
Apr 25, 2017
Merged

[Console] Fix dispatching throwables from ConsoleEvents::COMMAND #22435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
62 changes: 32 additions & 30 deletions src/Symfony/Component/Console/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,17 @@ public function run(InputInterface $input = null, OutputInterface $output = null
$this->configureIO($input, $output);

try {
$e = null;
$exitCode = $this->doRun($input, $output);
} catch (\Exception $e) {
} catch (\Exception $x) {
$e = $x;
} catch (\Throwable $x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Casting from Error (it can't be anything else) to Exception here is a behaviour change - applications that use console will find their fatal error handlers are never triggered.

Copy link
Member

Choose a reason for hiding this comment

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

It was already the case (https://github.com/symfony/symfony/pull/22435/files/6d6b04ae977d322213a4ac1bd1c84bfd32313611#diff-38eebdda7d6ebfcb7405b91167f05c9cL860).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, then something else in this commit is causing the behaviour change in PhpSpec reported here #22678

Copy link
Contributor

@ogizanagi ogizanagi May 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The original error was re-thrown before (the \Throwable, not FatalThrowableError): https://github.com/symfony/symfony/pull/22435/files/6d6b04ae977d322213a4ac1bd1c84bfd32313611#diff-38eebdda7d6ebfcb7405b91167f05c9cL874

Actually, being more accurate, the original error was re-thrown if you used an event dispatcher (and the event listeners don't set another exception), as I mentioned in this issue and this pr

This exception and error handling is hell and inconsistent, making it so hard to keep BC by trying to handle exception and errors the same way. I still believe two different flags for exceptions & errors would have saved us from some headaches. 😅

$e = new FatalThrowableError($x);
}

if (null !== $e) {
if (!$this->catchExceptions) {
throw $e;
throw $x;
}

if ($output instanceof ConsoleOutputInterface) {
Expand Down Expand Up @@ -182,6 +189,7 @@ public function doRun(InputInterface $input, OutputInterface $output)
$input = new ArrayInput(array('command' => $this->defaultCommand));
}

$this->runningCommand = null;
// the command name MUST be the first element of the input
$command = $this->find($name);

Expand Down Expand Up @@ -839,47 +847,41 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
}

if (null === $this->dispatcher) {
try {
return $command->run($input, $output);
} catch (\Exception $e) {
throw $e;
} catch (\Throwable $e) {
throw new FatalThrowableError($e);
}
return $command->run($input, $output);
}

$event = new ConsoleCommandEvent($command, $input, $output);
$this->dispatcher->dispatch(ConsoleEvents::COMMAND, $event);
$e = null;

try {
$this->dispatcher->dispatch(ConsoleEvents::COMMAND, $event);

if ($event->commandShouldRun()) {
try {
$e = null;
if ($event->commandShouldRun()) {
$exitCode = $command->run($input, $output);
} catch (\Exception $x) {
$e = $x;
} catch (\Throwable $x) {
$e = new FatalThrowableError($x);
} else {
$exitCode = ConsoleCommandEvent::RETURN_CODE_DISABLED;
}
if (null !== $e) {
$event = new ConsoleExceptionEvent($command, $input, $output, $e, $e->getCode());
$this->dispatcher->dispatch(ConsoleEvents::EXCEPTION, $event);

if ($e !== $event->getException()) {
$x = $e = $event->getException();
}

$event = new ConsoleTerminateEvent($command, $input, $output, $e->getCode());
$this->dispatcher->dispatch(ConsoleEvents::TERMINATE, $event);
} catch (\Exception $e) {
} catch (\Throwable $e) {
}
if (null !== $e) {
$x = $e instanceof \Exception ? $e : new FatalThrowableError($e);
$event = new ConsoleExceptionEvent($command, $input, $output, $x, $x->getCode());
$this->dispatcher->dispatch(ConsoleEvents::EXCEPTION, $event);

throw $x;
if ($x !== $event->getException()) {
$e = $event->getException();
}
} else {
$exitCode = ConsoleCommandEvent::RETURN_CODE_DISABLED;
$exitCode = $e->getCode();
}

$event = new ConsoleTerminateEvent($command, $input, $output, $exitCode);
$this->dispatcher->dispatch(ConsoleEvents::TERMINATE, $event);

if (null !== $e) {
throw $e;
}

return $event->getExitCode();
}

Expand Down
87 changes: 53 additions & 34 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 @@ -977,15 +977,28 @@ public function testRunDispatchesAllEventsWithException()
$this->assertContains('before.foo.caught.after.', $tester->getDisplay());
}

public function testRunWithError()
public function testRunDispatchesAllEventsWithExceptionInListener()
{
if (method_exists($this, 'expectException')) {
$this->expectException('Exception');
$this->expectExceptionMessage('dymerr');
} else {
$this->setExpectedException('Exception', 'dymerr');
}
$dispatcher = $this->getDispatcher();
$dispatcher->addListener('console.command', function () {
throw new \RuntimeException('foo');
});

$application = new Application();
$application->setDispatcher($dispatcher);
$application->setAutoExit(false);

$application->register('foo')->setCode(function (InputInterface $input, OutputInterface $output) {
$output->write('foo.');
});

$tester = new ApplicationTester($application);
$tester->run(array('command' => 'foo'));
$this->assertContains('before.caught.after.', $tester->getDisplay());
}

public function testRunWithError()
{
$application = new Application();
$application->setAutoExit(false);
$application->setCatchExceptions(false);
Expand All @@ -997,7 +1010,13 @@ public function testRunWithError()
});

$tester = new ApplicationTester($application);
$tester->run(array('command' => 'dym'));

try {
$tester->run(array('command' => 'dym'));
$this->fail('Error expected.');
} catch (\Error $e) {
$this->assertSame('dymerr', $e->getMessage());
}
}

/**
Expand Down Expand Up @@ -1087,32 +1106,6 @@ public function testTerminalDimensions()
$this->assertSame(array($width, 80), $application->getTerminalDimensions());
}

protected function getDispatcher($skipCommand = false)
{
$dispatcher = new EventDispatcher();
$dispatcher->addListener('console.command', function (ConsoleCommandEvent $event) use ($skipCommand) {
$event->getOutput()->write('before.');

if ($skipCommand) {
$event->disableCommand();
}
});
$dispatcher->addListener('console.terminate', function (ConsoleTerminateEvent $event) use ($skipCommand) {
$event->getOutput()->writeln('after.');

if (!$skipCommand) {
$event->setExitCode(113);
}
});
$dispatcher->addListener('console.exception', function (ConsoleExceptionEvent $event) {
$event->getOutput()->write('caught.');

$event->setException(new \LogicException('caught.', $event->getExitCode(), $event->getException()));
});

return $dispatcher;
}

public function testSetRunCustomDefaultCommand()
{
$command = new \FooCommand();
Expand Down Expand Up @@ -1151,6 +1144,32 @@ public function testCanCheckIfTerminalIsInteractive()
$inputStream = $application->getHelperSet()->get('question')->getInputStream();
$this->assertEquals($tester->getInput()->isInteractive(), @posix_isatty($inputStream));
}

protected function getDispatcher($skipCommand = false)
{
$dispatcher = new EventDispatcher();
$dispatcher->addListener('console.command', function (ConsoleCommandEvent $event) use ($skipCommand) {
$event->getOutput()->write('before.');

if ($skipCommand) {
$event->disableCommand();
}
});
$dispatcher->addListener('console.terminate', function (ConsoleTerminateEvent $event) use ($skipCommand) {
$event->getOutput()->writeln('after.');

if (!$skipCommand) {
$event->setExitCode(ConsoleCommandEvent::RETURN_CODE_DISABLED);
}
});
$dispatcher->addListener('console.exception', function (ConsoleExceptionEvent $event) {
$event->getOutput()->write('caught.');

$event->setException(new \LogicException('caught.', $event->getExitCode(), $event->getException()));
});

return $dispatcher;
}
}

class CustomApplication extends Application
Expand Down