[Console] fixed PHP7 Errors are now handled and converted to Exceptions #19813

fonsecas72 · 2016-09-01T09:35:41Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#18484
License	MIT
Doc PR

HeahDude · 2016-09-01T10:09:29Z

ping @romqin @skigun :)

TomasVotruba · 2016-09-01T10:16:53Z

Test would be great for this.

GrahamCampbell · 2016-09-01T13:44:01Z

src/Symfony/Component/Console/Application.php

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


Shouldn't we be throwing the original throwable here. This is a BC break?

What does it mean?
That I should delete the $e = new FatalThrowableError($e); line?

I propose this:

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

I think it would be better to just add another function and call that at the end of each try catch?

try { $exitCode = $command->run($input, $output); } catch (\Exception $e) { $this->someFuntion($e); throw $e; } catch (\Throwable $x) { $this->someFuntion(new FatalThrowableError($e)); throw $e; }

I will go with whatever solution you find better /cc @nicolas-grekas :)

It's a pure matter of coding style, I don't care :)

nicolas-grekas · 2016-09-06T10:17:29Z

Looks like this is untested, can you add a test case please?
The composer.json file misses a requirement for the debug component now (~2.7).

fonsecas72 · 2016-09-06T11:30:31Z

@nicolas-grekas it's done I believe, thanks
Is there anything else I can do to improve this?

nicolas-grekas · 2016-09-07T11:37:10Z

src/Symfony/Component/Console/composer.json

