Conversation
andrerom
force-pushed
the
EZP-28183
branch
2 times, most recently
from
59256f4 to
613a1e7
Compare
andrerom
force-pushed
the
EZP-28183
branch
4 times, most recently
from
8fa2688 to
2e88455
Compare
bdunogier
left a comment
bdunogier
left a comment
There was a problem hiding this comment.
I'm okay with the idea, but have a few comments. It is probably easier and safer for all sides to separate BC from the new implementation.
|
|
||
| if ($this->logger instanceof LoggerInterface) { | ||
| $this->logger->notice( | ||
| 'X-Location-Id rewritten to use ezplatform-http-cache tag header, see \<bundle\>/docs/using_tags.md for more info' |
There was a problem hiding this comment.
What about a good old E_USER_DEPRECATED rather than a notice ? Those are way more visible by developers. As long as our own code doesn't throw it ;)
| $tags += $response->headers->get($this->tagHeader, null, false); | ||
| } | ||
|
|
||
| // Set headers |
There was a problem hiding this comment.
I know this is WIP but I really wouldn't leave such comments :-)
Also applies to the comments above. If the statements aren't readable (the ->has() one is readable, in my opinion, for instance), private functions can be used.
| } | ||
|
|
||
| // Set headers | ||
| // @todo we need to use abstract tag writer to also be able to support Fastly |
There was a problem hiding this comment.
What does the abstract tag writer do that we don't do here ? Since we use the header name as set in the fos configuration, it should work with Fastly, should it not ?
There was a problem hiding this comment.
I meant to check with @vidarl, but the value is different in Fastly examples, it's separated by space: https://docs.fastly.com/guides/purging/getting-started-with-surrogate-keys#understanding-surrogate-keys
So if Fastly does not support what we currently do*, then we need to come up with something for abstracting this. And we might actually have to do that anyway, as if we want to promote usage of FosHttpCache TagsHandler for instance for custom tag usage, without hard-coding xkey header, it currently always writes comma separated values in 1.x (2.x to but it has a formater object you can change to something else).
- Respond with several headers to comply with xkey doc: https://github.com/varnish/varnish-modules/blob/master/docs/vmod_xkey.rst
There was a problem hiding this comment.
I had overlooked the separator difference, thanks. Then we need an abstraction of some sorts, yes.
I will add this reason to the FOS http cache 2.x story, as a value of it.
src/Resources/config/view_cache.yml
Outdated
| class: EzSystems\PlatformHttpCacheBundle\EventSubscriber\XLocationIdResponseSubscriber | ||
| arguments: | ||
| - '%fos_http_cache.tags.header%' | ||
| - '@?logger' |
There was a problem hiding this comment.
If we use E_USER_DEPRECATED, this isn't required anymore.
|
@bdunogier Update, also added spec. @vidarl As discussed above, could you comment on if Fastly is ok with the separated headers approach we current take here or if it needs space separated values like it's doc suggests? |
|
I did not follow everything but it seems that #21 is useless now. I like this one! Can't wait to test it ;) |
bdunogier
left a comment
bdunogier
left a comment
There was a problem hiding this comment.
+1 besides a nitpick (not blocker).
| public function rewriteCacheHeader(FilterResponseEvent $event) | ||
| { | ||
| $response = $event->getResponse(); | ||
| if (!$response->headers->has('X-Location-Id')) { |
There was a problem hiding this comment.
Nitpick: I'd make 'X-Location-Id' a class constant.
There was a problem hiding this comment.
But it would be public 😢 Un indispensable?
There was a problem hiding this comment.
No, it was a simple nitpick. I don't think it's an issue that the constant is public, but if it itches your goat, you can skip it :)
There was a problem hiding this comment.
On the other side, in this case it will never and can never change, so actually should be fine even for my 🐐 So will fix that on merge :)
|
Merged with 🐐 's and all , thanks all. |
|
thx @andrerom tested and it works well! |
|
|
||
| // @todo we need to use abstract tag writer to also be able to support Fastly | ||
| // FOS has some stuff around this in 2.x but not in a good way in 1.x | ||
| $response->headers->set($this->tagHeader, array_unique($tags)); |
There was a problem hiding this comment.
I don't like this. Now we have logic for setting xkey both here and in
This should be done in one place.
I fancy to replace the whole ConfigurableResponseCacheConfigurator with FOS\HttpCacheBundle\Handler\TagHandler, but that is not quite straight forward.
So instead I am thinking just adding a TagHandler and make sure ConfigurableResponseCacheConfigurator and XLocationIdResponseSubscriber uses that one for setting the headers. What do you say?
There was a problem hiding this comment.
Okay, Adding our own TagHandler based on FOS\HttpCacheBundle\Handler was not a good idea for several reasons.
One was that we are not able to run it's constructor without also adding a CacheInvalidator ( kinda replaces our PurgeClient) and that it's tag handling doesn't really suit our needs in regards to either xkey nor fastly.
There was a problem hiding this comment.
I'll then create our abstraction for this
There was a problem hiding this comment.
yep, thread marked 🏷 as obsolete 😏
5 participants
@bdunogier @Plopix : Here is somewhat what I had in mind here to begin with. This approach does not need to use response tagger in a way it was made for, and does not need to force caching of all REST responses. It just does one thing, transform the old value to the new one, on top of that it triggers deprecation warning pointing to doc we will need to write on how to deal with tags... (:
Todo: