Cached pages returned in wrong language when browser language used
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | language system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | i18n sprint, needs backport to D6 |
Bug description:
On multilingual sites, when caching is enabled and language negotiation is set to "Path prefix with language fallback", page caches are set to the first visitor's browser language preferences in cases of fallback. All subsequent visitors get pages in that first visitor's language rather than their own.
Cause:
Setting the language based on browser language means there are multiple languages in which a particular page/path can be served. However, since page caching is determined on the basis of the request URI, only one cache can be set per page/path.
More generally: The Drupal cache system doesn't support multiple versions of a page per URL. Any time a particular URL may be served in more than one version, the first visitor's version will determine the cache.
Steps to reproduce:
See http://drupal.org/node/330156#comment-1116611
Potential fixes:
-------
1. Drop browser language detection support (or limit it to authenticated users).
This approach would restrict browser language detection to authenticated users, who don't get late page caches, or else remove the "Path prefix with language fallback" option altogether.
Doing so would address the issue but lose some valuable functionality.
2. In the case of a language fallback, forward to the new path, including the correct prefix.
By forwarding, we could ensure that the path matches the language of the page served.
This approach would mean:
- Move the DRUPAL_BOOTSTRAP_LANGUAGE bootstrap phase to come before DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE. This is needed because we need to determine a potential language/path mismatch before consulting the cache.
- In the case that the path doesn't include a prefix for the negotiated language, abort bootstrapping and goto a prefixed path.
3. Include language information in the cache key.
If we determine language before caching, we can include language information in the cache key and so have language-appropriate cache results.
This approach would mean:
- Move the DRUPAL_BOOTSTRAP_LANGUAGE bootstrap phase to come before DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE.
- In page_get_cache() and page_set_cache(), call a helper function to add the language information to the cache key. E.g., append something like
'cache_language=' . $language->language.
-------
I think all these options are worth considering, as well as other approaches others may come up with.
Options 2 and 3 are significant API changes. They represent two different basic approaches. Option 2 reflects the position that we should never have two different versions of the same URL. Option 3 assumes that having two different versions of the same URL should sometimes be supported. (See this related issue/discussion: #282191: Allow different interface language for the same path.)
And even if we solve late page caching, we still have early page caching to worry about. Neither option 2 nor option 3 addresses the problem presented by DRUPAL_BOOTSTRAP_EARLY_PAGE_CACHE. If we have file-based caching, invoked before we have database access etc., how can we negotiate language ahead of invoking that cache so we know which version of a page we're concerned with?
#1
Here's a draft patch for the third approach I outlined above.
#2
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.
#3
The last submitted patch failed testing.
#4
drupal_get_cache_id() was missing a return. Only failure remaining is the hook_boot() hook_exit() test now.
#5
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.
#6
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.
#7
+ 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)
#8
Thanks catch!
If we're introducing the ability to set cache keys enabling different versions of the same page, language is only one of the possible reasons this might be desired.
So, instead of hard-coding language into the page cache key, maybe we introduce a method for setting the page cache key.
And then rework the order in which page_get_cache and hook_boot are called.
If we want modules to be able to affect the cache ID, we need to call hook_boot before fetching the cache. Our current order has page_get_cache called before hook_boot. This order seems designed to allow calling of hook_boot when no cached page is found even if aggressive caching is enabled. But why, since we explicitly say that hook_boot is incompatible with aggressive caching?
I'll look at working that into the next patch iteration.
#9
Changes in this patch as per #8:
1. Introduce a method for setting the page cache key. Call this method in language negotiation. Resulting page cache keys will look like:
http://example.com/node/21?language=en
2. Change bootstrapping in DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE to call hook_boot before fetching the cache, unless aggressive caching is enabled, in which case call it only after looking for the cache. This way, modules can alter the page cache key if e.g. they use a different method for language negotiation at hook_boot, or otherwise provide different versions of a given page.
@c960657: thanks. This iteration adds a key as well as a value and so shd address potential conflicts.
#10
Here's a patch for D6 based on Gábor's suggested approach (my original approach 2) of redirecting to language-prefixed paths for Drupal 6 to avoid needing the change cache keys.
Gábor, can you elaborate on the problems that changing the key would cause?
We need to initialized the language system before consulting the cache. And, to do so, we need to initialize the variable system.
I've introduced a new global variable, $language_redirect, that is set to TRUE if user preference or browser settings have determined language. If so, we first call hook_boot - allowing modules to affect the outcome, e.g., by implementing their own language determination - and then conditionally redirect.
The redirection has to take into account the default language. Unprefixed paths are reserved for the default language. We only redirect if we are at a language other than the default one.
And we need to determine the normal path of $_GET['q]. This part of the patch seems particularly awkward--probably I'm overlooking a cleaner way to do this.
#11
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()).
.
#12
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...
#13
The last submitted patch failed testing.
#14
hmm, thought .txt didn't get tested (it's supposed to fail, but not those specific failures). reattaching #9.
#15
And changing status.
#16
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.#17
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?
#18
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.
#19
The last submitted patch failed testing.
#20
Need to look into those failures, just re-rolled for offset this time.
#21
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&).#22
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-ModifiedandETagheaders, 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.
#23
If Drupal emits a
Vary: Accept-Languageheader, 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.#24
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.
#25
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.
#26
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')).#27
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.
#28
c960657 - thanks!
Here's a test which simply demonstrates the bug in HEAD rerolled from the one I posted above. Should have two failures.
#29
The last submitted patch failed testing.
#30
Nice work catch.
Some of the complexity of this issue is related to the special use of non-prefixed languages. By default, en doesn't have a prefix. But what if the default language does have a prefix? In that case, we will again have duplicate pages. E.g., if 'fr' is the default language and has a prefix of 'fr', the pages 'node' and 'fr/node' will be the same. Should we instead forward in that case even if we haven't found a user-preferred language and are defaulting to the default language (the final fallback in language_initialize())?
To decide this question, it might help to add a test where we set the default language to one with a prefix. Do the tests still pass?
Or for simplicity should we just ignore the question of non-prefixed languages and leave it to the existing issues? #146084: Default path prefix for English (and DBTNG it), #338055: Require path prefixes for all languages.
#31
Since it confirmed the bug we're addressing, that patch failure was a success ;)
#32
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.
#33
require_once DRUPAL_ROOT . './includes/common.inc';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.#34
#35
Modified test to check for Etag headers and minus the extra dot - all passes. Thanks again for the in depth review c960657.
#36
Added some additional code comments to the test, otherwise no changes.
#37
The last submitted patch failed testing.
#38
#39
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.
#40
And the D7 patch this time.
#41
The last submitted patch failed testing.
#42
Re-rolled after the user session changes.
#43
The last submitted patch failed testing.
#44
Here's one which doesn't completely break user logins.
#45
Backported to D6. Seems to work nicely! One particular thing: The call to
variable_init()had to be moved up toDRUPAL_BOOTSTRAP_LANGUAGE, becausedrupal_init_language()needs the variables. We don't have theDRUPAL_BOOTSTRAP_VARIABLESstep under D6.#46
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 todrupal_goto(), which will also invokehook_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 (fromdrupal_goto()'s invocation ofhook_exit()), and a second time after (when bootstrapping the redirect's target page).#47
The last submitted patch failed testing.
#48
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
#49
Since this makes language negotiation and page caching incompatible, I'm bumping to critical.
#50
+ * @return+ * Boolean indicating whether the language set is valid for the current
+ * path.
*/
function language_initialize() {
// Get the page from the cache.- $cache = $cache_mode == CACHE_DISABLED ? FALSE : page_get_cache(TRUE);
The comments do not wrap at 80 characters:
+ // Visit the page again with browser language set to English, and confirm that+ // If the language has been reset from the default, redirect+ // to the new language.
#51
Thanks for the review, I think think this should cover everything.
#54
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.
#56
Here's a straight backport for D6.
#57
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.
#59
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:
#60
stella: agreed.
Nice work catch. This latest patch addresses the issue without the potentially disruptive bootstrap changes in previous iterations.
Introducing a new global shouldn't be done lightly, but in this case it seems warranted and could be used for purposes beyond this language-specific use case.
#61
+ 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.
#62
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.
#63
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.
#64
And for Drupal 6.
#65
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;
}
#66
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.
#67
Nice, I like drupal_skip_page_cache(). Looks like you attached the wrong patch however. ;)
#69
oh dear.
#70
The last submitted patch failed testing.
#71
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-Languageheader so as not to mess up with proxy and client-level caches.#72
@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?).
#73
@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.
#74
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.
#75
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.