Cached pages returned in wrong language when browser language used

In Drupal 6, to keep the caching keys intact, we should redirect to the proper language URL instead when identified based on browser or user prefs (case 2 above). I am not sure what people think about applicability of that to Drupal 7.

drupal_get_cache_id() was missing a return. Only failure remaining is the hook_boot() hook_exit() test now.

And the hook_boot/exit test had a bad check for page cache id, replaced it with a similar assertion to the page cache test itself. So back to code needs reviews. Will try to benchmark this next, see what the impact is.

This is on a vanilla install of HEAD with no content, all numbers are requests per second.

No measurable difference without page cache, as you'd expect.

Aggressive caching, APC disabled:

HEAD:
66.29
67.31
67.98

Patch:
66.65
68.43
68.59

With APC enabled:
HEAD
213.60
215.24
214.45

Patch:
211.05
216.68
214.73

Normal cache:
HEAD:
190.17
196.45
191.77

Patch:
188.93
194.83
188.51

As far as I can see, no appreciable difference either way despite the extra include.

+ return $base_root . request_uri() . $language->language;
I suggest adding a space (or some other character not valid in URLs) before the language code to avoid conflicts in rare cases where one language code is a substring of another.

Example:
URL=http://example.org/foo language=es-sv (Spanish, as spoken in El Salvador)
URL=http://example.org/fooes- language=sv (Swedish)

The main reason I can think why changing the caching keys would be a problem is immediately after update - leading to a lot of stale keys in the cache_page table, and having to rebuild the cache. While re-building the page cache is expensive, it's a one-off expense, and we'd probably only need to do a cache_clear_all(NULL, 'page') on sites with language determination - the others could be left to slowly clear out and rebuild their page caches over time.

Ideally keys would be exactly the same for monolingual sites - it looks like #9 already accounts for that. While rebuilding the page cache is expensive, that's a one-off expense. Adding the redirect means two bootstraps for every user who needs to be redirected - although I'm not at all clear how many of such redirects might happen on multilingual sites in day to day operation. So, if we could decide whether its relatively safe to change the cache keys for multi-lingual sites (which are the ones affected by this anyway), then it would seem ideal to go for that approach.

Having said that, in both recent patches I'm concerned about moving hook_boot() around, especially for Drupal 6 - and having its execution change dependent on locale settings. Seems more potentially fragile than just going ahead and changing the cache keys. While it makes sense to leave things open for modules to intercept the page cache key setting and generation, some other modules use hook_boot() for completely different purposes, and we'd be adding that weight to normal cached pages for everyone. For Drupal 7 we can document the change (or even add another hook_intercept_page_cache()).
.

Here's an initial test to confirm the original bug - it's not working properly yet, I had the language negotiation picking up browser language at one point, but doesn't seem to be any more...

hmm, thought .txt didn't get tested (it's supposed to fail, but not those specific failures). reattaching #9.

And changing status.

I suggest using something other than ?, e.g. space, for separating the URL and the remaining part of the cache key. Otherwise the key looks like a URL, but not the actual URL being cached. Using space as separator will make it clearer that the key is actuall a URL and some more.

I don't know how much of the pages are cached, but there's a good chance this approach will conflict with #282178: Language negotiation overhaul, since the patch in this issue relies on Drupal having one language set rather than two (content/interface). Perhaps this patch could prepare Drupal for the patch in #282178: Language negotiation overhaul by taking those two languages in account already?

Some more benchmarks on #14.

- I unhid system_test.module so that we can see the impact of calling hook_boot() earlier. system_test.module adds a watchdog call to every page request. With HEAD I get about c. 6.45/6.6 reqs/sec with it disabled, c.6.3/6.42 reqs/sec enabled, with no page caching - very small but were it to make a difference on cached pages we'd notice I think.

This is on a completely blank Drupal install, ab -c -n500 - all numbers are requests per second.

APC disabled, no page cache.
HEAD:
6.36
6.37

Patched:
/node
6.49 reqs
6.35

Normal page cache:
HEAD:
42.51
41.35

Patch:
42.45
41.11

Aggressive:
HEAD
67.16
69.65

Patch
66.00
68.21

So no appreciable difference that I can see.

Patch applied with offset, re-rolled to include c960657's feedback - so using a space rather than '?' - also used drupal_query_string_encode() rather than http_build() there. We possibly don't need the encoding at all since this is just a cache key rather than a URL though.

Need to look into those failures, just re-rolled for offset this time.

I think using http_build_query() is better than drupal_query_string_encode(). The latter depends on drupal_urlencode() whose output depends on a variable, and the function is about to be modified in #339958: Cached pages returned in wrong language when browser language used, so http_build_query() is more stable.

If you use http_build_query(), you should supply the third argument – otherwise the separator character is based on the arg_separator.output PHP ini setting (for some reason, Drupal's default.settings.php sets this to & instead of &).

I think the only acceptable approach is #2.

The approach currently worked on here can cause issues, for a very simple reason: a paged served from the cache will be returned with Last-Modified and ETag headers, and any proxy server on the way could return the page cached in a different language.

After all, it was a main design objective of the language negotiation system of D6 to always return the same content from a given URL. After all URLs are supposed to be stable, and it's the only way HTTP/1.1 caching could possibly work.

If Drupal emits a Vary: Accept-Language header, proxy servers are able to deal with the same URL returning a page in two different languages. So I think #3 is also feasible, even with HTTP caching.

It's technically possible, but it's always better to have stable URL: one URL should as most as possible always return the same content. I think it's easier and cleaner just to redirect the user.

This is a re-roll of #10 for D7 which enforces a redirect. All tests pass but I've not done much manual testing and no new tests introduced. Note I had trouble writing a test for this earlier in the issue trying to alter ACCEPT_LANGUAGE for a page request - any ideas on how to get around that would be great.

It recently became possible to add HTTP request headers to the internal browser in SimpleTest. You should be able to set HTTP_ACCEPT_LANGUAGE using e.g. $this->drupalGet('', array(), array('Accept-Language: en')).

And some modifications in line with the other patch to ensure we don't do page_get_cache() redundantly if we're going to redirect anyway.

c960657 - thanks!

Here's a test which simply demonstrates the bug in HEAD rerolled from the one I posted above. Should have two failures.

Here's #27 with a slightly modified test - can't work out how to get simpletest to follow the redirect properly (yet?), but it confirms that the cache is only getting set in the correct language. On default 'en' path and requiring prefixes we should probably let those other issues deal with those and confine this to the bootstrap changes.

        require_once DRUPAL_ROOT . './includes/common.inc';

The literal string should not start with a period. This generates a warning:
Warning: require_once(/home/chsc/www/drupal7./includes/common.inc): failed to open stream: No such file or directory in /home/chsc/www/drupal7/includes/bootstrap.inc on line 1152

Without this warning, simpletest automatically follows the HTTP redirect. You can get test the actual URL using $this->assertEqual($base_url . '/fr', $this->getUrl()).

The patch actually redirects to http://example.org/fr/node rather than just http://example.org/fr.

You may use the $this->drupalGetHeader('ETag') trick used in BootstrapPageCacheTestCase to verify that the page isn't just saved to the cache but also served from the cache on the second page load.

Modified test to check for Etag headers and minus the extra dot - all passes. Thanks again for the in depth review c960657.

Added some additional code comments to the test, otherwise no changes.

Here's a backport for D6. I confirmed it doesn't break massively, but didn't do thorough testing yet.

And a re-roll for D7 to keep the test bot happy.

And the D7 patch this time.

Re-rolled after the user session changes.

Here's one which doesn't completely break user logins.

Backported to D6. Seems to work nicely! One particular thing: The call to variable_init() had to be moved up to DRUPAL_BOOTSTRAP_LANGUAGE, because drupal_init_language() needs the variables. We don't have the DRUPAL_BOOTSTRAP_VARIABLES step under D6.

Mmmh. I'm not too sure that the patch (at least the D6 one I have just submitted) works that well. bootstrap_invoke_all('exit'); appears just before a call to drupal_goto(), which will also invoke hook_exit().

Even without that apparently redundant call, we might still have issues with the statistics module (that relies on hook_exit()), since if we redirect to the same node, the counter will be incremented once before the redirect (from drupal_goto()'s invocation of hook_exit()), and a second time after (when bootstrapping the redirect's target page).

Good catch about the duplicate call to hook_exit() - here's the HEAD patch re-rolled without that.

For statistics module, I don't think we need to deal with that here - hitting refresh has the same effect - see #90468: Only record unique hits in node counter stats

Since this makes language negotiation and page caching incompatible, I'm bumping to critical.

+ * @return
+ *   Boolean indicating whether the language set is valid for the current
+ *   path.
  */
 function language_initialize() {

AFAICT the return value is stdClass or NULL.

       // Get the page from the cache.
-      $cache = $cache_mode == CACHE_DISABLED ? FALSE : page_get_cache(TRUE);

The comment should be removed together with the line it is referring to.

The comments do not wrap at 80 characters:

+    // Visit the page again with browser language set to English, and confirm that

This line is 82 characters.

+      // If the language has been reset from the default, redirect
+      // to the new language.

The first line is only 66 characters, so there is room for several more words.

Thanks for the review, I think think this should cover everything.

Here's a somewhat simpler approach. If we fall back to browser language, and the language isn't the same as the default language, then we neither serve a cached page, nor cache the page request.

This is done by setting a $no_cache global, and unfortunately still requires changing the bootstrap order to avoid serving cached pages to anonymous users in the site's default language when negotiation is on (if we did that, we might as well completely remove language negotiation from browser as an option).

This version still passes all the tests, including the slightly modified new tests added with the patch.

Note that there's a more comprehensive solution to this for Drupal 7 at #54238: page cache might show messages - but that won't fix the issue here since we still need to have language_initialize() run before BOOTSTRAP_LATE_PAGE_CACHE. I'm hoping we can commit this, backport to Drupal 6 quickly, and then concentrate efforts on the more generic patch for Drupal 7.

Here's a straight backport for D6.

Thought about this some more, and realised that there's no point changing the bootstrap just to avoid serving cached pages to anonymous users where the language is taken from the browser.

So this updated patch is a lot smaller, and doesn't touch bootstrap.inc at all. Also updated the simpletest to remove the check for this edge case.

Quick summary:

The main bug here, is that if your site is in English, German and Swahili, and someone visits your front page with their browser set to request Swahili, all requests for the cached version of that page will see it in Swahili if the browser fallback is enabled. Then 10 minutes later, the cache is cleared and the first anonymous visitor has their language set to German, and so on.

This (hopefully final) version of the patch prevents the page being cached when language determination is used. However to avoid bootstrap re-ordering, visitors will be served cached versions of pages in the site default language when available - since language negotiation happens after the cached page is served. This is a much smaller edge case which only affects a subset of users in certain circumstances, and IMO that particular edge case is won't fix.

This means the approach here will be compatible with the upcoming changes in #370454: Simplify page caching (which are a big win), and it also means much less intrusive changes to Drupal 6.

I think this looks good and can be marked RTBC. Catch has summarized the underlying issue in the last comment, but to briefly summarize the patch:

• Previously the page wasn't cached for anonymous users if caching was disabled. Now, in addition to that, the page isn't cached if the language set is the browser language but isn't the site default language.
• Includes a set of simple tests for ensuring that cached pages are served in the correct language.

+  global $user, $no_cache;
...
+  if (variable_get('cache', CACHE_DISABLED) != CACHE_DISABLED && $no_cache != TRUE) {

$no_cache is not defined anywhere else. So the proper test is isset($no_cache) && !$no_cache.

However, while I can see and understand the actual issue, the entire, negated logic feels very poor. Please try hard to think of a way to do the opposite, or, at the very least, find a better name for that global variable.

I applied the #57 D6 patch + the advice from sun at #61 to a production site (http://musescore.org) which was having this issue. Will report back my findings later. Thanks in advance to all developers involved.

Even smaller patch. No need to set a new global variable because we already have one (hat tip quicksketch on the imagecache in core issue).

It's not pretty, but it's a tiny change to fix a critical issue, and we absolutely need to backport this - so assuming it can go in I'd support leaving this issue open for a better system in Drupal 7 once that's done.

And for Drupal 6.

Thanks catch for pointing me to this issue. You mentioned in the #371374: Add ImageCache UI Core issue that "we need a generic way of doing this" (temporarily disabling the page cache), but the ImageCache issue was not the place to do it. This patch could use a generic way also, and then the ImageCache patch could use it. Would this be the appropriate place to add such a generic function?

Here's a stab at an implementation, which would live in bootstrap.inc below page_get_cache()

/**
 * Temporarily disable the page cache for this request
 */
function page_set_cache_temporary() {
  global $conf;

  // The global conf variable affects all variable_get() requests, but does not persist across requests.
  // Setting the cache value in memory disables the cache only for a single request.
  $conf['cache'] = CACHE_DISABLED;
}

Doing it as a new function in D6 wouldn't do any harm, so let's try to fix this here. It's only a couple lines change either way anyway.

Don't like the function name though, sounds like we're going to set the page cache temporarily. Here's one with drupal_skip_page_cache() which was the least worst I could think of.

Nice, I like drupal_skip_page_cache(). Looks like you attached the wrong patch however. ;)

oh dear.

As I stated in #22 and #24, I don't believe that simply skipping page cache is the way to go. I would still prefer that we redirect the user to the proper, language-prefixed, URL. If we don't want to do that, we at least really need to output a Vary: Accept-Language header so as not to mess up with proxy and client-level caches.

@Damien - that was implemented around #48 or so, but requires changing the bootstrap order - a change which would have to be backported to D6, and would also conflict with chx's page caching revamp which makes the page caching the first or second bootstrap phase. Are you going to support a bootstrap re-ordering in D6 (or do you have an approach which doesn't require this up your sleeve?).

@catch: If language_initialize() redirects the user to the proper, language-prefixed, URL, this page will never get cached. Basically having the same effect as your "don't cache" approach.

Summary of irc conversation with Damien:

if we do #73 (which the patches around #48 did) - then we still break language negotiation for any user visiting a cached page - so it's still a partial fix, just one which requires slightly more work. The only way the bug can be properly fixed for all users is by re-ordering the bootstrap to put language negotiation before cached pages are served.

Two other possibilities - ensuring that Drupal never outputs non-prefixed pages. Or add something to the session when language negotiation is done, and disable page caching for the visitor until that's in place.

Damien, if we don't use the temporary disabling of the page cache, can you think of an alternative for #371374: Add ImageCache UI Core? I was crossing my fingers that this would get done here, since currently we need it for that patch also.

subscribing

Would a Content-Language HTTP header be taken into account by HTTP proxies/caches?

People looking for an immediate workaround for this issue (for Drupal 6) may have luck using the globalredirect module. This should prevent a path from displaying in two different languages at the same path.
Note that globalredirect is not normally active on the front page, however there is a patch posted in issue #317305: When browser language is used, foreign language redirection breaks on the front page. that will make it so.

For your information, there is dirty workaround using apache rewrite rules which solve this problem by addind hard coded languages in htacess. It's only for browser language detection and drop "/" frontpage.

This is not the drupal's way but perhap's can help people who absolutely need language browser detection with drupal cache.

We did this for example :

"Path prefix with language fallback" should be activated in drupal.

  RewriteCond %{HTTP:Accept-Language} ^fr [NC]
  RewriteCond %{REQUEST_URI} ^/$
  RewriteRule .* /fr [L,R]

  RewriteCond %{HTTP:Accept-Language} ^de [NC]
  RewriteCond %{REQUEST_URI} ^/$
  RewriteRule .* /de [L,R]

## Default language is /en, we don't use /
  RewriteCond %{REQUEST_URI} ^/$
  RewriteRule .* /en [L,R]

@niQo Thats a pretty interesting fix you have there. Any way to write a condition + rule for all language in one time, using some kind of reg exp?

I didn't test this yet, but perhaps it's in the right direction:

RewriteCond %{REQUEST_URI} ^/$
RewriteCond %{HTTP:Accept-Language} ^(de|es|fr|it|ja|ru|en) [NC]
RewriteRule .* /%1 [L,R]

Does this mean Drupal should be able to edit .htaccess depending on the available languages?

@Xano No, Drupal won't be able to edit the htaccess file. You'll have to do it manually for each language enabled.

subscribing

This issue has been fixed in #282191: TF #1: Allow different interface language for the same path by disabling the browser detection when page cache is enabled and we have an anonymous user. The reasons are explained in #282191-16: TF #1: Allow different interface language for the same path. In #50-#61 we agree that we might want to find an alternative solution.

Marking as fixed, if we want to find another way to fix this and enhancing the current solution we should open a new issue taking into account the current situation.

Re-open the issue if you think we should keep handling this here (at least change the issue title in that case).

This still needs a solution for Drupal 6.

[please ignore this post, I forgot I should never work on critical core bugs this late ;)]

What about something like the following?

function locale_boot() {
  global $user, $language;
  $cache = variable_get('cache', CACHE_DISABLED);
  $language_negotiation = variable_get('language_negotiation', LANGUAGE_NEGOTIATION_NONE);

  // If we have an anonymous user, page cache is enabled, the URL has no prefix
  // and browser language negotiation is set, we perform a redirect to the
  // prefixed URL. 
  if (empty($user->uid) && $cache != CACHE_DISABLED && $language_negotiation == LANGUAGE_NEGOTIATION_PATH) {
    $args = isset($_GET['q']) ? explode('/', $_GET['q']) : array();
    $prefix = array_shift($args);

    // Initialize language as we need the proper language negotiation to be
    // performed.
    drupal_bootstrap(DRUPAL_BOOTSTRAP_LANGUAGE);

    if (!empty($language->prefix) && $prefix != $language->prefix) {
      // We need full path support to perform the redirect.
      drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH);

      require_once './includes/common.inc';
      $url = url($_GET['q'], array('query' => drupal_query_string_encode($_GET, array('q'))));
      $code = variable_get('locale_redirect_http_code', 301);

      // Since we are in a hook_boot() implementation cache mode is normal, so
      // we can safely call hook_exit().
      bootstrap_invoke_all('exit');
      header("Location: $url", TRUE, $code);
      exit;
    }
  }
}

