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

Overview of editorUrl status and suggestion to make everybody happy :-) #8519

Closed
ThomasLandauer opened this issue Dec 13, 2022 · 6 comments
Closed

Comments

@ThomasLandauer
Copy link
Contributor

Feature request

History

Out of the box, the output looks like this:

 ------ ------------------------------------------------------------------------------------------------------------------------ 
  Line   config/packages/framework.php                                                                                           
 ------ ------------------------------------------------------------------------------------------------------------------------ 
  22     Call to an undefined method Symfony\Config\Framework\SessionConfig|Symfony\Config\FrameworkConfig::enabled().           
 ------ ------------------------------------------------------------------------------------------------------------------------

Now, in phpstan/phpstan-src#515 I introduced editorUrl:
With this in your phpstan.neon:

editorUrl: 'phpstorm://open?file=%%file%%&line=%%line%%'

...the output changes to:

 ------ ------------------------------------------------------------------------------------------------------------------------ 
  Line   config/packages/framework.php                                                                                           
 ------ ------------------------------------------------------------------------------------------------------------------------ 
  22     Call to an undefined method Symfony\Config\Framework\SessionConfig|Symfony\Config\FrameworkConfig::enabled().           
         ✏️ phpstorm://open?file=/home/.../config/packages/framework.php&line=22                                                
 ------ ------------------------------------------------------------------------------------------------------------------------

AFAIK, this line is clickable in any console (if you register the phpstorm:// protocol). And this is also the documented status at https://phpstan.org/user-guide/output-format#opening-file-in-an-editor

But in phpstan/phpstan-src#1013 @Seldaek (probably to fix a wrapping issue reported in #5013 by @ruudk) changed the output to a different hyperlink format which is not supported by all consoles.

So what I am getting since then is:

------ ------------------------------------------------------------------------------------------------------------------------ 
 Line   config/packages/framework.php                                                                                           
------ ------------------------------------------------------------------------------------------------------------------------ 
 22     Call to an undefined method Symfony\Config\Framework\SessionConfig|Symfony\Config\FrameworkConfig::enabled().           
        ✏️  config/packages/framework.php                                                                                       
------ ------------------------------------------------------------------------------------------------------------------------

This is not clickable anymore (for me); and this is the regression that @Chris53897 reported in #7796 (and this is still not fixed in 1.9.3).

Now in phpstan/phpstan-src#2035 @janedbal changed the visible part of this hyperlink to show the full file path.
At least, that's what I'm guessing from the description, cause hyperlinks aren't working for me, so I can't show you the output ;-)

Solution

Let's summarize what people are wanting:

  1. @Chris53897 and me want the old phpstorm://... format back, cause that's the only one that's working in their console.
  2. @Seldaek and @ruudk want to keep the new hyperlink format, cause the phpstorm://... links can get too long, and if the line gets wrapped, the link is not working anymore (right?).
  3. @janedbal wants the full file path to be displayed. (Ironically, the old phpstorm://... would have featured this ;-)

1 and 2 look incompatible to me, so I can't see any other way than to support both by introducing a new config option. My suggestion: Use the new editorUrlTitle for this: If it is present, use the new hyperlink format (with editorUrlTitles value as visible part); if it's not present, use the old phpstorm://... format.

@janedbal
Copy link
Contributor

Cannot you just use editorUrlTitle to use the old phpstorm://...?

@Seldaek
Copy link
Contributor

Seldaek commented Dec 13, 2022

I can only speak for me but phpstan/phpstan-src#1013 was simply to shorten output and get clickable links in terminals supporting the hyperlinks (see https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda for an outdated list - as it seemed to be most terminals it seemed fine, and in practice it appears like it mostly did not cause any issue except for you and a few others?)

IMO the feature request there should be directed at your terminal author so they support hyperlinks.

@ruudk
Copy link
Contributor

ruudk commented Dec 13, 2022

I switched from iTerm to Warp and it does not (yet) support this type of links. But I agree, Warp should fix that instead. Going back to the old style where we just dump the whole URI doesn't make sense to me.

And if you really want it, you can use a custom formatter like I did myself.

@ThomasLandauer
Copy link
Contributor Author

Cannot you just use editorUrlTitle to use the old phpstorm://...?

Good idea, that works indeed! (Besides some "flickering" which I haven't investigated further)

And I found out that hyperlinks are in fact supported by Konsole (=default on KDE Plasma), but you have to enable it manually (deeply hidden in the options).
But the point is that I'm hesitant to enable an "obscure" linking feature, for the security reasons discussed at https://bugs.kde.org/show_bug.cgi?id=379294

So in fact it comes down to two possible ways:

  1. Leave it as is. People wanting the phpstorm://... links need to put the same in editorUrl and editorUrlTitle.
  2. Change the meaning of editorUrlTitle like I suggested above.

I can live with both, but I would still prefer 2, for two reasons:

  • People not wanting the hyperlink feature don't need to bother with editorUrlTitle (i.e. easier as now). Whereas people wanting the hyperlinks need to decide what they want as visible text anyway (i.e. same effort as now).
  • I wouldn't have to find out where the flickering in Konsole comes from...

@ondrejmirtes
Copy link
Member

I prefer to leave it as it is. Thanks.

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants