Skip to content

[Debug] Mimic __toString php behavior in FlattenException #28879

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
Merged
fabpot merged 1 commit into from
Mar 31, 2019
Merged

[Debug] Mimic __toString php behavior in FlattenException #28879

fabpot merged 1 commit into from
Mar 31, 2019
+70 −0

Conversation

@Deltachaos
Copy link
Contributor

@Deltachaos Deltachaos 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.

stof
stof reviewed Oct 15, 2018
View reviewed changes
@Deltachaos Deltachaos force-pushed the flatten-trace-as-string branch from 13eb183 to 1fdaa39 Compare October 15, 2018 15:11
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 15, 2018 •
edited
Loading

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 flatten-trace-as-string branch from 1fdaa39 to 701a540 Compare October 15, 2018 15:12
@Deltachaos
Copy link
Contributor Author

Deltachaos 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 flatten-trace-as-string branch 6 times, most recently from 3fa4f16 to 0323df8 Compare October 15, 2018 15:57
@nicolas-grekas nicolas-grekas added this to the next milestone Oct 17, 2018
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 17, 2018

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

@fabpot
Copy link
Member

fabpot 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
Copy link
Contributor Author

Deltachaos commented Oct 17, 2018 •
edited
Loading

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

nicolas-grekas
nicolas-grekas reviewed Oct 20, 2018
View reviewed changes
@fabpot
Copy link
Member

fabpot commented Feb 21, 2019

Any news here?

@Deltachaos Deltachaos force-pushed the flatten-trace-as-string branch 3 times, most recently from 4cc7e46 to ff2f7d2 Compare February 21, 2019 13:03
@Deltachaos Deltachaos force-pushed the flatten-trace-as-string branch from ff2f7d2 to 514a1b5 Compare February 21, 2019 13:05
@Deltachaos
Copy link
Contributor Author

Deltachaos commented Feb 21, 2019

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

@fabpot
Copy link
Member

fabpot commented Mar 31, 2019

Thank you @Deltachaos.

@fabpot fabpot merged commit 514a1b5 into symfony:master Mar 31, 2019
fabpot added a commit that referenced this pull request Mar 31, 2019
@fabpot
feature #28879 [Debug] Mimic __toString php behavior in FlattenExcept…
67083b6 
…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 mentioned this pull request May 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

Assignees

No one assigned

Labels

Debug Feature Status: Reviewed

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

5 participants

@Deltachaos @nicolas-grekas @fabpot @stof @carsonbot