subscribing

I solved this issue with Varnish cache, avoiding the issue with Drupal. See http://groups.drupal.org/node/62768 and browse through the comments.

subscribing

subscribe

Subscribing. I'm experiencing this issue on a site in 22 languages!

I also need the ip2locale, ip2cc, ip2country, geoip modules and therefore I cannot avoid this issue using Varnish cache.

This is present in the latest development snapshot, please don't change the version.

Changed status to active as that patch didn't work.

Sorry didn't see previous comment...

Downgrading to major as this does not render completely unusable the site.

Upgrading back to critical: I know my english guests seeing a mongolian front page don't agree with you plach :)

If your problem is so critical, all I can say is that this worked for me in case you are 'not' using Varnish.

@aidanlis also, don't forget to apply this patch on the D core in case you work with exotic languages which I think you do (3 versions of Chinese is exotic).

Thanks toemaz, I have the patches applied and everything is working for me, but I think the "critical" status still applies here until it's fixed in D6. Plach is your patch in #88 ready for review?

@aidanlis:

I know my english guests seeing a mongolian front page don't agree with you plach

I'm sorry but looking at the priority guidelines I'd say they are wrong, obviously the perception of priority is subjective :)

Plach is your patch in #88 ready for review

It's only a proposal to introduce a redirect to the prefixed version of the page as suggested by Damien. It's not a patch and can only be tested by manually applying it.

@plach you're indeed correct

I'm also seeing this with a standard install. For me, the easiest work-around is MultiLink Redirect which provides a redirect from generic front-page url to language-prefixed url - but, this is my own module therefore I'm happy with it - it's overkill if all you need is a simple fix to the front-page caching problem.

Apache redirect as suggested in http://drupal.org/node/339958#comment-2503184 also seems to work fine.

#63: cache_negotiation.patch queued for re-testing.

Cache Exclude is a good solution for the problem. I excluded <front> and enabled normal caching resulting in a much better performance for anonymous users. Thanks for the tip sypl !

I have an improved version of #88 deployed on http://graphicdesignworlds.it which seems to perform well and is implemented through a contrib custom module. I ain't sure we can make such a big change for D6, perhaps it should just live in contrib for D6/D7.

We definitely need Gabor's and/or webchick's feedback before going on with a core patch implementing #88.

Edit: perhaps we just want to backport the D7 solution, i.e. disabling browser fallback for cached pages and make #88 live in contrib.

+1

Subscribing

This may be specific to my install, but is anyone else still seeing this behavior for anything other than the homepage?

All pages other than the homepage seem to successfully redirect and have the path-prefixed URLs cached.

I spoke too soon. It was specific to my install, but the contrib module that was giving the (desirable) behavior was Global Redirect, as first suggested by intuited in #78.

I've since submitted a patch at the Global Redirect issue mentioned in #77 which solves this issue completely in contrib. It borrows ideas from #88.

I recommend you try Global Redirect and the patch here.

Nevermind, the solution above does not work.

Subscribing

i know this is for D6, but I created a sandbox module that prevents this problem from happening by executing a real redirect instead of serving the content under the home page URL: Front page redirect.

I guess it will be very easy to backport it: the lookup using the i18n_variable module should be replaced by looking in $conf['i18n_variables'].

@fietserwin, took a quick look at your code and you're falling into a similar trap. hook_init() isn't called on cached pages; you'd want to implement it in a hook_boot(). That being said, you may be duplicating functionality of the Global Redirect module.

The page should not be cached because I redirect (the redirect may be cached though). I am still doing some tests though, also in a first D6 backport version.

On most sites, the front-page would be one of the most frequently accessed pages, so preventing it from being cached using the Cache Exclude module does not seem to me like the best solution for performance reasons.

Redirecting via a Drupal module such as Global Redirect or MultiLink Redirect is not the best solution either for performance, since quite a lot of Drupal code and MySQL queries will be run before the the user is redirected to the correct (cached) page.

So, my advice would be to redirect using Apache Rewrite, as suggested by toemaz in #81 - I have implemented this on client sites and it works well. For situations where mod_rewrite is not available then my choice would be MultiLink Redirect, but I'm biased :) Global Redirect should work too I think.

I'll do some performance comparisons and post them here.

The solution form #81 won't always cope correctly with a header like "Accept-Language: nl,en;q=0.7,fr;q=0.3". On a French-English site I want the English version. But performance wise it may still be a good bet.

There is now a patch for global redirect that works fine for me and solves this problem for D6 in contrib, though your default language must have a prefix. I guess #81 and this patch can both be applied and you will get the fast redirect if the most preferred language of the user is supported, and the slower contrib solution if not.

[Edit] Oops, posted twice...

I'm now wondering whether an Apache rewrite would be better/faster than a redirect - could make it look like the relevant language version really was at the front-page address. Just an idea, haven't tried it.

Should be faster. But be careful with (intermediate) caching when serving different content under the same URL. The rewrite should also add a Vary header.

Will there be a solution for D7 or D8?

It is possible to solve this in D6 and D7 in contrib by including a file from settings.php:

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 "?language=de" or "&language=fr" 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 still a serious problem in D7...

The bug reported in the OP does not occur in D7. Please, leave the version alone.

@plach this problem occur in D7 as well.

Browser language negotiation is disabled when page cache is enabled in D7. Hence the bug reported in the OP cannot happen. Please don't change the version again without a list of steps to reproduce the bug described in the OP (page cached in the wrong language) on a fresh D7 installation.

I would like to propose another solution. #81 works great unless your default language doesn't have a path prefix in which case you need a bit extra...

In your settings.php (there may be a more appropriate place but hook_init() is too late) you can put:

if ($_SERVER['REQUEST_URI'] == '/') {
  $_SERVER['HTTP_ACCEPT_LANGUAGE'] = 'en';
}

This basically forces the root domain to always be in English whatever the browser's language. Since this runs after mod_rewrite it works perfectly well in conjunction with #81.

