I'm new to symfony, so I'm not sure where are all the places where it makes sense to actually catch the throwable and where not. I added most places that seemed logical and when I wasn't sure, I added it anyway. I'm hoping you guys (and girls?) can point out the places where the catch should not be added, I'll fix it and then I can create several PR's for the older branches. A lot of this IMHO should go also to 3.0.

carsonbot added the Status: Needs Review label

May 12, 2016

Copy link

Member

stof commentedMay 12, 2016

some of these changes should actually be done in 2.3 or in 2.7 too. Can you open 3 PRs instead ?

one for 2.3 changing all affected places relevant for 2.3
another one for 2.7 changing the places relevant in 2.7+ (not changing the places already changed in the previous PR)
another one for 2.8 for 2.8+ changes (you can update the current PR for that by removing parts of the patch as it already targets the 2.8 branch)

Copy link

ContributorAuthor

fprochazka commentedMay 12, 2016

I wasn't aware the 2.3 is still maintained :) I'll do that then.

Copy link

ContributorAuthor

fprochazka commentedMay 12, 2016

@stof I think it would be more productive to have 90% of the discussion under one PR and I can split it after this is cleaned, so I'm not doing it over 4 branches.

fprochazka force-pushed thephp7-throwable branch 5 times, most recently fromeeae733 to0fd9ef3Compare

May 12, 2016 17:01

fprochazka reviewed

May 12, 2016

View reviewed changes

src/Symfony/Component/Console/Exception/FatalThrowableError.php Outdated


		namespace Symfony\Component\Console\Exception;

		class FatalThrowableError extends \ErrorException

Copy link

ContributorAuthor

fprochazkaMay 12, 2016

Choose a reason for hiding this comment

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

This, and the other one inSecurity/Http are copied fromSymfony\Component\Debug\Exception\FatalErrorException, since there is no direct dependency.

fprochazka force-pushed thephp7-throwable branch from0fd9ef3 tofab7d23Compare

May 12, 2016 17:32

stof reviewed

May 12, 2016

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Routing/DelegatingLoader.php Outdated

		}catch (\Exception$e) {
		// unable to optimize unknown notation
		}catch (\Throwable$e) {
		// unable to optimize unknown notation

Copy link

Member

stofMay 12, 2016

Choose a reason for hiding this comment

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

-1 for this one. Getting an Error in the parser should not happen when it is an unknown notation (the parser is expected to throw an Exception in such case), so it would only hide bugs

Copy link

ContributorAuthor

fprochazkaMay 12, 2016

Choose a reason for hiding this comment

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

Makes sense, having it more specific and only throw away something, not everything seems more sane 👍

fprochazka changed the title~~Catch \Throwable where finally cannot be used~~Catch \Throwable

May 12, 2016

fprochazka reviewed

May 13, 2016

View reviewed changes

src/Symfony/Component/Console/Exception/FatalThrowableError.php Outdated

		$e->getCode(),
		$severity,
		$e->getFile(),
		$e->getLine()

Copy link

ContributorAuthor

fprochazkaMay 13, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Btw, why is theThrowable not added as previous inSymfony\Component\Debug\Exception\FatalThrowableError ? It doesn't feel right to throw it away completely.

Copy link

Member

nicolas-grekasMay 16, 2016

Choose a reason for hiding this comment

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

The original is not thrown away: the state of the new FatalThrowableError has all bits from the original Error exception. Adding it as previous would defeat the purpose of FatalThrowableError, which is to pass the \Exception type hints (even for exceptions in the previous chain).

Copy link

Member

nicolas-grekas commentedMay 13, 2016

I'm mostly 👎 but on a few spots: \Error should not be dealt with as regular exceptions. They are a sign of serious issues and recovering after them is encouraging not fixing the related issues to me.
Should be thought on a case by case basis.

Copy link

Member

stof commentedMay 13, 2016

yeah, catching them is fine in places where they are rethrown after some cleanup (use case forfinally in newer versions of PHP), but other places should probably let them bubble most of the time

nicolas-grekas reviewed

May 15, 2016

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php Outdated

		}catch (\Exception$e) {
		$exitCode =1;
		$rows[] =array(sprintf('<fg=red;options=bold>%s</>','\\' ===DIRECTORY_SEPARATOR ?'ERROR' :"\xE2\x9C\x98"/* HEAVY BALLOT X (U+2718) */),$message,$e->getMessage());
		}catch (\Throwable$e) {

Copy link

Member

nicolas-grekasMay 15, 2016

Choose a reason for hiding this comment

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

should be removed

nicolas-grekas reviewed

May 15, 2016

View reviewed changes

src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php Outdated

		// the response that comes back is simply ignored
		if (isset($options['ignore_errors']) &&$options['ignore_errors'] &&$this->dispatcher) {
		$event =newGetResponseForExceptionEvent($this->kernel,$request, HttpKernelInterface::SUB_REQUEST,$e);
		}catch (\Throwable$e) {

Copy link

Member

nicolas-grekasMay 15, 2016

Choose a reason for hiding this comment

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

should be reverted OR discussed in a dedicated PR

Copy link

Member

nicolas-grekas commentedMay 15, 2016•
edited
Loading

@fprochazka thanks for this PR. I added a comment everywhere I think catchingThrowable should be reverted. All the other non-commented places look fine to me.

fprochazka reviewed

May 16, 2016

View reviewed changes

src/Symfony/Component/Debug/Tests/Fixtures/ToStringThrower.php Outdated


		// Using user_error() here is on purpose so we do not forget
		// that this alias also should work alongside with trigger_error().
		returnuser_error($e,E_USER_ERROR);

Copy link

ContributorAuthor

fprochazkaMay 16, 2016

Choose a reason for hiding this comment

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

So... should I still create a testcase for this with new fixture for throwable?

Copy link

ContributorAuthor

fprochazka commentedMay 16, 2016

@nicolas-grekas I've removed those where I agree, but I would like a better reasoning behind those I left untouched, that you've commented. They seem important to me.

fprochazka force-pushed thephp7-throwable branch 5 times, most recently from1b61b94 to76f5f90Compare

May 19, 2016 12:08

Copy link

ContributorAuthor

fprochazka commentedMay 19, 2016•
edited
Loading

All the comments are solved, so I'm gonna start spliting this to more branchesas @stof said

This was referencedMay 19, 2016

Catch \Throwable#18812

Closed

Catch \Throwable#18813

Merged

Catch \Throwable

de671f4

fprochazka force-pushed thephp7-throwable branch 2 times, most recently from38b4d5c tode671f4Compare

May 19, 2016 14:07

Copy link

ContributorAuthor

fprochazka commentedMay 19, 2016

Should I try to help with PR for 3.0? Or will the mergers solve the possible conflicts and throw away changes that are not relevant in 3.0? As I'm sure there will be some.

Copy link

Member

nicolas-grekas commentedMay 20, 2016

👍

nicolas-grekas added Status: Reviewed and removed Status: Needs Review labels

May 20, 2016

Copy link

Member

xabbuh commentedMay 23, 2016

👍

fabpot added a commit that referenced this pull request

May 23, 2016

bug#18813Catch \Throwable (fprochazka)

f55680b

This PR was merged into the 2.7 branch.Discussion----------Catch \Throwable| Q             | A| ------------- | ---| Branch?       | 2.7, 2.8, 3.0| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | Yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aRelated#18765,#15949Depends on#18812Commits-------103526b Catch \Throwable

Copy link

Member

fabpot commentedMay 23, 2016

Thank you@fprochazka.

fabpot merged commitde671f4 intosymfony:2.8

May 23, 2016

fabpot added a commit that referenced this pull request

May 23, 2016

bug#18765Catch \Throwable (fprochazka)

eff863b

This PR was merged into the 2.8 branch.Discussion----------Catch \Throwable| Q             | A| ------------- | ---| Branch?       | 2.8, 3.0| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | Mostly!| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aThe first commit is based on#15949Depends on#18813,#18812----I'm new to symfony, so I'm not sure where are all the places where it makes sense to actually catch the throwable and where not. I added most places that seemed logical and when I wasn't sure, I added it anyway. I'm hoping you guys (and girls?) can point out the places where the catch should not be added, I'll fix it and then I can create several PR's for the older branches. A lot of this IMHO should go also to 3.0.Commits-------de671f4 Catch \Throwable

fprochazka deleted the php7-throwable branch

May 23, 2016 13:31

fabpot mentioned this pull request

May 26, 2016

Release v3.1.0-RC1#18885

Merged

nicolas-grekas added a commit that referenced this pull request

May 30, 2016

bug#18812Catch \Throwable (fprochazka)

a60c355

This PR was squashed before being merged into the 2.3 branch (closes#18812).Discussion----------Catch \Throwable| Q             | A| ------------- | ---| Branch?       | 2.3, 2.7, 2.8, 3.0| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | Yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aRelated#18765,#15949Commits-------893cf00 Catch \Throwable

This was referencedJun 6, 2016

Release v2.8.7#18982

Merged

Release v3.0.7#18983

Merged

rob006 mentioned this pull request

Oct 24, 2016

CatchThrowable inyii\base\ErrorHandler::handleException()yiisoft/yii2#12701

Merged

Labels

Status: Reviewed

Movatterモバイル変換

Uh oh!

Catch \Throwable#18765

Catch \Throwable#18765

Uh oh!

Conversation

fprochazka commentedMay 12, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

stof commentedMay 12, 2016

Uh oh!

fprochazka commentedMay 12, 2016

Uh oh!

fprochazka commentedMay 12, 2016

Uh oh!

fprochazkaMay 12, 2016

Choose a reason for hiding this comment

Uh oh!

stofMay 12, 2016

Choose a reason for hiding this comment

Uh oh!

fprochazkaMay 12, 2016

Choose a reason for hiding this comment

Uh oh!

fprochazkaMay 13, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasMay 16, 2016

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedMay 13, 2016

Uh oh!

stof commentedMay 13, 2016

Uh oh!

nicolas-grekasMay 15, 2016

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasMay 15, 2016

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedMay 15, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

fprochazkaMay 16, 2016

Choose a reason for hiding this comment

Uh oh!

fprochazka commentedMay 16, 2016

Uh oh!

fprochazka commentedMay 19, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

fprochazka commentedMay 19, 2016

Uh oh!

nicolas-grekas commentedMay 20, 2016

Uh oh!

xabbuh commentedMay 23, 2016

Uh oh!

fabpot commentedMay 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fprochazka commentedMay 12, 2016•
edited
Loading

fprochazkaMay 13, 2016•
edited
Loading

nicolas-grekas commentedMay 15, 2016•
edited
Loading

fprochazka commentedMay 19, 2016•
edited
Loading