[Debug] Support any Throwable object in FlattenException #26528

derrabus · 2018-03-14T14:12:42Z

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

chalasr · 2018-03-14T14:25:18Z

src/Symfony/Component/Debug/Exception/FlattenException.php

+     *
+     * @return static
+     */
+    public static function create($throwable, $statusCode = null, array $headers = array())


your #26447 (comment) comment applies as well, removing a typehint breaks childs

You're right, it will raise a warning. I'll add another deprecation then.

nicolas-grekas · 2018-03-14T16:05:13Z

src/Symfony/Component/Debug/Exception/FlattenException.php

+     */
    public static function create(\Exception $exception, $statusCode = null, array $headers = array())
+    {
+        @trigger_error(sprintf('"%s" is deprecated since Symfony 4.1. Use "createFromThrowable()" instead.', __METHOD__), E_USER_DEPRECATED);


maybe we should not deprecate this method and save us the version bumps?

a lot of code in this class will never be reached in case the throwable is an \Error (see instanceof *ExtensionInterface checks).
If we really want a flatten error, couldn't it just be a new FlattenError? extracting the reusable logic in a trait if any

Tbh, given the error handling in place I fear that anyways the result will be more confusing than useful here, perhaps it's just me

👍 for FlattenError

I did not expect those version bumps to be a problem, but we can add the depreciation at a later point, I guess. I'm going to remove the deprecation an revert the changes I made to other components.

If I added a separate class for errors, I would need to duplicate a lot of code, lose the ability to properly type-hint for FlattenException and I'd still have to do type-switches when working with arbitrary throwables. Sorry, but this is really a bad idea, imho.

derrabus · 2018-03-15T08:47:54Z

I've remove the deprecation of the create method and rolled back the changes in TwigBundle and HttpKernel for now, as @nicolas-grekas suggested. PR is ready.

nicolas-grekas · 2018-03-15T08:50:03Z

src/Symfony/Component/Debug/Tests/Exception/FlattenExceptionTest.php

    public function testStatusCode()
    {
-        $flattened = FlattenException::create(new \RuntimeException(), 403);
+        $flattened = FlattenException::createFromThrowable(new \RuntimeException(), 403);


unless required, these changes should also be reverted, this will help merges as usual

Understandable, although I'd prefer to keep the test as it is. I'll change it.

derrabus · 2018-03-15T09:17:18Z

All right, the change set should be minimal now. Still, my goal would be to remove the create method with Symfony 5, because it's redundant now.

nicolas-grekas · 2018-03-16T09:59:00Z

src/Symfony/Component/Debug/Exception/FlattenException.php

+    public static function createFromThrowable(\Throwable $throwable, ?int $statusCode = null, array $headers = array()): self
    {
        $e = new static();
-        $e->setMessage($exception->getMessage());


sorry, anynother change: let's keep the old name, it's still fine

Fair enough.

derrabus · 2018-03-16T10:14:57Z

Status: Needs Review

instabledesign

Sounds good.

derrabus · 2018-03-16T12:15:09Z

The failure on Travis looks unrelated.

derrabus · 2018-03-19T19:25:27Z

Anything left to do here?

fabpot · 2018-03-27T12:09:54Z

src/Symfony/Component/Debug/Exception/FlattenException.php

+        return static::createFromThrowable($exception, $statusCode, $headers);
+    }
+
+    public static function createFromThrowable(\Throwable $exception, ?int $statusCode = null, array $headers = array()): self


What about using new instead (FlattenException::new() looks better to me)?

it's not possible to call a function "new" because the keyword is reserved...

Works in php 7 https://3v4l.org/N5iEr

Well, then the developer would have the choice between create() and new() which looks rather confusing to me. Also, I would not recommend to use keywords as method names even if it currently works.

fabpot · 2018-03-27T12:10:06Z

src/Symfony/Component/Debug/Exception/FlattenException.php

    public function setTraceFromException(\Exception $exception)
    {
-        $this->setTrace($exception->getTrace(), $exception->getFile(), $exception->getLine());
+        @trigger_error(sprintf('"%s" is deprecated since Symfony 4.1. Use "setTraceFromThrowable()" instead.', __METHOD__), E_USER_DEPRECATED);


should be ... since Symfony 4.1, use ...

fabpot · 2018-04-06T05:54:15Z

Thank you @derrabus.

@instabledesign

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

…r (derrabus) This PR was merged into the master branch. Discussion ---------- FlattenException might also represent an instance of Error This PR documents symfony/symfony#26528 and fixes #9636. Commits ------- cd309c9 FlattenException might also represent an instance of Error.

carsonbot added Status: Needs Review Feature Deprecation labels Mar 14, 2018

derrabus changed the title ~~Support any Throwable object in FlattenException~~ [Debug] Support any Throwable object in FlattenException Mar 14, 2018

derrabus force-pushed the flatten-exception-with-throwable branch from 4813746 to ba00693 Compare March 14, 2018 14:15

chalasr reviewed Mar 14, 2018

View reviewed changes

derrabus force-pushed the flatten-exception-with-throwable branch 4 times, most recently from 10cef07 to 9a2cf0c Compare March 14, 2018 15:49

nicolas-grekas added this to the 4.1 milestone Mar 14, 2018

nicolas-grekas reviewed Mar 14, 2018

View reviewed changes

nicolas-grekas mentioned this pull request Mar 14, 2018

[Debug] Allow flatten throwable #26447

Closed

derrabus force-pushed the flatten-exception-with-throwable branch from 9a2cf0c to f2782ad Compare March 15, 2018 08:42

nicolas-grekas reviewed Mar 15, 2018

View reviewed changes

derrabus force-pushed the flatten-exception-with-throwable branch 3 times, most recently from 4fdb120 to 47b06a7 Compare March 15, 2018 09:11

nicolas-grekas requested changes Mar 16, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 16, 2018

derrabus force-pushed the flatten-exception-with-throwable branch from 47b06a7 to f78a15a Compare March 16, 2018 10:07

carsonbot added Status: Needs Review and removed Status: Needs Work Status: Needs Review labels Mar 16, 2018

instabledesign approved these changes Mar 16, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Mar 16, 2018

derrabus force-pushed the flatten-exception-with-throwable branch from f78a15a to 25e5cdf Compare March 16, 2018 10:49

nicolas-grekas approved these changes Mar 22, 2018

View reviewed changes

fabpot requested changes Mar 27, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Mar 27, 2018

Support any Throwable object in FlattenException.

96c1a6f

derrabus force-pushed the flatten-exception-with-throwable branch from 25e5cdf to 96c1a6f Compare April 5, 2018 17:04

carsonbot added Status: Reviewed and removed Status: Needs Work labels Apr 6, 2018

fabpot approved these changes Apr 6, 2018

View reviewed changes

fabpot merged commit 96c1a6f into symfony:master Apr 6, 2018

derrabus deleted the flatten-exception-with-throwable branch April 6, 2018 08:22

javiereguiluz mentioned this pull request Apr 20, 2018

[Debug] Support any Throwable object in FlattenException symfony/symfony-docs#9636

Closed

derrabus mentioned this pull request Apr 30, 2018

FlattenException might also represent an instance of Error symfony/symfony-docs#9710

Merged

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

Uh oh!

[Debug] Support any Throwable object in FlattenException #26528

[Debug] Support any Throwable object in FlattenException #26528

Uh oh!

Conversation

derrabus commented Mar 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derrabus commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derrabus commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derrabus commented Mar 16, 2018

Uh oh!

instabledesign left a comment

Choose a reason for hiding this comment

Uh oh!

derrabus commented Mar 16, 2018

Uh oh!

derrabus commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Apr 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nicolas-grekas Apr 2, 2018 •

edited

Loading