Skip to content

[HttpKernel] Fix getFileLinkFormat() to avoid returning the wrong URL in Profiler #32818

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
Sep 8, 2019
Merged

[HttpKernel] Fix getFileLinkFormat() to avoid returning the wrong URL in Profiler #32818

merged 1 commit into from
Sep 8, 2019

Conversation

Arman-Hosseini
Copy link
Contributor

Q A
Branch? >= 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32444
License MIT
Doc PR N/A

I had this problem and I noticed that an issue had already been registered.
So I registered the pull request.

@Arman-Hosseini
Copy link
Contributor Author

Cover

@nicolas-grekas
Copy link
Member

I'm not sure this is the correct fix. Can you check what "urlFormat" is in the constructor, when this happens? Maybe a callback is passed, and this callback has the bug?

@Arman-Hosseini
Copy link
Contributor Author

Arman-Hosseini commented Jul 30, 2019

I've already reviewed it @nicolas-grekas
The base path was previously created by "FileLinkFormatter::generateUrlFormat" method.

You can also see the "urlFormat" in the following path:
symfony/symfony/src/Symfony/Bundle/WebProfilerBundle/Resources/config/profiler.xml#63

    <service id="debug.file_link_formatter.url_format" class="string">
        <factory class="Symfony\Component\HttpKernel\Debug\FileLinkFormatter" method="generateUrlFormat" />
        <argument type="service" id="router" />
        <argument>_profiler_open_file</argument>
        <argument>?file=%%f&amp;line=%%l#line%%l</argument>
    </service>

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 30, 2019

OK, thanks for the reminder, works for me.
Could you add a test case please?

@Arman-Hosseini
Copy link
Contributor Author

A test case with PHPUnit ?
@nicolas-grekas

@nicolas-grekas
Copy link
Member

Yes please

@javiereguiluz
Copy link
Member

@Arman-Hosseini creating a test for this is not a trivial thing so, if you need help, you can copy+paste this test and make the needed changes to test your feature:

public function testWhenNoFileLinkFormatAndRequest()

@Arman-Hosseini
Copy link
Contributor Author

Hello @nicolas-grekas and @javiereguiluz .
I apologize. I am involved in a project and after that I only register a pull request in free time.
Do I have to create a separate class to do this, or is it enough to do a few tests within that class?

@nicolas-grekas
Copy link
Member

Yes, please do add a method on the class that'd be perfect.

@fabpot
Copy link
Member

fabpot commented Sep 8, 2019

Thank you @Arman-Hosseini.

fabpot added a commit that referenced this pull request Sep 8, 2019
…e wrong URL in Profiler (Arman-Hosseini)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Fix getFileLinkFormat() to avoid returning the wrong URL in Profiler

| Q             | A
| ------------- | ---
| Branch?       | >= 3.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #32444   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A <!-- required for new features -->

I had this problem and I noticed that an issue had already been registered.
So I registered the pull request.

Commits
-------

9ed5f03 Fix getFileLinkFormat() to avoid returning the wrong URL in Profiler
@fabpot fabpot merged commit 9ed5f03 into symfony:3.4 Sep 8, 2019
@Arman-Hosseini Arman-Hosseini deleted the patch-6 branch September 9, 2019 16:11
This was referenced Oct 7, 2019
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