Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Unwrap errors in FlattenException#26028
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4ce7ae8 toc63a3d1Compare| publicfunction__construct(\Throwable$e) | ||
| { | ||
| $this->throwable =$e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should only keep the class name, not the instance, to make this lighter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We then would need to keep the original message as well, becauseFatalThrowableError tinkers with it. But yes, I can change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
then would need to keep the original message as well
Let's remove the tinkering yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okay, I'll remove it.
…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/AWhile 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().
| // assertEquals() does not like NAN values. | ||
| $this->assertEquals($array[$i][0],'float'); | ||
| $this->assertTrue(is_nan($array[$i++][1])); | ||
| $this->assertNan($array[$i][1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be revert as discussed previously
c63a3d1 tod547ab9Compared547ab9 tof14d7d6Comparederrabus commentedFeb 4, 2018
The fabbot failure can be ignored, see#26030 (comment) |
nicolas-grekas commentedFeb 4, 2018
Thank you@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/AThis 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.
…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/AAlternative 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-grekasCommits-------96c1a6f Support any Throwable object in FlattenException.
This is probably the most straightforward way to solve#26025.
FlattenExceptionis now unwrappingFatalThrowableErrorinstances 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
FlattenExceptionis used for rendering and logging, everything should be fine. But this PR changesFlattenException's internal behavior. If a piece of code relied on errors appearingFatalThrowableErrorinside aFlattenException, that code would break.