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

[HttpKernel] HttpCache works with cloned request object #23546

Closed
leofeyer opened this issue Jul 17, 2017 · 2 comments
Closed

[HttpKernel] HttpCache works with cloned request object #23546

leofeyer opened this issue Jul 17, 2017 · 2 comments

Comments

@leofeyer
Copy link
Contributor

leofeyer commented Jul 17, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? yes
Symfony version 3.3.4

In the HttpCache::fetch() method does a $subRequest = clone $request; and passes the $subRequest variable to the forward() method, so all the request attributes end up on the $subRequest object instead of the $request object.

    protected function fetch(Request $request, $catch = false)
    {
        $subRequest = clone $request;

        // send no head requests because we want content
        if ('HEAD' === $request->getMethod()) {
            $subRequest->setMethod('GET');
        }

        // avoid that the backend sends no content
        $subRequest->headers->remove('if_modified_since');
        $subRequest->headers->remove('if_none_match');

        $response = $this->forward($subRequest, $catch);  // <-- $subRequest instead of $request

        if ($response->isCacheable()) {
            $this->store($request, $response);
        }

        return $response;
    }

This causes the request object to be empty when passed to $kernel->terminate($request, $response).

Here is the request object without using the HTTP cache:

Is this the expected behavior when using the HTTP cache? Because some of our kernel.terminate event listeners need to read the request attributes. Also, it seems kind of inconsistent to me.

@nicolas-grekas
Copy link
Member

I think it's the expected behavior: the cache middleware shouldn't have access to the parameters set by the app. That's the behavior of using eg Varnish as another middleware caching layer, we should not diverge from this reference.

@fabpot
Copy link
Member

fabpot commented Jul 28, 2017

Being as close as possible as a reverse proxy that would be in front of an app is indeed the goal here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants