Skip to content

Update 'ide' example in framework.rst #7513

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Update 'ide' example in framework.rst #7513

wants to merge 7 commits into from

Conversation

harcod
Copy link
Contributor

@harcod harcod commented Feb 19, 2017

The example 'ide' configuration was different from the actual format required for PhpStorm. See README at https://github.com/aik099/PhpStormProtocol to verify the correct format as

phpstorm://open?url=file://%f&line=%l

@harcod
Update 'ide' example in framework.rst
1588242 
The example 'ide' configuration was different from the actual format required for PhpStorm.  See README at https://github.com/aik099/PhpStormProtocol
javiereguiluz
javiereguiluz approved these changes Feb 22, 2017
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I've asked in the Symfony Slack and some actual users of PHPStorm have said that the proposed change is correct.

Thanks @harcod.

javiereguiluz
javiereguiluz requested changes Feb 22, 2017
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry ... but there has been some confusion!! The existing syntax works, but the proposed changes don't work!!

Then we'll need more reviews to accept this proposal.

@pierredup
Copy link
Contributor

The syntax phpstorm://open?url=file://%f&line=%l is only used by the plugin https://github.com/aik099/PhpStormProtocol.
Since PHPStorm 8 EAP 138.190, PHPStorm supports the protocol natively using the existing syntax used by the docs.

So IMO there is no change needed.

@xabbuh
Copy link
Member

xabbuh commented Feb 22, 2017

👍 see also https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L134

Status: Reviewed

@xabbuh
Copy link
Member

xabbuh commented Feb 22, 2017

Since PHPStorm 8 EAP 138.190, PHPStorm supports the protocol natively using the existing syntax used by the docs.

Does that mean we should update the default value in the Symfony code instead?

@pierredup
Copy link
Contributor

I've created a PR to update the value in Symfony symfony/symfony#21712

@harcod
Copy link
Contributor Author

harcod commented Feb 22, 2017

Updated docs as suggested by @pierredup in symfony/symfony#21712

@harcod harcod mentioned this pull request Feb 22, 2017
Closed
pierredup
pierredup approved these changes Feb 23, 2017
View reviewed changes
Copy link
Contributor

@pierredup pierredup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@xabbuh xabbuh added the On hold label Feb 24, 2017
@harcod
phpstorm-protocol was eliminated since the Windows and Linux handlers…
6e8e0be 
… now support the same format scheme as MacOS.
pierredup
pierredup approved these changes Mar 1, 2017
View reviewed changes
Copy link
Contributor

@pierredup pierredup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@harcod
Copy link
Contributor Author

harcod commented Mar 1, 2017

This documentation change reflects the changes made by symfony/symfony#21813

HeahDude
HeahDude reviewed Mar 1, 2017
View reviewed changes
reference/configuration/framework.rst Outdated
@@ -214,9 +214,15 @@ ide
Symfony turns file paths seen in variable dumps and exception messages into
links that open those files right inside your browser. If you prefer to open
those files in your favorite IDE or text editor, set this option to any of the
following values: ``phpstorm`` (requires `PhpStormProtocol`_), ``sublime``,
following values: ``phpstorm``, ``sublime``,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can now be longer.

HeahDude
HeahDude approved these changes Mar 1, 2017
View reviewed changes
Copy link
Contributor

@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍

@xabbuh
Copy link
Member

xabbuh commented Mar 3, 2017

Thank you @harcod.

xabbuh added a commit that referenced this pull request Mar 3, 2017
@xabbuh
minor #7513 Update 'ide' example in framework.rst (harcod)
6940a12 
This PR was squashed before being merged into the 3.2 branch (closes #7513).

Discussion
----------

Update 'ide' example in framework.rst

The example 'ide' configuration was different from the actual format required for PhpStorm.  See README at https://github.com/aik099/PhpStormProtocol to verify the correct format as

> phpstorm://open?url=file://%f&line=%l

Commits
-------

606bfb1 Update 'ide' example in framework.rst
xabbuh added a commit that referenced this pull request Mar 3, 2017
@xabbuh
[#7513] fix line length
631c68b
@xabbuh xabbuh closed this Mar 3, 2017
xabbuh added a commit that referenced this pull request Mar 4, 2017
@xabbuh
Merge branch '3.2'
c7af8cf 
* 3.2: (24 commits)
  Minor rewords
  Update session_configuration.rst
  [#7522] minor wording improvement
  [#7513] fix line length
  Update 'ide' example in framework.rst
  [#7219] move versionadded in front of description
  Improved the image for Doctrine + Web Debug Toolbar
  [Serializer] Docs for the PropertyInfo integration
  Finished this PR
  section chronology in comment
  Removed the example about Redis
  Update cache.rst
  Update bundles/configuration.rst
  Fix typos
  Removed tip that was meant for the 2.x versions
  fix usage of the session flag bag API
  Add caution related to deprecate mail transport
  Use https URLs when possible
  [Requirements] Clarify what are JSON and ctype
  Proposed a minor reword
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz requested changes

@pierredup pierredup pierredup approved these changes

@HeahDude HeahDude HeahDude approved these changes

Assignees
No one assigned
Labels
On hold Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@harcod @pierredup @xabbuh @javiereguiluz @HeahDude @carsonbot