This is how I got around this issue. It seems that is working fine for me (I'm using Drupal 7):

my_module_init(){
  if(!user->uid){
    global $language;
    require_once DRUPAL_ROOT . '/includes/locale.inc';
    $languages = language_list('enabled');
    $languages = $languages[1];
    $language = $languages[locale_language_from_browser($languages)];
    $language->provider = 'locale-session';
  }
}

As this issue been solved? I have the exact same problem with drupal 7.

Liked the look of #131 but couldn't get it to work. My solution for Drupal 7 below. n.b you will need to resave your language negotiation settings on the admin page to get the hook to apply.

function MYmodule_language_negotiation_info_alter(array &$language_providers) {
	//as standard language detection by browser is disabled if page caching is on
	//this is to prevent language specific versions of pages being cached based on a user preference
	//we circumvent this issue in MYmodule_init and so disable the page cache test here
	unset($language_providers['locale-browser']['cache']);
}

function MYmodule_init(){
	//if no language has been specified in the URL we prevent content on this path being cached
	//this allows fallback to language detection via browser settings
	//performance should not be significantly hindered as the vast majority of pages will be accessed with a language prefix
	//see also MYmodule_language_negotiation_info_alter
	//n.b requires all languages to have a prefix set and URL detection to be the primary form of language negotiation 
	
	//get the language set in the URL prefix
	$languages = language_list('enabled');
    $languages = $languages[1];  
	$path = urldecode($_SERVER['REQUEST_URI']);
	$path = substr($path, 1); //strip leading slash or language_url_split_prefix will always return false
	$urlLang =  language_url_split_prefix($path, $languages);
	
	//if no language specified in URL prevent page from being cached
	$defaultCacheBool = drupal_page_is_cacheable();
	if (!$urlLang[0]) {
		drupal_page_is_cacheable(FALSE);
	} else {
		drupal_page_is_cacheable($defaultCacheBool);
	}
}

Works for me but very hacky. Would still like to see a genuine fix.

For D7 we based our fix on #131 as well, but in our case to redirect anonymous users to the frontpage at a language-prefixed URL. Since the frontpage is cached in the default language, we only need to redirect users with another preferred language set in their browsers.

function my_module_boot() {
  global $user;
  $request_uri = request_uri(); // Get URI cross platform.
  $parsed_url = parse_url($request_uri); // Separate path from query string.

  // If the user is anonymous and the requested path equals Drupal's base path (= frontpage without language prefix).
  if (empty($user->uid) && $parsed_url['path'] === $GLOBALS['base_path']) {
    // Get enabled languages.
    require_once DRUPAL_ROOT . '/includes/locale.inc';
    $enabled_languages = language_list('enabled');

    // Get the browser's preferred language (langcode) or false on failure.
    $langcode_from_browser = locale_language_from_browser($enabled_languages[1]);

    // If the browser's preferred language is found and it differs from the site's default langcode.
    if ($langcode_from_browser && $langcode_from_browser !== language_default()->language) {
      // Redirect the user to a language prefixed URL.
      header('Vary: Accept-language', false);
      header('Content-Language: ' . $langcode_from_browser);
      header('Location: /' . $langcode_from_browser . $request_uri, true, 302);
      exit;
    }
  }
}

Is redirecting a SEO safe approach?

Is redirecting a SEO safe approach?

If you add a "Vary: Accept-Language" header it should be.

Hi fietserwin,

Thanks for the comment. I changed our code by adding the "Vary: Accept-language" and "Content-language: [langcode]" headers, but also changing the HTTP response code from 301 (moved permanently) to 302 (found).

Thanks again!

#135 is exactly the behavior I expect from Drupal core, thanks a lot for this.

I made changes to prevent saving wrong cache when language is detected by browser

Please see #129.

There's a quick'n dirty fix for D7 in #81, if you're not familiar with writing modules. Some say that the Boost module does the job as well for Drupal 7.

RewriteCond %{REQUEST_URI} ^/$
RewriteCond %{HTTP:Accept-Language} ^(bg|hr|sr|sl|hu|pt|es|en) [NC]
RewriteRule .* /%1 [L,R]

Hello knalstaaf,

I use your fix and it's useful for the main domain (www.domain.com). But, I have several subdomains with no languages, and this fix is functionnal on my subdomains. Now the subdomains aren't accessible.
Is it possible to change the condition and create the fix for only the master domain?

Thanks.