Skip to content

Enhance cacheability check in isRequestCacheable method#25

Merged
mage-os-ci merged 1 commit into
mainfrom
rhoerr-fix-native-fpc-hit
Apr 19, 2026
Merged

Enhance cacheability check in isRequestCacheable method#25
mage-os-ci merged 1 commit into
mainfrom
rhoerr-fix-native-fpc-hit

Conversation

@rhoerr

@rhoerr rhoerr commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Problem

The BFCache optimization in module-theme-optimization removes no-store from Cache-Control headers to enable browser back/forward cache. However, for Magento's native FPC (non-Varnish/Fastly), this only works for FPC cache misses, not hits.

Root Cause

The BFCache plugin intercepts setNoCacheHeaders() and checks for the public.*s-maxage pattern to identify cacheable pages:

// beforeSetNoCacheHeaders()
if ($this->isRequestCacheable($cacheControl)) {  // checks for public.*s-maxage
    $this->isRequestCacheable = true;
}

// afterSetNoCacheHeaders()
if ($this->isRequestCacheable) {
    $cacheControlHeader->removeDirective('no-store');
}

This works on MISS because the response has Cache-Control: public, s-maxage=X before processing.

On HIT, the response is loaded from cache with already-processed headers: Cache-Control: max-age=0, must-revalidate, no-cache — no public or s-maxage to match.

The Trigger

HttpPlugin::beforeSendResponse() calls setNoCacheHeaders() when the HTTP context vary string doesn't match the vary cookie:

if ($currentVary !== $varyCookie) {
    $subject->setNoCacheHeaders();
}

On FPC HITs, this mismatch frequently occurs because Kernel::buildResponse() creates a new context from cached data but doesn't propagate it to the shared Context singleton that HttpPlugin checks.

Result

FPC HITs receive no-store in the response, defeating the BFCache optimization for the most common case (cached pages).


Solution

Extend the BFCache plugin's pattern detection to recognize FPC HIT responses.

FPC HITs have Cache-Control with:

  • No public (stripped during original processing)
  • No private (never set for cacheable pages)
  • No no-store (stripped by BFCache on original MISS)

A more proper solution might involve fixing the core HttpPlugin Context, but this is a far less invasive way to achieve the same result.

@rhoerr rhoerr requested a review from a team as a code owner April 9, 2026 18:37
@rhoerr

rhoerr commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

@GrimLink FYI I think this same issue affects Hyva's implementation.

@sprankhub

Copy link
Copy Markdown
Member

If I follow correctly, this only happens due to the PHP code change. However, Hyvä recommends to just change the VCL in their docs. So I don't think this is an issue in Hyvä.

@sprankhub

Copy link
Copy Markdown
Member

Good catch, by the way :)

@rhoerr

rhoerr commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

If I follow correctly, this only happens due to the PHP code change. However, Hyvä recommends to just change the VCL in their docs. So I don't think this is an issue in Hyvä.

Varnish is a separate matter--this only affects Magento's native FPC.

@mage-os-ci mage-os-ci merged commit 3df39ec into main Apr 19, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants