[Debug] Support showing exceptions as plain text #18722

c960657 · 2016-05-07T14:16:31Z

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

The exception listener displays a pretty HTML page. This is nice when developing web pages but less pratical when making a JSON API, working in the terminal etc.

In some cases you can just omit enabling the Symfony exception listener under these circumstances and just use PHP's built-in error messages, but that makes exceptions be handled by a very different code path. The server does not send a Content-Type: text/plain header, so the stack trace looks garbled if you view them in a regular browser (outside the view source window). And sometimes you don't register the class with set_exception_handler() but simply calls it to get a nicely formatted version of an exception, e.g. in Silex\ExceptionHandler->onSilexError.

This patch adds support for generating plain-text output in ExceptionHandler. The output is more similar to the HTML version than that of PHP's built-in error messages.

The patch also adds a some detection logic that tries to guess whether HTML or plain-text is appropriate in a given request. This is just a guess but hopefully a better one than always assuming HTML.

While moving the HTML generation code to HtmlFormatter I made a few minor changes:

I removed the vendor prefixes for the border-radius CSS rules. These are supported by all modern browsers.
I changed the htmlspecialchars() flag from ENT_QUOTES (escape both single and double quotes) to ENT_COMPAT (escape double quotes, not single quotes). This avoids some unnecessary escaping (makes it easier to read in view source mode). Escaping single quotes is only required for values enclosed in single quoted HTML attributes - all our attributes are double-quoted.
I added a <title> element, making the output valid HTML5.

phansys · 2016-05-08T15:44:25Z

src/Symfony/Component/Debug/Formatter/Formatter.php

+
+use Symfony\Component\Debug\Exception\FlattenException;
+
+interface Formatter


Missing Interface suffix.

phansys · 2016-05-08T21:15:10Z

src/Symfony/Component/Debug/Formatter/FormatterInterface.php

+     * Gets the formatted exception.
+     *
+     * @param FlattenException The exception to format.
+     * @param bool             Whether to output detailed debug information.


Did you miss the @return docblock?

Yes, sorry, fixed :)

phansys · 2016-05-08T21:40:48Z

src/Symfony/Component/Debug/Exception/FlattenException.php

@@ -47,7 +48,16 @@ public static function create(\Exception $exception, $statusCode = null, array $
            $statusCode = 500;
        }

