Page cache only uses URL as cache ID, not HTTP Accept headers or language

Seems to me that an alter hook in drupal_page_set_cache() and drupal_page_get_cache() would do it. drupal_page_get_cache_alter() would need to be added to bootstrap_hooks() so that modules implementing it get loaded early enough.

The above will work fine until we have the long term goal of pages stored in render cache (no issue yet AFAIK) and have #636454: Cache tag support

Attached patch does as i described in #1. I added docs for the new hooks but could not think of a test that doesn't duplicate existing tests for drupal_alter() and cache get/set. I don't see any novel, testable code here.

FYI for mobile_theme, in drupal 7 you can set drupal_page_is_cacheable(FALSE) to prevent caching. No need to patch core. There are workarounds for D6 too.

#2: page_cache.diff queued for re-testing.

Just tested this out and it works as designed. Marking RTBC.

@beejebus - could you paste in the code you refer to. Is it part of the patch? I don't see any code like that.

This isn't taking into account variable_get('page_cache_invoke_hooks');

Also drupal_alter() isn't designed to work outside a full bootstrap, it will likely need the equivalent of bootsrap_invoke_alter(). Currently the patch would mean loading the module_implements() cache for every request to see if any module implements the hook, which is likely to have a big negative impact on page caching.

Both of these make me think we should look at doing #1331486: Move module_invoke_*() and friends to an Extensions class before this, since that will necessitate cleaning up the bootstrap hooks mess that this is running into.

I'm not sure if is the same issue. On a D7 site I used language negotiation ONLY to browser detection and page cache was ON. I received the same content regardless of browser preferred language. That's because cache_page uses the full URL as CID which in this case doesn't contain the language. So, for content having the same path in 2 languages, having ONLY browser detection as language negotiation, will return the same cached content.

I'm going to mark this as won't fix. You can easily build your own cache implementation that would segregate by whatever you can think of (domain, phase of the moon, etc.).

Looks like core just doesn't work for some non-url language negotiation schemes. See #9. Reopening and marking as a bug.

@moshe weitzman,

Page cache CID should be build by context. URL is only a part of context.

For the language negotiation issue see #339958: Cached pages returned in wrong language when browser language used and #282191: TF #1: Allow different interface language for the same path.

@catch, I saw those but we need a unified CONTEXT BASED mechanism for referring a page cache item. URL, language, HTTP Headers, etc. I'm not very familiar yet with WSCCI progress but as I understood contexts are bundles of request parameters like: method (GET, POST, ...), URL, HTTP headers, etc.

We need CIDs for cache_page that reflects the contexts, not only a part of the context (URL). http://example.com/node/1 have to cache something for English, other content for Romanian. Also it needs (maybe) other page cache for mobile version. Etc, etc.

@claudiu.cristea: there will be no unified context based system in Drupal 7. The bug you reported in #9 is a current bug in Drupal 7. Therefore, there should be a fix for it which does not require a unified context system.

When there is a context system in Drupal 8, we could look at potentially adding additional request information to page cache entries, but that is more of a feature request than the bug reported here.

@catch, agree that WSCCI is bringing a big architecture switch so that we cannot follow the classical Fix D8 > Backport D7 > Fix D7. For this reason we can use 2 tickets:

1. D7: Fix the bug only for URL+Language context in #339958: Cached pages returned in wrong language when browser language used.
2. D8: This ticket was opened for D8 so let's keep it to fix the bug in D8. For D8 it's a bug fix (inherited from D7) but also a feature request as you say. Postponing till context system will be committed.

For those that need this in contrib without patching core, you can use a similar approach (only implemented for Apache so far) in a settings.inc that is included from settings.php before the page cache is used:

#1768556: Context Mobile Detect does not work with page cache enabled has a patch that makes a contrib module work that needs contextual page cache.

The idea is very simple. We adjust the request_uri() by changing the parameters to include a "?device=1" or "&device=3" depending if the request uri has parameters or not.

On non Apache browsers will it just not work as I did not implement the other edge cases for request_uri(). As request_uri() is used for the cache key, this will always work and allow dynamic page cache keys, but the code needs to be run before bootstrap in settings.php, so it is very limited in functionality.

