[Debug] Different trace message style on application code #23152
Conversation
…ation code (rentalhost)
|
@rentalhost by default we expand the first code sample outside the vendor/ dir, so you can go directly to that (because, as you said, there's nothing to do in vendor/ because you can't change that). I see that you are using Laravel ... maybe that feature is not working in your case? By the way, you won't see this in the next Laravel version to be released very soon, because they are adding back the Whops library. |
|
@javiereguiluz Well, for some reason, I see all mixed on stacktrace: vendor and user code. It make a lot of sense to me, once that the user code calls some vendor method that, via Closures, for instance, call an user code again (and so on). But, I still have a big difficulty that is to locate "what is my code and what is not" (in general, is on my code that I should starts to debug to find out a problem). This picture is from a code example from Laravel (I injected this modified class to my project to test it before PR), but I tried too on Tests and it works fine (on case, I could identify easily what is user and vendor code). And yep, I know that Laravel will back to Whoops, but for some reason seems that symfony/debug is being used on current versions. Possibly I will not see this change on Laravel on future, but I guess that is a useful feature for symfony users anyway. |
| $vendorRoot = static::getVendorRoot(); | ||
| $traceFileRelative = str_replace($vendorRoot, '', realpath($trace['file'])); | ||
|
|
||
| if (strpos($traceFileRelative, '/vendor/') === 0) { |
There was a problem hiding this comment.
the generic logic to get vendors and composer root is a bit more involving, see LinkStub::getComposerRoot():
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/VarDumper/Caster/LinkStub.php#L64
There was a problem hiding this comment.
It is a great idea reuse that. But currently the VarDumper is not a dependency of Debug component. Can I turn it a dependency?
There was a problem hiding this comment.
ping @rentalhost, ok for this comment?
There was a problem hiding this comment.
Hm... How much this method is better than the currently implemented? I mean, I have based the vendor directory on the ClassLoader class from composer, and it is more simple than LinkStub::getComposerRoot() method. And just copy/paste don't solve the problem, I should adapt the code anyway.
There was a problem hiding this comment.
@rentalhost sorry for the delay. The code to look for vendors in LinkStub handles two things yours doesn't:
- there can be several vendor directories - eg the global one, the app's, phpunit's when running test, to consider a not uncommon situation
- the vendor directories can have a different name than "vendor"
|
I don't disagree with this feature ... but I disagree with the proposed visual design of this feature. If this feature is finally accepted, I'll propose a different design to see if it's accepted by the reviewers. Thanks! |
|
@javiereguiluz What you like to change? I can do it for now, once that it is not the "final" yet. We can discuss about your idea here. |
|
It would be great to have similar changes for the CLI output, and for the plain text stack trace (obviously not with color in that particular case). |
|
@javiereguiluz I let you drive this PR forward :) |
|
@rentalhost any time to finish this PR by the end of the week? feature freeze is coming :) |
|
I'd like to not rush this feature until we can think of a better visual design. Thanks! |
|
Moving to 4.1 then. |
|
@javiereguiluz What do we do here? There is no rush, but after 6 months of wait, I think we need to take a decision here. |
|
Closing in favor of #26671 Thanks @rentalhost for your work on this pull request. It made Symfony better in the end even if your code is not going to be merged. |
… (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>  It works like a toggle ... and it's compatible with the overall toggle of each trace header:  When exceptions are complex, the amount of noise removed is massive: ## Before  ## After  </details> Commits ------- 510b05f More compact display of vendor code in exception pages
With this PR, the debug stacktrace now differ the background-color of the row when the file is outside the vendor folder, make more clear to identify where the problem starts (that is generally from user code).
Example: