8000 [Console] add support for catching `\Throwable` errors by lyrixx · Pull Request #50420 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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

Jump to bottom
Merged
chalasr merged 1 commit into from
Jul 10, 2023
Merged

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

chalasr 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

@lyrixx lyrixx requested a review from chalasr as a code owner May 24, 2023 17:12
@carsonbot carsonbot added this to the 6.4 milestone May 24, 2023
@ghost
Copy link
ghost commented May 24, 2023

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.

maxbeckers
maxbeckers approved these changes May 31, 2023
View reviewed changes
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

chalasr
chalasr requested changes Jun 5, 2023
View reviewed changes
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
nicolas-grekas commented Jun 5, 2023

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 •
edited
Loading

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 lyrixx force-pushed the console-handle-throwable branch from aeac386 to 83417ea Compare July 7, 2023 11:21
@lyrixx
Copy link
Member Author
lyrixx commented Jul 7, 2023 •
edited
Loading

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

I think it's ready ✅

GromNaN
GromNaN reviewed Jul 7, 2023
View reviewed changes
GromNaN
GromNaN reviewed Jul 7, 2023
View reviewed changes
@lyrixx lyrixx force-pushed the console-handle-throwable branch from 83417ea to 8fc6dd2 Compare July 7, 2023 11:54
@lyrixx lyrixx changed the title [Console] The application also catch \Throwable exceptions [Console] add support for catching \Throwable errors Jul 7, 2023
chalasr
chalasr reviewed Jul 7, 2023
View reviewed changes
chalasr
chalasr reviewed Jul 10, 2023
View reviewed changes
@lyrixx lyrixx force-pushed the console-handle-throwable branch from 8fc6dd2 to 0c2552e Compare July 10, 2023 08:00
@lyrixx lyrixx force-pushed the console-handle-throwable branch from 0c2552e to 7a35645 Compare July 10, 2023 08:05
@lyrixx lyrixx force-pushed the console-handle-throwable branch from 7a35645 to 1baeb3d Compare July 10, 2023 11:49
@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
@lyrixx lyrixx deleted the console-handle-throwable branch July 10, 2023 18:48
Seldaek added a commit to composer/composer that referenced this pull request Jul 28, 2023
@Seldaek
Add support for Application::setCatchErrors in symfony 6.4+, refs sym…
f4738d9 
…fony/symfony#50420
@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 :)

This was referenced Oct 21, 2023
Merged
Merged
@llupa llupa mentioned this pull request Feb 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GromNaN GromNaN GromNaN approved these changes

@derrabus derrabus derrabus approved these changes

@chalasr chalasr chalasr approved these changes

+1 more reviewer

@maxbeckers maxbeckers maxbeckers approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Console Feature Status: Reviewed

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

No catch Throwable errors

8 participants

@lyrixx @chalasr @nicolas-grekas @Seldaek @GromNaN @derrabus @maxbeckers @carsonbot
0