+        switch ($statusCode) {
+            case 404:
+                $title = 'Sorry, the page you are looking for could not be found.';


If you're proposing this feature for an API, I think the term "page" should be reworded. I really don't know which could be better: "route", "resource", "path", "endpoint", "URI"?

"route", "resource" and "endpoint" are very technical terms that most regular users wont understand.

I prefer "URL" (I agree with this WHATWG spec that the terms URI and IRI are confusing and not widely used).

Inspired by Google's 404 page I suggest the following:
"Sorry, the requested URL was not found on this server."

I think this is neutral, to the point, and does not use too much tech lingo.

nicolas-grekas · 2016-05-08T23:08:38Z

If I recall correctly, we already refused a PR of mine that ended up echo non-json output in a json response. Which would also be the case here, isn't it?

c960657 · 2016-05-11T20:48:45Z

@nicolas-grekas Which PR are you referring to?

nicolas-grekas · 2016-05-11T21:14:39Z

I can't find it bu basically, it was about allowing dump() to generate its output in a json advertised page.

c960657 · 2016-05-12T06:05:49Z

I think you are referring to PR #14478, but there isn't much discussion there. If somebody has arguments against this proposal, please post them here.

fabpot · 2016-06-22T05:55:10Z

ping @nicolas-grekas

nicolas-grekas · 2016-06-22T06:23:48Z

src/Symfony/Component/Debug/Debug.php

     */
-    public static function enable($errorReportingLevel = E_ALL, $displayErrors = true)
+    public static function enable($errorReportingLevel = E_ALL, $displayErrors = true, FormatterInterface $formatter = null)


I don't think we need add this argument: this is a helper, let's keep it simple, if people want to inject their formatter, they can do the job by themselves.

nicolas-grekas · 2016-06-22T06:56:13Z

I've got the point :) In fact, I like the idea: when html is not the best dumping format, switching to text.
I'm wondering if an implementation based on VarDumper wouldn't be better to both enhance HTMl rendering and allow easily switching to text (or even CLI colored output if we were to use the handler this way btw).

fabpot · 2016-11-09T23:40:08Z

@c960657 What's the status of this PR?

c960657 · 2016-11-15T17:54:08Z

@fabpot Hmm, I kind of forgot about this. Let's see if we can agree on how to proceed.

@nicolas-grekas (this is a response to an old question) I like the thought about just dumping the stack trace with VarDumper, but that would imply a big change to how the stack traces look. Personally I think the output from VarDumper is rather ugly, so this may be a controversial change. I think it is a good idea, but I would like to get some consensus about this before proceeding.

fabpot · 2017-03-05T21:04:29Z

@nicolas-grekas Any feedback?

julienfalque · 2017-03-06T09:25:14Z

What about supporting JSON as well?

nicolas-grekas · 2017-07-06T15:38:06Z

@c960657 still interested in moving forward? This needs a rebase on 3.4, where the HTML output has changed a lot. Maybe you should remove all that part and focus on text/plain only in this PR (and keep the rest for new PRs.)
Please see my previous comments also.

c960657 · 2017-07-13T21:38:53Z

Sorry about closing the PR — fat-finger error.

@nicolas-grekas I have rerolled the patch.

The title stuff is removed, and Debug::enable() no longer takes a $formatter argument.

The HtmlFormatter has been updated with the new HTML layout. But I'm not sure what you meant with “Maybe you should remove all that part and focus on text/plain only in this PR”?

@fabpot

This PR was merged into the 2.7 branch. Discussion ---------- [Debug] Missing escape in debug output | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | When pretty-printing an exception, the debug handler does not properly escape array keys. The problem only occurs when debug output is enabled, so this is not considered a [security issue](http://symfony.com/doc/current/contributing/code/security.html) (according to @fabpot), because the debug tools [should not be used in production](https://symfony.com/doc/current/components/debug.html#usage). A test for this is included in my patch for #18722. Commits ------- 636777d [Debug] HTML-escape array key

nicolas-grekas · 2017-09-26T16:27:18Z

@c960657 up to finish this one for the end of the week? I already rebased it locally but cannot push on your repos, would you mind allowing it (or rebasing?)
Then, there'll be some details I'd like to change before merging.

c960657 · 2017-09-27T05:34:23Z

@nicolas-grekas I have granted you access to my repo. Let me know if you need anything else from me.

nicolas-grekas · 2017-09-27T15:25:39Z

Looking more carefully at the proposed implementation, I think we should not do this move.
I actually spend some time rebasing the code on latest 3.4, then trying to open/close the new interfaces as strictly as I could in order to easy future maintenance. Then I gave up. Not because we cannot do it nor because the current is behind (that's not the case at all, the code is really fine.)

But the reason is that I still have VarDumper in mind. I read the output of VarDumper may not be looking nice to everyone, yet I still think this is a much better move. It should be noted that the output improved a lot in 3.4 for exception traces. The inspection capabilities in VarDumper are far superior than what we might achieve here in the future (because we just won't do it twice and the amount of work put in VarDumper is already huge.)

So, instead of adding a new concept in ExceptionHandler (the FormatterInterface), I'd really prefer to work on using VarDumper in Debug, and improving it if needed.

I do share the goal of having a nice non-HTML output. CliDumper provides it already.

@c960657 sorry about this, and thank you for your work!

nicolas-grekas · 2017-09-28T09:14:41Z

Replaced by issue #24354 so we won't forget about it.

carsonbot added the Status: Needs Review label May 7, 2016

c960657 force-pushed the debug-text branch 3 times, most recently from 6be52fb to 7c28248 Compare May 7, 2016 15:12

javiereguiluz added Feature Debug labels May 8, 2016

phansys reviewed May 8, 2016
View reviewed changes

c960657 force-pushed the debug-text branch from 7c28248 to 68d2179 Compare May 8, 2016 21:10

phansys reviewed May 8, 2016
View reviewed changes

c960657 force-pushed the debug-text branch from 68d2179 to afcefda Compare May 8, 2016 21:17

phansys reviewed May 8, 2016
View reviewed changes

nicolas-grekas reviewed Jun 22, 2016
View reviewed changes

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

nicolas-grekas mentioned this pull request Jan 16, 2017

Incorrect variable dumps raw profiler content into web #21170

Closed

nicolas-grekas self-requested a review April 8, 2017 18:58

nicolas-grekas removed their request for review July 11, 2017 10:11

c960657 force-pushed the debug-text branch from afcefda to b220083 Compare July 13, 2017 21:26

c960657 closed this Jul 13, 2017

c960657 reopened this Jul 13, 2017

c960657 force-pushed the debug-text branch 3 times, most recently from a1782bf to 22ecf2f Compare July 13, 2017 21:38

c960657 force-pushed the debug-text branch from 22ecf2f to 669bbc1 Compare July 13, 2017 21:41

c960657 force-pushed the debug-text branch 2 times, most recently from 3c4e2c9 to 1063443 Compare July 26, 2017 13:27

c960657 mentioned this pull request Jul 26, 2017

[Debug] Missing escape in debug output #23684

Merged

nicolas-grekas self-requested a review July 26, 2017 13:55

c960657 force-pushed the debug-text branch from 1063443 to 4971e2f Compare August 6, 2017 15:49

nicolas-grekas changed the base branch from master to 3.4 September 26, 2017 15:55

[Debug] Support showing exceptions as plain text

ce73240

nicolas-grekas force-pushed the debug-text branch from 4971e2f to ce73240 Compare September 27, 2017 06:14

nicolas-grekas modified the milestones: 3.4, 4.1 Sep 28, 2017

nicolas-grekas mentioned this pull request Sep 28, 2017

[Debug] Support showing exceptions as plain text #24354

Closed

nicolas-grekas closed this Sep 28, 2017


		use Symfony\Component\Debug\Exception\FlattenException;

		interface Formatter

Uh oh!

[Debug] Support showing exceptions as plain text #18722

[Debug] Support showing exceptions as plain text #18722

Uh oh!

Conversation

c960657 commented May 7, 2016 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phansys May 8, 2016

Choose a reason for hiding this comment

Uh oh!

phansys May 8, 2016

Choose a reason for hiding this comment

Uh oh!

c960657 May 8, 2016

Choose a reason for hiding this comment

Uh oh!

phansys May 8, 2016

Choose a reason for hiding this comment

Uh oh!

c960657 May 11, 2016

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented May 8, 2016

Uh oh!

c960657 commented May 11, 2016

Uh oh!

nicolas-grekas commented May 11, 2016

Uh oh!

c960657 commented May 12, 2016

Uh oh!

fabpot commented Jun 22, 2016

Uh oh!

nicolas-grekas Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jun 22, 2016

Uh oh!

fabpot commented Nov 9, 2016

Uh oh!

c960657 commented Nov 15, 2016

Uh oh!

fabpot commented Mar 5, 2017

Uh oh!

julienfalque commented Mar 6, 2017

Uh oh!

nicolas-grekas commented Jul 6, 2017

Uh oh!

c960657 commented Jul 13, 2017

Uh oh!

nicolas-grekas commented Sep 26, 2017

Uh oh!

c960657 commented Sep 27, 2017

Uh oh!

nicolas-grekas commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Sep 28, 2017

Uh oh!

Uh oh!

c960657 commented May 7, 2016 •

edited by nicolas-grekas

Loading

nicolas-grekas commented Sep 27, 2017 •

edited

Loading