[RTM] Introduced auto cache tag invalidation on DCAs for better DX #1478
Conversation
Toflar
changed the title
|
ping @contao/developers This is ready to be reviewed. The goal here is that we invalidate tags based on a scheme ( @aschempp you might want to include the response tagger service on your abstract fragments PR 😎 |
|
Of course, still depends on contao/manager-bundle#67 which is ready to be merged though, so would be glad if that happens soon @leofeyer so we can continue the work on master 😄 |
|
Shouldn't we also tag pages? |
|
Ah, I see. It is done in the |
| @@ -389,6 +390,16 @@ private function setCacheHeaders(Response $response) | |||
| $response->setSharedMaxAge($objPage->cache); | |||
| } | |||
|
|
|||
| // Tag response | |||
| if (System::getContainer()->has('fos_http_cache.http.symfony_response_tagger')) | |||
There was a problem hiding this comment.
I think this code does not belong here. We cannot not assume that the return value of FrontendTemplate::getResponse() is always a page, can we? The code should go into the PageRegular class IMHO.
There was a problem hiding this comment.
I think it's correct here. It's the old FrontenTemplate::output() that was only called once and sent the response to the browser. It's the same thing for getResponse() now and we also set all the other caching headers regarding the page here (like disabling if FE preview is active and stuff).
There was a problem hiding this comment.
The old output() method still exists and the getResponse() method is not its replacement! You might as well use a front end template for an API or ESI response, therefore we cannot be sure it is always a page.
There was a problem hiding this comment.
So why are all the other caching headers set there?
There was a problem hiding this comment.
Is this a serious question? An API or ESI response can have cache headers, which does not make it a page.
There was a problem hiding this comment.
Of course it's a serious question, I don't have time for non-serious questions. ESI requests should certainly not use FrontendTemplate::getResponse(). It would set caching headers from the page configuration which makes no sense. An ESI fragment should do exactly the opposite.
There was a problem hiding this comment.
I'm using FrontendTemplate::getResponse a lot, so @leofeyer is right with his concern. Though it looks like the method is incorrect and should not add the page cache information…
There was a problem hiding this comment.
It would set caching headers from the page configuration which makes no sense
That's true …
There was a problem hiding this comment.
So I'm moving the relevant ones to PageRegular? Nobody can really re-use that but this concept has to be reworked for CMF routing anyway.
There was a problem hiding this comment.
No, you don't have to move them. It is all based on $objPage, so either we change this or we might as well leave everything as it is.
| */ | ||
| protected function getCacheTags($table, array $ids = array()) | ||
| { | ||
| $ns = 'contao.dca.'; |
There was a problem hiding this comment.
As discussed on the hackathon, we want to rename this to contao.db..
| protected function getCacheTags($table, array $ids = array()) | ||
| { | ||
| $ns = 'contao.dca.'; | ||
| $table = preg_replace('/^tl_/', '', $table, 1); |
There was a problem hiding this comment.
As also discussed, we don't want to strip the tüdelü prefix.
Toflar
changed the title
|
Updated according to latest discussions. This PR is RTM now imho. |
|
Thank you @Toflar. |
Automatically invalidates DCA entries when users edit them in the back end.
For testing purposes I've added corresponding tags on the page response.
Depends on contao/manager-bundle#67 which itself depends on other PR's again. Leaving it here for reference and to be updated.