8000 [WebProfilerBundle] Redirects are not detected · Issue #24730 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WebProfilerBundle] Redirects are not detected #24730

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.

Sign up for GitHub

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

Jump to bottom
Closed
vudaltsov opened this issue Oct 28, 2017 · 7 comments
Closed

[WebProfilerBundle] Redirects are not detected #24730

vudaltsov opened this issue Oct 28, 2017 · 7 comments
Labels
Bug Status: Needs Review WebProfilerBundle

Comments

@vudaltsov
Copy link
Contributor
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.4 
<?php

namespace App\Controller;

use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Response;

class IndexController extends AbstractController
{
    /**
     * @Route(name="index")
     */
    public function indexAction()
    {
        return $this->redirectToRoute('redirect');
    }

    /**
     * @Route("/redirect", name="redirect")
     */
    public function redirectAction()
    {
        return new Response('<html><body></body></html>');
    }
}

image
@mateuszsip
Copy link
Contributor

Is not related to configuration?

@vudaltsov
Copy link
Contributor Author

@kejwmen, no. Profiler should show Redirected from ... regardless of web_profiler.intercept_redirects.

@sroze
Copy link
Contributor
sroze commented Oct 31, 2017

This feature would only work if you have the session enabled as you can see in the RequestDataCollector. Can you confirm you have the same issue with sessions enabled?

@sroze
Copy link
Contributor
sroze commented Oct 31, 2017

Maybe you don't have the session started (anymore?) since @nicolas-grekas' updates. Not sure how we can have this feature working without session tbh...

sroze added a commit to sroze/symfony that referenced this issue Oct 31, 2017
@sroze
Let the storage manage the session starts (fixes symfony#24730)
f9fa707
@sroze sroze mentioned this issue Oct 31, 2017
Closed
@vudaltsov
Copy link
Contributor Author
vudaltsov commented Oct 31, 2017 •
edited
Loading

The session was not enabled in the framework.session config...

With

framework:
    session:
        handler_id: ~

Everything works as expected on 3.4 beta-2.

My fault. But with flex you have to be much more attentive not to forget to enable things.
I was testing other stuff so far and didn't think of the session at all.

@weaverryan
Copy link
Member

Yea... this worries me a little bit actually... are there any other spots where not having the session enabled might silently cause some weird issues?

@sroze
Copy link
Contributor
sroze commented Nov 1, 2017

@weaverryan same. I've proposed this PR to fix the issue: #24774

@fabpot fabpot closed this as completed Nov 2, 2017
fabpot added a commit that referenced this issue Nov 2, 2017
@fabpot
bug #24774 [HttpKernel] Let the storage manage the session starts (sr…
2e3df94 
…oze)

This PR was squashed before being merged into the 3.4 branch (closes #24774).

Discussion
----------

[HttpKernel] Let the storage manage the session starts

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

HttpKernel's request collector should not really care if the session has started or not, be let the storage decide. Without the session, it is not possible to track the redirected pages.

I don't think the consideration of "we should not start the session if not needed by the user's code" applies here as if this is running, that is very likely that the user is running the dev environment anyway.

Commits
-------

95d0b72 [HttpKernel] Let the storage manage the session starts
@nicolas-grekas nicolas-grekas mentioned this issue Jan 6, 2018
Closed
@sroze sroze mentioned this issue Jan 8, 2018
Merged
fabpot added a commit that referenced this issue Jan 8, 2018
@fabpot
bug #25719 [HttpKernel] Uses cookies to track the requests redirectio…
5f05774 
…n (sroze)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Uses cookies to track the requests redirection

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

In order to track the redirections across requests, we need to have some state. So far, we've been using the session but some users have complained about it (#24774, #24730). The idea is that we don't actually need the session, we can use cookies.

It's a tradeoff: using a cookie would mean that both the redirection and the target page will not be cachable (because of the Set-Cookie to set the sf_redirect and the one to clear it).

As it's only on dev, it seems fair to say that having no cache (because of `Set-Cookie`s) is a better side effect than starting the session.

Commits
-------

83f2579 Uses cookies to track the requests redirection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review WebProfilerBundle
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @javiereguiluz @weaverryan @sroze @mateuszsip @vudaltsov @carsonbot
0