Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Add PHPStorm helper link format for Mac#21712
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| 'emacs' =>'emacs://open?url=file://%%f&line=%%l', | ||
| 'sublime' =>'subl://open?url=file://%%f&line=%%l', | ||
| 'phpstorm' =>'phpstorm://open?url=file://%%f&line=%%l', | ||
| 'phpstorm' =>'phpstorm://open?file=%f&line=%l', |
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.
% must stay escaped for DIC parameters
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.
Re-added the escape characters
harcod commentedFeb 22, 2017
I don't think you should make this modification at all.
It appears this is only true for PHPStorm 8 onMacOS. Windows and Linux versions of PHPStorm still requirehttps://github.com/aik099/PhpStormProtocol (orhttps://github.com/sanduhrs/phpstorm-url-handler on Linux). Thedocumentation even referencesPhpStormProtocol as a requirements for using the 'phpstorm' value for 'ide'. Perhaps, there should be a new 'phpstorm-mac' value in the code with the new format ('phpstorm://open?file=%f&line=%l') and the documentation should be changed to have a generic example such as 'myide://open?url=file://%%f&line=%%l' to avoid confusion with phpstorm. In my particular case, I would have had zero problems if I had just set the value of the 'ide' to 'phpstorm'. My mistake was seeing 'phpstorm://open..." in the example for another editor and using that misleading example instead. |
pierredup commentedFeb 22, 2017
I've updated the PR to only change the value in the case of Mac. I'm not close to a Windows PC at the moment, but I'll double check Windows and Linux tomorrow. I think the docs should be updated to mention specifically usinghttps://github.com/aik099/PhpStormProtocol andhttps://github.com/sanduhrs/phpstorm-url-handler on Windows and Linux respectively (instead of just specifying that PhpStormProtocol is needed) and the examples should change to |
harcod commentedFeb 22, 2017
I updated the docs as suggested intosymfony/symfony-docs#7513 |
pierredup commentedFeb 23, 2017
I double checked Windows and Linux, and this PR is ready. Status: Needs Review |
| 'emacs' =>'emacs://open?url=file://%%f&line=%%l', | ||
| 'sublime' =>'subl://open?url=file://%%f&line=%%l', | ||
| 'phpstorm' =>'phpstorm://open?url=file://%%f&line=%%l', | ||
| 'phpstorm' =>'Darwin' ===PHP_OS ?'phpstorm://open?file=%%f&line=%%l' :'phpstorm://open?url=file://%%f&line=%%l', |
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.
Not sure if that's really a good idea (useful at all) since this won't work anyway when Symfony is running in a virtual machine, will it?
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.
checking the OS will indeed not play well with VMs, as phpstorm will run on the host, not on the VM.
We should have a separate config key IMO
pierredup commentedFeb 23, 2017
Changed the config to use a separate key to cater for VMs |
| 'emacs' =>'emacs://open?url=file://%%f&line=%%l', | ||
| 'sublime' =>'subl://open?url=file://%%f&line=%%l', | ||
| 'phpstorm' =>'phpstorm://open?url=file://%%f&line=%%l', | ||
| 'phpstorm-mac' =>'phpstorm://open?file=%%f&line=%%l', |
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.
Are we sure that this will only ever be supported on Mac only? If not, what aboutphpstorm-official,phpstorm-core, or something along those lines? (I'm not a phpstorm user, so not sure if that makes sense).
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.
I opted forphpstorm-core
pierredup commentedFeb 24, 2017
@harcod Can you please make the changes to the docs PR to use |
javiereguiluz commentedFeb 24, 2017
I'm probably too late, but ... what if we change these values as follows:
The reason is that ultimately PHPStorm will do the right thing and support their correct native format ... so that should be |
pierredup commentedFeb 24, 2017 • 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.
Won't that be a BC break, for everyone using I agree |
fabpot commentedFeb 24, 2017
I don't think there is such a thing as BC break for such a parameter. It has no impact on user's code. |
pierredup commentedFeb 24, 2017
It doesn't have an impact on the code, but it has an impact on the current experience and expectations. It would still require a change in the config to keep the current experience. But if you think thats not a problem then I'm happy to make the change |
fabpot commentedFeb 24, 2017
I think that's definitely something we can do in a minor release, with a proper entry in the changelog. |
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.
👍
harcod commentedFeb 27, 2017
I have submitted PRs to the Windows and Linux versions of the phpstorm protocol handlers that would allow them to support both thenative PhpStorm Mac scheme as well as their current scheme. If these are incorporated, then this PR could just eliminate the "phpstorm-protocol" completely and only support the native PhpStorm format. |
pierredup commentedMar 1, 2017
Since both PRs have been merged, we only need to support the official format. I have updated the PR accordingly |
fabpot commentedMar 1, 2017
So, this new version of the PR should be merged into 2.7 as a bug fix, right? |
pierredup commentedMar 1, 2017 • 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.
Uh oh!
There was an error while loading.Please reload this page.
The current file link format is based onhttps://github.com/aik099/PhpStormProtocol, but PHPStorm supports the protocol natively using a different syntax (Since PHPStorm 8)