Problem/Motivation

If the session language provider is enabled and page cache is enabled (and #815526: Session language switcher prevents pages from being cached is fixed), we can have duplicate page cache entries for the same content because the language switcher block appends the language query string parameter even for the default language link. If no language parameter is found we may fall back to the default language and thus having the same content for, say, http://example.org/ and http://example.org/?language=en.

Proposed resolution

Fix the issue by forbidding query string language parameters when page cache is enabled and the parameter value matches the default language.

Original report by @plach

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions
Reroll the patch if it no longer applies. Instructions
Manually test the patch Novice Instructions
Add steps to reproduce the issue Novice Instructions
CommentFileSizeAuthor
#67 815544-67.patch1.48 KBsahil.goyal
#66 reroll_diff_63-66.txt1.87 KBMedha Kumari
#66 815544-66.patch2.36 KBMedha Kumari
#65 interdiff_62-65.txt592 bytessahil.goyal
#65 815544-65.patch2.21 KBsahil.goyal
#63 reroll_diff_62-63.txt1.44 KBAkram Khan
#63 815544-63.patch2.21 KBAkram Khan
#62 815544-62.patch3 KBAnchal_gupta
#62 interdiff-815544-61_62.txt3.51 KBAnchal_gupta
#61 815544-61.patch5.6 KBNitin shrivastava
#56 815544-56.patch2.23 KBpradeepjha
#54 815544-54.patch2.24 KBvacho
#43 815544-43.patch1.5 KBrpayanm
#43 815544-interdiff.txt1.49 KBrpayanm
#35 drupal8-language_cache_session-815544-35.patch1.5 KBwranvaud
#34 drupal-language_cache-815544-34.patch2.98 KBwranvaud
#26 drupal8.language-system.815544-26.patch1.49 KBAlbert Volkman
#23 drupal8.language-system.815544-23.patch1.49 KBAlbert Volkman
#21 drupal8.language-system.815544-21.patch1.47 KBAlbert Volkman
#18 drupal8.language-system.815544-18.patch1.33 KBAlbert Volkman
#15 language_dup_cache-815544-15.patch2.77 KBAlbert Volkman
#13 language_dup_cache-815544-13.patch2.75 KBAlbert Volkman
#8 language_dup_cache-815544-8.patch7.62 KBplach
#2 language_dup_cache-815544-2.patch1.97 KBplach
#1 language_dup_cache-815544-1.patch1.97 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review
FileSize
language_dup_cache-815544-1.patch1.97 KB

The attached patch should fix the issue by forbidding query string language parameters when page cache is enabled and the parameter value matches the default language.

This depends on #815526-1: Session language switcher prevents pages from being cached to be tested properly.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
language_dup_cache-815544-2.patch1.97 KB 
+++ includes/locale.inc	2 Jun 2010 01:36:59 -0000
@@ -290,6 +292,12 @@ function locale_language_url_rewrite_ses
+    // default language. ¶

Trailing whitespace.

Powered by Dreditor.

Status: Needs review » Needs work

The last submitted patch, language_dup_cache-815544-2.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review

Huh? Now the bot reports green. Changing the status back to CNR.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Postponed

The latest patch does not take into account possible language providers "in between" session and default ones.

Postponed to #817114: Deprecate the session language detection method.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ includes/locale.inc	2 Jun 2010 01:36:59 -0000
@@ -262,7 +262,7 @@ function locale_language_url_rewrite_url
+  static $default, $cache, $query_rewrite, $query_param, $query_value;

Tests may fail, because these are persistent statics, not drupal_static()s (i.e., not reset for each test case).

+++ includes/locale.inc	2 Jun 2010 01:36:59 -0000
@@ -271,9 +271,11 @@ function locale_language_url_rewrite_ses
+      $cache = variable_get('cache', 0);

0 should be CACHE_DISABLED, I guess.

+++ includes/locale.inc	2 Jun 2010 01:36:59 -0000
@@ -290,6 +292,12 @@ function locale_language_url_rewrite_ses
+    // When page cache is enabled default language switch links do not need the
+    // query string parameter as there is no other possibile fallback than the
+    // default language.
+    elseif ($cache && $options['query'][$query_param] == $default) {
+      unset($options['query'][$query_param]);
+    }

I'm missing a simple test that proves that we're fixing an actual bug with that patch. Also, shouldn't we simply disable page caching if we negotiated a session language?

Powered by Dreditor.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Postponed » Needs work

Changing to "needs work" since #817114: Deprecate the session language detection method has been postponed to this one.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
language_dup_cache-815544-8.patch7.62 KB

@sun:

0 should be CACHE_DISABLED, I guess.

Incredibly we have no such constant in core.

I'm missing a simple test that proves that we're fixing an actual bug with that patch.

Let's see if this flies, then I'll write some simpletests.

Also, shouldn't we simply disable page caching if we negotiated a session language?

In core, browser detection requires page cache to be turned off, while user detection requires an authenticated user.

The attached patch implements a variant of the approach suggested above (through a minor API change): to ensure that anonymous users always get different URLs for different contents, only language detection methods defining an URL rewrite callback are allowed to be executed in this context. Moreover a language detection method can (still) specify a cache mode preference.

This way we are sure that no other language detection method can be enabled below the session one without affecting the URL itself (and thus, hopefully, the page content).

After ensuring this we can safely skip the language parameter for the default language, thus avoiding page cache duplicates.

The patch additionally performs a small change to language_initialize() to allow each language object to track the language provider that detected it. Actually this change is not used in the patch (it's a heritage of a previous attempt), but I thought it can be useful and decided to keep it.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +API change

Small but actual API change.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +Needs committer feedback

After beta I'm afraid this needs an ok from committers, as the latest patch introduces a minor API change.

As you can read in http://api.drupal.org/api/function/hook_language_negotiation_info/7, the 'url_rewrite' callback tells the language system whether a language detection method may affect URLs. This patch introduces a different behavior when this callback is not set: the language detection method is ignored if we have an anonymous user and page cache is enabled. We still have a way to force the previous behavior by setting the 'url_rewrite' value to FALSE (i.e. giving it an empty value).

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Bugs are fixed in the development version first, backported then.

The patch needs a reroll.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Issue tags: +Novice, +Needs reroll

Tagging

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Needs review
FileSize
language_dup_cache-815544-13.patch2.75 KB

This is my attempt at simply bring in #8 to D8.

Status: Needs review » Needs work

The last submitted patch, language_dup_cache-815544-13.patch, failed testing.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Needs review
FileSize
language_dup_cache-815544-15.patch2.77 KB

Oops.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: -Novice, -Needs committer feedback, -API change, -Needs reroll

#15: language_dup_cache-815544-15.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs committer feedback, +API change, +Needs reroll

The last submitted patch, language_dup_cache-815544-15.patch, failed testing.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Needs review
FileSize
drupal8.language-system.815544-18.patch1.33 KB

Re-roll.

ceardach’s picture

ceardach CreditAttribution: ceardach commented
Issue tags: -Needs reroll

Remove "Needs reroll" because latest patch passes.

vsawant’s picture

vsawant CreditAttribution: vsawant commented
Issue summary: View changes
Status: Needs review » Needs work

This patch no longer applies.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Needs review
FileSize
drupal8.language-system.815544-21.patch1.47 KB

Here's a re-roll, but I haven't tested it locally.

Status: Needs review » Needs work

The last submitted patch, 21: drupal8.language-system.815544-21.patch, failed testing.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
FileSize
drupal8.language-system.815544-23.patch1.49 KB

Updating $user var to use currentUser property.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: drupal8.language-system.815544-23.patch, failed testing.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Needs review
FileSize
drupal8.language-system.815544-26.patch1.49 KB

D'oh.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented 
+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
@@ -139,12 +146,12 @@ function getLanguageSwitchLinks(Request $request, $type, $path) {
+      // Always set the language query parameter. It will be removed if necessary
+      // in locale_language_url_rewrite_session().

Thanks!

Btw, this comment is outdated and does not wrap correctly at column 80 :)

henk queued 26: drupal8.language-system.815544-26.patch for re-testing.

benjifisher queued 26: drupal8.language-system.815544-26.patch for re-testing.

johnennew’s picture

johnennew CreditAttribution: johnennew commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs committer feedback, -API change +Amsterdam2014
johnennew’s picture

johnennew CreditAttribution: johnennew commented
Issue summary: View changes
Issue tags: +API change
sdelbosc’s picture

sdelbosc CreditAttribution: sdelbosc commented

-- Content removed (posted on wrong issue) --

wranvaud’s picture

wranvaud CreditAttribution: wranvaud commented

I'm in drupalcon amsterdam, working on this issue

wranvaud’s picture

wranvaud CreditAttribution: wranvaud commented
Status: Needs work » Needs review
FileSize
drupal-language_cache-815544-34.patch2.98 KB

API changed, function locale_language_url_rewrite_session is now LanguageNegotiationSession::processOutbound(). I've updated the comment pointed by @plach at comment #27

wranvaud’s picture

wranvaud CreditAttribution: wranvaud commented
FileSize
drupal8-language_cache_session-815544-35.patch1.5 KB

Cleaning up patch, replaced tab with spaces, fixed indentation

Mac_Weber’s picture

Mac_Weber CreditAttribution: Mac_Weber commented 
+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
@@ -137,12 +144,12 @@ function getLanguageSwitchLinks(Request $request, $type, $path) {
+      if ($langcode == $language_query) {

shall this comparison be "==="?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

Patch still applies. Removing the API change tag, since the patch in #35 doesn't appear to involve one from what I can tell.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue tags: -API change
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue tags: +Needs tests

Also, would adding a test for this be feasible?

pingers’s picture

pingers CreditAttribution: pingers commented 
  1. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
@@ -115,6 +115,13 @@ public function processOutbound($path, &$options = array(), Request $request = N
+    if (!$this->currentUser->id() && $cache && isset($options['query'][$this->queryParam]) && $options['query'][$this->queryParam] == $default) {

    Use $this->currentUser->isAnonymous(). 

  2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
@@ -137,12 +144,12 @@ function getLanguageSwitchLinks(Request $request, $type, $path) {
+      // necessary in LanguageNegotiationSession::processOutbound().

    "It may be removed in Lang...." is simpler language.

Mac_Weber’s picture

Mac_Weber CreditAttribution: Mac_Weber commented 
+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
@@ -115,6 +115,13 @@ public function processOutbound($path, &$options = array(), Request $request = N
+    if (!$this->currentUser->id() && $cache && isset($options['query'][$this->queryParam]) && $options['query'][$this->queryParam] == $default) {

$cache is undefined

Mac_Weber’s picture

Mac_Weber CreditAttribution: Mac_Weber commented
Status: Needs review » Needs work
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm at SeeD EM commented
Status: Needs work » Needs review
FileSize
815544-interdiff.txt1.49 KB
815544-43.patch1.5 KB

Here the patch.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs review » Needs work

Patch in #43 doesn't address #41.

disasm’s picture

disasm CreditAttribution: disasm at AppliedTrust commented
Issue tags: +Needs manual testing

Lets get some manual testing and automated tests written for this issue.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano as a volunteer commented

Working on this on the mentored sprint in DCon Barcelona.
Will do some manual testing and steps to reproduce.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vacho’s picture

vacho CreditAttribution: vacho at Skilld commented
Issue tags: +Needs reroll
vacho’s picture

vacho CreditAttribution: vacho at Skilld commented
Issue tags: -Needs reroll
FileSize
815544-54.patch2.24 KB

Patch rerolled. However $cache and $default still undefined.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

pradeepjha’s picture

pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commented
FileSize
815544-56.patch2.23 KB

Patch rerolled for 8.8.x.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Nitin shrivastava’s picture

Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
815544-61.patch5.6 KB

Created patch for 9.4.x. Please review this patch and move it

Anchal_gupta’s picture

Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commented
FileSize
interdiff-815544-61_62.txt3.51 KB
815544-62.patch3 KB

I have fixed the CCF error

Akram Khan’s picture

Akram Khan
Primary language English
Location Cuttack, Odisha
CreditAttribution: Akram Khan as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
FileSize
815544-63.patch2.21 KB
reroll_diff_62-63.txt1.44 KB

Trying to Fix

Status: Needs review » Needs work

The last submitted patch, 63: 815544-63.patch, failed testing. View results

sahil.goyal’s picture

sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
FileSize
815544-65.patch2.21 KB
interdiff_62-65.txt592 bytes

trying to fix the CI issue as there is some CSpell errors right there so corresponding to that providing the patch along with the interadiff

Medha Kumari’s picture

Medha Kumari
Location Jaipur
CreditAttribution: Medha Kumari as a volunteer and at Dotsquares Ltd. commented
Status: Needs work » Needs review
FileSize
815544-66.patch2.36 KB
reroll_diff_63-66.txt1.87 KB

Rerolled the patch#63 in Drupal 9.4.x.

sahil.goyal’s picture

sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
FileSize
815544-67.patch1.48 KB

Trying to fix the CCF, As i seen in my #65 i missed to add some line of code, So thanx @Medha Kumari to resolve it.

Status: Needs review » Needs work

The last submitted patch, 67: 815544-67.patch, failed testing. View results

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Duplicate page cache entries when using the session language switch links » Remove the language query string when using session language switch links

Re-titling since the title makes this seem like a page cache bug, it's more about URL cleanliness.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.