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

Don't send default cache header for 301 redirects #18220

Merged
merged 1 commit into from Jun 15, 2016

Conversation

e-moe
Copy link
Contributor

@e-moe e-moe commented Mar 17, 2016

Q A
Branch? "master"
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #17139
License MIT
Doc PR see comments from #17139

@@ -41,6 +41,10 @@ public function __construct($url, $status = 302, $headers = array())
if (!$this->isRedirect()) {
throw new \InvalidArgumentException(sprintf('The HTTP status code is not a redirect ("%s" given).', $status));
}

if (301 == $status && !array_key_exists('cache-control', $headers)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it's correct place for this code. Looking forward for review

Copy link
Member

Choose a reason for hiding this comment

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

I would instead move this into the main Response class just before sending the headers to the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabpot, in Response class headers already computed and contains unnecessary 'no-cache' directive. We can't check wether is was set automatically or explicitly by user. see \Symfony\Component\HttpFoundation\ResponseHeaderBag::computeCacheControlValue

@javiereguiluz javiereguiluz changed the title 17139: do not send default cache header for 301 redirects Don't send default cache header for 301 redirects Mar 17, 2016
@e-moe
Copy link
Contributor Author

e-moe commented Mar 18, 2016

probably we can add $withDefaultCacheControl parameter to \Symfony\Component\HttpFoundation\ResponseHeaderBag::__construct and do something like this:

/**
     * Constructor.
     *
     * @param array $headers An array of HTTP headers
     * @param bool $withDefaultCacheControl
     */
    public function __construct(array $headers = array(), $withDefaultCacheControl = true)
    {
        parent::__construct($headers);

        if ($withDefaultCacheControl && !isset($this->headers['cache-control'])) {
            $this->set('Cache-Control', '');
        }
    }

then, in Response constructor:

$this->headers = new ResponseHeaderBag($headers, 301 != $status); or, even better check against array like $this->headers = new ResponseHeaderBag($headers, in_array($status, self::$useDefaultCacheControl));

@fabpot
Copy link
Member

fabpot commented Jun 15, 2016

I'm going to merge this as is.

@fabpot
Copy link
Member

fabpot commented Jun 15, 2016

Thank you @e-moe.

@fabpot fabpot merged commit cf253a9 into symfony:master Jun 15, 2016
fabpot added a commit that referenced this pull request Jun 15, 2016
This PR was merged into the 3.2-dev branch.

Discussion
----------

Don't send default cache header for 301 redirects

| Q             | A
| ------------- | ---
| Branch?       | "master"
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #17139
| License       | MIT
| Doc PR        | see comments from #17139

Commits
-------

cf253a9 17139: do not send default cache header for 301 redirects
@fabpot fabpot mentioned this pull request Oct 27, 2016
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.

None yet

4 participants