Admin toolbar and contextual links should always be rendered in the admin language (if set)

Add the combined patch and add the enable option for the new feature.

I've rerolled the patch in #158 🐛 Admin toolbar and contextual links should always be rendered in the admin language (if set) Needs work so that it applies on the latest versions (10.1.x, 10.0.4 & 9.5.4).

Rerolled #152 for 9.5.4 .

Fixes #161 patch.

The patch in comment #162 applied cleanly to 9.5.5

Add the combined patch and add the enable option for the new feature. for 9.5.5

Patch #162 has applied but contextual links are totally disappeared in some cases in layout builder

I see #149 says that #146 and #148 are not needed, and #164 seems to be an extension of those. Should anyone outside of the people posting those patches be using them? I'm not sure what the point is of an enable option, given that is just honoring existing config.

I started a new branch based off 11.x with the patch from #152 and the feedback from #156. Only the refactoring of the WebDriver based test hasn't been done yet. Let's stop posting patches now to avoid confusion, especially patching combining multiple issues. These don't belong here.

Regarding the typehint related feedback in #156: I'm not sure we should add typehints if the parent class hasn't decided on a typehint. For example, CacheContextInterface::getLabel currently has string documented as return type, but we also have to account for instances of TranslatableMarkup there. How exactly that should be done hasn't been decided yet, so we shouldn't either in the child class.

Regarding #160, there's a separate issue for that problem in the login_destination queue: #3261748: LoginDestinationToolbarLinkBuilder implements the decorator pattern in a wrong way → .

This patch represents the current state of MR created in #170. We prefer patch files as long as the merge request urls are considered dangerous, see https://github.com/cweagans/composer-patches/issues/347.

This time with a valid patch file extension...

HI @timohuisman,

thank you for the heads up!

I'm not sure what #173 was, but it was not created with https://git.drupalcode.org/project/drupal/-/merge_requests/4604.diff.
Attached is the new 11.x MR 4604 at commit 7975cb1f.

#175 gives me :

PHP Fatal error:  Cannot redeclare Drupal\language\ConfigurableLanguageManager::setCurrentLanguage() in public/core/modules/language/src/ConfigurableLanguageManager.php on line 305

Drupal 10.1

The patch in #173 was working for us in 10.1.x. But it no longer applies in 10.2.0, nor does the patch from MR 4604. I'll take a look to see what's changed, but it may be several days until I have time.

Rerolled the patch #173 🐛 Admin toolbar and contextual links should always be rendered in the admin language (if set) Needs work for Drupal 10.2.0

Fixes #178 🐛 Admin toolbar and contextual links should always be rendered in the admin language (if set) Needs work patch

#179 🐛 Admin toolbar and contextual links should always be rendered in the admin language (if set) Needs work works, thx!

I didn't review the code but just wanna report that #179 works well for me on a big site running Drupal 10.2.1!

#179 🐛 Admin toolbar and contextual links should always be rendered in the admin language (if set) Needs work

Works on drupal 11.0-dev, thanks!

Patch #179 works fine in Drupal 10.2.2 with PHP 8.2.

The patch in #179 fixed this for us. The merge requests !4604 and !6351 both include the patch file, which shouldn't be included in the MR's but at least !6351 looks otherwise ok.

#179 works for me in Drupal 10.2.2 using PHP8.1.

+1 for #179. Drupal 10.2.6 using PHP 8.2.

sokru → changed the visibility of the branch 2313309-admin-toolbar-language to hidden.

sokru → changed the visibility of the branch 2313309-9.3 to hidden.

sokru → changed the visibility of the branch 2313309-179-11.x to hidden.

I hide all other branches than MR 4604. I rebased it and addressed the parts of feedback from #156 that where missing, except for
- Typehints not done per #170.
- #156.5: Is still open question.
- #156.8: Instead of adding typehint, made https://www.drupal.org/node/3349470 → , that seems still need a work with tests (AutowireTest)

This still needs work:
- Fix issues on MR 4604
- Needs change record
- Needs release note

Fixed the tests and created a draft for change record and release notes.

@sokru thanks for hiding the old MRs, also good to hide all patches since those aren't needed anymore and the bot could pick them up.

Left some comments on the MR.

All the feedback has been addressed.

Feedback does appear to be addressed

But CR https://www.drupal.org/node/3456217 → is empty

LRoels → changed the visibility of the branch 2313309-9.3 to active.

LRoels → changed the visibility of the branch 2313309-admin-toolbar-should to active.

LRoels → changed the visibility of the branch 2313309-admin-toolbar-language to active.

LRoels → changed the visibility of the branch 2313309-admin-toolbar-language to hidden.

LRoels → changed the visibility of the branch 2313309-admin-toolbar-should to hidden.

LRoels → changed the visibility of the branch 2313309-9.3 to hidden.

Added details to CR https://www.drupal.org/node/3456217 →

Tbh I am not entirely sure do I understand the whole process and issue but I tested this with Umami. The site has english and spanish out of the box. These are my findings.

