-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
@@ -100,6 +100,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', | |||
'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)) { | ||
throw new NotFoundHttpException(sprintf('The file "%s" cannot be opened.', $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
Can you submit a PR on Silex-WebProfiler to be sure that everything will still work if used outside of the context of the full framework? |
b127be7
to
3bf030a
Compare
3bf030a
to
837fdf3
Compare
@@ -52,6 +52,12 @@ public function load(array $configs, ContainerBuilder $container) | |||
$container->setParameter('web_profiler.debug_toolbar.intercept_redirects', $config['intercept_redirects']); | |||
$container->setParameter('web_profiler.debug_toolbar.mode', $config['toolbar'] ? WebDebugToolbarListener::ENABLED : WebDebugToolbarListener::DISABLED); | |||
} | |||
|
|||
$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!
@@ -35,7 +35,7 @@ class ProfilerController | |||
private $templates; | |||
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?
@@ -76,7 +76,7 @@ private function getFileLinkFormat() | |||
if ($request instanceof Request) { | |||
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) { | ||
throw new NotFoundHttpException('The base dir should be set.'); |
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
When having no
framework.ide
configured orframework.ide = symfony
the file link open the source in a web view (eg_profiler/open?file=/src/AppBundle/Controller/DefaultController.php&line=50#line50
).