[ErrorHandler] Improve error messages in CLI #42261

javiereguiluz · 2021-07-26T14:00:47Z

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

This is still WIP. Don't look or comment the code changes. Thanks!

Context

When there's an exception in your Symfony app and you're in the command console, you see something like this:

Although sometimes you see exceptions like this:

I see two problems here:

Inconsistency: not all exceptions are displayed the same way in the console
Design: both errors provide lots of information, but it should be easier to spot the exact issue

Problem

To me the main problem is designing this error message around "stack traces" (like in Java) instead of focusing on the error cause and location (like in modern languages/tools).

See "Rails + Pry" exception message:

See "Collision" exception message (used by Laravel and compatible with Symfony too):

See the amazing "Rust" error messages:

Proposal

I'd like to redesign Symfony exception messages to match that modern design.

The three main elements of the console output should be:

What happened (the exception message)
Where did it happen (file path, line number and code extract)
How/why did it happen (stack trace)

The code of this PR is a quick proof-of-concept of this idea:

The logical order would be "What -> Where -> Why" ... but "Why" is a long and ugly stack trace which hides the most important information ("What" and "Where"). That's why the order is "Why -> What -> Where".

What do you think?

Do you agree that we need to improve this?
If yo do, do you like this proposal about what ideas to follow for the redesign?
Would you agree on removing the stack trace completely form console output? (I very rarely need or use it in the console ... but I want to read other opinions)
What could we do to fix the consistency problems and always display all exception messages in the same way in the console?

Thanks!

parijke · 2021-07-26T14:15:01Z

Sounds like a plan to me... I would like to add (if possible)

What happened (the exception message)
Where did it happen (file path, line number and code extract)
How/why did it happen (stack trace)
Suggestion how to solve this

sstok · 2021-07-26T14:47:33Z

FYI. I'm not in favor of removing the stack trace (completely) in the CLI as there are times this is critically important.
Except for syntax and parsing errors as it doesn't add much usefulness then.

wouterj · 2021-07-26T15:14:11Z

For completeness, something I suggested on Slack: are there disadvantages to always using nunomaduro/collision (e.g. in the flex recipe) and use the current exception rendering when it's not installed?

Suggestion how to solve this

I would say this should be included in the exception message. We often try to make exception messages actionable, so please open an issue if you find a vague non-actionable exception message :)

Ahummeling · 2021-07-26T18:04:51Z

I think it could be improved, but wouldn't consider it a critical priority by any means.
I'm okay with the initial proposal, but I think that this is something for which no standard can be made. Everyone processes information differently, I think different preferences will always be present amongst developers.

Right now we can configure our console output through means of verbosity flags. Perhaps it would be do-able to describe the output of error messages in means of smaller message parts. With those parts being the 3 points being named.
Have a configurable service responsible for formatting the console output, have some sensible defaults and allow the developer to modify the configuration as is done with any component.
Just some idea that shot into my mind, just putting it out there.

On another note, I generally find the exception messages with red backgrounds rather difficult to read on the jetbrains built-in terminals, because of the white text. Maybe it's some configuration about graphics, I don't know, but some other colleagues of mine are also struggling with it. Not sure if that falls anywhere within the scope of this topic, though :)

tadhgboyle · 2021-11-04T23:25:01Z

src/Symfony/Component/ErrorHandler/ErrorRenderer/CliErrorRenderer.php

+        $result .= $textBold.$textBrightRed.\get_class($exception).$resetStyle."\n";
+        $result .= $exception->getMessage()."\n\n";
+
+        $sourceCode =file($exception->getFile());


missing space?

fabpot · 2022-07-20T17:00:07Z

I like the idea, I think we need to move forward and merge something along the lines of this PR. We will always be able to improve in the future (so, we don't need it to be perfect).

94noni · 2022-10-28T11:57:39Z

@javiereguiluz friendly ping :) can we test if for sf6.2 ? cc @fabpot
with new profiler design, the cli design refresh could be nice

wouterj · 2022-10-28T14:01:40Z

