Catch \Throwable #18765

fprochazka · 2016-05-12T15:54:00Z

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/a

The first commit is based on #15949
Depends 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.

stof · 2016-05-12T16:17:36Z

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)

fprochazka · 2016-05-12T16:19:11Z

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

fprochazka · 2016-05-12T16:47:25Z

@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 · 2016-05-12T17:02:24Z

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

+
+namespace Symfony\Component\Console\Exception;
+
+class FatalThrowableError extends \ErrorException


This, and the other one in Security/Http are copied from Symfony\Component\Debug\Exception\FatalErrorException, since there is no direct dependency.

stof · 2016-05-12T17:38:06Z

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

@@ -94,6 +97,8 @@ public function load($resource, $type = null)
                    $controller = $this->parser->parse($controller);
                } catch (\Exception $e) {
                    // unable to optimize unknown notation
+                } catch (\Throwable $e) {
+                    // unable to optimize unknown notation


-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

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

fprochazka · 2016-05-13T09:39:43Z

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

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


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

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).

nicolas-grekas · 2016-05-13T12:40:25Z

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.

stof · 2016-05-13T12:48:19Z

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

nicolas-grekas · 2016-05-15T08:23:03Z

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

@@ -148,6 +148,9 @@ protected function execute(InputInterface $input, OutputInterface $output)
            } 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) {


should be removed

nicolas-grekas · 2016-05-15T14:54:55Z

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

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


should be reverted OR discussed in a dedicated PR

nicolas-grekas · 2016-05-15T14:57:28Z

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

fprochazka · 2016-05-16T10:47:46Z

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

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


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

fprochazka · 2016-05-16T11:14:44Z

@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 · 2016-05-19T12:09:35Z

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

fprochazka · 2016-05-19T14:08:32Z

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.

nicolas-grekas · 2016-05-20T15:03:02Z

👍

xabbuh · 2016-05-23T06:59:05Z

👍

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/a Related #18765, #15949 Depends on #18812 Commits ------- 103526b Catch \Throwable

fabpot · 2016-05-23T09:01:23Z

Thank you @fprochazka.

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/a The first commit is based on #15949 Depends 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

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/a Related #18765, #15949 Commits ------- 893cf00 Catch \Throwable

carsonbot added the Status: Needs Review label May 12, 2016

fprochazka force-pushed the php7-throwable branch 5 times, most recently from eeae733 to 0fd9ef3 Compare May 12, 2016 17:01

fprochazka reviewed May 12, 2016
View reviewed changes

fprochazka force-pushed the php7-throwable branch from 0fd9ef3 to fab7d23 Compare May 12, 2016 17:32

stof reviewed May 12, 2016
View reviewed changes

fprochazka changed the title ~~Catch \Throwable where finally cannot be used~~ Catch \Throwable May 12, 2016

fprochazka reviewed May 13, 2016
View reviewed changes

nicolas-grekas reviewed May 15, 2016
View reviewed changes

fprochazka reviewed May 16, 2016
View reviewed changes

fprochazka force-pushed the php7-throwable branch 5 times, most recently from 1b61b94 to 76f5f90 Compare May 19, 2016 12:08

This was referenced May 19, 2016

Catch \Throwable #18812

Closed

Catch \Throwable #18813

Merged

Catch \Throwable

de671f4

fprochazka force-pushed the php7-throwable branch 2 times, most recently from 38b4d5c to de671f4 Compare May 19, 2016 14:07

nicolas-grekas added Status: Reviewed and removed Status: Needs Review labels May 20, 2016

fabpot merged commit de671f4 into symfony:2.8 May 23, 2016

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

This was referenced Jun 6, 2016

Release v2.8.7 #18982

Merged

Release v3.0.7 #18983

Merged

rob006 mentioned this pull request Oct 24, 2016

Catch Throwable in yii\base\ErrorHandler::handleException() yiisoft/yii2#12701

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch \Throwable #18765

Catch \Throwable #18765

fprochazka commented May 12, 2016 •

edited

Loading

stof commented May 12, 2016

fprochazka commented May 12, 2016

fprochazka commented May 12, 2016

fprochazka May 12, 2016

stof May 12, 2016

fprochazka May 12, 2016

fprochazka May 13, 2016 •

edited

Loading

nicolas-grekas May 16, 2016

nicolas-grekas commented May 13, 2016

stof commented May 13, 2016

nicolas-grekas May 15, 2016

nicolas-grekas May 15, 2016

nicolas-grekas commented May 15, 2016 •

edited

Loading

fprochazka May 16, 2016

fprochazka commented May 16, 2016

fprochazka commented May 19, 2016 •

edited

Loading

fprochazka commented May 19, 2016

nicolas-grekas commented May 20, 2016

xabbuh commented May 23, 2016

fabpot commented May 23, 2016


		namespace Symfony\Component\Console\Exception;

		class FatalThrowableError extends \ErrorException

Catch \Throwable #18765

Catch \Throwable #18765

Conversation

fprochazka commented May 12, 2016 • edited Loading

stof commented May 12, 2016

fprochazka commented May 12, 2016

fprochazka commented May 12, 2016

fprochazka May 12, 2016

Choose a reason for hiding this comment

stof May 12, 2016

Choose a reason for hiding this comment

fprochazka May 12, 2016

Choose a reason for hiding this comment

fprochazka May 13, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas May 16, 2016

Choose a reason for hiding this comment

nicolas-grekas commented May 13, 2016

stof commented May 13, 2016

nicolas-grekas May 15, 2016

Choose a reason for hiding this comment

nicolas-grekas May 15, 2016

Choose a reason for hiding this comment

nicolas-grekas commented May 15, 2016 • edited Loading

fprochazka May 16, 2016

Choose a reason for hiding this comment

fprochazka commented May 16, 2016

fprochazka commented May 19, 2016 • edited Loading

fprochazka commented May 19, 2016

nicolas-grekas commented May 20, 2016

xabbuh commented May 23, 2016

fabpot commented May 23, 2016

fprochazka commented May 12, 2016 •

edited

Loading

fprochazka May 13, 2016 •

edited

Loading

nicolas-grekas commented May 15, 2016 •

edited

Loading

fprochazka commented May 19, 2016 •

edited

Loading