I selected spanish as my admin preferred language.

For the language change block on Umami site it seems that these are using spanish

But "Umami Home Banner" block these are not translated.

Tbh, I am not sure are these in the scope of this issue.
Otherwise it seems that all the top admin nav/toolbar is respecting the admin language and they are spanish even though I am browsing the site in english.

Thanks @sleitner for updating the CR

@ronttizz your issue sounds like a block issue. Would recommend searching the issue queue to see if there is a similar ticket and leave a comment. Sometimes a simple comment of "saw this today on 10.3 while testing xyz" could bring people back to it. Joy of open source! haha

Feedback on the MR here appears to be address

This looks good to me. Anyone tried it with navigation module? Make it a follow-up if it doesn't work we don't need to solve it here, This has always annoyed me so big +1 to this :)

After updating the site from version 10.3.1 to 10.3.2 and applying patch #179, the site went down - `The website encountered an unexpected error.`
The main error:

 Error: Class "Drupal\language\AdminLanguageRender" not found in /app/web/core/modules/language/language.module on line 148 #0 /app/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(552): language_element_info_alter(Array, NULL, NULL)

What is weird is that the class is indeed there and I can even access it by clicking ctrl+click over the class, which shows that it does find it

Has anyone faced anything like this with core 10.3.2?

I applied the patch on a site that was already running Drupal 10.3.2 (php 8.2.13, Apache 2.4.54) and could not replicate the behaviour that was described in #209, the patch worked as expected on this install.

Specifically I did the following:
- put the patch link in the "patches" section of the root composer.json and ran composer install, the patch (in #179) applied cleanly.
- logged into the site as an administrator and opened a page in dutch, the admin had dutch set as the "Administration pages language" on his profile, the administration menu items in the toolbar showed in dutch, as expected.
- changed the "Administration pages language" in the admin profile to english and opened the same dutch page, the administration menu links now showed in english, as expected.

Based on this behaviour it seems that the patch functions correctly on this install running Drupal 10.3.2, I also checked the logs for anything unusual but all was working perfectly fine. Hope this helps!

I started the update over and this time it worked as expected. Might have been a local issue
Thanks for looking

I left some review comments on the MR that should be addressed.

Resolved the threads created by @alexpott and created a follow-up issue for Navigation module 📌 [PP-1] Navigation module should honor admin language on frontpage and node route Postponed .

Gave it another lap but mostly small stuff.

Looking at the CR they may need a little work, especially https://www.drupal.org/node/3456217 → should include the service being called in the example.

Addressed the latest feedback. However I didn't modify the change record, since I could not think up of any useful example and I did not find any other change record which would give an examples of new services. Given the age of this issue I think the CR can be updated later if someone comes up with any useful examples.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

False positive

Confirming patch #179 🐛 Admin toolbar and contextual links should always be rendered in the admin language (if set) Needs work works on D10.2.7 and PHP8.2.

I am using MR diff to patch D10.3.6, php 8.3. Works.

I performed the same routine as I used in #210 to check the patch in #179, this time using Drupal 10.3.6 and PHP 8.3.13. The patch works as expected.

Hello,
Getting error while applying Mr
attaching screenshot

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

MR doesn't apply on 11.1.x

Thanks @kallevu for the update on the MR! I tried working on that on the global contrib weekend but failed miserably. It would be nice if this could move forward now that it seems to produce green results too.

I can confirm what @yevko's saying: the MR does not apply on 11.1.x

This change is rejected on "modules/language/language.module", probably because the dev-version of 11 looks quite different there:

--- modules/language/language.module
+++ modules/language/language.module
@@ -19,6 +19,7 @@
 use Drupal\Core\Language\LanguageInterface;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\Session\AccountInterface;
+use Drupal\language\AdminLanguageRender;
 use Drupal\language\Entity\ContentLanguageSettings;
 use Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUI;
 use Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl;

The MR is not supposed to be applied to 11.1.x but on 11.x branch. If this gets accepted to 11.x then we can create a MR for 11.1.x.

Resolved most threads but left just a few more.

Question what happens if admin_langcode is not included.

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

robin.houtevelts → made their first commit to this issue’s fork.

I've addressed the threads in the MR.

"What happens if admin_langcode is not set?"

Consider the following setup:

1. Site default language: EN
2. Current interface language: NL
3. User's admin language: set to - No preference -

Currently:

1. AccountAdminLanguageCacheContext calls Account::getPreferredAdminLangcode(fallback_to_default: TRUE)
2. AdminLanguageRender calls Account::getPreferredAdminLangcode(fallback_to_default: FALSE)

This difference results in the toolbar and contextual links rendering in NL, while the user.admin_language cache context falls back to EN. This inconsistency indeed needs to be addressed.

AccountAdminLanguageCacheContext needs to accurately reflect the user's actual setting. In this case - No preference -, so it should use fallback_to_default: FALSE instead of TRUE.

(👆This still needs to happen.)

While testing, I noticed some contextual links were translated and others were not. The tests pass, but I'm not confident.
I'm including a recording.