With the release of 6.2 beta1 last week, it has reached feature freeze now. Let's focus on getting the ecosystem ready for Symfony 6.2 and get back to new features after SymfonyCon 😃

94noni · 2022-10-28T14:37:43Z

ho yes indeed!

94noni · 2023-02-02T19:26:58Z

friendly ping @javiereguiluz cf my previous comment 🙂

I know it is yet one another ping
nonetheless reading PR desc and the 👍🏻 on it and comments, I think this could be a nice start :)
I propose even to continue it, thank you!

yceruto

😍

javiereguiluz · 2023-02-03T14:19:22Z

I'm still working on this. I'm not fully ready yet to commit the changes, but I'll try to do soon. Thanks.

javiereguiluz · 2023-02-10T12:45:44Z

Here's a screenshot of the latest changes:

Comments:

The idea is to NOT display the stack trace for certain exceptions (e.g., like @nicolas-grekas suggested to me, for ParseException)
In the above screenshot I forced to display the stack trace for ParseException to show a full example
I propose to display the 👻 emoji as a nod to the ghost that Symfony shows in exception pages

Some questions:

Besides ParseException exceptions, I don't know how to display this error page for errors in console. I always see the error display of the Symfony Console component, which only displays the exception message. If this is OK and the error page of this PR is almost never shown ... shouldn't we just close this PR as "won't fix"? I don't think the effort is worth it.
The "exception line number" given by PHP is always wrong by 1 in my own tests. That's why I use "line number - 1" in code. What am I doing wrong?
What if we hide all the vendor lines in the stack trace?

Thanks!

theofidry · 2023-02-10T13:51:39Z

@javiereguiluz personally I much prefer it over the old version. Not quite as much as collision, but I also don't know what to suggest to really improve it further.

Regarding the stack trace: I would suggest to only display it on verbose or higher.

stof · 2023-02-10T13:55:12Z

I always see the error display of the Symfony Console component, which only displays the exception message.

that's only true at normal verbosity. In verbose mode, it also shows the stack trace.

pbowyer · 2023-02-10T14:22:19Z

I ❤️ the Collision display (I'd not seen it before this post) but for a fairer comparison, how does it handle an exception where you need to see a stack trace?

theofidry · 2023-02-21T22:12:14Z

src/Symfony/Component/ErrorHandler/ErrorRenderer/CliErrorRenderer.php

+        ], '', __FILE__);
+        $projectPrefix = \dirname($vendorPrefix).DIRECTORY_SEPARATOR;
+
+        return str_replace($projectPrefix, '', $absolutePath);


Instead you can use Path directly:

Path::makeRelative($absolutePath, $projectPrefix);

The component currently doesn't depend on symfony/filesystem - and probably not worth trying to include it just for this.

theofidry · 2023-02-21T22:13:43Z

src/Symfony/Component/ErrorHandler/ErrorRenderer/CliErrorRenderer.php

+        $terminalSupportsLinks = 'JetBrains-JediTerm' !== getenv('TERMINAL_EMULATOR') && (!getenv('KONSOLE_VERSION') || (int) getenv('KONSOLE_VERSION') > 201100);
+        $href = sprintf('file://%s#L%d', $filePath, $lineNumber);
+
+        return $terminalSupportsLinks ? "\033]8;;{$href}\033\\{$content}\033]8;;\033\\" : $content;


would there be no way to extract this bit? This is repeated from

symfony/src/Symfony/Component/VarDumper/Dumper/CliDumper.php

Lines 445 to 446 in 5a2ee5a

$this->handlesHrefGracefully ??= 'JetBrains-JediTerm' !== getenv('TERMINAL_EMULATOR')

&& (!getenv('KONSOLE_VERSION') || (int) getenv('KONSOLE_VERSION') > 201100);

too

Repeating that isn't ideal - add a CliDumper::handlesHrefGracefully() public static function? It's not a blocker for me - it can be done later or not.

fabpot · 2023-03-29T18:32:56Z

@javiereguiluz I'd love to be able to merge this one in 6.3 :)

