Skip to content

[WebProfilerBundle] Render file links for twig templates (Fix #24355) #24356

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

Closed
wants to merge 3 commits into from

Conversation

pquerner
Copy link

@pquerner pquerner commented Sep 28, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Fixed tickets #24355 and #24218
License MIT

PR #24236 introduced a bug for Windows machines and this PR attempts to fix it.
See related discussions in tickets #24355 and #24218 (and starting from comment #24236 (comment))

//Edit
I'm not sure why so many commits are attached here, this is the commit I wanted to PR:

pquerner@2aeed1f

@stof
Copy link
Member

stof commented Sep 28, 2017

This is an invalid fix:

  • it only fixes a single place in the codebase for your own usecase of using a javascript expression as link. All other places are still broken (links to PHP files for controllers for instance).
  • it has the potential of breaking things for people using something else than JS code (where JS escaping might hurt).

The issue you have is that your own pattern expects extra escaping. This should be handled inside the file_link implementation, at the time doing the replacement. this would mean doing a custom implementation of the FileLinkFormatter and replacing the service.

@pquerner
Copy link
Author

I'm not sure why I'm getting javascript code and others dont.
So I thought I could fix it right there in the template.
Param "url" for the escape twig function does not work.

But I guess you are correct, best would be to fix it somewhere here
\Symfony\Bridge\Twig\Extension\CodeExtension::getFileLink

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Sep 28, 2017
Pascal added 2 commits September 28, 2017 12:40
by checking if we run on Windows and $file contains a backslash char
if we do, we apply PHPs addslashes function to $file
@stof
Copy link
Member

stof commented Sep 28, 2017

@pquerner you have this either in your FrameworkBundle config (framework.ide) or in your php.ini for debug.file_link_format.

@@ -202,6 +202,10 @@ public function formatFile($file, $line, $text = null)
*/
public function getFileLink($file, $line)
{
//If on Windows, we must escape $file if it contains backslash char
if('\\' === DIRECTORY_SEPARATOR && FALSE !== strpos($file, '\\')) {
$file = addslashes($file);
Copy link
Member

Choose a reason for hiding this comment

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

As said before, this is a nogo as most cases don't want such escaping. You will probably break it for all other users.

@pquerner
Copy link
Author

True, xdebug.file_link_format gives me that URL. ._.
javascript:var rq = new XMLHttpRequest(); rq.open('GET', 'http://localhost:8091?message=%f:%l', true); rq.send(null);

@stof
Copy link
Member

stof commented Sep 28, 2017

btw, XDebug would suffer from the same issue, as it would not escape the path for usage inside a JS string either.

But why do you need to use XHR to access the URL ? Why not using http://localhost:8091?message=%f:%l as link ?

@pquerner
Copy link
Author

pquerner commented Sep 28, 2017

I dont need to use XHR, but I cannot get it without javascript. I disabled Xdebug, disabled the Xdebug config in PHP.ini (file_link_format), cleared cache and it still outputs the javascript stuff. I have no idea what I have done to my env. :/

In my cache files I could find this

    /*
     * Gets the private 'debug.file_link_formatter' shared service.
     *
     * @return \Symfony\Component\HttpKernel\Debug\FileLinkFormatter
     */
    protected function getDebug_FileLinkFormatterService()
    {
        return $this->services['debug.file_link_formatter'] = new \Symfony\Component\HttpKernel\Debug\FileLinkFormatter('javascript:var rq = new XMLHttpRequest(); rq.open(\'GET\', \'http://localhost:8091?message=%f:%l\', true); rq.send(null);');
    }
//\appProdProjectContainer::getDebug_FileLinkFormatterService

But I dont know where that is generated.

@stof
Copy link
Member

stof commented Sep 28, 2017

@pquerner have you restarted PHP-FPM (or Apache if you use mod_php) ? Otherwise, the php.ini change won't be taken into account (PHP-FPM loads the config when starting)

@stof
Copy link
Member

stof commented Sep 28, 2017

and have you cleared the prod cache ? The CLI runs in the dev environment by default, not the prod one. But the code snupped you gave is from the prod environment

@pquerner
Copy link
Author

The code is there in prod and dev environment. Yes, I have added env parameter to cache:clear. I tried running in prod env once, but then I memorized the profiler is unavailable then.

@pquerner
Copy link
Author

pquerner commented Sep 28, 2017

Yes, I have restarted my apache server (I develop things locally via XAMPP, it has a control panel where you can start/stop the server easily) multiple times. Tried different browsers too, no luck. (Even tried server:run too, not that this matters I think)

@ro0NL
Copy link
Contributor

ro0NL commented Sep 29, 2017

Are you using this maybe? https://github.com/computerminds/parrot/blob/edfa1b99e629a090129908d34eee40ad7930765b/parrot-config/php/parrot-base.ini#L62

Anyway, im pretty convinced this happens on your side :) can you confirm xdebug config is correct, looking at phpinfo() or so?

@pquerner
Copy link
Author

Yes, I already found out it was the XDebug config. #24356 (comment)

Ive been reconfiguring my local dev maschine to use a different XAMPP version and now it works like expected. Sorry for the big trouble, guys.
Nothing needs to be changed for your original PR.

I feel stupid :(

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.

5 participants