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.

Remaining tasks

Convert to an MR
Add test

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

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

Issue fork drupal-815544

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Active » Needs review
StatusFileSize
new 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
commented
StatusFileSize
new 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
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
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
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
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
commented
Status: Needs work » Needs review
StatusFileSize
new 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
commented
Issue tags: +API change

Small but actual API change.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
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
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 commented
Issue tags: +Novice, +Needs reroll

Tagging

albert volkman’s picture

albert volkman commented
Status: Needs work » Needs review
StatusFileSize
new 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 commented
Status: Needs work » Needs review
StatusFileSize
new language_dup_cache-815544-15.patch2.77 KB

Oops.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
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 commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.language-system.815544-18.patch1.33 KB

Re-roll.

ceardach’s picture

ceardach commented
Issue tags: -Needs reroll

Remove "Needs reroll" because latest patch passes.

vsawant’s picture

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

This patch no longer applies.

albert volkman’s picture

albert volkman commented
Status: Needs work » Needs review
StatusFileSize
new 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 commented
StatusFileSize
new drupal8.language-system.815544-23.patch1.49 KB

Updating $user var to use currentUser property.

albert volkman’s picture

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 commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.language-system.815544-26.patch1.49 KB

D'oh.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
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 commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs committer feedback, -API change +Amsterdam2014
johnennew’s picture

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

sdelbosc commented

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

wranvaud’s picture

wranvaud commented

I'm in drupalcon amsterdam, working on this issue

wranvaud’s picture

wranvaud commented
Status: Needs work » Needs review
StatusFileSize
new 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 commented
StatusFileSize
new drupal8-language_cache_session-815544-35.patch1.5 KB

Cleaning up patch, replaced tab with spaces, fixed indentation

mac_weber’s picture

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
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
commented
Issue tags: -API change
jhedstrom’s picture

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

Also, would adding a test for this be feasible?

pingers’s picture

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 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 commented
Status: Needs review » Needs work
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new 815544-interdiff.txt1.49 KB
new 815544-43.patch1.5 KB

Here the patch.

jhedstrom’s picture

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

Patch in #43 doesn't address #41.

disasm’s picture

disasm 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
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 commented
Issue tags: +Needs reroll
vacho’s picture

vacho commented
Issue tags: -Needs reroll
StatusFileSize
new 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 commented
StatusFileSize
new 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 commented
Status: Needs work » Needs review
StatusFileSize
new 815544-61.patch5.6 KB

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

anchal_gupta’s picture

anchal_gupta commented
StatusFileSize
new interdiff-815544-61_62.txt3.51 KB
new 815544-62.patch3 KB

I have fixed the CCF error

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
StatusFileSize
new 815544-63.patch2.21 KB
new 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 commented
StatusFileSize
new 815544-65.patch2.21 KB
new 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
commented
Status: Needs work » Needs review
StatusFileSize
new 815544-66.patch2.36 KB
new reroll_diff_63-66.txt1.87 KB

Rerolled the patch#63 in Drupal 9.4.x.

sahil.goyal’s picture

sahil.goyal commented
StatusFileSize
new 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
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.

quietone’s picture

quietone commented
Issue summary: View changes

nexusnovaz made their first commit to this issue’s fork.

nexusnovaz opened merge request !9533

nexusnovaz’s picture

nexusnovaz commented

Converted patch #66 to an MR (!9533). Unsure where to start on tests, though i will be following

mrinalini9 made their first commit to this issue’s fork.

mrinalini9’s picture

mrinalini9 commented
Status: Needs work » Needs review

Fixed pipeline failure issues in the MR, please review it.
Thanks!

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

previously tagged for tests which still appear to be needed.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone’s picture

quietone commented
Issue tags: -Novice

I am removing the 'Novice' tag because what task someone new to Drupal contribution is not clearly defined. And adding a test is typically not suitable. And if it manual testing, then steps are needed on how to do that.