[ErrorHandler] Add button to copy the path where error is thrown #40052

lmillucci · 2021-02-01T08:01:24Z

Q	A
Branch?	5.x for features
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR

As a developer I want to copy the path where an exception is thrown on my IDE to check what is the problem. At the moment I have to select the path on the error page and copy it. I think that this process could be simplified through a "copy path" button that copies the error path on the clipboard.

I was thinking about something like this:

(Page style should be improved)
Let me know what you think. If you think it is a good idea I could improve this PR

carsonbot · 2021-02-01T08:01:25Z

Hey!

You, my friend, deserve a BIG HUG for making this PR.

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

nicolas-grekas · 2021-02-01T09:20:02Z

Dunno why @carsonbot closed this one.

lmillucci · 2021-02-01T09:21:48Z

I think it was closed because I didn't read well the first comment
To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

I'm moving this to "ready for review"

ro0NL · 2021-02-01T10:23:13Z

src/Symfony/Component/ErrorHandler/Resources/views/trace.html.php

@@ -25,6 +25,7 @@
                <span class="trace-method"><?= $trace['function']; ?></span>
            <?php } ?>
            (line <?= $lineNumber; ?>)
+            <button class="hidden" data-clipboard-text="<?php echo implode(\DIRECTORY_SEPARATOR, $filePathParts); ?>">Copy path</button>


should we suffix :$lineNumber? it works well in most editors AFAIK.

noniagriconomie · 2021-02-01T10:27:55Z

hello, isnt this "similar" to https://symfony.com/doc/current/reference/configuration/framework.html#ide?

src/Symfony/Component/ErrorHandler/Resources/views/trace.html.php

yceruto · 2021-02-01T14:16:00Z

hello, isn't this "similar" to https://symfony.com/doc/current/reference/configuration/framework.html#ide?

Yes, but not all IDEs support that feature, I think it would be worth it.

👍 on my side.

nicolas-grekas · 2021-02-04T09:55:06Z

Oh does the icon work in dark mode? Can you please update your screenshot?

lmillucci · 2021-02-04T11:08:22Z

These are the updated screenshots:

PS: What about coding standard issues found by fabbot? Should I fix them?

nicolas-grekas

(failures are false positives)

jderusse · 2021-02-04T11:52:38Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

@@ -698,6 +707,14 @@
                        });
                    }

+                    /* Prevents from disallowing clicks on "copy to clipboard" elements inside toggles */
+                    var copyToClipboardElements = toggles[i].querySelectorAll('span[data-clipboard-text]')


Why using for (...) here and .forEach at line 40.
Same for addEventListener(element vs element.addEventListener)

It would be even better to use event delegation instead.

I think that this code is actually useless because the click listener for toggle it is not attached to span elements. What do you think?

