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

Merge the Cache-Control header in PageRegular #6892

Merged
merged 3 commits into from Feb 14, 2024

Conversation

MarkejN
Copy link
Contributor

@MarkejN MarkejN commented Feb 13, 2024

I want to return caching information in my frontent module controller and merge it with the page's cache time, as described here: https://docs.contao.org/dev/framework/caching/#inline-fragments. For this to work, the main response from PageRegular must also have the Contao-Merge-Cache-Control header set:

if ($this->currentStrategy && $response->headers->has(self::MERGE_CACHE_HEADER)) {
if ($isMainRequest) {
$this->currentStrategy->update($response);

Toflar
Toflar previously approved these changes Feb 13, 2024
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

I've already checked with @MarkejN on Slack.

Every controller must decide if it wants to give subrequests the ability to have Cache-Control headers merged. If it does not, our listener will not merge the headers so currently the entire logic is broken for regular pages.

I think this is the right place to do it at the moment. But I would also want @aschempp to review.

@Toflar Toflar requested a review from aschempp February 13, 2024 16:07
MarkejN and others added 2 commits February 14, 2024 07:52
Co-authored-by: Yanick Witschi <yanick.witschi@terminal42.ch>
@leofeyer leofeyer changed the title Merge cache-control header in PageRegular Merge the Cache-Control header in PageRegular Feb 14, 2024
@leofeyer leofeyer merged commit 970a1ed into contao:4.13 Feb 14, 2024
17 checks passed
@leofeyer
Copy link
Member

Thank you @MarkejN.

@aschempp
Copy link
Member

Looks like this got broken in #3601 because the headers is only set for the FragmentTemplate now. I don't understand why that would need to be the case though.

I think this code should be added to FrontendTemplate::setCacheHeaders but unfortunately this PR was already merged after less than 24 hours 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants