Deprecate things that prevent \Throwable from bubbling down #33065

fancyweb · 2019-08-08T16:35:12Z

Q	A
Branch?	4.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes (todo update CHANGELOGS & UPGRADES)
Tests pass?	yes
Fixed tickets	#32605
License	MIT
Doc PR	-

~~The goal of this PR is to allow \Throwable forwarded from theErrorHandler::handleException() method (cf #33038) to bubble down in our code base without having to convert them into exceptions.~~

~~WIP because I'm blocked by 2 things caused by GetResponseForExceptionEvent::getThrowable() returning a \Throwable instead of an \Exception.~~

~~1. This \Throwable end up in DataCollectorInterface::collect() (cf ProfilerListener). So that looks impossible to support in 4.4. What can we do?~~
~~2. The second blocker is ExceptionListener::duplicateRequest(). We are not gonna rename this method to support \Throwable I guess. What can we do?~~

Since there are blockers to do it in 4.4, let's prepare for the future and deprecate the things that block us.

src/Symfony/Component/HttpKernel/Event/GetResponseForExceptionEvent.php

src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php

src/Symfony/Component/HttpKernel/Event/GetResponseForExceptionEvent.php

src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php

src/Symfony/Bridge/Twig/DataCollector/TwigDataCollector.php

src/Symfony/Component/Console/Application.php

Tobion · 2019-10-11T14:14:32Z

The other PRs have been merged. We should try to finish this one.

Tobion · 2019-11-04T16:39:29Z

Errors should not be handled like exceptions - they hold separate semantics and should follow a dedicated path.

Following this logic, we should also handle exceptions and errors differently in the ErrorRenderer and we should not change typehints to throwable but actually have different methods for each. So all of these deprecations are not needed.

Tobion · 2019-11-04T16:53:55Z

all BC breaks in master have a corresponding deprecation in 4.4

Then we can go back to what Thomas originally implemented: Just introduce a new method on ExceptionEvent: getThrowable and deprecate getException.

nicolas-grekas · 2019-11-04T17:04:36Z

Following this logic, we should also handle exceptions and errors differently in the ErrorRenderer

Not really required, because ErrorHandler is the last resort logic: it's a generic enough place who's purpose is to report what happened, no matter what.

and we should not change typehints to throwable but actually have different methods for each. So all of these deprecations are not needed.

Yes, that's what I've been wondering also. So, case by case: on Application, we're on a generic enough logic that only reports back what happened - similar to ErrorHandler. On DataCollector, we collect what happened without disrupting the code flow. On listeners, that's not the same story at all - there, any userland logic can mess up with Error and break any reporting.

Then we can go back to what Thomas originally implemented: Just introduce a new method on ExceptionEvent: getThrowable and deprecate getException.

That wouldn't be nice in terms of naming, neither in term of FC/BC (another boring thing forced on users) + skips my arguments about the necessity to split between Error and Exception :)

Tobion · 2019-11-04T17:43:00Z

I'm removing myself from this Throwable topic as I've already spend alot of time reviewing this. My opinion is that wrapping Error into an ErrorException just for it to comply with the ExceptionEvent typehints is nonesense and a legacy behavior. And this is what we originally wanted to fix. You argue that error and exception should be handled differently but yet the current behavior and proposed behavior is pretending an error to be an exception. So I don't know anymore where this is going.

Changes after review

nicolas-grekas

That's a nice first step, thanks for working on this complex topic.

src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php

nicolas-grekas · 2019-11-05T17:24:21Z

Thank you @fancyweb.

… down (fancyweb) This PR was merged into the 4.4 branch. Discussion ---------- Deprecate things that prevent \Throwable from bubbling down | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes (todo update CHANGELOGS & UPGRADES) | Tests pass? | yes | Fixed tickets | #32605 | License | MIT | Doc PR | - ~The goal of this PR is to allow `\Throwable` forwarded from the`ErrorHandler::handleException()` method (cf #33038) to bubble down in our code base without having to convert them into exceptions.~ ~WIP because I'm blocked by 2 things caused by `GetResponseForExceptionEvent::getThrowable()` returning a `\Throwable` instead of an `\Exception`.~ ~1. This `\Throwable` end up in `DataCollectorInterface::collect()` (cf `ProfilerListener`). So that looks impossible to support in 4.4. What can we do?~ ~2. The second blocker is `ExceptionListener::duplicateRequest()`. We are not gonna rename this method to support `\Throwable` I guess. What can we do?~ Since there are blockers to do it in 4.4, let's prepare for the future and deprecate the things that block us. Commits ------- abef506 Deprecate things that prevent \Throwable from bubbling down

nicolas-grekas · 2019-11-05T17:32:31Z

now merged into master

…r (fancyweb) This PR was merged into the 5.0-dev branch. Discussion ---------- Remove \Throwable bubbling down support deprecation layer | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Remove deprecation layer of #33065 Commits ------- ffcfdb4 Remove \Throwable support deprecation layer

carsonbot added Status: Needs Review Deprecation labels Aug 8, 2019

fancyweb force-pushed the allow-throwable-1 branch from 9b979ef to 584f4d6 Compare August 8, 2019 16:36

yceruto added this to the next milestone Aug 8, 2019

jderusse reviewed Aug 8, 2019

View reviewed changes

Tobion reviewed Aug 9, 2019

View reviewed changes

src/Symfony/Component/HttpKernel/Event/GetResponseForExceptionEvent.php Outdated Show resolved Hide resolved

Tobion reviewed Aug 9, 2019

View reviewed changes

src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php Outdated Show resolved Hide resolved

fancyweb force-pushed the allow-throwable-1 branch 3 times, most recently from 2d97e89 to 686a041 Compare August 9, 2019 10:27

fancyweb changed the title ~~[WIP] Allow \Throwable to be rendered~~ [WIP] Deprecate things that prevent \Throwable from bubbling down Aug 9, 2019

fancyweb force-pushed the allow-throwable-1 branch 5 times, most recently from 3c4d592 to ee0b458 Compare August 9, 2019 15:51

fancyweb requested a review from xabbuh as a code owner August 9, 2019 15:51

Tobion reviewed Aug 9, 2019

View reviewed changes

src/Symfony/Bridge/Twig/DataCollector/TwigDataCollector.php Outdated Show resolved Hide resolved

fancyweb force-pushed the allow-throwable-1 branch from ee0b458 to f179713 Compare August 12, 2019 09:02

Tobion reviewed Aug 13, 2019

View reviewed changes

src/Symfony/Component/Console/Application.php Outdated Show resolved Hide resolved

Tobion mentioned this pull request Aug 21, 2019

Added new ErrorController + Preview and enabling there the error renderer mechanism #33271

Merged

2 tasks

Tobion mentioned this pull request Oct 26, 2019

[Routing] Deprecate RouteCollectionBuilder #32937

Merged

7 tasks

Tobion modified the milestones: next, 4.4 Oct 27, 2019

fancyweb force-pushed the allow-throwable-1 branch 2 times, most recently from 251c11b to 5146ad0 Compare October 28, 2019 21:16

fancyweb requested a review from sroze as a code owner October 28, 2019 21:16

fancyweb force-pushed the allow-throwable-1 branch 2 times, most recently from 837615b to 2dbeb57 Compare October 28, 2019 21:31

fancyweb force-pushed the allow-throwable-1 branch 2 times, most recently from 21e8bc6 to 8e67186 Compare November 4, 2019 18:15

fancyweb force-pushed the allow-throwable-1 branch 2 times, most recently from 2bdbda7 to 879c634 Compare November 4, 2019 19:46

nicolas-grekas approved these changes Nov 4, 2019

View reviewed changes

fancyweb mentioned this pull request Nov 4, 2019

[HttpKernel] Add kernel.error event to handle uncaught \Error #34232

Closed

Tobion suggested changes Nov 4, 2019

View reviewed changes

src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels Nov 4, 2019

fancyweb force-pushed the allow-throwable-1 branch 3 times, most recently from d62a2fc to 139f4b4 Compare November 5, 2019 16:58

Deprecate things that prevent \Throwable from bubbling down

abef506

nicolas-grekas force-pushed the allow-throwable-1 branch from 139f4b4 to abef506 Compare November 5, 2019 17:24

nicolas-grekas merged commit abef506 into symfony:4.4 Nov 5, 2019

nicolas-grekas mentioned this pull request Nov 5, 2019

Deprecate things that prevent \Throwable from bubbling down symfony/symfony-docs#12593

Closed

fancyweb deleted the allow-throwable-1 branch November 5, 2019 17:56

fancyweb mentioned this pull request Nov 6, 2019

Remove \Throwable bubbling down support deprecation layer #34265

Merged

Tobion mentioned this pull request Nov 9, 2019

[HttpKernel] rename ExceptionEvent to ErrorEvent and remove the ErrorException wrapper #34306

Closed

This was referenced Nov 12, 2019

Release v4.4.0-BETA1 #34338

Merged

Release v5.0.0-BETA1 #34339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate things that prevent \Throwable from bubbling down #33065

Deprecate things that prevent \Throwable from bubbling down #33065

fancyweb commented Aug 8, 2019 •

edited

Loading

Tobion commented Oct 11, 2019

Tobion commented Nov 4, 2019 •

edited

Loading

Tobion commented Nov 4, 2019

nicolas-grekas commented Nov 4, 2019 •

edited

Loading

Tobion commented Nov 4, 2019 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas commented Nov 5, 2019

nicolas-grekas commented Nov 5, 2019

Deprecate things that prevent \Throwable from bubbling down #33065

Deprecate things that prevent \Throwable from bubbling down #33065

Conversation

fancyweb commented Aug 8, 2019 • edited Loading

Tobion commented Oct 11, 2019

Tobion commented Nov 4, 2019 • edited Loading

Tobion commented Nov 4, 2019

nicolas-grekas commented Nov 4, 2019 • edited Loading

Tobion commented Nov 4, 2019 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Nov 5, 2019

nicolas-grekas commented Nov 5, 2019

fancyweb commented Aug 8, 2019 •

edited

Loading

Tobion commented Nov 4, 2019 •

edited

Loading

nicolas-grekas commented Nov 4, 2019 •

edited

Loading

Tobion commented Nov 4, 2019 •

edited

Loading