Local tasks should honor selected admin interface language (if set)

Quick attempt to implement this feature

CS fixes

Maybe this patch will pass the tests. Adding patch with an interdiff.

Removing the patch.

So far it looks like #5 fixes it on my local test sites (3 different installs with varying degree of customization).
Though it would probably be better to get the failing tests fixed since they seem directly related to the change made by the patch.

With the patch from #8 applied on 8.7.5, I get the following fatal error:

Error: Call to undefined method Drupal\Core\Language\Language::get() in Drupal\Core\Menu\LocalTaskManager->getTitle() (line 205 of core/lib/Drupal/Core/Menu/LocalTaskManager.php).

The patch from #5 seems to work as expected from my (limited) testing so far.

I used the patch from #8 and got the same error as @malcomio. I took a look at the code and noticed there were 2 ->get() usages.
I created a new patch and a interdiff. The patch seems to be working now on my local install (D8.8.0).

Local actions needs to be translated as well, I tested with the code that is added in the patch for this issue but for local actions and it seems to work well. Should we add it here or make an own issue for that change?

Patch in #14 breaks more then it fixes.

Setting the language back to the site default language breaks everything when you are not viewing the site in the default language. We should set it back to it's previous value instead. There was no method for getting the default langcode from the stringTranslation service, so I added that.

See attached patch. (patch was against 8.8.4, so migh not validate against latest core)

Trying to fix failed test from previous patch

Unrelated test failure

Patch #21 applied on 9.1 and working well.

Patch #21 still applies on 9.2.x and 9.3.x and still works. Thanks! :)
The patch looks good and expands test coverage so let's mark it RTBC to let the testbot do its magic on the last release!

Confirming RTBC on #21!

