-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Added a default ide file link web view #19973
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
Conversation
e9d1c13 to
fbd4a6f
Compare
0f180bc to
a0267a7
Compare
| 'macvim' => 'mvim://open?url=file://%%f&line=%%l', | ||
| 'emacs' => 'emacs://open?url=file://%%f&line=%%l', | ||
| 'sublime' => 'subl://open?url=file://%%f&line=%%l', | ||
| 'symfony' => '/_profiler/open?file=%%f&line=%%l#line%%l#'.json_encode(dirname($container->getParameter('kernel.root_dir')).DIRECTORY_SEPARATOR).':""', |
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.
the app root doesn't always match the server root
|
|
||
| $filename = dirname($this->kernelRootDir).DIRECTORY_SEPARATOR.$file; | ||
|
|
||
| if (preg_match("'(^|[/\\\\])\.\.?([/\\\\]|$)'", $file) || !is_readable($filename)) { |
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.
$filename => $file, we shouldn't leak the kernel root dir
a0267a7 to
06ad113
Compare
|
@jeremyFreeAgent can we open this page relatively simple in a modal overlay? Not leaving the current page. Simple iframe loading would be just fine i guess.. |
|
I like this. Can you take @nicolas-grekas comments into account? |
41fc036 to
6a0340e
Compare
b127be7 to
3bf030a
Compare
3bf030a to
837fdf3
Compare
| } | ||
|
|
||
| $profilerController = $container->getDefinition('web_profiler.controller.profiler'); | ||
| $profilerController->replaceArgument(6, dirname(dirname(dirname(dirname(dirname(dirname(dirname(dirname(__DIR__))))))))); |
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.
does work with standalone web profiler bundle (from subtree split)
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.
does or does not?
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.
what about taking the common dir prefix of kernel.root_dir & __DIR__
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.
does not!
| private $toolbarPosition; | ||
| private $cspHandler; | ||
| private $kernelRootDir; | ||
| private $rootDir; |
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.
baseDir?
| return array( | ||
| $request->getSchemeAndHttpHost().$request->getBaseUrl().$this->urlFormat, | ||
| dirname($this->rootDir).DIRECTORY_SEPARATOR, '', | ||
| $this->rootDir.DIRECTORY_SEPARATOR, '', |
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.
baseDir?
837fdf3 to
9eccb2b
Compare
9eccb2b to
ba6bcca
Compare
|
👍 (failure unrelated, know composer bug) |
|
I've made the changes in the Silex WebProfiler PR too. |
| */ | ||
| public function openAction(Request $request) | ||
| { | ||
| if (null === $this->baseDir) { |
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.
Should be much more explicit than that with a bit of context on how to fix this.
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.
Actually the extension replace the argument baseDir should be always set and that exception may not be thrown, ever. Perhaps we can remove that test here. What do you think?
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.
Indeed, let's keep it as is
|
Thank you @jeremyFreeAgent. |
This PR was merged into the 3.2-dev branch. Discussion ---------- Added a default ide file link web view | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - When having no `framework.ide` configured or `framework.ide = symfony` the file link open the source in a web view (eg `_profiler/open?file=/src/AppBundle/Controller/DefaultController.php&line=50#line50`).  Commits ------- ba6bcca Added a default ide file link web view
…reeAgent) This PR was merged into the 2.0.x-dev branch. Discussion ---------- Added the Symfony default ide file link web view This add the feature added by symfony/symfony#19973 Commits ------- 7e7f062 Added the Symfony default ide file link web view
…r (jeremyFreeAgent, javiereguiluz) This PR was merged into the master branch. Discussion ---------- Updated the ide option to use the default open in browser Related to symfony/symfony#19973 Commits ------- 2ee69c6 Added help about the framework.ide null value behaviour 3f4fcc7 Updated the help note about framework.ide 338e423 Updated the ide option to use the default open in browser
|
FYI @jeremyFreeAgent I've found a small problem with the open link URL, see #24868 |
…r (jeremyFreeAgent, javiereguiluz) This PR was merged into the master branch. Discussion ---------- Updated the ide option to use the default open in browser Related to symfony/symfony#19973 Commits ------- 2ee69c6 Added help about the framework.ide null value behaviour 3f4fcc7 Updated the help note about framework.ide 338e423 Updated the ide option to use the default open in browser
…r (jeremyFreeAgent, javiereguiluz) This PR was merged into the master branch. Discussion ---------- Updated the ide option to use the default open in browser Related to symfony/symfony#19973 Commits ------- 2ee69c6 Added help about the framework.ide null value behaviour 3f4fcc7 Updated the help note about framework.ide 338e423 Updated the ide option to use the default open in browser
…r (jeremyFreeAgent, javiereguiluz) This PR was merged into the master branch. Discussion ---------- Updated the ide option to use the default open in browser Related to symfony/symfony#19973 Commits ------- 2ee69c6 Added help about the framework.ide null value behaviour 3f4fcc7 Updated the help note about framework.ide 338e423 Updated the ide option to use the default open in browser
When having no
framework.ideconfigured orframework.ide = symfonythe file link open the source in a web view (eg_profiler/open?file=/src/AppBundle/Controller/DefaultController.php&line=50#line50).