Problem/Motivation

  1. Enable Language module
  2. Go to Configuration > Regional and Language > Languages and add a second language
  3. Go to the Detection and selection tab and disable URL detection and instead enable Session detection
  4. Enable the language switcher block on Structure > Block layout
  5. Go to the frontpage
  6. Click the language switch link for the second language in the list
  7. Click on the Home menu link. There should be no query parameter in the URL at this point
  8. Check the link to the second language in the language switcher link

Expected result:
Because you are already in that language the URL should be http://example.com

Actual result:
The link is actually http://example.com?language=[first] where [first] is the language code of the first language in the list. And sure enough, if you click on the second language, the language will switch back to first. (This is only actually visible if you also enable Interface Translation module, but you can check the value in the {sessions} table in the DB.)

What happens is that LanguageNegotiationSession::getLanguageSwitchLinks() adds the same $url object to multiple links. When those links are rendered, options are set on the $url object, but because it is the same object, they are in fact set for all links. Thus they "stick" for all links, unless they are explicitly overridden to a different value.

Proposed resolution

1. We need tests exposing the current bug.

2. LanguageNegotiationSession::getLanguageSwitchLinks() should clone the $url object before placing it into the links structure. We should also add a comment why we need to clone.

3. We should consider adapting LanguageNegotiationUrl::getLanguageSwitchLinks() in the same way. I don't think it's possible to actually trigger a functional bug in this case, but the reasoning is the same. We are actually rendering the same URL object multiple times, when conceptually we want to render different URL objects.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the language switcher switches the language to the wrong one in some circumstances
Issue priority Normal because the default configuration is unaffected by this
Prioritized changes Prioritized because it is a bugfix.
Disruption No disruption
CommentFileSizeAuthor
#13 test_only.patch2.79 KBmaxocub
#13 session_language_switch_links-2505263-13.patch4.84 KBmaxocub
#13 interdiff.txt788 bytesmaxocub
#7 interdiff.txt725 bytesmaxocub
#7 test_only.patch2.79 KBmaxocub
#7 session_language_switch_links-2505263-6.patch4.84 KBmaxocub
#3 test_only.patch2.8 KBmaxocub
#3 session_language_switch_links-2505263-3.patch4.84 KBmaxocub
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -sprint

You don't seem to be working on this and neither anyone else, so removing from sprint.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Assigned: Unassigned » maxocub
Issue tags: +sprint
maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Active » Needs review
FileSize
session_language_switch_links-2505263-3.patch4.84 KB
test_only.patch2.8 KB

Here's a first patch.

The last submitted patch, 3: session_language_switch_links-2505263-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: test_only.patch, failed testing.

The last submitted patch, 3: session_language_switch_links-2505263-3.patch, failed testing.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review
FileSize
session_language_switch_links-2505263-6.patch4.84 KB
test_only.patch2.79 KB
interdiff.txt725 bytes

The last submitted patch, 3: test_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: test_only.patch, failed testing.

The last submitted patch, 7: session_language_switch_links-2505263-6.patch, failed testing.

The last submitted patch, 7: session_language_switch_links-2505263-6.patch, failed testing.

The last submitted patch, 7: test_only.patch, failed testing.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review
FileSize
interdiff.txt788 bytes
session_language_switch_links-2505263-13.patch4.84 KB
test_only.patch2.79 KB

Status: Needs review » Needs work

The last submitted patch, 13: test_only.patch, failed testing.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review

Apparently I just needed to use TRUE & FALSE instead of '1' & '0' to change the language detection settings.

Status: Needs review » Needs work

The last submitted patch, 13: test_only.patch, failed testing.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at erdfisch commented
Status: Needs review » Reviewed & tested by the community

Wow, impressive detective work @maxocub! I followed all the patches and had absolutely no clue why they were failing. Nice find! The latest patch looks great and includes exactly the "Steps to reproduce" from the issue summary as an automated test. Beautiful.

One minor note:

+++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
@@ -393,6 +394,58 @@ protected function doTestLanguageLinkActiveClassAnonymous() {
+    $edit = array(
+      'predefined_langcode' => 'fr',
+    );

Just as a future reference: In new code we try to use the new $edit = [...]; PHP 5.4 array syntax in core.

It would be silly IMHO, though, to hold up the commit on that.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at erdfisch commented
Issue summary: View changes

Added beta evaluation.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach as a volunteer commented

@maxocub:

Great work, thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Wow, nice find, fix, and tests!

Committed and pushed to 8.0.x. Thanks!

I don't know that we ever got 100% agreement on enforcing short array syntax everywhere (though I agree it's common) so we can maybe file a follow-up "Novice" patch to clean that up.

  • webchick committed 68ff6eb on 8.0.x 
    Issue #2505263 by maxocub, tstoeckler: Session language switch links are...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -sprint

Yay, great work! Thanks!

YesCT’s picture

YesCT CreditAttribution: YesCT commented

looking at this for background on another issue, and noticed some nits.

issue for them: #2581817: Follow-up for #2505263: clean up comments

maxocub’s picture

maxocub CreditAttribution: maxocub commented

@YesCT: noted!

maxocub’s picture

maxocub CreditAttribution: maxocub commented

  • alexpott committed 6efce42 on 8.0.x 
    Issue #2581817 by YesCT: Follow-up for #2505263: clean up comments

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Version: 8.0.x-dev » 9.x-dev
Assigned: maxocub » Unassigned
maxocub’s picture

maxocub CreditAttribution: maxocub as a volunteer and commented

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.