Skip to content
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

[WebProfilerBundle] Use absolute URL for profiler links #31349

Merged
merged 1 commit into from May 22, 2019

Conversation

Projects
None yet
6 participants
@martijnhartlief
Copy link

commented May 1, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? n/a
Fixed tickets n/a
License MIT
Doc PR n/a

Generate absolute URL's so you can have a different (sub)domain for the profiler and the controller that uses it.

Also uses the link which the controller generated instead of always generating a new link in Twig. The changes shouldn't inpact normal behavior.

@stof

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Can you give more details about how this solves your need to have different subdomains ? I don't think this is true here (most URLs generated in the controller are for redirections from a profiler controller to another one, and so they are already in the right subdomain anyway AFAICT)

@martijnhartlief

This comment has been minimized.

Copy link
Author

commented May 7, 2019

Well perhaps my fixes are totally wrong. What I noticed is that the profiler doesn't follow it's own host configuration. It generates links relative to itself, which makes sense, but when you "include" the profiler in that page it uses a different host, it will link to the parent host, and not the profiler's host. If your host is example.domain.com and the profiler's host is domain.com it generates the link /_profiler/token. But when you actually click it the page uses example.domain.com/_profiler/token. Which result's in a 404 if you don't have a route listening to that path.

Bear in mind I only tried to fix the probably unintended result of the profiler controller assuming it's running under the same host.

So to "force" the profiler to generate the correct link for the profiler, no matter where you requested from I added "Route::ABSOLUTE_URL" in the controller that generates it.

Then i found out that value is never read in the template because it will always generate the path in the template. There is probably a reason for this, but it resulted that the template generates the route again. Which also assumes it's already under the correct host. The controller passes the generated url, but is never used.

Maybe just changing to absolute_url() in twig is the correct way, and the actual controller doesn't need any modifications.

I know this is probably an edge case, but i would love to have all profiler links go to the same host instead of "wild-carding" it to all hosts, which is currently not possible.

@stof

This comment has been minimized.

Copy link
Member

commented May 9, 2019

hmm, as the toolbar is loaded asynchronously in an AJAX request, the host requirements indeed don't allow solving things properly for links generated in the toolbar items (the router will consider that the toolbar is already on a domain matching the requirement and so does not need to force an absolute URL, but the toolbar then gets injected into the page and relative links are resolved there).

The solution to your problem is to use url() instead of path() in the item template, and keeping only the change on line 167 of ProfilerController in your patch (the redirections don't need this change to work fine).

@martijnhartlief

This comment has been minimized.

Copy link
Author

commented May 9, 2019

I made the suggested changes

@fabpot

fabpot approved these changes May 21, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@martijnhartlief Can you squash the commits before I merge? Thank you.

Use absolute URL for when the profiler's domain differs from the cont…
…roller's domain which initialises the profiler.

@martijnhartlief martijnhartlief force-pushed the martijnhartlief:absolute-url branch from 3218359 to 13ee1fa May 22, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Thank you @martijnhartlief.

@nicolas-grekas nicolas-grekas merged commit 13ee1fa into symfony:3.4 May 22, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request May 22, 2019

bug #31349 [WebProfilerBundle] Use absolute URL for profiler links (A…
…lumbrados)

This PR was merged into the 3.4 branch.

Discussion
----------

[WebProfilerBundle] Use absolute URL for profiler links

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | n/a
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Generate absolute URL's so you can have a different (sub)domain for the profiler and the controller that uses it.

Also uses the link which the controller generated instead of always generating a new link in Twig. The changes shouldn't inpact normal behavior.

Commits
-------

13ee1fa Use absolute URL for when the profiler's domain differs from the controller's domain which initialises the profiler.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.