NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

fonsecas72 wants to merge2 commits intosymfony:2.7fromfonsecas72:ticket_18484

Closed

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

fonsecas72 wants to merge2 commits intosymfony:2.7fromfonsecas72:ticket_18484

Conversation

Copy link

fonsecas72 commentedSep 1, 2016

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

carsonbot added Status: Needs Review Console Bug labels

Sep 1, 2016

Copy link

Contributor

HeahDude commentedSep 1, 2016

ping@romqin @skigun :)

Copy link

Contributor

TomasVotruba commentedSep 1, 2016

Test would be great for this.

GrahamCampbell reviewed

Sep 1, 2016

View reviewed changes

src/Symfony/Component/Console/Application.php


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

Copy link

Contributor

GrahamCampbellSep 1, 2016

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasSep 7, 2016

Choose a reason for hiding this comment

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

I agree

Copy link

Author

fonsecas72Sep 7, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasSep 7, 2016

Choose a reason for hiding this comment

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

I propose this:

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

Copy link

Contributor

GrahamCampbellSep 7, 2016

Choose a reason for hiding this comment

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

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

Copy link

Contributor

GrahamCampbellSep 7, 2016

Choose a reason for hiding this comment

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

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

Copy link

Author

fonsecas72Sep 7, 2016

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasSep 7, 2016

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekas commentedSep 6, 2016

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 force-pushed theticket_18484 branch fromc41c4de to1949951Compare

September 6, 2016 11:03

Copy link

Author

fonsecas72 commentedSep 6, 2016

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

fonsecas72 force-pushed theticket_18484 branch 2 times, most recently from999beb4 toa5250fdCompare

September 6, 2016 12:53

nicolas-grekas reviewed

Sep 7, 2016

View reviewed changes

src/Symfony/Component/Console/composer.json Outdated

		"require": {
		"php":">=5.3.9"
		"php":">=5.3.9",
		"symfony/debug":"~2.7,>=2.7.14"

Copy link

Member

nicolas-grekasSep 7, 2016

Choose a reason for hiding this comment

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

Should be ~2.7,>=2.7.2

fonsecas72 force-pushed theticket_18484 branch 3 times, most recently from8e1b521 to93ae8c6Compare

September 7, 2016 11:59

GrahamCampbell reviewed

Sep 7, 2016

View reviewed changes

src/Symfony/Component/Console/composer.json

		"require": {
		"php":">=5.3.9"
		"php":">=5.3.9",
		"symfony/debug":"~2.7,>=2.7.2"

Copy link

Contributor

GrahamCampbellSep 7, 2016

Choose a reason for hiding this comment

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

^2.7.2?

Copy link

Contributor

GrahamCampbellSep 7, 2016

Choose a reason for hiding this comment

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

also, what about 3.x?

Copy link

Member

nicolas-grekasSep 7, 2016

Choose a reason for hiding this comment

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

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

Copy link

Contributor

GrahamCampbellSep 7, 2016

Choose a reason for hiding this comment

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

ok :)

Copy link

Author

fonsecas72 commentedSep 7, 2016•
edited
Loading

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

nicolas-grekas reviewed

Sep 7, 2016

View reviewed changes

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


		/**
		* @expectedException \LogicException
		* @expectedException \RuntimeException

Copy link

Member

nicolas-grekasSep 7, 2016

Choose a reason for hiding this comment

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

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 =newFatalThrowableError($x);            }if (null !==$e) {$event =newConsoleExceptionEvent($command,$input,$output,$e,$e->getCode());$this->dispatcher->dispatch(ConsoleEvents::EXCEPTION,$event);if ($e !==$event->getException()) {$x =$e =$event->getException();                }$event =newConsoleTerminateEvent($command,$input,$output,$e->getCode());$this->dispatcher->dispatch(ConsoleEvents::TERMINATE,$event);throw$x;            }

Copy link

Author

fonsecas72Sep 7, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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

fonsecas72 force-pushed theticket_18484 branch from8f16615 to7c2d7b3Compare

September 7, 2016 16:56

Hugo Fonseca added2 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 theticket_18484 branch from7c2d7b3 to65e53ecCompare

September 7, 2016 17:08

Copy link

Member

nicolas-grekas commentedSep 8, 2016

👍

Copy link

Member

fabpot commentedSep 8, 2016

Thank you@fonsecas72.

fabpot added a commit that referenced this pull request

Sep 8, 2016

bug#19813[Console] fixed PHP7 Errors are now handled and converted …

ddf8b27

…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

fabpot closed this

Sep 8, 2016

Copy link

Author

fonsecas72 commentedSep 8, 2016

Thank you@TomasVotruba @nicolas-grekas @GrahamCampbell 👍

fonsecas72 deleted the ticket_18484 branch

September 8, 2016 15:38

Copy link

Contributor

TomasVotruba commentedSep 8, 2016

@fonsecas72 Glad to help :)

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 referencedOct 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

fabpot added a commit that referenced this pull request

Dec 5, 2016

bug#20736[Console] fixed PHP7 Errors when not using Dispatcher (ker…

f9bceb8

…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/aOriginal 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 handledCommits-------899fa79 [Console] fixed PHP7 Errors when not using Dispatcher