Skip to content

Redesigned the exception pages #20951

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

Closed
wants to merge 5 commits into from

Conversation

javiereguiluz
Copy link
Member

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

Here are some before/after screenshots:

Before After
exception-before-1 exception-after-1
Before After
exception-before-2 exception-after-2
Before After
exception-before-3 exception-after-3

And here is the new design in action because everything is very dynamic and you can click anywhere to reveal/collapse things:

exception-in-action

@@ -0,0 +1,42 @@
{% block favicon_symfony_ghost %}
{# PNG file encoded as base64 #}
{% spaceless %}
Copy link
Member

Choose a reason for hiding this comment

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

spaceless is wrong. It won't remove anything as there is no HTML tag there. Use whitespace control instead

{% endblock %}

{% block icon_chevron_left %}
<svg width="1792" height="1792" viewBox="0 0 1792 1792" xmlns="http://www.w3.org/2000/svg"><path fill="#FFF" d="M1427 301L896 832l531 531q19 19 19 45t-19 45l-166 166q-19 19-45 19t-45-19L429 877q-19-19-19-45t19-45l742-742q19-19 45-19t45 19l166 166q19 19 19 45t-19 45z"/></svg>
Copy link
Member

Choose a reason for hiding this comment

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

what about putting them in .svg files instead, and including them as @Twig/Exception/image/chevron_left.svg instead, as done in the WebProfiler ? It would mean that SVG icons are managed as real SVG files (allowing github and other tools to preview them for instance)

}
//]]></script>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

why an empty div ?

@@ -139,4 +139,5 @@

{% block body %}
{% include '@Twig/Exception/exception.html.twig' %}
{{ include('@WebProfiler/Profiler/base_js.html.twig') }}
Copy link
Member

Choose a reason for hiding this comment

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

this makes it impossible to use a debug environment where the WebProfilerBundle is not enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This exception_full thing is still work in progress. I'm having some problems with the special exceptions managed by ExceptionHandler

@Tobion
Copy link
Contributor

Tobion commented Dec 16, 2016

Nice work! Looking at the screenshots, I saw that the plain text stack traces do not include function parameters anymore. E.g. it only says call_user_func_array without the parameters (the html stack traces shows them). Is this on purpose? Looks like valuable info is lost there.

@stof
Copy link
Member

stof commented Dec 16, 2016

@Tobion good catch, I missed this change.

I think that the plain-text version should keep the args, otherwise copy/pasting the plaintext trace in a bug report will give only partial info

@javiereguiluz
Copy link
Member Author

@Tobion @stof yes, that's done on purpose. The main reason is that I wanted to have the same stack traces as the rest of the industry (from Java to Node.js). @stof made me change it a bit to display the (ugly) full namespace of the classes. But I don't think adding the args is necessary there. Java, Node.js, etc. don't do that and we already show that information in the first panel, which is where a developer should always look for info (the stack trace is to just share some info about the exception with other colleagues or publicly).

Another reason is that arguments can contain highly sensitive information. In the Symfony repo we regularly ask users to share the stack traces. We don't want them to share critical information. That's why we also remove your local project directory in the file paths (the first panel shows the absolute file paths when you put your mouse over the file paths, but the stack traces only shows paths relative to the project root directory).

@mvrhov
Copy link

mvrhov commented Dec 17, 2016

@javiereguiluz: Please take a look on how tracy/tracy does it. We've replaced the default exceptions with that via TracyBundle in all of our projects. The exceptions are a lot more detailed and having having dumps deduped in production in the log directory is invaluable.

@wouterj
Copy link
Member

wouterj commented Dec 17, 2016

Very nice job @javiereguiluz! Some little notes:

  • Looking at a previously proposed PR to improve the exception page, can we maybe use the var-dumper for the arguments in the detailed trace? This would be more awesome than just having object(Post).
  • Can we use the var-dumper colors in the code example, to make all colorscheme's consistent throughout exception page - profiler page - dump output?
  • The top sections of the page look way to dense compared to the below sections. This ratio doesn't seem to be 100% correct. Can we maybe add some vertical whitespace at the top? (e.g. the red box with the exception message in it).
  • Not sure about this one, but the second exception seems more hidden than in the original design (at first, I couldn't find the second exception at all). This is a bit of a problem, as the second exception is often more usefull than the first one (e.g. with Twig or file loader exceptions).
  • You probably are already aware of this, but the basic exception page that's triggered when a fatal error occurs also need to be updated: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Debug/ExceptionHandler.php#L203-L393

@King2500
Copy link
Contributor

In the sense of DX I suggest the visible source snippet should be the first user land code, not from vendor/* libraries. What do you think?

@javiereguiluz
Copy link
Member Author

(I haven't replied to most of your comments yet ... but I'm reading them with keen interest, so please keep adding comments and proposing changes. Thanks!)

@robfrawley
Copy link
Contributor

robfrawley commented Dec 19, 2016

@javiereguiluz

First off: amazing work. Love the direction. Think this should absolutely be the basis for whatever eventually makes it into master. I'll be adding some more comments (re the code itself) but as for the design:

  • The top section needs some additional vertical space. The first row (black) is fine, but the two below it should have more verticle space.
  • The second row order (with left chevron) seems "off" to me, would prefer the items were reverse with right chevron.
  • Syntax highlighting has too much red: there is enough red elsewhere and that makes it overpowering. Can we use a colorful syntax highlighting?
  • Very strongly think the stack trace should remain intact (include parameters). The information provided there can be very important to understand the context and provide a valuable resolution in the context of an issue/ticket. Someone posting sensitive information (or censoring sensitive information) should be their own responsibility.
  • On "Exceptions" tab, the InvalidArgumentException doesn't have the message under it as Twig_Error_Loader does; what happens if there are more than two exceptions with the sub-text (the exception message)?

@robfrawley
Copy link
Contributor

As @mvrhov mentioned, nette/tracy does a really nice job; it is definitely worth taking a look at their live examples to glean some inspiration for this Symfony exception page re-design/re-write. I love the simplicity, yet the power, of the call stack functionality.

@sstok
Copy link
Contributor

sstok commented Dec 19, 2016

Indeed. I think it would be useful to include the Environment, HTTP request, HTTP response as this could be related to the failure.

Concerning the parameters, is it possible add a JavaScript toggle to hide/show them?

@gaetan-petit
Copy link

The red header is a bit too much.
My suggestion: just keep the red line mentioning the error and the HTTP code status.
Go back to grey for the error message and ghost.

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Dec 26, 2016
@sstok
Copy link
Contributor

sstok commented Dec 27, 2016

@gaetan-petit I'm having mixed feelings on that, the exception only happens when something is wrong and red indicates danger giving you a direct indication something is wrong, so only the upper bar would a bit small.

But when you use a dark UI or can't bare bright colors I can imagine this would be to much.
I have actually spend 10 years daily on a website where the main color was red and white. So I'm used to bright colors 😄

@robfrawley
Copy link
Contributor

robfrawley commented Dec 27, 2016

@gaetan-petit I actually like the move away from dull grays, on both the redesigned debug toolbar and profiler pages, as well as this new exception pages.

@stof
Copy link
Member

stof commented Jan 19, 2017

@javiereguiluz what is the status of this PR ? Are you continuing the work ?

@javiereguiluz
Copy link
Member Author

Yes, I'm continuing the work ... but I'm taking a break here to think about your comments and proposals.

@fabpot
Copy link
Member

fabpot commented Feb 16, 2017

Any news? Do we want this to be ready for 3.3?

@javiereguiluz
Copy link
Member Author

I'll have this ready for Symfony 3.3 (we're still 7 weeks away from the "feature freeze"). I want to make some of the changes proposed by reviewers.

@fabpot
Copy link
Member

fabpot commented Mar 22, 2017

Last call for 3.3.

@javiereguiluz
Copy link
Member Author

OK, I've updated this PR to make it mergeable.


I've made these changes as requested by reviewers:

  • Fixed all errors spotted by @stof (thanks for the review!)
  • Moved icons/images to individual image files
  • Added some vertical white space to the main exception message
  • Reversed the order of the chained exceptions: "Parent > Child 1 > Child 2" instead of "Child 2 < Child 1 < Parent"
  • Reduce the font size when the main exception message is long
  • Instead of always expanding the first code snippet, now the first code snippet that belongs to the application is expanded
  • Included again all the function/method arguments in the text-based trace

I've decided to ignore these suggestions:

  • "Too much red" (I think it has the appropriate amount of red for an exception page ... but I can be wrong, and we'll see that when people actually use this page and give more feedback)
  • "Use VarDumper for arguments" (This behavior is very different from the existing exception page, and I'm afraid of the performance penalty that could introduce hundreds of calls to VarDumper. I'd recommend to try to make this change in Symfony 3.4)

@robfrawley
Copy link
Contributor

@javiereguiluz Looks like this will be a great change. Can't wait for its inclusion!

@javiereguiluz
Copy link
Member Author

The failing tests on AppVeyor are unrelated. Please tell me if I can do anything else to make this mergeable for 3.3. Thanks!

@fabpot
Copy link
Member

fabpot commented Apr 5, 2017

Thank you @javiereguiluz.

@fabpot fabpot closed this in d33c0ee Apr 5, 2017
@sstok
Copy link
Contributor

sstok commented Apr 6, 2017

It seems something is broken 😭 cleared, cache bootstrap file, assets. No dice.
In Firefox it's broken, but it works in chrome 🤔

schermafbeelding 2017-04-06 om 16 55 24

According to Firebug the trace-details table seems to rendered to wide, while it's parent node shows the correct size.

Edit. It seems .trace-code is not properly limited or wrapping.

@fabpot fabpot mentioned this pull request May 1, 2017
fabpot added a commit that referenced this pull request Jul 6, 2017
…ustmi)

This PR was squashed before being merged into the 3.3 branch (closes #22439).

Discussion
----------

[DX] [TwigBundle] Enhance the new exception page design

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

- [x] Fix the problem with wrapping wide lines (#20951 (comment))

I got motivated by recent PR #20951 and redesigned the exception page even more.

Compare before: ![before](https://cloud.githubusercontent.com/assets/885946/25052220/0756b74e-2151-11e7-98b6-c99fd9eaabec.png)
with after: ![after](https://cloud.githubusercontent.com/assets/885946/25052225/0c76581a-2151-11e7-96ff-eb502ee8e97b.png)
(Ignore the the "sf" toolbar icon in the middle of the page. This is how Firefox does full-page screenshots.)

The most noticeable changes:
- removed double line spacing (it just does not feel natural)
- file context increased from 3 to 10 lines (it helps me to better orientate in the code)
- added horizontal scrollbar for the cases when the code is wider
- the highlighted line color is more saturated in order to be noticed easily

Commits
-------

43212b9 [DX] [TwigBundle] Enhance the new exception page design
symfony-splitter pushed a commit to symfony/twig-bundle that referenced this pull request Jul 6, 2017
…ustmi)

This PR was squashed before being merged into the 3.3 branch (closes #22439).

Discussion
----------

[DX] [TwigBundle] Enhance the new exception page design

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

- [x] Fix the problem with wrapping wide lines (symfony/symfony#20951 (comment))

I got motivated by recent PR symfony/symfony#20951 and redesigned the exception page even more.

Compare before: ![before](https://cloud.githubusercontent.com/assets/885946/25052220/0756b74e-2151-11e7-98b6-c99fd9eaabec.png)
with after: ![after](https://cloud.githubusercontent.com/assets/885946/25052225/0c76581a-2151-11e7-96ff-eb502ee8e97b.png)
(Ignore the the "sf" toolbar icon in the middle of the page. This is how Firefox does full-page screenshots.)

The most noticeable changes:
- removed double line spacing (it just does not feel natural)
- file context increased from 3 to 10 lines (it helps me to better orientate in the code)
- added horizontal scrollbar for the cases when the code is wider
- the highlighted line color is more saturated in order to be noticed easily

Commits
-------

43212b9a90 [DX] [TwigBundle] Enhance the new exception page design
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
This PR was squashed before being merged into the 3.3-dev branch (closes symfony#20951).

Discussion
----------

Redesigned the exception pages

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

Here are some before/after screenshots:

| Before | After
| --- | ---
| ![exception-before-1](https://cloud.githubusercontent.com/assets/73419/21258148/f8fd6482-c37b-11e6-9efe-1bcf7b323c0f.png) | ![exception-after-1](https://cloud.githubusercontent.com/assets/73419/21258156/016059cc-c37c-11e6-8bab-80456189d614.png)

| Before | After
| --- | ---
| ![exception-before-2](https://cloud.githubusercontent.com/assets/73419/21258171/11198a46-c37c-11e6-8a28-ae45e19e3eaf.png) | ![exception-after-2](https://cloud.githubusercontent.com/assets/73419/21258223/4cb9ac66-c37c-11e6-93db-0db2c204dc0b.png)

| Before | After
| --- | ---
| ![exception-before-3](https://cloud.githubusercontent.com/assets/73419/21258239/5a0747ac-c37c-11e6-923e-564322e862a6.png) | ![exception-after-3](https://cloud.githubusercontent.com/assets/73419/21258246/62ad8b00-c37c-11e6-8838-3c1824c18287.png)

---

And here is the new design in action because everything is very dynamic and you can click anywhere to reveal/collapse things:

![exception-in-action](https://cloud.githubusercontent.com/assets/73419/21258261/7445f140-c37c-11e6-9318-f3807fe38689.gif)

Commits
-------

9d0c263 Redesigned the exception pages
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.