Skip to content

Deprecate things that prevent \Throwable from bubbling down #33065

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 5, 2019
Merged

Deprecate things that prevent \Throwable from bubbling down #33065

merged 1 commit into from
Nov 5, 2019

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Aug 8, 2019 •
edited
Loading

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.

@fancyweb fancyweb force-pushed the allow-throwable-1 branch from 9b979ef to 584f4d6 Compare August 8, 2019 16:36
@yceruto yceruto added this to the next milestone Aug 8, 2019
@fancyweb fancyweb force-pushed the allow-throwable-1 branch 3 times, most recently from 2d97e89 to 686a041 Compare August 9, 2019 10:27
@fancyweb fancyweb changed the title [WIP] Allow \Throwable to be rendered [WIP] Deprecate things that prevent \Throwable from bubbling down Aug 9, 2019
@fancyweb fancyweb force-pushed the allow-throwable-1 branch 5 times, most recently from 3c4d592 to ee0b458 Compare August 9, 2019 15:51
@fancyweb fancyweb requested a review from xabbuh as a code owner August 9, 2019 15:51
@Tobion Tobion mentioned this pull request Aug 21, 2019
Merged
2 tasks
@Tobion
Copy link
Contributor

Tobion commented Oct 11, 2019

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

@Tobion Tobion mentioned this pull request Oct 26, 2019
Merged
7 tasks
@Tobion Tobion modified the milestones: next, 4.4 Oct 27, 2019
@fancyweb fancyweb force-pushed the allow-throwable-1 branch 2 times, most recently from 251c11b to 5146ad0 Compare October 28, 2019 21:16
@fancyweb fancyweb requested a review from sroze as a code owner October 28, 2019 21:16
@fancyweb fancyweb force-pushed the allow-throwable-1 branch 2 times, most recently from 837615b to 2dbeb57 Compare October 28, 2019 21:31
@Tobion
Copy link
Contributor

Tobion commented Nov 4, 2019 •
edited
Loading

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
Copy link
Contributor

Tobion commented Nov 4, 2019

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
Copy link
Member

nicolas-grekas commented Nov 4, 2019 •
edited
Loading

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
Copy link
Contributor

Tobion commented Nov 4, 2019 •
edited
Loading

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.

@fancyweb fancyweb force-pushed the allow-throwable-1 branch 2 times, most recently from 21e8bc6 to 8e67186 Compare November 4, 2019 18:15
@Tobion Tobion dismissed their stale review November 4, 2019 19:01

Changes after review

@fancyweb fancyweb force-pushed the allow-throwable-1 branch 2 times, most recently from 2bdbda7 to 879c634 Compare November 4, 2019 19:46
nicolas-grekas
nicolas-grekas approved these changes Nov 4, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

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

@fancyweb fancyweb mentioned this pull request Nov 4, 2019
Closed
@fancyweb fancyweb force-pushed the allow-throwable-1 branch 3 times, most recently from d62a2fc to 139f4b4 Compare November 5, 2019 16:58
@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

nicolas-grekas added a commit that referenced this pull request Nov 5, 2019
@nicolas-grekas
feature #33065 Deprecate things that prevent \Throwable from bubbling…
72dd176 
… 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 nicolas-grekas merged commit abef506 into symfony:4.4 Nov 5, 2019
@nicolas-grekas nicolas-grekas mentioned this pull request Nov 5, 2019
Closed
@nicolas-grekas
Copy link
Member

now merged into master

@fancyweb fancyweb deleted the allow-throwable-1 branch November 5, 2019 17:56
@fancyweb fancyweb mentioned this pull request Nov 6, 2019
Merged
nicolas-grekas added a commit that referenced this pull request Nov 8, 2019
@nicolas-grekas
minor #34265 Remove \Throwable bubbling down support deprecation laye…
4cc88b1 
…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
@Tobion Tobion mentioned this pull request Nov 9, 2019
Closed
This was referenced Nov 12, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@jderusse jderusse jderusse left review comments

@Tobion Tobion Tobion requested changes

@yceruto yceruto yceruto left review comments

@xabbuh xabbuh Awaiting requested review from xabbuh

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
Deprecation Status: Needs Work
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@fancyweb @Tobion @nicolas-grekas @derrabus @jderusse @yceruto @carsonbot