Skip to content

[Debug] Allow flatten throwable #26447

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
Closed
wants to merge 1 commit into from
Closed

[Debug] Allow flatten throwable #26447

wants to merge 1 commit into from

Conversation

instabledesign
Copy link
Contributor

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26429
License MIT

Allow Throwable to be Flatten.

ostrolucky
ostrolucky suggested changes Mar 7, 2018
View reviewed changes
Copy link
Contributor

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

This is BC break. Please read Symfony BC promise. Correct solution is to create new class and deprecate this one.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Mar 12, 2018
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 12, 2018 •
edited
Loading

Is this required? The Debug component converts Error to FatalErrorException, so that this has never been required before. What's your use case?

@instabledesign
Copy link
Contributor Author

instabledesign commented Mar 12, 2018 •
edited
Loading

I have a consumer with try catch(\Throwable) and i want to send by mail the flatten exception.
For the moment i'll add

if (!$exception instanceof \Exception) {
    $exception = new FatalThrowableError($exception);
}
$flattenException = FlattenException::create($exception);

but why dont allow?

@derrabus
Copy link
Member

I already felt like doing this when working on #26028, but I decided against it because of the BC break @ostrolucky mentioned. Right now, Error objects are converted to FatalThrowableError in order to not break the interface of the FlattenException class. But apart from that, there are no technical reasons to perform that conversion.

Allowing FlattenException to work with Throwable instances would improve the overall utility of that class and remove technical debt.

derrabus
derrabus requested changes Mar 13, 2018
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

This block can probably be simplified now:

https://github.com/instabledesign/symfony/blob/d79c528b400eaeecc53e97cc4c5f3cbf7df33650/src/Symfony/Component/Debug/Exception/FlattenException.php#L60-L66

src/Symfony/Component/Debug/Exception/FlattenException.php
@@ -33,7 +33,7 @@ class FlattenException
private $file;
private $line;

public static function create(\Exception $exception, $statusCode = null, array $headers = array())
public static function create(\Throwable $exception, $statusCode = null, array $headers = array())
Copy link
Member

Choose a reason for hiding this comment

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

This change will break applications that extend this class and override the create method.

src/Symfony/Component/Debug/Exception/FlattenException.php
@@ -178,7 +178,7 @@ public function getTrace()
return $this->trace;
}

public function setTraceFromException(\Exception $exception)
public function setTraceFromException(\Throwable $exception)
Copy link
Member

Choose a reason for hiding this comment

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

This is a BC break for the same reasons.

@derrabus
Copy link
Member

Backwards-compatible solutions could be:

  • Deprecate FlattenException in favor of a new FlattenThrowable class. That's a lot of work because FlattenException is used quite often.
  • Remove the \Exception type-hints and add \Throwable type-hints in Symfony 5.

@derrabus derrabus mentioned this pull request Mar 14, 2018
Merged
@nicolas-grekas
Copy link
Member

Closing in favor of #26528, which is more advanced now. Thank you @instabledesign for raising the topic.

@instabledesign
Copy link
Contributor Author

instabledesign commented Mar 15, 2018 •
edited
Loading

pwned my first contribution 😢

@sroze
Copy link
Contributor

sroze commented Mar 15, 2018

Don't give up, next time will be better 😉

@derrabus
Copy link
Member

I'm really sorry for that, @instabledesign. I should've waited a couple more days, I suppose. 😓

I just wanted to slip this in before the feature freeze.

fabpot added a commit that referenced this pull request Apr 6, 2018
@fabpot
feature #26528 [Debug] Support any Throwable object in FlattenExcepti…
47e2bd3 
…on (derrabus)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Debug] Support any Throwable object in FlattenException

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #26429
| License       | MIT
| Doc PR        | N/A

Alternative to #26447 without BC breaks, follows #26028.

This PR removes the need to convert `Throwable` objects into exceptions when working with `FlattenException`.

ping @instabledesign @ostrolucky @nicolas-grekas

Commits
-------

96c1a6f Support any Throwable object in FlattenException.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ostrolucky ostrolucky ostrolucky requested changes

@derrabus derrabus derrabus requested changes

Assignees
No one assigned
Labels
Bug Debug Status: Needs Work
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
6 participants
@instabledesign @nicolas-grekas @derrabus @sroze @ostrolucky @carsonbot