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.

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

Unwrap errors in FlattenException #26028

Merged
merged 1 commit into from Feb 4, 2018

Conversation

Projects
None yet
4 participants
@derrabus
Contributor

derrabus commented Feb 3, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? maybe
Deprecations? no
Tests pass? no (but probably unrelated?)
Fixed tickets #26025
License MIT
Doc PR N/A

This is probably the most straightforward way to solve #26025. FlattenException is now unwrapping FatalThrowableError instances and logs the wrapped error instead. The consequence of this change is that the real error class is displayend on TwigBundle's exception page and the profiler.

Regarding BC: If we assume that FlattenException is used for rendering and logging, everything should be fine. But this PR changes FlattenException's internal behavior. If a piece of code relied on errors appearing FatalThrowableError inside a FlattenException, that code would break.

bildschirmfoto 2018-02-02 um 20 08 42

public function __construct(\Throwable $e)
{
$this->throwable = $e;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 3, 2018

Member

We should only keep the class name, not the instance, to make this lighter

This comment has been minimized.

@derrabus

derrabus Feb 4, 2018

Contributor

We then would need to keep the original message as well, because FatalThrowableError tinkers with it. But yes, I can change that.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

then would need to keep the original message as well

Let's remove the tinkering yes

This comment has been minimized.

@derrabus

derrabus Feb 4, 2018

Contributor

Okay, I'll remove it.

nicolas-grekas added a commit that referenced this pull request Feb 3, 2018

minor #26029 Removed unused parameter from flattenDataProvider() (der…
…rabus)

This PR was merged into the 2.7 branch.

Discussion
----------

Removed unused parameter from flattenDataProvider()

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no, but unrelated
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | N/A

While working on #26028, I noticed that FlattenExceptionTest::FlattenExceptionTest.php() exposes two arguments although all tests consuming that provider only use the first one. This PR removes the unnecessary second argument.

Commits
-------

d6f4f51 Removed unused parameter from flattenDataProvider().
@@ -253,7 +264,7 @@ function () {},
// assertEquals() does not like NAN values.
$this->assertEquals($array[$i][0], 'float');
$this->assertTrue(is_nan($array[$i++][1]));
$this->assertNan($array[$i][1]);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

should be revert as discussed previously

@derrabus

This comment has been minimized.

Contributor

derrabus commented Feb 4, 2018

The fabbot failure can be ignored, see #26030 (comment)

@chalasr

chalasr approved these changes Feb 4, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 4, 2018

Thank you @derrabus.

@nicolas-grekas nicolas-grekas merged commit f14d7d6 into symfony:master Feb 4, 2018

0 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Some changes should be done to comply with our standards.
Details

nicolas-grekas added a commit that referenced this pull request Feb 4, 2018

feature #26028 Unwrap errors in FlattenException (derrabus)
This PR was merged into the 4.1-dev branch.

Discussion
----------

Unwrap errors in FlattenException

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | maybe
| Deprecations? | no
| Tests pass?   | no (but probably unrelated?)
| Fixed tickets | #26025
| License       | MIT
| Doc PR        | N/A

This is probably the most straightforward way to solve #26025. `FlattenException` is now unwrapping `FatalThrowableError` instances and logs the wrapped error instead. The consequence of this change is that the real error class is displayend on TwigBundle's exception page and the profiler.

Regarding BC: If we assume that `FlattenException` is used for rendering and logging, everything should be fine. But this PR changes `FlattenException`'s internal behavior. If a piece of code relied on errors appearing `FatalThrowableError` inside a `FlattenException`, that code would break.

<img width="402" alt="bildschirmfoto 2018-02-02 um 20 08 42" src="https://user-images.githubusercontent.com/1506493/35760077-0b202940-087e-11e8-9b98-8e4ba269780c.png">

Commits
-------

f14d7d6 Unwrap errors in FlattenException.

@derrabus derrabus deleted the derrabus:display-error-class branch Feb 4, 2018

fabpot added a commit that referenced this pull request Apr 6, 2018

feature #26528 [Debug] Support any Throwable object in FlattenExcepti…
…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.

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment