Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[VarDumper] Use hyperlinks in CliDescriptor#29613
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[VarDumper] Use hyperlinks in CliDescriptor#29613
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cool :)
ostrolucky commentedDec 15, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Problem with this is that link will be now inaccessible for users not using terminal which supports hyperlinks |
nicolas-grekas commentedDec 19, 2018
@ostrolucky true. I think that's fine personally: most common terminals handle the links already and windows didn't make it clickable anyway. |
ostrolucky commentedDec 19, 2018
yeah not a big deal in this one case, it's really minor inconvenience |
nicolas-grekas commentedDec 20, 2018
Thank you@ogizanagi. |
This PR was merged into the 4.3-dev branch.Discussion----------[VarDumper] Use hyperlinks in CliDescriptor| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | Part of#29585 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/ALeverages#29168 to add the hyperlink directly on the source text instead of printing below:#### Before (or with `symfony/console < 4.3`)#### AfterCommits-------e54e219 [VarDumper] Use hyperlinks in CliDescriptor
TomasVotruba commentedDec 21, 2018
I wonder how this could be leveraged withawesome console PHPStorm plugin |
ogizanagi commentedDec 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Well, using this plugin, there's nothing much to do: both Problem is though the hyperlink feature used here breaks the whole PhpStorm terminal emulator :/ Perhaps we could maintain a list of terminals actually supporting this feature to avoid such issues (at least ignore hyperlinks for PhpStorm terminal emulator, i.e: |
This PR was merged into the 4.1 branch.Discussion----------[VarDumper] Add descriptors tests| Q | A| ------------- | ---| Branch? | 4.1 <!-- see below -->| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AJust adding some tests for these classes & fixing a small issue with the optional `project_dir` entry from `source` context provider.If merged before#29613, I'll update tests in it (796ca6b) (otherwise could be picked on merge).Commits-------8570b8a [VarDumper] Add descriptors tests
…lator (ogizanagi)This PR was merged into the 4.3-dev branch.Discussion----------[Console][VarDumper] Ignore href for PhpStorm terminal emulator| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#29613 (comment) <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/ASo, as explained in#29613 (comment), the hyperlink feature currently breaks the PhpStorm terminal (the output stops abruptly, sometimes the whole terminal emulator freezes). Currently, a simple `dump(new \Exception());` would be enough to break it.Hence I think we should at least ignore hyperlinks for this terminal emulator.📝https://youtrack.jetbrains.com/issue/IDEA-204536 feature request has been opened on JetBrains YouTrack.Commits-------0f65a76 [Console][VarDumper] Ignore href for PhpStorm terminal emulator
Leverages#29168 to add the hyperlink directly on the source text instead of printing below:
Before (or with
symfony/console < 4.3)After