Browser language detection broken as soon as a request with no Accept-Language header happens

this is a duplicate of #2430335: Browser language detection is not cache aware no ?

Thanks so much for the patch! I've been trying to fix this for hours now. Seems to work perfectly.

@kibrick which fix did you use to solve this problem ? Can anyone help me ?

Missed this.

Great that you already wrote a test, not that the patch with the fix needs to include both the fix *and* the test.

Also, the issue title isn't very helpful, which is one reason why I didn't find this issue, and possibly others neither. I'll close #3024535: LanguageNegotiationBrowser needs to trigger the page cache kill switch even when there is no Accept-Language header as a duplicate, I'd suggest to use my title or something similar.

The fix is working as expected. Extended the test with assertion of the page caching, which was also a main point of this patch, to not cache the page, when the Accept-Language header is not present.

1. +++ b/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
   @@ -0,0 +1,134 @@
   +
   +  public static $modules = ['language', 'locale', 'content_translation', 'node'];
   +
   

   missing documentation property.

2. +++ b/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
   @@ -0,0 +1,134 @@
   +    parent::setUp();
   +    // User to manage languages.
   +    $this->admin = $this->drupalCreateUser([], NULL, TRUE);
   +    $this->drupalLogin($this->admin);
   +
   

   no need to create protected properties for variables that are only used in the context of one method, use $admin instead.

3. +++ b/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
   @@ -0,0 +1,134 @@
   +    // Create FR.
   +    $this->drupalPostForm('/admin/config/regional/language/add', [
   +      'predefined_langcode' => 'fr',
   +    ], 'Add language');
   +    // Set language detection to url and browser detection.
   +    $this->drupalPostForm('/admin/config/regional/language/detection', [
   

   Tests usually enable languages with ConfigurabeLanguage::createFromLangcode(), should be quite a bit faster.

4. +++ b/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
   @@ -0,0 +1,134 @@
   +
   +    // Create node and translation.
   +    $this->node = $this->createNode([
   +      'title' => 'TestEN',
   +      'type' => 'page2',
   +    ]);
   +    $this->drupalGet('/node/'.$this->node->id().'/translations/add/en/fr');
   +    $this->drupalPostForm(NULL, [
   +      'title[0][value]' => 'TestFR',
   +    ], 'Save (this translation)');
   +
   

   lets try to avoid a dependency on node, we just need some kind of url that we can post against as an anonymous user.

   Maybe we can use the system_test.echo route instead or some other page that's accessible to anonymous users.

5. +++ b/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
   @@ -0,0 +1,134 @@
   +
   +    // Check normal behaviour.
   +    $this->drupalGet('/en/node/' . $this->node->id(), [], ['Accept-Language' => 'en']);
   +    $this->assertSession()->responseHeaderEquals('Content-Language', 'en');
   +    $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'MISS');
   +
   +    $this->drupalGet('/fr/node/' . $this->node->id(), [], ['Accept-Language' => 'en']);
   +    $this->assertSession()->responseHeaderEquals('Content-Language', 'fr');
   +    $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'MISS');
   +
   +    $this->drupalGet('/node/' . $this->node->id(), [], ['Accept-Language' => 'en']);
   +    $this->assertSession()->responseHeaderEquals('Content-Language', 'en');
   +    $this->assertFalse($this->drupalGetHeader('X-Drupal-Cache'));
   +
   

   setUp() shouldn't have asserts, move those to the method. Also, "normal behavior" is not a very good desccription, maybe test correct cache header or so?

Made all the changes according to your review.

Thanks, looks good now I think. We only test for MISS now and not HIT, I *think* that's enough, as a MISS also means that the page is cacheable and was cached. But we could also make that explicit and do another request that would return a HIT.

Actually, lets do that and at the end of a the current test. Then we now that the existing URL with language prefix is still cached and returned from cache after requests without a language prefix.

+++ b/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
@@ -0,0 +1,106 @@
+  }
+
+
+  /**
+   * Tests with browsers with and without Accept-Language header.

extra empty line here, can be fixed on commit guess.

Added the test for HIT at the end, and also removed the extra empty line.

Thanks, looks good now I think.

We'll see what happens in #2430335: Browser language detection is not cache aware exactly, but whatever we do there, we do have explicit test coverage now for how page cache is expected to behave with different combinations of headers/URL's, and we can adjust it there accordingly.

Patch looks good to me as well. Please let's merge the IS with the one @Berdir posted at #3024535: LanguageNegotiationBrowser needs to trigger the page cache kill switch even when there is no Accept-Language header and fix the nit below:

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
@@ -35,13 +35,14 @@ public function getLangcode(Request $request = NULL) {
+    // Internal page cache with multiple languages and browser negotiation
+    // could lead to wrong cached sites. Therefore disabling the internal
+    // page cache.

Comment does not wrap at column 80.

Made a pass at merging the IS from https://www.drupal.org/project/drupal/issues/3024535 (we have an interest in this issue moving forward).

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
@@ -35,13 +35,14 @@ public function getLangcode(Request $request = NULL) {
+    // Internal page cache with multiple languages and browser negotiation
+    // could lead to wrong cached sites. Therefore disabling the internal
+    // page cache.

@plach, the longest comment there (ends with "browser negotiation") ends at column 74 when applied, unless I'm missing something.

Added issue summary from https://www.drupal.org/project/drupal/issues/3024535 to this issue; cannot repro the code style issue mentioned in https://www.drupal.org/project/drupal/issues/2986325#comment-12958068 (with the patch applied to 8.7.x, all comments added are under 80 columns).

all comments added are under 80 columns

My bad, I wasn't clear: the comment can be rewrapped to fit 80 chars better. Specifically, "page" fits after "internal".

I will fix that on commit, thanks for the IS update.

Committed 1cc42e0 and pushed to 8.7.x. Thanks!

Streamlined the IS.

Uncrediting myself.

Briefly discussed this with @catch: this might involve some (hopefully beneficial) behavior changes, so let's keep it 8.7.x only. It's safe to apply the committed patch to 8.6.x for people needing the fix right now.