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] Do not quote numbers in stack trace #19079

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

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

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


* `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

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

@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
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
fabpot added a commit that referenced this pull request Jun 20, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants