EZP-29144: Make websiteToolbarAction's Response cacheable #152
Conversation
| { | ||
| $request = $this->getRequest(); | ||
| $response = new Response(); | ||
| if ($this->getParameter('content.view_cache') === true) { |
There was a problem hiding this comment.
Can these be injected through constructor instead fetching them through container?
There was a problem hiding this comment.
Yes, that is better. Fixed, thanks.
| // response as it can depend on user role for instance. X-User-Hash cannot | ||
| // be used since the website toolbar can have Owner( Self ) Policy Limitation. | ||
| if ($request->headers->has('Cookie')) { | ||
| $response->setVary('Cookie'); |
There was a problem hiding this comment.
wondering if we should always set this even if there are no cookie, as otherwise the non varying cache will be set in varnish and used also by requests with cookies afaik.
There was a problem hiding this comment.
I'll check that to be sure and change it if needed.
There was a problem hiding this comment.
On further taught: In practice it provably won't happen if eZ puts vary by user hash in request as well.
There was a problem hiding this comment.
@andrerom from my tests it looks like eZ indeed puts vary by user hash in the request. But otherwise, vary by cookie would be needed. So I'll change that fragment to always set it, just to be on the safe side.
| @@ -67,7 +82,7 @@ public function __construct( | |||
| */ | |||
| public function websiteToolbarAction($locationId, Request $request) | |||
| { | |||
| $response = new Response(); | |||
| $response = $this->buildResponse(); | |||
There was a problem hiding this comment.
as we already have request as argument, maybe pass it on as argument is cleaner.
There was a problem hiding this comment.
We don't need request in buildResponse method anymore, so I'm just going to remove that.
JIRA issue: EZP-29144
The following PR: ezsystems/DemoBundle#194 fixes the issue in the Symfony stack by loading the Website Toolbar using ESI, so that it won't be cached alongside the rest of the page content ("Vary: X-User-Hash" is used there, which is wrong when we need to deal with Owner ( Self ) Policy Limitation).
This PR makes websiteToolbarAction's Response cacheable with "Vary: Cookie". I used the content cache parameters to determine the headers.