94noni · 2023-06-23T13:44:55Z

Hello 👋

I have understood that the console component may/will have some improvements for v6.4/v7.0

So i am asking if this could/should be also part of it?

javiereguiluz · 2023-09-20T13:08:42Z

I've made some final tweaks in this PR.

This is how it looks when there's no trace:

And this is the full error with the exception trace:

The issue that I mentioned above still persists. I can't see this exception in the CLI, unless I introduce a PHP parser error. I never see this for normal errors:

So, I guess almost nobody will see this ... so we can be softer in the final reviews. Thanks!

ostrolucky · 2023-09-20T13:22:50Z

So solving the inconsistency issue is no longer in scope of this PR?

javiereguiluz · 2023-09-20T14:18:46Z

@ostrolucky yes. Actually, it's not a bad inconsistency.

See the previous error: some env var is missing and you see this error:

I'd say that the error message is both very clear and very concise. It's better than using the newer format ... which is more convenient for other errors where seeing some code context is useful.

By the way, that old format error is generated here:

symfony/src/Symfony/Component/Console/Application.php

Line 895 in f9327be

    
           $messages[] = sprintf('<comment>%s</comment>', OutputFormatter::escape(sprintf('In %s line %s:', basename($e->getFile()) ?: 'n/a', $e->getLine() ?: 'n/a')));

I don't know if we could just change that and not break things.

javiereguiluz · 2023-09-20T14:35:08Z

I have two questions for @nicolas-grekas:

Should we take into account the NO_COLOR env var here? If we do, how can we get that information? I tried with (new Terminal())->supportsColors() but that method doesn't exist.

Are test errors solvable or should we refactor tests entirely? Specifically, this -->

symfony/src/Symfony/Component/ErrorHandler/Tests/phpt/exception_rethrown.phpt

Lines 32 to 39 in f9327be

    
           --EXPECTF-- 
        
           LOG: Uncaught Exception: foo 
        
           EHLO 
        
           Exception {%S 
        
             #message: "foo" 
        
             #code: 0 
        
             #file: "%s" 
        
             #line: 27

I can't make it work with the ANSO escape sequences used for the new errors.

Thanks!

wouterj · 2023-10-11T08:11:59Z

I would remove the ghost emoji. Until recently, my terminal emulator would crash when displaying emoji's. I'm not sure, but I guess there are more terminal emulators that can't properly deal with emoji's.

weaverryan

A few notes, but mostly, this seems like a nice DX improvement (and Javier clearly focused on the details how how it looked) and a safe spot. If there are some tweaks that are needed, they should be easy to make.

weaverryan · 2023-10-17T15:06:50Z

src/Symfony/Component/ErrorHandler/ErrorRenderer/CliErrorRenderer.php

+        ], '', __FILE__);
+        $projectPrefix = \dirname($vendorPrefix).DIRECTORY_SEPARATOR;
+
+        return str_replace($projectPrefix, '', $absolutePath);


The component currently doesn't depend on symfony/filesystem - and probably not worth trying to include it just for this.

weaverryan · 2023-10-17T15:09:40Z

src/Symfony/Component/ErrorHandler/ErrorRenderer/CliErrorRenderer.php

 use Symfony\Component\ErrorHandler\Exception\FlattenException;
-use Symfony\Component\VarDumper\Cloner\VarCloner;
 use Symfony\Component\VarDumper\Dumper\CliDumper;


This use statement is not needed anymore, and neither is the class_exists below.

weaverryan · 2023-10-17T15:10:18Z

src/Symfony/Component/ErrorHandler/ErrorRenderer/CliErrorRenderer.php

+        $terminalSupportsLinks = 'JetBrains-JediTerm' !== getenv('TERMINAL_EMULATOR') && (!getenv('KONSOLE_VERSION') || (int) getenv('KONSOLE_VERSION') > 201100);
+        $href = sprintf('file://%s#L%d', $filePath, $lineNumber);
+
+        return $terminalSupportsLinks ? "\033]8;;{$href}\033\\{$content}\033]8;;\033\\" : $content;


