Skip to content

[Debug] Do not quote numbers in stack trace #19079

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
merged 1 commit into from
Jun 19, 2016
Merged

[Debug] Do not quote numbers in stack trace #19079

merged 1 commit into from
Jun 19, 2016

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Jun 16, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

In the debug output from the exception handler, integers and floats are quoted. This adds unnecessary noise to the error page and is just wrong.

Fixing this requires introduces two new type descriptions in the output from getTrace(), so the change is not fully backwards compatible.

@c960657 c960657 force-pushed the debug-unquote-numbers branch 2 times, most recently from 670bd75 to c560ce5 Compare June 16, 2016 16:39
fabpot
fabpot reviewed Jun 16, 2016
View reviewed changes
UPGRADE-4.0.md
@@ -1,6 +1,11 @@
UPGRADE FROM 3.x to 4.0
=======================

Debug
-----
* `FlattenException::getTrace()` now returns additional type descriptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing empty line after the title

@c960657 c960657 force-pushed the debug-unquote-numbers branch from c560ce5 to 9cb6bcc Compare June 16, 2016 17:30
@fabpot
Copy link
Member

fabpot commented Jun 16, 2016

What about just removing the (string) cast of the fallback. I think this should be enough.

@c960657
Copy link
Contributor Author

c960657 commented Jun 16, 2016

@fabpot I'm not sure which cast you are referring to?

If we remove the cast in FlattenException::flattenArgs(), we will be labeling numbers as string, and such values are explicitly quoted in CodeExtension::formatArgs() and ExceptionHandler::formatArgs(). And it seems wrong to label numbers as strings.

If we remove the cast in CodeExtension::formatArgs() and ExceptionHandler::formatArgs(), we might as well remove the special case for string also and handle strings and numbers using the same var_export().

@fabpot
Copy link
Member

fabpot commented Jun 16, 2016

In $formattedValue = str_replace("\n", '', var_export($this->escapeHtml((string) $item[1]), true));

@c960657 c960657 force-pushed the debug-unquote-numbers branch from 9cb6bcc to 9b708c2 Compare June 16, 2016 21:03
@c960657
Copy link
Contributor Author

c960657 commented Jun 16, 2016

Like this?

@c960657
Copy link
Contributor Author

c960657 commented Jun 16, 2016

I also changed the flags for htmlspecialchars() in CodeExtension to avoid escaping the single-quotes generated by var_export(). This change is similar to that in #18841 (and probably should have been included in that fix).

@fabpot
Copy link
Member

fabpot commented Jun 17, 2016

Yes! New integer and float types can be removed now.

@c960657
Copy link
Contributor Author

c960657 commented Jun 17, 2016

It seems wrong to label integers and floats as string, doesn’t it?

xabbuh
xabbuh reviewed Jun 17, 2016
View reviewed changes
UPGRADE-4.0.md

* `FlattenException::getTrace()` now returns additional type descriptions
`integer` and `float`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information should also be in the changelog.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xabbuh I am confused about the branching/release strategy and how changes such as this one are handled. I suppose the change will be included in Symfony 3.2 (and no previous versions), right? Which changelog file should I add it to? The newest CHANGELOG file in the master branch is for 3.1.

Copy link
Member

@xabbuh xabbuh Jun 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wer only have one changelog Liter (damn you auto-completion) per component. You can just start a new section by adding a new headline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xabbuh Done. I hope I got it right.

@fabpot
Copy link
Member

fabpot commented Jun 17, 2016

@c960657 Right

@c960657 c960657 force-pushed the debug-unquote-numbers branch from 9b708c2 to fb10b33 Compare June 18, 2016 20:40
@fabpot
Copy link
Member

fabpot commented Jun 19, 2016

Thank you @c960657.

@fabpot fabpot merged commit fb10b33 into symfony:master Jun 19, 2016
fabpot added a commit that referenced this pull request Jun 19, 2016
@fabpot
feature #19079 [Debug] Do not quote numbers in stack trace (c960657)
414a4b4 
This PR was merged into the 3.2-dev branch.

Discussion
----------

[Debug] Do not quote numbers in stack trace

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

In the debug output from the exception handler, integers and floats are quoted. This adds unnecessary noise to the error page and is just wrong.

Fixing this requires introduces two new type descriptions in the output from getTrace(), so the change is not fully backwards compatible.

Commits
-------

fb10b33 [Debug] Do not quote numbers in stack trace
@xabbuh xabbuh mentioned this pull request Jun 20, 2016
Merged
fabpot added a commit that referenced this pull request Jun 20, 2016
@fabpot
minor #19116 [Debug] fix resource type test on HHVM (xabbuh)
8868e0e 
This PR was merged into the 3.2-dev branch.

Discussion
----------

[Debug] fix resource type test on HHVM

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

Commits
-------

2d03ee8 [Debug] fix resource type test on HHVM
@c960657 c960657 deleted the debug-unquote-numbers branch June 21, 2016 06:45
@fabpot fabpot mentioned this pull request Oct 27, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
BC Break Debug Feature Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@c960657 @fabpot @xabbuh @carsonbot