@@ -16,7 +16,8 @@
        }
    ],
    "require": {
-        "php": ">=5.3.9"
+        "php": ">=5.3.9",
+        "symfony/debug": "~2.7,>=2.7.14"


Should be ~2.7,>=2.7.2

GrahamCampbell · 2016-09-07T12:30:54Z

src/Symfony/Component/Console/composer.json

@@ -16,7 +16,8 @@
        }
    ],
    "require": {
-        "php": ">=5.3.9"
+        "php": ">=5.3.9",
+        "symfony/debug": "~2.7,>=2.7.2"


also, what about 3.x?

we don't use ^ yet
we'll add 3.x when merging into 2.8 (the only branch that opens to 3.x)

fonsecas72 · 2016-09-07T13:23:21Z

@nicolas-grekas the original exception is being re-throw and tests are updated/fixed now

nicolas-grekas · 2016-09-07T16:47:24Z

src/Symfony/Component/Console/Tests/ApplicationTest.php

@@ -914,7 +914,7 @@ public function testRunWithDispatcher()
    }

    /**
-     * @expectedException        \LogicException
+     * @expectedException        \RuntimeException


So my bad, this can't change, that would be a bc break.
I propose to tweak the implementation like this, so that the original exception is preserved, except when a listener decides to alter it:

try { $e = null; $exitCode = $command->run($input, $output); } catch (\Exception $x) { $e = $x; } catch (\Throwable $x) { $e = new FatalThrowableError($x); } 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); throw $x; }

oh now I get it! I've added a little test for that too. I hope everything is ok now :)

nicolas-grekas · 2016-09-08T06:09:07Z

👍

fabpot · 2016-09-08T15:09:03Z

Thank you @fonsecas72.

…to Exceptions (fonsecas72) This PR was squashed before being merged into the 2.7 branch (closes #19813). Discussion ---------- [Console] fixed PHP7 Errors are now handled and converted to Exceptions | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18484 | License | MIT | Doc PR | Commits ------- d3c613b [Console] fixed PHP7 Errors are now handled and converted to Exceptions

fonsecas72 · 2016-09-08T15:29:44Z

Thank you @TomasVotruba @nicolas-grekas @GrahamCampbell 👍

TomasVotruba · 2016-09-08T17:27:21Z

@fonsecas72 Glad to help :)

…adus) This PR was squashed before being merged into the 2.7 branch (closes #20736). Discussion ---------- [Console] fixed PHP7 Errors when not using Dispatcher | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17257, #20110, #20111 | License | MIT | Doc PR | n/a Original fix, #19813, works only when there is event dispatcher available. This PR fix the issue also for scenario without event dispatcher. Closes #20110 issue and #20111 PR connected to it. Closing #17257 , as everywhere the error is converted to exception and it should be handled Commits ------- 899fa79 [Console] fixed PHP7 Errors when not using Dispatcher

carsonbot added Status: Needs Review Console Bug labels Sep 1, 2016

GrahamCampbell reviewed Sep 1, 2016
View reviewed changes

fonsecas72 force-pushed the ticket_18484 branch from c41c4de to 1949951 Compare September 6, 2016 11:03

fonsecas72 force-pushed the ticket_18484 branch 2 times, most recently from 999beb4 to a5250fd Compare September 6, 2016 12:53

nicolas-grekas reviewed Sep 7, 2016
View reviewed changes

fonsecas72 force-pushed the ticket_18484 branch 3 times, most recently from 8e1b521 to 93ae8c6 Compare September 7, 2016 11:59

GrahamCampbell reviewed Sep 7, 2016
View reviewed changes

nicolas-grekas reviewed Sep 7, 2016
View reviewed changes

fonsecas72 force-pushed the ticket_18484 branch from 8f16615 to 7c2d7b3 Compare September 7, 2016 16:56

Hugo Fonseca added 2 commits September 7, 2016 18:08

[Console] fixed PHP7 Errors are now handled and converted to Exceptions

a8ebd79

[Console] added symfony/debug 2.7 dependency

65e53ec

fonsecas72 force-pushed the ticket_18484 branch from 7c2d7b3 to 65e53ec Compare September 7, 2016 17:08

fabpot closed this Sep 8, 2016

fonsecas72 deleted the ticket_18484 branch September 8, 2016 15:38

GrahamCampbell mentioned this pull request Sep 13, 2016

Fixed bad merge #19932

Closed

nicolas-grekas mentioned this pull request Sep 30, 2016

added catching of php7 fatal exceptions in application #20110 #20111

Closed

This was referenced Oct 3, 2016

Release v2.7.19 #20140

Merged

Release v2.8.12 #20143

Merged

Release v3.1.5 #20144

Merged

keradus mentioned this pull request Dec 3, 2016

[Console] fixed PHP7 Errors when not using Dispatcher #20736

Closed

jstuhli mentioned this pull request Dec 14, 2016

[Console] fixed PHP7 Errors are now handled in Application::run and a… #20917

Closed

ogizanagi mentioned this pull request Dec 14, 2016

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

Closed

ro0NL mentioned this pull request Nov 13, 2017

[Console] remove dead code #24962

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] fixed PHP7 Errors are now handled and converted to Exceptions #19813

[Console] fixed PHP7 Errors are now handled and converted to Exceptions #19813

fonsecas72 commented Sep 1, 2016

HeahDude commented Sep 1, 2016

TomasVotruba commented Sep 1, 2016

GrahamCampbell Sep 1, 2016

nicolas-grekas Sep 7, 2016

fonsecas72 Sep 7, 2016 •

edited

Loading

nicolas-grekas Sep 7, 2016

GrahamCampbell Sep 7, 2016

GrahamCampbell Sep 7, 2016

fonsecas72 Sep 7, 2016

nicolas-grekas Sep 7, 2016

nicolas-grekas commented Sep 6, 2016

fonsecas72 commented Sep 6, 2016

nicolas-grekas Sep 7, 2016

GrahamCampbell Sep 7, 2016

GrahamCampbell Sep 7, 2016

nicolas-grekas Sep 7, 2016

GrahamCampbell Sep 7, 2016

fonsecas72 commented Sep 7, 2016 •

edited

Loading

nicolas-grekas Sep 7, 2016

fonsecas72 Sep 7, 2016 •

edited

Loading

nicolas-grekas commented Sep 8, 2016

fabpot commented Sep 8, 2016

fonsecas72 commented Sep 8, 2016

TomasVotruba commented Sep 8, 2016

[Console] fixed PHP7 Errors are now handled and converted to Exceptions #19813

[Console] fixed PHP7 Errors are now handled and converted to Exceptions #19813

Conversation

fonsecas72 commented Sep 1, 2016

HeahDude commented Sep 1, 2016

TomasVotruba commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fonsecas72 Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Sep 6, 2016

fonsecas72 commented Sep 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fonsecas72 commented Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

fonsecas72 Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Sep 8, 2016

fabpot commented Sep 8, 2016

fonsecas72 commented Sep 8, 2016

TomasVotruba commented Sep 8, 2016

fonsecas72 Sep 7, 2016 •

edited

Loading

fonsecas72 commented Sep 7, 2016 •

edited

Loading

fonsecas72 Sep 7, 2016 •

edited

Loading