Repeating that isn't ideal - add a CliDumper::handlesHrefGracefully() public static function? It's not a blocker for me - it can be done later or not.

javiereguiluz · 2023-10-17T15:37:19Z

Folks, I realize this might be disappointing for some, but I'm closing this PR as "won't fix".

As it current state, this PR is almost "useless". Why? Because the new design is almost never visible, only when you have some PHP syntax error. That's why I don't think all this complication is not worth it.

For Symfony 7.1 we could try to update the styles of ALL errors in the CLI. Personally, I don't think we have to do this because current CLI error design is fine. But if someone wants to propose some tweaks and enhancements, we can consider those.

Thanks!

javiereguiluz added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Jul 26, 2021

javiereguiluz added this to the 5.4 milestone Jul 26, 2021

javiereguiluz requested a review from yceruto as a code owner July 26, 2021 14:00

carsonbot added Status: Needs Review ErrorHandler Feature labels Jul 26, 2021

fabpot modified the milestones: 5.4, 6.1 Oct 29, 2021

tadhgboyle reviewed Nov 4, 2021

View reviewed changes

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

yceruto approved these changes Feb 3, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 3, 2023

javiereguiluz force-pushed the better_cli_errors branch from 10455d8 to 2c7b2fc Compare February 10, 2023 12:38

theofidry reviewed Feb 21, 2023

View reviewed changes

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

javiereguiluz added 4 commits September 20, 2023 15:06

[ErrorHandler] Improve error messages in CLI

2ca42c3

-

9e29800

-

9c881f3

Final tweaks

87530ef

javiereguiluz force-pushed the better_cli_errors branch from f21c7c0 to 87530ef Compare September 20, 2023 13:06

-

1703b6e

nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 6, 2023

94noni approved these changes Oct 11, 2023

View reviewed changes

weaverryan approved these changes Oct 17, 2023

View reviewed changes

javiereguiluz closed this Oct 17, 2023

	$this->handlesHrefGracefully ??= 'JetBrains-JediTerm' !== getenv('TERMINAL_EMULATOR')
	&& (!getenv('KONSOLE_VERSION') \|\| (int) getenv('KONSOLE_VERSION') > 201100);

Uh oh!

[ErrorHandler] Improve error messages in CLI #42261

[ErrorHandler] Improve error messages in CLI #42261

Uh oh!

Conversation

javiereguiluz commented Jul 26, 2021

Context

Problem

Proposal

What do you think?

Uh oh!

parijke commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sstok commented Jul 26, 2021

Uh oh!

wouterj commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ahummeling commented Jul 26, 2021

Uh oh!

tadhgboyle Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jul 20, 2022

Uh oh!

94noni commented Oct 28, 2022

Uh oh!

wouterj commented Oct 28, 2022

Uh oh!

94noni commented Oct 28, 2022

Uh oh!

94noni commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yceruto left a comment

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commented Feb 3, 2023

Uh oh!

javiereguiluz commented Feb 10, 2023

Uh oh!

theofidry commented Feb 10, 2023

Uh oh!

stof commented Feb 10, 2023

Uh oh!

pbowyer commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theofidry Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

weaverryan Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

theofidry Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

weaverryan Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Mar 29, 2023

Uh oh!

94noni commented Jun 23, 2023

Uh oh!

javiereguiluz commented Sep 20, 2023

Uh oh!

ostrolucky commented Sep 20, 2023

Uh oh!

javiereguiluz commented Sep 20, 2023

Uh oh!

javiereguiluz commented Sep 20, 2023

Uh oh!

wouterj commented Oct 11, 2023

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

weaverryan Oct 17, 2023

parijke commented Jul 26, 2021 •

edited

Loading

wouterj commented Jul 26, 2021 •

edited

Loading

94noni commented Feb 2, 2023 •

edited

Loading

pbowyer commented Feb 10, 2023 •

edited

Loading

weaverryan Oct 17, 2023 •

edited

Loading

weaverryan Oct 17, 2023 •

edited

Loading