Enjoy!

- Fabian

This is also breaking RESTful Web Services (and, presumably the REST support already in D8 core, which is based on the same code):

#1969466: Fix GET /node/1 with page caching

which is preventing us from deploying it on Drupal.org for the D7 port:

#1710850: Deploy RestWS for D7 project issue JSON

Unless I misinterpret the history here, this is a legit bug that we need to fix in D8 (and backport to D7). If I'm barking up the wrong tree, please point me to the more appropriate issue.

Thanks!
-Derek

It seems to me that this must also be a critical bug for WSCCI, unless they have already solved this in some other way. It would be good if someone familiar with the most up-to-date status of WSCCI could validate the presumption stated in #19.

This is why I've been arguing that we switch to HttpCache for some time: #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache

Mark Sonnabaum is convinced that if we fix our headers (part of that issue) then we can adapt our current cache system to take into account whatever the Http headers say to take into account. I'm skeptical, but the plan is to split that issue up to just do the header tweaking first, then he'll take a crack at it.

See also:
#1855260: Page caching broken by accept header-based routing
#1973206: ViewSubscriber with page caching - Denial of Service (SA-CONTRIB-2013-042)

I think at least some of these can be marked duplicates.

Larry Garfield confirmed assumption in #19: the page cache does not work with WSCCI, and that is a release blocker: https://twitter.com/Crell/status/324646238643441664?p=v

Crell, thanks for the cross-links. Symfony HttpCache sounds promising, although we may still need this issue for D7.

If we do end up keeping our old {cache_page} with an alter hook on $cid, varchar(255) is probably not big enough – hashing this might be wise.

Alter hook on $cid sounds like an absolutely horrific idea, and I will try to forget you mentioned it.

For D8, the correct solution is to use the Vary header in HTTP, which will say what values (path, cookie, accept, etc.) to use for caching. Any cache system we ship with in D8 MUST know how to handle that.

For D7... I have no idea. Not my problem. :-)

Alter hook on $cid sounds like an absolutely horrific idea, and I will try to forget you mentioned it.

Crell, Can you elaborate? That was the patch Moshe proposed in #2, and so far nobody else has recoiled in horror. :-) It's also conceptually similar to the vcl_hash() function in Varnish.

I agree that using the Vary header has the edge in theoretical purity. Does the Symfony approach have a way to normalize the incoming headers? For example hashing the Accept header in response to Vary:Accept sounds great in theory, until you realize that every browser sends a different value for Accept, and many of them are almost nonsensical. Without normalization your cache hit rate would be abysmal.

Edit: huh, here's another one: #1855260: Page caching broken by accept header-based routing

I don't see how HTTPCache helps this - that also caches by URL and doesn't take into account accept header or anything else. It's got absolutely nothing to do with how to fix this issue, it's just an implementation detail of where the fix might possibly end up.

Also like grendzy says both Vary and Accept really aren't very helpful here.

There's at least a couple of issues:

1. If you're serving the same content by language/content type on the same path, then that needs to be reflected in the cid (whether internal caching or reverse proxy). The whole point of reverse proxy caching is not to hit PHP, accept header and language normalization are done in PHP.

2. If you're serving content at different paths you're OK in the sense that the cache will be correct, however you'd still need a layer in front of the cache system redirecting to the correct place if that was going to work for requests to pages that are already cached - i.e #339958: Cached pages returned in wrong language when browser language used which dealt with language preferences in the browser.

So it then comes down to whether we allow language negotiation (lots of dependencies including configuration) and accept parsing and normalization (likely the same) to run for cached pages, which is going to significantly reduce the effectiveness of internal caching in the first place.

This is a release blocker and should be marked as such. See Crell#24: "Any cache system we ship with in D8 MUST know how to handle that." (emphasis his).

This is not a Drupal.org D7 launch blocker. #1969466: Fix GET /node/1 with page caching is the issue to watch for us (needed for #1710850: Deploy RestWS for D7 project issue JSON).

Marking as duplicate of #1855260: Page caching broken by accept header-based routing. If there's something left here that's not in there, please re-open with a new issue summary.