jderusse · 2021-02-04T11:54:08Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+                    var copyToClipboardElements = toggles[i].querySelectorAll('span[data-clipboard-text]')
+                    for (var k = 0; k < copyToClipboardElements.length; k++) {
+                        addEventListener(copyToClipboardElements[k], 'click', function(e) {
+                            e.stopPropagation();


IMHO we could always stopPropagation (and not ONLY for items in SPAN/toggle). And moving this logic at line 44

Nyholm · 2021-02-06T16:17:50Z

I am with @noniagriconomie. I am also a bit confused =)

The way I see it, on the error page, you see the relative path to each line in the stack trace. If you click at any of them it will open in PHPStorm... if you dont have it configured that way, you will get the file opened in the broswer and there you will see the absolute path.

nicolas-grekas · 2021-02-07T08:32:00Z

src/Symfony/Component/ErrorHandler/Resources/views/trace.html.php

@@ -25,6 +25,9 @@
                <span class="trace-method"><?= $trace['function']; ?></span>
            <?php } ?>
            (line <?= $lineNumber; ?>)
+            <span class="icon icon-copy hidden" data-clipboard-text="<?php echo implode(\DIRECTORY_SEPARATOR, $filePathParts).':'.$lineNumber; ?>">


I think the data-clipboard-text should contain the full path to the file - not the formatted path that links to the IDE.
This means this should contain $trace['file'], html-escaped.

I'm not sure about that... when running app in docker, absolute path is useless, while relative pat works in IDE, and console (when CWD=root of project)

Ah, you're right. We do have a way to map paths from docker to host, but it is bound to the IDE URLs. Can't we use that map somehow to compute absolute paths for the host?

Good point raised by Nyholm.

nicolas-grekas · 2021-02-16T10:38:56Z

I think that allowing a copy/paste of the path of the file is interesting.
In order to account for Docker/etc, we could allow passing a 3rd $format argument to FileLinkFormatter::format(), that would be used instead of $fmt[0] when possible, on the last line here:

symfony/src/Symfony/Component/HttpKernel/Debug/FileLinkFormatter.php

Lines 49 to 59 in 4d91b8f

    
           public function format(string $file, int $line) 
        
           { 
        
               if ($fmt = $this->getFileLinkFormat()) { 
        
                   for ($i = 1; isset($fmt[$i]); ++$i) { 
        
                       if (0 === strpos($file, $k = $fmt[$i++])) { 
        
                           $file = substr_replace($file, $fmt[$i], 0, \strlen($k)); 
        
                           break; 
        
                       } 
        
                   } 
        
                   return strtr($fmt[0], ['%f' => $file, '%l' => $line]);

Then we would patch HtmlRenderer::getFileLink to also handle and forward the same optional argument.

@lmillucci works for you? Can you please update the PR to account for all latest comments?

lmillucci · 2021-02-17T08:48:01Z

I think that allowing a copy/paste of the path of the file is interesting.
In order to account for Docker/etc, we could allow passing a 3rd $format argument to FileLinkFormatter::format(), that would be used instead of $fmt[0] when possible, on the last line here:

symfony/src/Symfony/Component/HttpKernel/Debug/FileLinkFormatter.php

Lines 49 to 59 in 4d91b8f

public function format(string $file, int $line)

{

if ($fmt = $this->getFileLinkFormat()) {

for ($i = 1; isset($fmt[$i]); ++$i) {

if (0 === strpos($file, $k = $fmt[$i++])) {

$file = substr_replace($file, $fmt[$i], 0, \strlen($k));

break;

}

}

return strtr($fmt[0], ['%f' => $file, '%l' => $line]);

Then we would patch HtmlRenderer::getFileLink to also handle and forward the same optional argument.

@lmillucci works for you? Can you please update the PR to account for all latest comments?

I'm not sure about the behavior of the $format argument of FileLinkFormatter::format(). I can add it to the function to override the default format but how should I use it in this situation?

fabpot · 2021-06-23T09:59:46Z

I would display the copy button only on hover to avoid having too many icons on the page.

lmillucci · 2021-07-16T06:32:21Z

I've updated the branch to display the icon only on hover. I'm still not sure about what should I do with FileLinkFormatter

This is an example:

fabpot · 2021-07-25T15:11:21Z

@lmillucci Can you rebase on the current 5.4 branch to resolve the conflicts? Ping me when it's done so that I can merge this PR. Thank you.

lmillucci · 2021-07-26T07:03:30Z

@fabpot rebased 👍

fabpot · 2021-07-26T07:21:35Z

Thank you @lmillucci.

lmillucci force-pushed the errorhandler-copy-error-path branch from 1da52a0 to 057e2f5 Compare February 1, 2021 08:11

carsonbot closed this Feb 1, 2021

nicolas-grekas reopened this Feb 1, 2021

lmillucci marked this pull request as ready for review February 1, 2021 09:21

lmillucci requested a review from yceruto as a code owner February 1, 2021 09:21

carsonbot added Status: Needs Review ErrorHandler Feature Deprecation labels Feb 1, 2021

ro0NL reviewed Feb 1, 2021

View reviewed changes

yceruto reviewed Feb 1, 2021

View reviewed changes

src/Symfony/Component/ErrorHandler/Resources/views/trace.html.php Outdated Show resolved Hide resolved

lmillucci force-pushed the errorhandler-copy-error-path branch from 4620b46 to 18d25cd Compare February 3, 2021 13:00

nicolas-grekas added this to the 5.x milestone Feb 4, 2021

nicolas-grekas previously approved these changes Feb 4, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 4, 2021

jderusse reviewed Feb 4, 2021

View reviewed changes

nicolas-grekas reviewed Feb 7, 2021

View reviewed changes

lmillucci force-pushed the errorhandler-copy-error-path branch from 3bee7af to d1a60bb Compare July 26, 2021 06:55

fabpot approved these changes Jul 26, 2021

View reviewed changes

[ErrorHandler] Add button to copy the path where error is thrown

5779f86

fabpot force-pushed the errorhandler-copy-error-path branch from d1a60bb to 5779f86 Compare July 26, 2021 07:21

fabpot merged commit a19735b into symfony:5.4 Jul 26, 2021

This was referenced Nov 5, 2021

Release v5.4.0-BETA1 #43937

Merged

Release v6.0.0-BETA1 #43939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ErrorHandler] Add button to copy the path where error is thrown #40052

[ErrorHandler] Add button to copy the path where error is thrown #40052

lmillucci commented Feb 1, 2021 •

edited

Loading

carsonbot commented Feb 1, 2021

nicolas-grekas commented Feb 1, 2021

lmillucci commented Feb 1, 2021 •

edited

Loading

ro0NL Feb 1, 2021

noniagriconomie commented Feb 1, 2021

yceruto commented Feb 1, 2021 •

edited

Loading

nicolas-grekas commented Feb 4, 2021

lmillucci commented Feb 4, 2021

nicolas-grekas left a comment

jderusse Feb 4, 2021

yceruto Feb 4, 2021

lmillucci Feb 17, 2021

jderusse Feb 4, 2021

Nyholm commented Feb 6, 2021

nicolas-grekas Feb 7, 2021

jderusse Feb 7, 2021

nicolas-grekas Feb 7, 2021

nicolas-grekas commented Feb 16, 2021

lmillucci commented Feb 17, 2021

fabpot commented Jun 23, 2021

lmillucci commented Jul 16, 2021

fabpot commented Jul 25, 2021

lmillucci commented Jul 26, 2021

fabpot commented Jul 26, 2021

[ErrorHandler] Add button to copy the path where error is thrown #40052

[ErrorHandler] Add button to copy the path where error is thrown #40052

Conversation

lmillucci commented Feb 1, 2021 • edited Loading

carsonbot commented Feb 1, 2021

nicolas-grekas commented Feb 1, 2021

lmillucci commented Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

noniagriconomie commented Feb 1, 2021

yceruto commented Feb 1, 2021 • edited Loading

nicolas-grekas commented Feb 4, 2021

lmillucci commented Feb 4, 2021

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Feb 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Feb 16, 2021

lmillucci commented Feb 17, 2021

fabpot commented Jun 23, 2021

lmillucci commented Jul 16, 2021

fabpot commented Jul 25, 2021

lmillucci commented Jul 26, 2021

fabpot commented Jul 26, 2021

lmillucci commented Feb 1, 2021 •

edited

Loading

lmillucci commented Feb 1, 2021 •

edited

Loading

yceruto commented Feb 1, 2021 •

edited

Loading