-
-
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
Sorry, something went wrong.
All reactions
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.. |
All reactions
Sorry, something went wrong.
I like this. Can you take @nicolas-grekas comments into account? |
All reactions
Sorry, something went wrong.
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? |
All reactions
Sorry, something went wrong.
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)
Sorry, something went wrong.
All reactions
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?
Sorry, something went wrong.
All reactions
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__
Sorry, something went wrong.
All reactions
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!
Sorry, something went wrong.
All reactions
@@ -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?
Sorry, something went wrong.
All reactions
@@ -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?
Sorry, something went wrong.
All reactions
837fdf3
to
9eccb2b
Compare
9eccb2b
to
ba6bcca
Compare
👍 (failure unrelated, know composer bug) |
All reactions
Sorry, 8000 something went wrong.
I've made the changes in the Silex WebProfiler PR too. |
All reactions
Sorry, something went wrong.
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.
Sorry, something went wrong.
All reactions
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?
Sorry, something went wrong.
All reactions
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
Sorry, something went wrong.
All reactions
Thank you @jeremyFreeAgent. |
All reactions
Sorry, something went wrong.
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 |
All reactions
Sorry, something went wrong.
…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
fabpot
nicolas-grekas
stof
Successfully merging this pull request may close these issues.
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
).