Browser language detection is not cache aware

Any news on that?

Hi everyone, I have a similar problem, my project is configured via detection and selection URL by domains method and I need to switch language to the correct domain accordingly to the user's browser preferences. But in my case, when using detection and selection via browser I'm constantly switched to my preferred language without the ability to switch language only once(what I'm achieved with custom event subscriber and redirect). I added a custom code that redirects user but now I'm having a problem with SEO as Google search bots is redirected too, and all machine services return 302 status code besides the basic English one as those bots I guess have English as a preference. Is there a way for redirect to not mess with bots or to detect them and not redirect if that not human-users, none of those patches fixed behavior when the user is constantly redirected and none of them set the right domain in the URL as a result I getting always URL from with browser switch to the preferred language? Has anyone solved this problem somehow?

Since the cache ID is actually cached, the $vary_headers passed to \Drupal\page_cache\StackMiddleware\PageCache::getCacheId() actually did nothing.

I created a merge request from #2430335-194: Browser language detection is not cache aware → and did some changes to cache the cache ID by $vary_headers.

I fixed the visibility of the modules variable of the test. Drupal\Tests\language\Functional\LanguageBrowserDetectionAcceptLanguageTest still fails.

X-Drupal-Cache is now always present. It is MISS for the first access and HIT for further accesses.

Left some comments on MR

I don't know if adding a CR from D9 is correct anymore but will leave as is.

added 2 questions to MR

I don't know why this is not a local variable. But since it only reads the request, a clone is not necessary.

Believe all feedback has been addressed around MR but don't really follow the issue summary, specifically the proposed solution section. Will leave in review if anyone else wants to pick up.

This includes #3023104. As one of the authors of the cache policy mechanism, I'd be disappointed if this approach would make it into core. Citing from #3023104-4: Introduce "Vary" page cache response policy → :

Regrettably this approach clearly violates the ResponsePolicyInterface contract. While not stated explicitly in the documentation, the check() method is certainly not expected to actually change the response.

I suggest to move the logic into a response event subscriber instead.

cola → changed the visibility of the branch 2430335-browser-language-detection to hidden.

bbrala → changed the visibility of the branch 2430335-browser-language-detection to active.

I played around with this and moved the implementation to a eventsubscriber as asked for by @znerol. I see one things that is very weird though as per comment in the code:

    /**
     *  Either Drupal\Tests\page_cache\Functional\PageCacheVaryTest::testPageCacheWithVary
     *  fails or Drupal\Tests\language\Functional\LanguageBrowserDetectionAcceptLanguageTest::testAcceptLanguageEmptyDefault
     *  fails if you remove one of those. Not sure why
     */
    $events[KernelEvents::RESPONSE][] = ['onRespond', -100];
    $events[KernelEvents::RESPONSE][] = ['onRespond', 100];
    return $events;

Not sure why, those priorities sometimes confuse me a little.

Well at least everything is green. So thats good. But it some weirdness with the event subscriber. Not sure how that should work. I didn't investigate the redirects and such. Mostly just refactored the current recent patch to work with an event subscriber.

Left some comments in the MR.

I recently discussed thie with catch.

Using the header and vary on that is very dangerous for cache purposes. It will make very very many variations, which results in bad caching and performance. Not to mention possible memroy issues in caches or high cache trashing.

We did think about how we could possible work around that.

What if;

We normalize the prermutation of the language into the resulting language, and cache that normalization and the resulting language.

So perhaps a site with en_EN and fr_FR. A browser comes along and tells the site "Hi i'm en-GB,en-US;q=0.9,en;q=0.8". The site would find out what this will end up as so

en-GB,en-US;q=0.9,en;q=0.8 magic => en_EN, we cache this and threat serve cache asif en_EN. This would mean (at least on the drupal side) only the records for inLang => outLang exist, which are pretty small and we dont end up with loads of difference permutations.

Not entirely sure how we would implement that, but feels like a way to have drupal cache be happy.

The little devil on my shoulder also has a voice though. Which does tell me this is kinda a. can of worms as this could also easily break caching on the caches in front of Drupal, be it varnisch or maybe smoething like cloudflare. That is kinda the scary part. Perhaps contrib is better to have this live, instead of trying to get this into core.

Using the header and vary on that is very dangerous for cache purposes. It will make very very many variations, which results in bad caching and performance. Not to mention possible memroy issues in caches or high cache trashing.

While this is true for a web-based environment, it is not true for JSON:API.
Quoting @GabeSullice from https://www.drupal.org/project/drupal/issues/2794431#comment-12806367 🌱 [META] Formalize translations support Needs work

However, JSON API doesn't need to be very concerned about that because JSON API requests are not typically made directly via a browser. Instead, they're most frequently made via JavaScript or some other system where the Accept-Language header can be tightly controlled by a developer.

Re #21: Fair.

Maybe we need a way to create two (or more) groups of routes. One group is accessed by browsers, others by special clients. Then apply different sets of rules for request processing, content/language negotiation, authentication and caching to each of them.

However, page cache is only effective if it runs before request routing. So maybe the matching needs to happen on a path prefix.