Skip to content
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.

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

[Console] add support for catching \Throwable errors #50420

Merged
merged 1 commit into from Jul 10, 2023

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented May 24, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #46596
License MIT
Doc PR

@javaDeveloperKid
Copy link
Contributor

Historical question: were all the try..catch blocks in the Symfony code base modified from \Exception to \Throwable after the latter had been introduced? I can still see blocks that catch \Exception and I wonder if this is just an oversight or there is a reason behind this.

Copy link
Contributor

@maxbeckers maxbeckers left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

The broken test covers the behavior for which we cannot just do this unfortunately. Check #22690 for background (tldr errors must keep reaching custom error handlers).

@lyrixx
Copy link
Member Author

lyrixx commented Jun 5, 2023

Hmm, When using the component directly without the error handler, a command can crash badly :/

What about adding a flag to choose if we should handle throwable or not?

@chalasr
Copy link
Member

chalasr commented Jun 5, 2023

Sounds like a good idea 👍

@nicolas-grekas
Copy link
Member

The very lines before the try/catch setup a global error/exception handler, why does this crash badly as you write, while we have this in place?

@lyrixx
Copy link
Member Author

lyrixx commented Jun 5, 2023

@nicolas-grekas let's say you are building a project that run commands, when someone makes an error in the command, it will produces theses outputs:

Without this patch:

>/tmp/foobar foobar  hello
PHP Fatal error:  Uncaught Error: Call to undefined function hello2() in /tmp/foobar/foobar.php:9
Stack trace:
#0 [internal function]: hello()
#1 /home/gregoire/dev/github.com/jolicode/foobar/src/Console/Command/TaskCommand.php(152): ReflectionFunction->invoke()
#2 /home/gregoire/dev/github.com/jolicode/foobar/vendor/symfony/console/Command/Command.php(326): Foobar\Console\Command\TaskCommand->execute()
#3 /home/gregoire/dev/github.com/jolicode/foobar/vendor/symfony/console/Application.php(1063): Symfony\Component\Console\Command\Command->run()
#4 /home/gregoire/dev/github.com/jolicode/foobar/src/Console/Application.php(57): Symfony\Component\Console\Application->doRunCommand()
#5 /home/gregoire/dev/github.com/jolicode/foobar/vendor/symfony/console/Application.php(320): Foobar\Console\Application->doRunCommand()
#6 /home/gregoire/dev/github.com/jolicode/foobar/src/Console/Application.php(45): Symfony\Component\Console\Application->doRun()
#7 /home/gregoire/dev/github.com/jolicode/foobar/vendor/symfony/console/Application.php(174): Foobar\Console\Application->doRun()
#8 /home/gregoire/dev/github.com/jolicode/foobar/src/Console/ApplicationFactory.php(28): Symfony\Component\Console\Application->run()
#9 /home/gregoire/dev/github.com/jolicode/foobar/bin/foobar(14): Foobar\Console\ApplicationFactory::run()
#10 {main}
  thrown in /tmp/foobar/foobar.php on line 9

Fatal error: Uncaught Error: Call to undefined function hello2() in /tmp/foobar/foobar.php:9
Stack trace:
#0 [internal function]: hello()
#1 /home/gregoire/dev/github.com/jolicode/foobar/src/Console/Command/TaskCommand.php(152): ReflectionFunction->invoke()
#2 /home/gregoire/dev/github.com/jolicode/foobar/vendor/symfony/console/Command/Command.php(326): Foobar\Console\Command\TaskCommand->execute()
#3 /home/gregoire/dev/github.com/jolicode/foobar/vendor/symfony/console/Application.php(1063): Symfony\Component\Console\Command\Command->run()
#4 /home/gregoire/dev/github.com/jolicode/foobar/src/Console/Application.php(57): Symfony\Component\Console\Application->doRunCommand()
#5 /home/gregoire/dev/github.com/jolicode/foobar/vendor/symfony/console/Application.php(320): Foobar\Console\Application->doRunCommand()
#6 /home/gregoire/dev/github.com/jolicode/foobar/src/Console/Application.php(45): Symfony\Component\Console\Application->doRun()
#7 /home/gregoire/dev/github.com/jolicode/foobar/vendor/symfony/console/Application.php(174): Foobar\Console\Application->doRun()
#8 /home/gregoire/dev/github.com/jolicode/foobar/src/Console/ApplicationFactory.php(28): Symfony\Component\Console\Application->run()
#9 /home/gregoire/dev/github.com/jolicode/foobar/bin/foobar(14): Foobar\Console\ApplicationFactory::run()
#10 {main}
  thrown in /tmp/foobar/foobar.php on line 9

With this patch (there is color, and Symfony styling):

>/tmp/foobar foobar  hello

In foobar.php line 9:
                                       
  Call to undefined function hello2()  
                                       

hello

And if the developer wants more output, they can run the same command with -v flags

@lyrixx
Copy link
Member Author

lyrixx commented Jun 5, 2023

Very simple reproducer :

take only symfony/console, then run:

<?php

require __DIR__.'/vendor/autoload.php';

use Symfony\Component\Console\SingleCommandApplication;

(new SingleCommandApplication())
    ->setCode(function () {
        echo "AAA";
        foobar();
    })
    ->run()
;

@lyrixx
Copy link
Member Author

lyrixx commented Jul 7, 2023

I have rebased the PR, restored BC, added a new flag to handle errors, and added some tests.

I think it's ready ✅

@lyrixx lyrixx changed the title [Console] The application also catch \Throwable exceptions [Console] add support for catching \Throwable errors Jul 7, 2023
@chalasr
Copy link
Member

chalasr commented Jul 10, 2023

Thank you @lyrixx.

@chalasr
Copy link
Member

chalasr commented Jul 10, 2023

@Seldaek FYI

@chalasr chalasr merged commit 43066ff into symfony:6.4 Jul 10, 2023
8 of 9 checks passed
@lyrixx lyrixx deleted the console-handle-throwable branch July 10, 2023 18:48
@Seldaek
Copy link
Member

Seldaek commented Jul 28, 2023

@chalasr thanks for the ping, see composer/composer@f4738d97b (I guess that's why you pinged me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No catch Throwable errors
9 participants