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

More compact display of vendor code in exception pages #26671

Closed
wants to merge 2 commits into
base: master
from

Conversation

@javiereguiluz
Member

javiereguiluz commented Mar 26, 2018

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

I like the general idea proposed in #23152 ... but I don't like the implementation ... so this PR is an alternative implementation.

UPDATE: the proposed feature has been completely redesigned. See #26671 (comment)

The idea is to hide by default all information that comes from "vendors" (vendor/ or var/cache/ dir) because that code is out of your reach and you can't change it to fix your error.

In practice, each exception trace now would display a Hide vendors option enabled by default:

Click here to view the outdated images

hide-vendors

It works like a toggle ... and it's compatible with the overall toggle of each trace header:

hide-vendors

When exceptions are complex, the amount of noise removed is massive:

Before

before

After

after

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Mar 26, 2018

This is really nice!

@lyrixx

This comment has been minimized.

Member

lyrixx commented Mar 26, 2018

I have often some issues in vendor so for me it will be more complicated to get to the point.
If you really want to go this way, could you add a "remember my last choice" cookie ?
Like we do with the debug bar (expanded / collapsed)

Thanks

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 26, 2018

Would be great if the toggle could be inplace of the hidden vendors. Putting it on the top right corner makes it hard to spot.
We need a visual hint (some ellipses) that there is something hidden.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 26, 2018

Or instead of hidding the full vendor frame, can't we just hide the arguments?
I'd be extra cautious with hidding debugging info, because debugging generally needs full context to understand an issue.

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Mar 26, 2018

After reading your comments, I propose a complete redesign of this feature. Now we don't hide anything, we just make info more compact for vendors and keep the existing design for your code.

Simple exceptions

Before

v3-simple-before

After

v3-simple-after

Complex exceptions

Before

v3-before

After

v3-after

@javiereguiluz javiereguiluz changed the title from Hide vendors by default in exception pages to More compact display of vendor code in exception pages Mar 26, 2018

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Mar 26, 2018

That's really better this way. I love it 👍

@fabpot

fabpot approved these changes Mar 26, 2018

@sroze

Very nice idea 👍

<div class="trace-line">
{{ include('@Twig/Exception/trace.html.twig', { prefix: index, i: i, trace: trace, _display_code_snippet: _display_code_snippet }, with_context = false) }}
<div class="trace-line {{ _is_vendor_trace ? 'trace-from-vendor' }}">
{{ include('@Twig/Exception/trace.html.twig', { prefix: index, i: i, trace: trace, style: _is_vendor_trace ? 'compact' : _display_code_snippet ? 'expanded' }, with_context = false) }}

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member

Shouldn't it be style: _is_vendor_trace ? 'compact' : 'expanded' instead? 🤔

This comment has been minimized.

@javiereguiluz

javiereguiluz Mar 27, 2018

Member

This is a bit trickier because we have 3 states: compact for vendors, expanded for the only trace that displays the code snippet expanded (there should be just one) and normal which displays your code but doesn't expand the code snippet.

This comment has been minimized.

@phansys

phansys Mar 27, 2018

Contributor

Maybe I'm missing something, but doesn't this check will interpret any application path containing "/vendor/" as part of vendor code? I know my point is really border, but could we check if the path is really in the project's /vendor directory?

This comment has been minimized.

@javiereguiluz

javiereguiluz Mar 27, 2018

Member

Although you are right, I'd prefer to ignore that edge case: having /vendor/ in the file path and not being a real vendor. Moreover, in the last iteration of this feature we no longer hide anything, so the user won't miss anything important even in that edge case. Cheers!

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 27, 2018

Thank you @javiereguiluz.

@fabpot fabpot closed this Mar 27, 2018

fabpot added a commit that referenced this pull request Mar 27, 2018

feature #26671 More compact display of vendor code in exception pages…
… (javiereguiluz)

This PR was squashed before being merged into the 4.1-dev branch (closes #26671).

Discussion
----------

More compact display of vendor code in exception pages

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

I like the general idea proposed in #23152 ... but I don't like the implementation ... so this PR is an alternative implementation.

**UPDATE**: the proposed feature has been completely redesigned. See #26671 (comment)

~~The idea is to **hide by default all information that comes from "vendors"** (`vendor/` or `var/cache/` dir) because that code is out of your reach and you can't change it to fix your error.~~

~~In practice, each exception trace now would display a `Hide vendors` option enabled by default:~~

<details>
<summary>Click here to view the outdated images</summary>

![hide-vendors](https://user-images.githubusercontent.com/73419/37895113-a9d942bc-30e0-11e8-9ff4-191dcb057d60.png)

It works like a toggle ... and it's compatible with the overall toggle of each trace header:

![hide-vendors](https://user-images.githubusercontent.com/73419/37895137-b9e8d406-30e0-11e8-82bd-5729d32aaa63.gif)

When exceptions are complex, the amount of noise removed is massive:

## Before

![before](https://user-images.githubusercontent.com/73419/37895233-f9170eea-30e0-11e8-8c21-c514007d44d2.png)

## After

![after](https://user-images.githubusercontent.com/73419/37895240-fc2e50c0-30e0-11e8-9e5a-b7a73ba57b47.png)

</details>

Commits
-------

510b05f More compact display of vendor code in exception pages
@@ -22,10 +22,11 @@
<div id="trace-html-{{ index }}" class="sf-toggle-content">
{% set _is_first_user_code = true %}
{% for i, trace in exception.trace %}
{% set _display_code_snippet = _is_first_user_code and ('/vendor/' not in trace.file) and ('/var/cache/' not in trace.file) and (trace.file is not empty) %}
{% set _is_vendor_trace = trace.file is not empty and ('/vendor/' in trace.file or '/var/cache/' in trace.file) %}

This comment has been minimized.

@stof

stof Mar 28, 2018

Member

does this work on Windows ?

@stof

This comment has been minimized.

Member

stof commented Mar 28, 2018

This is changing only the template of the TwigBundle exception controller, not the output of the Debug component, so this does not solve the same case than #23152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment