Skip to content
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

[Debug] Mimic __toString php behavior in FlattenException #28879

Merged
merged 1 commit into from Mar 31, 2019

Conversation

Projects
None yet
5 participants
@Deltachaos
Copy link
Contributor

commented Oct 15, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR symfony/symfony-docs#

The Symfony\Component\Debug\Exception\FlattenException object is returned by Symfony\Component\HttpKernel\DataCollector\ExceptionDataCollector::getException method, but the docblock of this method indicates that it should return \Exception object.

As the FlattenException class should behave as much as possible like a php \Exception object, it should implement the same methods as \Exception.

This PR is adding __toString and getTraceAsString methods. Those methods are (in my opinion) the most useful methods of a \Exception object. A potential use case (where i am stumbled across this inconsistency) is to get the last exception of a request in a WebTestCase using the profiler and printing the trace.

@Deltachaos Deltachaos force-pushed the Deltachaos:flatten-trace-as-string branch from 13eb183 to 1fdaa39 Oct 15, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

Thanks for the PR.
I think this has been rejected in the past (I'm not 100% sure though).
This can leak sensitive info to me, in case the cast is not planned.
Looks risky IMHO

@Deltachaos Deltachaos force-pushed the Deltachaos:flatten-trace-as-string branch from 1fdaa39 to 701a540 Oct 15, 2018

@Deltachaos

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

@nicolas-grekas For the __toString() method i at least can think of situations where it can be executed accidentally when converting something to string (which is the same for other objects). But for the getTraceAsString method it is not providing any other data that can be fetched by calling other methods of the class as well.

So i don't really see cases where this can leak sensitive data?

@Deltachaos Deltachaos force-pushed the Deltachaos:flatten-trace-as-string branch 6 times, most recently from 3fa4f16 to 0323df8 Oct 15, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 17, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Fine for getTraceAsString. Do you have a use case where this would help?

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

As the FlattenException class should behave as much as possible like a php \Exception object Actually, that's never been a goal.

@Deltachaos

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

@fabpot if this is not the goal then at least the return type declaration of Symfony\Component\HttpKernel\DataCollector\ExceptionDataCollector::getException is not correct, as this indicates to return a \Exception but at least when using this method from a WebTestCase it returns a Symfony\Component\Debug\Exception\FlattenException.

It the behaivor wanted is that Symfony\Component\HttpKernel\DataCollector\ExceptionDataCollector::getException should return either \Exception or Symfony\Component\Debug\Exception\FlattenException then both should implement the same interface. The commit 0208800 has changed the original behaivor.

@nicolas-grekas The use case i have mentioned is to access the exception of a failed request in a WebTestCase.

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Any news here?

@Deltachaos Deltachaos force-pushed the Deltachaos:flatten-trace-as-string branch 3 times, most recently from 4cc7e46 to ff2f7d2 Feb 21, 2019

@Deltachaos Deltachaos force-pushed the Deltachaos:flatten-trace-as-string branch from ff2f7d2 to 514a1b5 Feb 21, 2019

@Deltachaos

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

@fabpot have changed the method name to what @nicolas-grekas suggested. Tests have run through.

@fabpot

fabpot approved these changes Mar 31, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Thank you @Deltachaos.

@fabpot fabpot merged commit 514a1b5 into symfony:master Mar 31, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 31, 2019

feature #28879 [Debug] Mimic __toString php behavior in FlattenExcept…
…ion (Deltachaos)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Debug] Mimic __toString php behavior in FlattenException

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

The `Symfony\Component\Debug\Exception\FlattenException` object is returned by `Symfony\Component\HttpKernel\DataCollector\ExceptionDataCollector::getException` method, but the docblock of this method indicates that it should return `\Exception` object.

As the `FlattenException` class should behave as much as possible like a php `\Exception` object, it should implement the same methods as `\Exception`.

This PR is adding `__toString` and `getTraceAsString` methods. Those methods are (in my opinion) the most useful methods of a `\Exception` object. A potential use case (where i am stumbled across this inconsistency) is to get the last exception of a request in a `WebTestCase` using the profiler and printing the trace.

Commits
-------

514a1b5 [Debug] Mimic __toString php behavior in FlattenException

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.