Return translated term name on views "Content: Has taxonomy term ID (with depth)"

@heikki Can you provide more detail on the view you are describing or even provide an export of an example view so we might reproduce the issue you are describing?

Rerolled the patch.

This works, and it looks like the proper solution (though check with views maintainers). Steps to reproduce, tested on Simplytest.me:

• Enable content_translation module
• add Spanish language
• Enable content translation to Spanish for taxonomy terms
• Create a taxonomy term (e.g. Butterfly) and translate it into Spanish (Mariposa)
• Change the taxonomy_term view to use the "Has taxonomy term ID (with depth)" argument, as well as the depth
• Ensure that "Replace title" is checked, and {{ arguments.term_node_tid_depth }} is used as the title replacement
• Go to the taxonomy term page in Spanish
• The English title will be shown instead of Spanish - this is the bug.

I'll attach the exported view config.

Thanks @dnotes, those steps to reproduce help. Adding them to the issue summary with screenshots of the before-and-after would be helpful.

Removing credit for @Munavijayalakshmi as the reroll was not needed there.

+++ b/core/modules/taxonomy/src/Plugin/views/argument/IndexTidDepth.php
@@ -133,7 +133,7 @@ public function query($group_by = FALSE) {
-      return $term->getName();
+      return \Drupal::entityManager()->getTranslationFromContext($term)->label();

If this Views plugin needs the entity manager, it should have it inject. It might even already have it.

Finally, since this issue describes a bug, we need an automated regression test for the bug, that can be attached with by itself and show a fail on testing, followed by the whole patch which should pass.

Thanks everyone!

I rewrote the patch using dependency injection as requested in #9.

Also I used entity.reposity instead of entity.manager since the entity.manager has been deprecated.

We still need to do tests etc for this.

Added steps to reproduce and screenshots to the description. This is with the patches from 2 and 6.

I'm a little confused that in order to fix this one-line bug, we are also updating the entire argument to use dependency injection and newer services. Shouldn't that be a separate issue applied to all the taxonomy term arguments at the same time? After all, those same changes are needed for all of the taxonomy term arguments, not just this one. If we update this because of a separate bug, then the code for the arguments get out of sync with each other and it just becomes a bigger mess than it is now.

Writing tests that incorporate changes to a view is beyond my capability.

I have also updated the taxonomy.tokens.inc with roughly the same fix as the following also wasn't working with Taxonomy:

use \Drupal\taxonomy\Entity\Term;
$token_service = \Drupal::service('token');
$term1 = Term::load(1236);
$output = $token_service->replace('[term:name]', array('term' => $term1), array('langcode' => 'fr'));
dpm($output);

Here is a patch just with my tokens fixes.

Another patch with some other corrections I needed to apply to Core 8.4.3. Will post an updated patch against dev soon.

Okay this is incorporates the fixes from #2765297-10: Return translated term name on views "Content: Has taxonomy term ID (with depth)" as well as fixes for tokens + regular Taxonomy using {{ arguments.tid }}

The same problem exists for the Nid / Content: ID filter.
I created a different issue for that: https://www.drupal.org/project/drupal/issues/2952252

Did you check special character behaviour for term names? We should ensure to be not double-escaped. For example & => not becoming => & in view title.

Hi, sorry for my bad english.

With this patch applied, I have a side effect on term alias.

I have a pathauto pattern set as "[term:parents]/[term:name]" for a vocabulary, when I save a term of this vocabulary, the alias generated for all languages of the term substitute the [term:name] token with the last translation saved instead of use the translated name.

The problem is the patch in taxonomy.tokens.inc

if ($type == 'term' && !empty($data['term'])) {
     $term = $data['term'];
+    $term = \Drupal::entityManager()->getTranslationFromContext($term);

This causes the term to be returned to be in the current active language if none was given to getTranslationFromContext.

Pathauto hands over the language as an option, so to fix it, it should be:

if ($type == 'term' && !empty($data['term'])) {
     $term = $data['term'];
+    $term = \Drupal::entityManager()->getTranslationFromContext($term, isset($options['langcode']) ? $options['langcode'] : NULL);

BTW, this also causes problems in bulk pathauto generation with patterns having term:name tokens. These are all generated in the current active language. If for example you run pathauto bulk while using the admin pages in french, all aliases will have the french translation, even if your pattern is set for entities in an other language.

Here's a patch for that....

Manual code review looks good so far from a logical perspective. Now we need some people to test and confirm.

I confirm that the patch in #23 works in D8.6.1

Thanks for the test, per #24 and #26 I think we are good to go unless some one can spot any issues here.

This could use some automated test coverage.

I tried the patch in #23 in D8.6.4 and 8.6.5, but it does not work for me.

1. +++ b/core/modules/taxonomy/src/Plugin/views/argument/IndexTidDepth.php
   @@ -39,7 +46,13 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +      $configuration,
   +      $plugin_id,
   +      $plugin_definition,
   +      $container->get('entity.manager')->getStorage('taxonomy_term'),
   +      $container->get('entity.repository')
   +    );
   

   lets update this and others to use entity_type.manager instead when we touch the line anyway.

2. +++ b/core/modules/taxonomy/taxonomy.tokens.inc
   @@ -98,6 +98,7 @@ function taxonomy_tokens($type, $tokens, array $data, array $options, Bubbleable
      if ($type == 'term' && !empty($data['term'])) {
        $term = $data['term'];
   +    $term = \Drupal::entityManager()->getTranslationFromContext($term, isset($options['langcode']) ? $options['langcode'] : NULL);
   

   this should also use the entity repository service

I have tested patch #23 on Drupal 8.6.5 and it works only if using "Content: Has taxonomy term ID (with depth)" as contextual filter.

In my case I have to use a Relationship with Term and then use it in the contextual filter "Taxonomy term: name" (I take the Term name in the URL and I override the title with {{ arguments.name }}, no other method works for me). In this case, patch dosn't work.

Works for me, tested on 8.7.2. Updated patch with remarks from @Berdir.

@daxgrapol I haven't tried reconstructing your case, but it looks to me you get your title straight from the path, so I think the translated name should be in there already.

1. +++ b/core/modules/taxonomy/src/Plugin/views/argument/IndexTidDepth.php
   @@ -22,6 +23,11 @@
   +   protected $entityRepository;
   

   Nitpick: we should always keep the same order between "EntityRepositoryInterface" & "EntityStorageInterface".
   Since we added the EntityStorageInterface last we can put $entityRepository after $termStorage.

2. +++ b/core/modules/taxonomy/src/Plugin/views/argument/IndexTidDepth.php
   @@ -30,9 +36,10 @@ class IndexTidDepth extends ArgumentPluginBase implements ContainerFactoryPlugin
   +    $this->entityRepository = $entityRepository;
        $this->termStorage = $termStorage;
   

   Same remark.

3. +++ b/core/modules/taxonomy/src/Plugin/views/argument/Taxonomy.php
   @@ -16,6 +17,11 @@
   +   protected $entityRepository;
   

   Same remark.

4. +++ b/core/modules/taxonomy/src/Plugin/views/argument/Taxonomy.php
   @@ -24,9 +30,10 @@ class Taxonomy extends NumericArgument implements ContainerFactoryPluginInterfac
   +    $this->entityRepository = $entityRepository;
   

   Same remark.

@berdir, should we test this against 8.8 or 8.7?

Updated patch with remarks from @mpp

Reviewed and tested.

Re-Confirming RTBC by manual code review and additional manual testing. Happy to see this committed soon :)
Thank you all!

As @xjm pointed out in #9, this still needs automated test coverage.

1. +++ b/core/modules/taxonomy/src/Plugin/views/argument/IndexTidDepth.php
   @@ -27,20 +28,32 @@ class IndexTidDepth extends ArgumentPluginBase implements ContainerFactoryPlugin
   +   * @var EntityRepositoryInterface
   

   Should be FQN.

2. +++ b/core/modules/taxonomy/src/Plugin/views/argument/Taxonomy.php
   @@ -21,13 +22,19 @@ class Taxonomy extends NumericArgument implements ContainerFactoryPluginInterfac
   +   * @var EntityRepositoryInterface
   

   ^

3. +++ b/core/modules/taxonomy/taxonomy.tokens.inc
   @@ -95,9 +95,10 @@ function taxonomy_tokens($type, $tokens, array $data, array $options, Bubbleable
   -  $taxonomy_storage = \Drupal::entityManager()->getStorage('taxonomy_term');
   +  $taxonomy_storage = \Drupal::entityTypeManager()->getStorage('taxonomy_term');
   

   This change is unrelated.

This change is unrelated.

@borisson_, see #30 by @berdir.

This needs to go into 8.8 first, where it won't apply because these entity manager related changes already happened there.

@mpp, this should only happen when we update the line anyway, there is no other change in that line so I don't think that makes it allowed.

Thanks for the feedback @berdir, @borisson_.

off-topic: The same problem seems to occur for search_api view arguments (in Drupal\search_api\Plugin\views\argument\SearchApiTerm), see https://www.drupal.org/project/search_api/issues/3064479

Rerolled against 8.8.

And added FQN as @borisson_ said in #40.

EDIT: commented on the wrong issue

I just ran into the problem and can confirm this problem still exists. Replacement patterns from contextual filters are not usable on multilang sites and return unexpected results. Due to this I'll set the priority to Major. There is no workaround available.

I can confirm patch #46 fixes the problems for us in a Drupal 8.9.2 installation without any side-effects experienced.

What's still missing since #9 & #39 is automated test coverage and there's still a TODO in the patch!

patch 46 needs a reroll for 9.1.x
patch 46 applies cleanly to 9.0.x
patch 46 applies cleanly to 8.9.x

I have re-roll the patch for 9.1.x

I tried the patch #46 taxonomy_translation_2765297_0.patch in D9.0.7 and everything is working well

Adding a few test cases to validate the scenarios.
Approach:
Pretty much same as mentioned in steps to reproduce section.
Except I've created view with different URL, so it doesn't have issues with default taxonomy terms path.

Fixed CS issues.

oh wow thanks @anmolgoyal74, I need this patch ! :)

Rerolled for D8.9.x
(I have a project that needs this)

Ok here's a patch that rolls fine with D8.9.x, D9.0.x, D9.1.x and D9.2.x

See interdiff and new patch

Contrary to what the testbot says, patch 63 applies cleanly to D8.9.x, D9.0.x, D9.1.x, D9.2.x

#63 works fine in 9.1.x. Thank you.

Thank you joseph.olstad your patch #63 works on D9.1.9.

Finally, confirming RTBC for #63, thank you very, very much @joseph.olstad and the others. Works on Drupal 9.2 and 9.3!

#63 contains tests, is that enough or are there any concerns to add further tests? Otherwise I'd suggest to remove that tag and set this RTBC?

Patch works properly. Removing testing tag and setting RTBC.

Looks great and it's fantastic to see the test coverage, but still needs a little work I'm afraid.

+++ b/core/modules/taxonomy/src/Plugin/views/argument/IndexTidDepth.php
@@ -26,13 +27,21 @@ class IndexTidDepth extends ArgumentPluginBase implements ContainerFactoryPlugin
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityStorageInterface $termStorage, EntityRepositoryInterface $entityRepository) {

+++ b/core/modules/taxonomy/src/Plugin/views/argument/Taxonomy.php
@@ -21,13 +22,19 @@ class Taxonomy extends NumericArgument implements ContainerFactoryPluginInterfac
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityStorageInterface $term_storage, EntityRepositoryInterface $entityRepository) {

Unfortunately we can't change these constructors without some BC layer (default it to NULL and inject from \Drupal and add a deprecation message that you now need to inject this, should be some examples in core). Anything extending these classes could break otherwise.
Edit: Or don't we do this dance for plugins?

Also, it seems we are changing 2 plugins, but only have test coverage for 1, so we need to extend the coverage to both plugins that get changed.

Reroll against 9.4.x latest branch & added reroll diff for reviewers.

Keeping it in NW for #72

While we do have test coverage, Lendude mentioned that one additional test is required.

See MR 6625

Pretty sure to fix the tests this yml file needs to be corrected/updated in a vanilla install of D11 or D10.2 . (With the patch applied of course)

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.taxonomy_translated_term_name_test.yml

I'm hoping someone can carry the baton from here.

- Tests are green now.
There was one deprecated config change in view. Sorted that out.

- Updated issue summary and moving it to Needs Review
- Removed Needs test tag since it is not required anymore.

Working on the test coverage for the second plugin.

- Added the test case for the another ViewsArgument plugin.
- Tests are passing on local as well as CI
- Unusual failure form nightwatch tests, triggered the job again.

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.

@mohit_rocks , thanks for the excellent work on the tests! Marking this as ready.

Hiding all the patch files since this issue is using an MR.

Added some review comments to the MR. This is a nice - we need to take care of classes that extend the core classes and I think there is a slightly better way of doing things.

Addressed the PR feedback.
I think https://git.drupalcode.org/project/drupal/-/merge_requests/6625#note_276326 would need some verification while doing review.
I'm somewhat confused about that.

Other point seems clear.

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.

ok, it's clean now.
RTBC

I've made a mistake here. DeprecatedServicePropertyTrait doesn't support entity storages. I'll fix the MR.

@alexpott do we need a change record for this one?

@mohit_aghera - yes we should have something small and simple... and the @see should point to it and not this issue. Great catch.

Can you add one, fix the MR and then set this back to RTBC?

Here's an example MR doing the same thing... https://www.drupal.org/node/3397213

Change record created. Back to RTBC

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.

Resolved the coding standard issue at last MR due to which the pipeline was failing and setting back the status to RTBC from Needs works

Please review and suggest if any other changes are required.

@adwivedi008, can you please be sure that we need to fix the issues?

Reason is comment in #100is saying that patch is no longer applied to the core.
In this case, you need to merge latest 11.x to the branch.
I tried that and it seemed to be working fine.

@adwivedi008 your fixes are not fixes - core's coding standards are checked on every commit to an MR - the pipeline was green after #99 but your changes broke the pipeline. The "drupal" coding standard as detailed by coder is not the the core coding standard. Core is working towards it but we should never make out of scope changes to satisfy it and we should adhere to the coding standard set in core/phpcs.xml.dist as the "drupal" coding standard from coder actually needs fixing. I have reverted your changes.

Updated branch with latest 11.x head since test case failures were related to 11.x HEAD only.
Keeping it to RTBC, will check if there are any failures.

Committed and pushed 18c45f5f23 to 11.x and 462d3ac638 to 10.3.x. Thanks!

It'd be good to finish up #2640994: Label token replacement for views entity reference arguments not working and put a more generic fix in place.

This caused the gitlab performance test jobs to fail over the weekend, with:

Remaining self deprecation notices (2)
  1x: The "Drupal\KernelTests\KernelTestBase::expectDeprecationMessage()" method is considered internal use expectDeprecation() instead. It may change without further notice. You should not extend it from "Drupal\Tests\taxonomy\Kernel\Views\ViewsArgumentDeprecationTest".
    1x in DrupalListener::endTest from Drupal\Tests\Listeners
  1x: The "Drupal\KernelTests\KernelTestBase::expectDeprecationMessageMatches()" method is considered internal use expectDeprecation() instead. It may change without further notice. You should not extend it from "Drupal\Tests\taxonomy\Kernel\Views\ViewsArgumentDeprecationTest".
    1x in DrupalListener::endTest from Drupal\Tests\Listeners

Since ViewsArgumentDeprecationTest appears to be mainly (only?) testing the constructor deprecation, I don't think that was necessary to add in the first place and we could just remove the test, but I didn't review to see if there was a better reason for adding it.

@catch
I added that initially and my intention was to validate two things.

1. Validate the deprecation message being thrown
2. I should be able use and invoke the older plugins with different constructor argument
   

   $plugin = \Drupal::service('plugin.manager.views.argument')->createInstance('taxonomy_views_argument_test', []);
       $this->assertInstanceOf(TaxonomyViewsArgumentTest::class, $plugin);

Happy to remove the test case entirely or just expect deprecation only.
Let me know your thoughts.

@catch @mohit_aghera this test only caused that because it unnecessarily used the trait. Remove the use and this will pass fine as we literally have hundreds of other deprecation tests doing the same thing.

I think leaving a test in in this case is okay because of the slightly more complicated dance of changing a parameter instead of just adding or removing a parameter.

Given this is just removing usage of a trait setting back to rtbc.

Makes sense to me - I can re-commit this a bit later but also think it'd be fine for @alexpott to commit the updated MR here since it's a tiny change.

Committed and pushed 1bcc7f34e6 to 11.x and 46564ee733 to 10.3.x. Thanks!

Second time lucky.