Skip to content

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

Merged
merged 1 commit into from
Oct 25, 2016

Conversation

jeremyFreeAgent
Copy link
Contributor

@jeremyFreeAgent jeremyFreeAgent commented Sep 19, 2016

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).

@@ -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).':""',
Copy link
Member

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));
Copy link
Member

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

@ro0NL
Copy link
Contributor

ro0NL commented Sep 20, 2016

@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..

@fabpot
Copy link
Member

fabpot commented Sep 23, 2016

I like this. Can you take @nicolas-grekas comments into account?

@jeremyFreeAgent jeremyFreeAgent force-pushed the feature/ide branch 4 times, most recently from 41fc036 to 6a0340e Compare October 3, 2016 12:27
@fabpot
Copy link
Member

fabpot commented Oct 3, 2016

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?

@@ -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__)))))))));
Copy link
Member

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)

Copy link
Member

Choose a reason for hiding this comment

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

does or does not?

Copy link
Member

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__

Copy link
Member

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;
Copy link
Member

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, '',
Copy link
Member

Choose a reason for hiding this comment

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

baseDir?

@nicolas-grekas
Copy link
Member

👍 (failure unrelated, know composer bug)

@jeremyFreeAgent
Copy link
Contributor Author

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.');
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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

@fabpot
Copy link
Member

fabpot commented Oct 25, 2016

Thank you @jeremyFreeAgent.

@fabpot fabpot merged commit ba6bcca into symfony:master Oct 25, 2016
fabpot added a commit that referenced this pull request Oct 25, 2016
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`).

![](https://cl.ly/2Z0W2J020p43/feature_ide.png)

Commits
-------

ba6bcca Added a default ide file link web view
fabpot added a commit to silexphp/Silex-WebProfiler that referenced this pull request Oct 27, 2016
…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
@fabpot fabpot mentioned this pull request Oct 27, 2016
javiereguiluz added a commit to symfony/demo that referenced this pull request Dec 14, 2016
…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
@mtibben
Copy link
Contributor

mtibben commented Nov 8, 2017

FYI @jeremyFreeAgent I've found a small problem with the open link URL, see #24868

sayjun0505 added a commit to sayjun0505/sym_proj that referenced this pull request Apr 16, 2023
…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
spider-yamet added a commit to spider-yamet/sym_proj that referenced this pull request Apr 16, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants