[FrameworkBundle] Add PHPStorm helper link format for Mac #21712
Conversation
| @@ -131,7 +131,7 @@ public function load(array $configs, ContainerBuilder $container) | |||
| 'macvim' => 'mvim://open?url=file://%%f&line=%%l', | |||
| '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.
% must stay escaped for DIC parameters
There was a problem hiding this comment.
Re-added the escape characters
|
I don't think you should make this modification at all.
It appears this is only true for PHPStorm 8 on MacOS. Windows and Linux versions of PHPStorm still require https://github.com/aik099/PhpStormProtocol (or https://github.com/sanduhrs/phpstorm-url-handler on Linux). The documentation even references PhpStormProtocol 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. |
|
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 using https://github.com/aik099/PhpStormProtocol and https://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 |
|
I updated the docs as suggested into symfony/symfony-docs#7513 |
|
I double checked Windows and Linux, and this PR is ready. Status: Needs Review |
| @@ -131,7 +131,7 @@ public function load(array $configs, ContainerBuilder $container) | |||
| 'macvim' => 'mvim://open?url=file://%%f&line=%%l', | |||
| '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.
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 10000 ?
There was a problem hiding this comment.
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
|
Changed the config to use a separate key to cater for VMs |
| @@ -132,6 +132,7 @@ public function load(array $configs, ContainerBuilder $container) | |||
| '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.
Are we sure that this will only ever be supported on Mac only? If not, what about phpstorm-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.
I opted for phpstorm-core
|
@harcod Can you please make the changes to the docs PR to use |
|
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 |
|
Won't that be a BC break, for everyone using I agree |
|
I don't think there is such a thing as BC break for such a parameter. It has no impact on user's code. |
|
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 |
|
I think that's definitely something we can do in a minor release, with a proper entry in the changelog. |
|
I have submitted PRs to the Windows and Linux versions of the phpstorm protocol handlers that would allow them to support both the native 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. |
|
Since both PRs have been merged, we only need to support the official format. I have updated the PR accordingly |
|
So, this new version of the PR should be merged into 2.7 as a bug fix, right? |
The current file link format is based on https://github.com/aik099/PhpStormProtocol, but PHPStorm supports the protocol natively using a different syntax (Since PHPStorm 8)