[Console] Fix dispatching throwables from ConsoleEvents::COMMAND #22435
Conversation
222eb31 to
0c819b5
Compare
|
I don't think it should. Symfony 3.3 introduces a new |
|
I think it should. That's the expected lifecycle of dispatched events to me, thus just a bug fix. |
0c819b5 to
3c8263a
Compare
b9b88f6 to
8823232
Compare
8823232 to
6d6b04a
Compare
|
Thank you @nicolas-grekas. |
…OMMAND (nicolas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [Console] Fix dispatching throwables from ConsoleEvents::COMMAND | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Exceptions thrown by `ConsoleEvents::COMMAND` listeners should be trigger `ConsoleEvents::EXCEPTION` events. (best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/22435/files?w=1)) Commits ------- 6d6b04a [Console] Fix dispatching throwables from ConsoleEvents::COMMAND
…as-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [Console] Review console.ERROR related behavior | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR is a follow up of #18140 that I wanted to do since a few weeks. It enhances this work with fixes and behavior changes. It embeds #22435 and resolves issues like the one described in #20808. - makes ConsoleErrorEvent *not* extend the deprecated ConsoleExceptionEvent - replace ConsoleErrorEvent::markErrorAsHandled by ConsoleErrorEvent::setExitCode - triggers the deprecation in a more appropriate moment - renames ExceptionListener to ErrorListener - tweaks the behavior in relation to #22435 Commits ------- a7c67c9 [Console] Review console.ERROR related behavior
| } catch (\Exception $e) { | ||
| } catch (\Exception $x) { | ||
| $e = $x; | ||
| } catch (\Throwable $x) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hm, then something else in this commit is causing the behaviour change in PhpSpec reported here #22678
There was a problem hiding this comment.
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. 😅
Exceptions thrown by
ConsoleEvents::COMMANDlisteners should be triggerConsoleEvents::EXCEPTIONevents.(best reviewed ignoring whitespaces)