[HttpFoundation] fixed the check of 'proxy-revalidate' in Response::mustRevalidate()

[axiac]

Q	A
Fixed tickets	#15262
License	MIT

'proxy-revalidate' is not a header on its own but a 'Cache-Control' directive
See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9

[xabbuh]

The change looks good to me, but you should add a test for this to avoid future regressions.

[jakzal]

@axiac have you got time to add a test? Would be also nice if you rebased to the latest 2.3 branch as tests are currently failing on your PR.

[axiac]

Sorry to answer so late, I was away from computer during the last two weeks and a half.

The directive proxy-revalidate is not used anywhere else in the Symfony's code or tests, the only place where it appears is the line I changed. I'm not sure how to test it (I'm not even sure I understand correctly how the Cache-Control directives work together).

In order to have something to test I think I should add a way to set the proxy-revalidate directive to the Response.

I am new to Symfony (I started working with it this year) and I am not sure what is the best way to do this. This is why I tried these alternative approaches:

• add a new method setProxyRevalidate() to the public API of class Response - see commit 337e288; for symmetry I should also add the method setMustRevalidate();
• change method Response#setCache() to provide support for must-revalidate and proxy-revalidate cache control directives - see commit 440bba7;
• both of the above - combined in commit cc92569.

Which approach do you think fits best with the current API (and design) of the Response class?

[xabbuh]

@axiac For example, you could add the following two tests after testMustValidate() to the ResponseTest class (the second test would fail without your patch):

    public function testMustRevalidateWithMustRevalidateCacheControlHeader()
    {
        $response = new Response();
        $response->headers->set('cache-control', 'must-revalidate');

        $this->assertTrue($response->mustRevalidate());
    }

    public function testMustRevalidateWithProxyRevalidateCacheControlHeader()
    {
        $response = new Response();
        $response->headers->set('cache-control', 'proxy-revalidate');

        $this->assertTrue($response->mustRevalidate());
    }

[axiac]

@xabbuh The tests you suggested look like they belong there. Thank you for your support.

[stof]

👍

Status: reviewed

[xabbuh]

👍

[stof]

Good catch, thanks @axiac.