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

Redesigned the exception pages #20951

Closed
wants to merge 5 commits into from

Conversation

Projects
@javiereguiluz
Copy link
Member

commented Dec 16, 2016

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 %}

This comment has been minimized.

Copy link
@stof

stof Dec 16, 2016

Member

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>

This comment has been minimized.

Copy link
@stof

stof Dec 16, 2016

Member

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>

This comment has been minimized.

Copy link
@stof

stof Dec 16, 2016

Member

why an empty div ?

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

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

This comment has been minimized.

Copy link
@stof

stof Dec 16, 2016

Member

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

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Dec 16, 2016

Author Member

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

@Tobion

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Dec 16, 2016

@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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2016

(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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2016

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

commented Dec 19, 2016

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Jan 19, 2017

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

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

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

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

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

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2017

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

This comment has been minimized.

Copy link
Member

commented Mar 22, 2017

Last call for 3.3.

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2017

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

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 5, 2017

Thank you @javiereguiluz.

@fabpot fabpot closed this in d33c0ee Apr 5, 2017

@sstok

This comment has been minimized.

Copy link
Contributor

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.

@nicolas-grekas nicolas-grekas moved this from WIP to DONE in Profiler panels Apr 20, 2017

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

fabpot added a commit that referenced this pull request Jul 6, 2017

bug #22439 [DX] [TwigBundle] Enhance the new exception page design (s…
…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

bug #22439 [DX] [TwigBundle] Enhance the new exception page design (s…
…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

feature symfony#20951 Redesigned the exception pages (javiereguiluz)
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
You can’t perform that action at this time.