[Console] Give errors back to error handler if not handled by console.error listeners #22261

chalasr · Apr 3, 2017

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22259
License	MIT
Doc PR	n/a

Re-throws errors if ConsoleErrorEvent::markErrorAsHandled() hasn't been called so that they can reach the global error handler, fixing the BC break.

curry684 · Apr 3, 2017

BC bug fixes should have a unit test to ensure the issue will not surface again.

chalasr · Apr 3, 2017

Yeah, I didn't find a relevant way to test this yet.

ogizanagi · Apr 4, 2017

Sure, it'll fix the BC break. Thank you for creating the issue and for this PR. 😃

In #20808 however, I also wanted to add support for handling errors natively & easily with the standalone component, when you don't use an error handler like the one in the Debug component.
It may not challenge this PR as is, but it depends how we'd like to proceed. With your solution, we could for instance provide a default error listener in the component, marking errors as handled by the Application and running Application::renderException() (as the DebugHandlersListener does). Problem: the exit code is set to 0 as soon as the event declares the exception is handled.

Regarding the test, why not just testing the error is properly re-thrown by Application? You don't need to test an error handler is called.

…e.error listeners

chalasr · Apr 4, 2017

Test added, I was missing something.

we could for instance provide a default error listener in the component, marking errors as handled by the Application and running Application::renderException() (as the DebugHandlersListener does).

As discussed together, I think we can first merge this as is in order to fix the introduced BC break and improve the situation in a next time but indeed, having a default error handler in the console seems like a good compromise to me.

PR ready, build failures unrelated.

chalasr · Apr 4, 2017

public function execute(InputInterface $input, OutputInterface $output) 
{
    $this->foo();
}

public function fooo() 
{
}

Before:

After:

nicolas-grekas

👍 but please note that I'm going to look at error handling in console a bit more deeply during feat freeze. I may change a few things to what #18140 did.

fabpot · Apr 4, 2017

Thank you @chalasr.

… by console.error listeners (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [Console] Give errors back to error handler if not handled by console.error listeners | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22259 | License | MIT | Doc PR | n/a Re-throws errors if `ConsoleErrorEvent::markErrorAsHandled()` hasn't been called so that they can reach the global error handler, fixing the BC break. Commits ------- 5a5bf54 [Console] Give errors back to error handlers if not handled by console.error listeners

chalasr · Apr 4, 2017

@nicolas-grekas One thing to note is that errors are still not rethrown in case the application has no event dispatcher (occurs only using the component independently), even after this. I just realize it and was going to look into but I'll wait then, don't hesitate to ping me for a hand.

stof · Apr 4, 2017

src/Symfony/Component/Console/Application.php

@@ -138,6 +138,9 @@ public function run(InputInterface $input = null, OutputInterface $output = null
                $e = null;
                $exitCode = 0;


technically, the debug handler handles the exception (rendering it in a special way), but could still want to use a failure exit code. We should let the event change the code.

Yes, that's the conclusion we came to with @chalasr regarding this part. I guess now it's better to wait for a PR with a bigger vision of the remaining issues with the component, as @nicolas-grekas seems to plan.

…onsole.error listeners (chalasr) This PR was merged into the 2.7 branch. Discussion ---------- [Console] Fix errors not rethrown even if not handled by console.error listeners | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22678 | License | MIT | Doc PR | n/a #22261 has been squashed while revisiting error handling, this fixes it again while keeping latest changes intact. __code__ ```php public function execute(InputInterface $input, OutputInterface $output) { $this->barr(); } public function bar() { } ``` __before__ ![before](http://image.prntscr.com/image/38aa3b46fed6439ead693908ab104fb3.png) __after__ ![after](http://image.prntscr.com/image/071322bfa52247c6a02eac6ef9d8284a.png) Commits ------- 75f098f Fix errors not rethrown even if not handled by console.error listeners

carsonbot added Status: Needs Review Console Bug labels Apr 3, 2017

chalasr mentioned this pull request Apr 3, 2017

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

Closed

chalasr force-pushed the console-error-handler branch from 20c4419 to 903ea95 Compare April 4, 2017 08:50

[Console] Give errors back to error handlers if not handled by consol…

5a5bf54

…e.error listeners

chalasr force-pushed the console-error-handler branch from 903ea95 to 5a5bf54 Compare April 4, 2017 08:56

chalasr mentioned this pull request Apr 4, 2017

[Console][DX] Error handling degradation in 3.3 #22259

Closed

nicolas-grekas added this to the 3.3 milestone Apr 4, 2017

nicolas-grekas approved these changes Apr 4, 2017

View reviewed changes

fabpot merged commit 5a5bf54 into symfony:master Apr 4, 2017

chalasr deleted the console-error-handler branch April 4, 2017 15:09

stof reviewed Apr 4, 2017

View reviewed changes

chalasr mentioned this pull request May 11, 2017

[Console] Fix errors not rethrown even if not handled by console.error listeners #22690

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Console] Give errors back to error handler if not handled by console.error listeners #22261

[Console] Give errors back to error handler if not handled by console.error listeners #22261

Uh oh!

chalasr commented Apr 3, 2017 •

edited

Loading

Uh oh!

curry684 commented Apr 3, 2017

Uh oh!

chalasr commented Apr 3, 2017 •

edited

Loading

Uh oh!

ogizanagi commented Apr 4, 2017

Uh oh!

chalasr commented Apr 4, 2017 •

edited

Loading

Uh oh!

chalasr commented Apr 4, 2017

Uh oh!

nicolas-grekas left a comment

Uh oh!

fabpot commented Apr 4, 2017

Uh oh!

chalasr commented Apr 4, 2017

Uh oh!

stof Apr 4, 2017

Uh oh!

ogizanagi Apr 4, 2017

Uh oh!

Uh oh!

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

Uh oh!

[Console] Give errors back to error handler if not handled by console.error listeners #22261

[Console] Give errors back to error handler if not handled by console.error listeners #22261

Uh oh!

Conversation

chalasr commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

curry684 commented Apr 3, 2017

Uh oh!

chalasr commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogizanagi commented Apr 4, 2017

Uh oh!

chalasr commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chalasr commented Apr 4, 2017

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

fabpot commented Apr 4, 2017

Uh oh!

chalasr commented Apr 4, 2017

Uh oh!

stof Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

ogizanagi Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chalasr commented Apr 3, 2017 •

edited

Loading

chalasr commented Apr 3, 2017 •

edited

Loading

chalasr commented Apr 4, 2017 •

edited

Loading