Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
\Drupal\language\Plugin\LanguageNegotiation::getLanguageSwitchLinks() and \LanguageNegotiationContentEntity::getLanguageSwitchLinks() do not set a language key in the links.
This can lead to incorrect switch links in the following situation (among others):
1. Add a language, so that you have at least two, let's say language A (let's say that's the default one) and language B (let's say with language code bb).
2. Change language negotiation to be session-based (and disable URL negotiation).
3. Enable the language switcher block.
4. Create an alias for a path in language A, say /pathA.
5. Create an alias for the same path in language B, say /pathB.
6. Visit /pathA.
At this point the language switcher should link to /pathB?language=bb, but it instead links to /pathA?language=bb
Proposed resolution
Add the language to the switch links.
Instead of copy-pasting even more code into the already ~80% copy-pasted methods this patch provides a trait for code re-use. As you can see from this and other previous bugs in this area it is very hard to get this stuff right, so this is not for some theoretical purpose but it is to prevent future bugs in core and contrib.
Remaining tasks
| Task | Novice task? | Contributor instructions | Complete? |
|---|---|---|---|
| Create a patch | Instructions | Yes | |
| Add automated tests | Instructions | Yes | |
| Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
None.
API changes
Due to the minor refactoring to improve future maintenance and stability this breaks contrib and custom language negotiation plugins if they extend LanguageNegotiationContentEntity, LanguageNegotiationSession or LanguageNegotiationUrl and one of the following is true:
1. They provide a private method called getLanguageManager() or processLanguageSwitchLink()
2. They provide a public or protected method called getLanguageManager() that does not return an object implementing \Drupal\Core\Language\LanguageManagerInterface
3. They provide a public or protected method called processLanguageSwitchLink() that takes different arguments from the ones introduced here.
3. They use a trait that has a method called getLanguageSwitchLinks() or processLanguageSwitchLink().
If this is considered a BC break, then this could be avoided as well, by simplifying the patch at the cost of future maintenance and stability. We could also considering doing the improved version for 8.1 and the extra-safe version for 8.0.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #41 | interdiff_39-41.txt | 2.52 KB | ravi.shankar |
| #41 | 2666634-41.patch | 14.75 KB | ravi.shankar |
| |||
| #39 | interdiff_38-39.txt | 2.64 KB | Tanuj. |
| #39 | 2666634-39.patch | 15.02 KB | Tanuj. |
| |||
| #3 | 2666634-3--tests-only.patch | 3.44 KB | tstoeckler |
Comments
Comment #2
tstoecklerComment #3
tstoecklerIn writing a test for #2666628: Language-aware aliases are broken with anything but path language negotiation I realized that the manual config edit is in fact not necessary. Updating the issue summary accordingly.
Also adding a test to the patch which is basically test as in the referenced issue, except for the assertions that validate that the alias is accessible, because that will be fixed there and is not related to the switch links itself. The tests-only patch is also the interdiff.
Note that the test failures in the test-only patch are cheating a little bit, because the xpatch relies on the
hreflangattribute which is not there (which is why there are 2 fails, even though only one link is incorrect), but if you run the tests locally you will see that the link is actually incorrect in the test.Comment #6
tstoecklerI didn't account for Drupal being installed in a subfir. Yay, tests!
Comment #8
tstoecklerWell it helps to actually run the tests from within a subdir. This should be green; I tested this both with a subdir and without.
Comment #17
manuel.adanStill present in 9.x. Build multilingual websites without the URL language detection is almost unuseful in Drupal at the moment.
Comment #19
james.williams CreditAttribution: james.williams at ComputerMinds commentedHere's a re-rolled version of the patch.
Comment #20
james.williams CreditAttribution: james.williams at ComputerMinds commentedArgh, sorry! Here's the re-rolled version...
Comment #21
james.williams CreditAttribution: james.williams at ComputerMinds commentedUpdated patch to resolve issues flagged by code checks.
Comment #23
james.williams CreditAttribution: james.williams at ComputerMinds commentedUpdated test to match newer alias form fields.
Comment #25
james.williams CreditAttribution: james.williams at ComputerMinds commentedUgh, sorry. I guess this is what I get for taking on what I thought could be a simple re-roll. Try again, updating the test in yet another way...
Comment #27
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed the fail tests, Please have a look.
Comment #28
james.williams CreditAttribution: james.williams at ComputerMinds commentedI'm not sure fixing the test like that is correct? The test is supposed to check that only the current language list item is marked as active on the language switcher block. The test is showing that no item is marked as active, which seems like a valid fail to me, indicating an actual bug fix is still needed?
Comment #29
james.williams CreditAttribution: james.williams at ComputerMinds commentedI've just spent most of the day investigating this. It turns out we are indeed running into this bug: #2845319: The highlighting of the active menu-link does not respect query strings and fragment identifiers ! So I'm sorry @vsujeetkumar, but that change to the test was incorrect - the patch should be failing, but because of that other bug, rather than anything introduced here.
I suspect we might be able to work around this by changing the test to check on a different page. However, since the test was quite deliberately originally written to test the special case of the switcher block on the homepage (#220559: Language switcher block highlights ALL languages as active - over a decade ago, for Drupal 6!), I think that check should remain in place, otherwise we're reducing existing test coverage!
I've run the tests locally with my patch from #25, as well as MR !866 from #2845319: The highlighting of the active menu-link does not respect query strings and fragment identifiers, which passed. So I think we need to get #2845319 resolved first, then we can push this one along.
In the meantime - reviews & testing are otherwise very welcome here!
Comment #30
james.williams CreditAttribution: james.williams at ComputerMinds commentedComment #31
james.williams CreditAttribution: james.williams at ComputerMinds commentedComment #32
james.williams CreditAttribution: james.williams at ComputerMinds commentedAlthough #29 was true, we can also handle that ourselves quite nicely here. It turns out that since the patch from #8 was written, #2689607: Language from URL negotiator does not add request query to language switcher links had added query string support, specifically to the
LanguageNegotiationUrlclass. So that needed extracting into the new trait too....and with that, regardless of #2845319: The highlighting of the active menu-link does not respect query strings and fragment identifiers tests pass locally, so I think we can progress here without that.
Comment #34
james.williams CreditAttribution: james.williams at ComputerMinds commentedRe-rolled, this should apply to latest 9.3.x as well as 9.4.x.
Comment #37
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #38
rossb89 CreditAttribution: rossb89 at ComputerMinds commentedRe-rolled for latest 9.5.x
Comment #39
Tanuj. CreditAttribution: Tanuj. as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed CCF on #38.
Comment #40
smustgrave CreditAttribution: smustgrave at Mobomo commentedgetLanguageManager() {1. new function should be typehinted
LanguageSwitchLinksTrait2. New trait will need a change record
getLanguageSwitchLinks(Request $request, $type, Url $url) {3. Should be typehinted
In LanguageNegotiationContentEntity4. NIT the space removal seems unneccessary
Rest seems good to me.
Comment #41
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAddressed point 1, 3 and 4 of comment #40, keeping the status needs work for change record as per point 2.