1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -127,8 +130,10 @@ class LocalTaskManager extends DefaultPluginManager implements LocalTaskManagerI
   -  public function __construct(ArgumentResolverInterface $argument_resolver, RequestStack $request_stack, RouteMatchInterface $route_match, RouteProviderInterface $route_provider, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, LanguageManagerInterface $language_manager, AccessManagerInterface $access_manager, AccountInterface $account) {
   +  public function __construct(ArgumentResolverInterface $argument_resolver, RequestStack $request_stack, RouteMatchInterface $route_match, RouteProviderInterface $route_provider, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, LanguageManagerInterface $language_manager, AccessManagerInterface $access_manager, AccountInterface $account, TranslationInterface $string_translation) {
   
   @@ -137,6 +142,7 @@ public function __construct(ArgumentResolverInterface $argument_resolver, Reques
   +    $this->stringTranslation = $string_translation;
   

   I think we need to provide a BC layer here, i.e. default to NULL, and if not set, use the \Drupal singleton to set the property.

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
   @@ -163,6 +163,16 @@ public function setDefaultLangcode($langcode) {
   +  public function getDefaultLangcode() {
   

   We're adding new API here. Are we sure we want to do that?

   Could we instead have setDefaultLangcode return the previously set value (if any)

   Tagging for needs subsystem maintainer review for this point.

3. I don't see any new tests for the changed behaviour, i.e. I was expecting to see a test that the local tasks were output in a users preferred admin language

@larowlan asked me to take a look. It is not apparent to me what caching considerations were made, to make sure the tabs are stored / cached with proper caching metadata so language switching will not spill into other versions of the page / component?

Patch #21 seems to be working well using Drupal 9.2.1 with French (admin + content) and English (content) languages.
Thanks

Fixed feedback of #29 and #31. Still needs test coverage.

Also, as mentioned in #2313309: Admin toolbar, Navigation and contextual links should always be rendered in the admin language (if set) when viewing the tabs in e.g. English when on a RTL language, should we update the styling of the tabs?

Added test coverage.

As discussed with @Gábor Hojtsy on Slack:

Jeroen Tubex 
@Gábor Hojtsy (he/him) @larowlan, I was working on https://www.drupal.org/project/drupal/issues/3054641 and had a couple of questions:
Should the local tasks render in the preferred admin language when the Account administration pages detection method is enabled? Or is setting a preferred admin language enough?
2. when viewing the local tasks in a RTL language and you have configured a preferred admin language e.g. English, should we update the styling of the local tasks?

Gábor Hojtsy (he/him)  2 hours ago
re 1, if you render it in the admin language even if the resulting page will not be the admin language (because you don’t have the account admin pages negotiation method enabled), then that would be a bad experience… why lure poor users into thinking they get to a page with the proper language if they are not?

Gábor Hojtsy (he/him)  2 hours ago
2. yeah I think we need the local tasks to follow the RTL/LTR of the language we display them in, otherwise it would be a mess

Patch is still missing 2 things:

• Only translate local tasks when the Account administration pages detection method is enabled.
• Show the tabs in RTL/LTR depending on the admin language.

Fixed 1.

Still need to fix 2. The easiest solution is probably adding a class to the local tasks block and theming the tabs based on that class instead of the dir="rtl" element on the html tag.

Fixed the last item. Tabs are now shown in the preferred admin language.

On D9.2.10 Patch #21 works well.
#38 patch does not apply
and #37 does apply, but is not working for me.

one difference between the patch in #21 and #38 is that the account administration pages detection method should be enabled.

Rerolled an updated patch against #45

Created diff for #48

Patch no longer applies. Needs reroll.

Rerolled the patch in #48, thanks!

Something in this is not working right with 9.4.0.
For first logged in page load on each content page (after clearing Durpal's cache) I get two warnings from the lines added to theme.inc:

Notice: Undefined index: primary_attributes in template_preprocess_menu_local_tasks() (line 1994 of core/includes/theme.inc).
Warning: Invalid argument supplied for foreach() in Drupal\Core\Template\Attribute->__construct() (line 85 of core/lib/Drupal/Core/Template/Attribute.php).
Notice: Undefined index: secondary_attributes in template_preprocess_menu_local_tasks() (line 1995 of core/includes/theme.inc).
Warning: Invalid argument supplied for foreach() in Drupal\Core\Template\Attribute->__construct() (line 85 of core/lib/Drupal/Core/Template/Attribute.php).

I had the same issue as described in #53. I think something went wrong during the reroll.
I created a new one. Let's see if this one works better.

Let's see if this fixes the failing tests.

D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

Rerolled patch in #57 for 10.x version.

Left some review comments.

Looks like a test still needs an update...?

Reroll of #57 for 9.5.x

Hiding files to avoid confusion

Skimmed the code, tested manually and included the manual steps to reproduce the issue.

Concept makes sense, I think the patch still needs work, however.

This comment is clearly outdated:
+ @trigger_error('The string_translation service must be passed to ' . __NAMESPACE__ . '\LocalTaskManager::__construct. It was added in Drupal 9.3.0 and will be required before Drupal 10.0.0.', E_USER_DEPRECATED);

Also, code style is a little weird - camel case local variables are being added next to snake case existing ones.

This change looks like it's makes calls to \Drupal methods instead of using DI:

+++ b/core/lib/Drupal/Core/Menu/Plugin/Block/LocalTasksBlock.php
@@ -86,6 +86,18 @@ public function build() {
     $config = $this->configuration;
     $cacheability = new CacheableMetadata();
     $cacheability->addCacheableDependency($this->localTaskManager);
+
+    $userAdminLangcode = \Drupal::currentUser()->getPreferredAdminLangcode(FALSE);
+    $negotiation_method_enabled = \Drupal::moduleHandler()->moduleExists('language') && \Drupal::service('language_negotiator')->isNegotiationMethodEnabled('language-user-admin');
+

Since you could pass the langcode as an option into the t() call, this seems like a bad idea to change the default langcode:

$this->stringTranslation->setDefaultLangcode($originalLangcode);

Also, I question the approach of setting the langcode when calling getTitle() versus informing the local task plugins about the language they should be using? e.g. set the langcode as part of: LocalTaskManager::createInstance()

I'm also noticing a bug(?) in this local task which is not returning a string:

\Drupal\comment\Plugin\Menu\LocalTask\UnapprovedComments::getTitle()

Or... we should fix the interface/docs

Also… why doesn’t this exist?
\Drupal\Core\StringTranslation\TranslatableMarkup::setOption()

looks like to add/change an option you have to extract the string and options and instantiate a new TranslatableMarkup

Just a reroll from #68 to 9.5.9 in case some other need if for their projects. #72 and #73 needs to be addressed.

This doesn't seem to work for local tasks added by Views, I guess that's because those translations are stored in config instead of interface translation.

Reroll for 10.1

created a new issue branch "3054641-11.x" for 11.x with patch #77 and claro templates updated. Let's use this for development and then only post static patches based on the MR 4757

Attached is a static patch of MR. 4757 at commit fdb7bfb8.

Not sure if it could be 1 change record but think we will need change records for

new template_process hook + secondary_attributes (not sure if these should be separate)
for new parameter needed for LocalTaskManager

Also may need manual visual testing for the css changes.

Also #76 should that be addressed here?

Reroll for 10.3. No other changes.

Re-roll for 11.1.

11.3.1

11.3.1 patch with fixed preprocess function

Re-roll the patch for 11.4.4

On newer Drupal versions, default_admin (heir to gin) theme is shipped and needs this fix as well.

I added support for default_admin on the previous patch against 11.4.4.

Will try to add it also to the MR but since it has changed to be against main branch, there are a lot of conflicts and I don't know if I will have time to sort them out.

I was a little bit worried that the 3054641-11.x branch was misleading as it kept the 11.x in the name even if it was moved to be against main, so I allowed myself to create the 3054641-main branch.

Merge conflicts corrected, and I added the missing default_admin twig template.