"Has taxonomy term" contextual filter does not take the language value into account

Triggering test bots.

Schema & upgrade looks finished, now it needs tests to prove the bug category

Setting back to needs work based on #13.

Hi guys,

Test in wip,
Method to be added to core/modules/taxonomy/tests/src/Functional/TermLanguageTest.php

  /**
   * Tests that taxonomy terms are well managed with translatables nodes.
   */
  public function testTermTranslatedLinkedToNodes() {

    $this->drupalPostForm('admin/structure/taxonomy/manage/' . $this->vocabulary->id(), $edit, t('Save'));
    $this->term = $this->createTerm($this->vocabulary, [
        'name' => 'Awesome node',
        'langcode' => 'bb',
    ]);

    // Create node.
    /** \Drupal\Core\Entity\EntityStorageInterface @var */
    $node_storage = \Drupal::entityTypeManager()->getStorage('node');

    /** \Drupal\node\NodeInterface @node */
    $node = $node_storage->create([
        'type' => 'article',
        'title' => 'Another node',
        'langcode' => 'bb',
        'field_tags' => [$this->term->id()],
    ]);
    $node->save();

    // TODO: use language manager to get the right language.
    $node->addTranslation('aa', [
        'title' => 'Node aa',
        'field_tags' => [],
    ]);
    $node->save();
    
    
    
    $url = $this->term->toUrl();
    $this->drupalGet($url);
    $this->assertSession()->responseContains('Awesome node');

    // TODO: See https://www.drupal.org/project/drupal/issues/2889486#comment-12142608 (scenario completion)


  }

In team with @qudec

Patch rerolled.

1. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -248,3 +249,96 @@ function taxonomy_update_8702(&$sandbox) {
   +function taxonomy_update_8703() {
   

   this updates the schema, but do we also need an update for al existing references? or can they be just left blank?

2. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -248,3 +249,96 @@ function taxonomy_update_8702(&$sandbox) {
   +      'default' => $language,
   

   this seems wrong, it should just be an empty string

retagging

Really nice work!

1. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -248,3 +249,95 @@ function taxonomy_update_8702(&$sandbox) {
   +function taxonomy_update_8703() {
   

   Update number will need to be updated to _88*

2. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -248,3 +249,95 @@ function taxonomy_update_8702(&$sandbox) {
   +    Database::getConnection()->schema()->dropIndex('taxonomy_index', 'term_node');
   +    Database::getConnection()->schema()->addIndex(
   

   No need to drop it first, addIndex drops it if it exists

3. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -248,3 +249,95 @@ function taxonomy_update_8702(&$sandbox) {
   +  $manager = \Drupal::entityDefinitionUpdateManager();
   +  $entity_type = $manager->getEntityType('taxonomy_term');
   +  $manager->updateEntityType($entity_type);
   
   +++ b/core/modules/taxonomy/taxonomy.module
   @@ -541,11 +542,13 @@ function taxonomy_build_node_index($node) {
   -          ->fields(['sticky' => $sticky, 'created' => $node->getCreatedTime()])
   

   Is this needed? We aren't updating the entity are we?

4. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -580,6 +583,10 @@ function taxonomy_node_predelete(EntityInterface $node) {
   +    // Not necessary to add a condition on the langcode because when deleting
   +    // a translation, taxonomy_node_update() is called, so the entries for all
   +    // languages of the node will be deleted then reconstructed.
   +    // And when deleting the whole node, every entries should be removed.
   

   "Since taxonomy_node_update() will handle the deletions of specific translations, we only need to handle the case where the whole node is deleted."
   Something like that maybe?

5. +++ b/core/modules/taxonomy/tests/src/Functional/TermIndexTest.php
   @@ -200,6 +201,92 @@ public function testTaxonomyIndex() {
   +    \Drupal::service('content_translation.manager')->setEnabled('node', 'article', TRUE);
   ...
   +    $node_storage = $this->container->get('entity_type.manager')->getStorage('node');
   

   Either use \Drupal:: or use $this->container, but let's not mix them in the same test.

The only remaining concern that I have is that doing updates for all nodes can be pretty heavy for some sites, but since we are only rebuilding a sub table here it might not be so bad. Also, don't see another way to prevent this being necessary.

fix on batch for post_update: incorrect start range for the query.

Trying to fix failed test

#41 works (D8.9.1)

Issue is still there. Please update the patch for 9.1

Just a general review.

core/modules/taxonomy/taxonomy.module ✗ 1 more
line 14 Unused use statement
core/modules/taxonomy/tests/src/Functional/TermIndexTest.php ✗ 1 more
343 Tag value indented incorrectly; expected 1 space but found 2

2 CS violations have to be fixed.

+++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyIndexUpdateTest.php
@@ -0,0 +1,66 @@
+      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.8.0.filled.standard.php.gz',
+      __DIR__ . '/../../../../../taxonomy/tests/fixtures/update/drupal-8.taxonomy-index-2889486.php',

Could use dirname(__DIR__, 5) . to replace __DIR__ . '/../../../../..;

Talked a little about it with @catch on Slack and lets see if we can't make this a little less heavy than doing updates on all nodes, if possible.

+++ b/core/modules/taxonomy/taxonomy.post_update.php
@@ -18,3 +18,53 @@ function taxonomy_removed_post_updates() {
+  $query = $node_storage->getQuery()
+    ->accessCheck(FALSE)
+    ->sort('nid')
+    ->range($sandbox['current_node'], $sandbox['limit']);

Does this work correctly? nid 50 doesn't need to be the 50th record, shouldn't it use a condition like "> $current_node" and not a range?
And can we find a way to limit the update to just nodes that currently have an entry in the index? It's not like we expect new nodes to be added to it I think?

+++ b/core/modules/taxonomy/taxonomy.post_update.php
@@ -44,12 +46,13 @@ function taxonomy_post_update_populate_taxonomy_index_langcode(&$sandbox = NULL)
-
+  $entity_ids = $database->select('taxonomy_index')
+    ->fields('taxonomy_index', ['nid'])
+    ->condition('nid', $sandbox['current_node'], '>')
+    ->distinct()
+    ->range(NULL, $sandbox['limit'])
+    ->execute()
+    ->fetchCol();

Let's add an explicit order by here.

Also range(NULL, $limit) removes any range from the query, it doesn't make it a LIMIT query.

Instead of storing $current_node as a node ID, can increment $start and $limit by 50 each time and remove the condition.

It should also use $settings['entity_update_batch_size'] rather than hardcoding 50.

Applied patch #63 and its not working.The non referenced translated nodes are still showing in the list.

Added "Needs change record" because:
- Fox existing sites one needs to manually update configurations to make use of this feature.

Changed to "Needs work" because one is not able to set views filter via UI.

- If importing manually patched views.view.taxonomy_term.yml file the filter works fine. But if one deletes it, its not possible to add it back.

I used following steps:
1. Clone drupal/core, git checkout 9.2.x
2. composer install
3. php core/scripts/drupal quick-start standard
4. Apply the patch.
5. Run update.php and clear Drupal cache.
6. Manually add filter according to images on #69. Save view.
7. Go to /admin/config/development/configuration/single/export
8. See how filter value is different from patch:
UI: '***LANGUAGE_language_interface***': '***LANGUAGE_language_interface***'
vs.
Patch: '***LANGUAGE_language_content***': '***LANGUAGE_language_content***'

If applying the patch before the site install. The views filter options look like:

vs.
One in #69:

It's missing "Content language selected for page" option.

Edit: Copy-paste error on 8.

Just doing a little tag tidying. Nice work everyone!!

Found a couple of issues, overall looks pretty good although I need to review the logic and update a bit more

1. +++ b/core/modules/taxonomy/src/TermStorageSchema.php
   @@ -60,10 +60,17 @@ protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $res
            ],
   +        'langcode' => [
   +          'description' => 'The langcode of the node.',
   +          'type' => 'varchar_ascii',
   +          'length' => 12,
   +          'not null' => TRUE,
   +          'default' => '',
   +        ],
   

   Is this the node langcode or the translation langcode, or both?

2. +++ b/core/modules/taxonomy/src/TermStorageSchema.php
   @@ -60,10 +60,17 @@ protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $res
   -      'primary key' => ['nid', 'tid'],
   +      'primary key' => ['nid', 'tid', 'langcode'],
          'indexes' => [
   -        'term_node' => ['tid', 'status', 'sticky', 'created'],
   +        'term_node' => ['tid', 'status', 'sticky', 'created', 'langcode'],
          ],
          'foreign keys' => [
   

   The index should be checked for performance - is this still the best order? Usually queries are ordered by sticky and created, so I woul dexpect it to be tid, status, langcode, sticky, created.

3. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -5,9 +5,110 @@
   +          'default' => 0,
   +        ],
   +        'status' => [
   +          'description' => 'Boolean indicating whether the node is published (visible to nonadministrators).',
   +          'type' => 'int',
   +          'not null' => TRUE,
   

   non-administrators

4. +++ b/core/modules/taxonomy/tests/src/Functional/TermIndexTest.php
   @@ -230,6 +231,92 @@ public function testTaxonomyIndex() {
   +      'fr' => 0,
   +    ]);
   +
   +    // Add a reference to the term in fr.
   +    $node = $node_storage->load($nid);
   

   We should refer to the languages by name rather than langcode in the comments I think.

5. +++ b/core/modules/taxonomy/tests/src/Functional/TermIndexTest.php
   @@ -230,6 +231,92 @@ public function testTaxonomyIndex() {
   +
   +    // Delete ur translation.
   +    $node = $node_storage->load($nid);
   +    $node->removeTranslation('ur');
   +    $node->save();
   

   Same here.

1. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyTermViewTest.php
   @@ -115,15 +115,34 @@ public function testTaxonomyTermView() {
    
   +    // Create a second term that will not reference the term in one of its
   +    // language.
   +    $edit = [];
   

   'one of its languages'

2. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyTermViewTest.php
   @@ -115,15 +115,34 @@ public function testTaxonomyTermView() {
        $this->assertNoText($original_title);
        $this->assertText($translated_title);
   +    // As node 2 do not reference the term, it should not appear on the
   +    // translated term page.
   

   Should be 'does not reference'

This fixes everything from #79 except the initial point #1. I'm not sure if it's node or translation langcode. ;) Haven't studied all this closely. Also haven't closely reviewed the rest of the patch. But all of @catch's concerns seem legit.

I didn't test the performance of the index, but I agree langcode should come before sticky + created (filter before sort).

Thanks,
-Derek

@Grimreaper: Thanks for clarifying. Updating the DB schema definition comment accordingly. Hope this is agreeable.

Patch #83 doesn't apply to 9.2.0, so I re-roll against 9.2.0 tag.

Reroll for 9.2.0 working patch.

Reroll patch for 9.3.x and Fixed the fails of #86

Fixed the custom command fails

Fixed the custom command fail error.

Added interdiff

reroll for 9.3.x

reroll for 9.2.5 (I hope)

test against latest 9.2.x

works on 9.2.5

reroll for 9.3.0

patch was empty for some reason

Reroll patch created for 9.5.x, Please have a look.

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 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.

Rerolling for 10.x, couldn't generate an inderdiff

Both phpstan and phpcs are unhappy about the latest patch.

Fixed CCF.

#105 Fixed CCF.

Well no, it still fails.
Please run the script locally before using the drupal.org resources.

core/scripts/dev/commit-code-check.sh --drupalci

Adding a reroll patch for 10.1.x and fixing all CCF issues and some other phpcs issues.

Removing credit for #101, 103, 105, 108, 109, and 111 for the bad rerolls. It's expected to check your patches before uploading

You can check for build errors make sure to run ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel...

Also some were missing interdiffs and none mentioning how the patch no longer applies per the new policy. The file sizes fluctuates why is that?

Honestly next reroll should start at 97

Support for 9.5.x based on #111.
Then I made some adjustments, and when both translation and Content Moderation were enabled in the site, an exception occurred.
Steps, 1 Add node in en language, and publish en language.
2 Add the translation of ja language for it, but do not publish ja language.
At this point there will be no information about the language in the taxonomy_index table.
It cannot be displayed in the view.
This is considered an unusual error on our site. Published languages should be visible to anonymous users.
Unpublished languages may be checked in the normal workflow, it cannot affect the normal published display.

fixed test.

Following the advice from #112, from #97, that seems more reasonable.
Also, this is only in 9.5.x.
10.x will be considered in the next few months.

Infinite loop updb.

Fixed CCF failure, added interdiff for same

Re-rolled patch from #118 for 10.2.x

Fixed CCF failure in #119, added interdiff for same

Patch in #120 is missing a use statement for Node class.
Updated patch in #120 to fix it. (created for 10.2.x)

Add patch for MR!7968.
Fixed phpcs and tests.

Added MR for roll-back to 10.3 and patch.

Fixed mysql issue with primary key creation on 10.3

Before anyone reviews can the MRs and patches be cleaned up. Not sure which MR is to be reviewed the most recent one is for 10.3 but should be against 11.x but don't want to make that assumption. Also patches should be hidden.

MR!9023 is for 10.3 (patch 127)

MR!7968 is for 11 (patch 124)

Left some comments on MR.

I have updated the code based on PR feedback and fixed the pipeline for the failing test. moving this to NR.
Kindly review.

Hello,
MR applied successfully, Both articles are displayed in both languages.
Also can add filter.
Check attachments-

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.

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.

Rebased & fixed conflicts . Moving back to review.

Hi,
Test status - Pass
Issue can be reproduced. Patch applied successfully and issue looks fixed.
Verified on Drupal 11.x- dev
Testing steps-

- Create an article with a tag, for example in english
- Translate the article in french and do not associate the french version with the tag.
- Go to the tag page in english
- See your article in english: OK, normal
- Go to the tag page in french
- See your article in french: the french version of the article is not associated with the tag

Also observed that,A new view filter is created: "Taxonomy term: Node language"
Attaching screenshots for reference.

RTBC+1

Left comments on MR, thanks for keeping it going!

Thank you for the review and feedback :)
I’ve addressed one of the points. Regarding the hook_update_N test coverage, I haven’t worked on this type of test before, so it is still pending. I’ll try to find a solution for it.

Created a patch from the latest changes; works fine on 11.2.4

I needed this patch for a Drupal 10.4 site, so I created branch 2889486-has-taxonomy-term-10.4 in the issue fork (but haven't created a merge request).

Attaching a patch of the changes. I just rebased so there is no interdiff.

I have an issue that I'm not sure fits in this ticket or not, but if it does, the patch doesn't seem to fix. I have a view of Items, and the Item content type has a "Category" taxonomy. So then I have

Item 1 in language A is tagged with Category "foo"
Item 1 in language B is tagged with Category "bar"

In the view, I have a contextual filter "Content: Has taxonomy term ID (with depth) (Default: Raw value from URL)" So that when I'm on the "foo" term page, I should see Item 1 returned if I'm in the A langcode. This is working, but the item still shows in the "bar" term page in the A langcode as well.

I've applied the patch, added a relationship to the term referenced, and then added the filter

(field_item_categories: Taxonomy term) Taxonomy term: Node language (= Interface text language selected for page)

And the item still shows when it shouldn't. I've tried with and without the relationship. Is this the same issue, or with it being a content view and not a taxonomy view is it different?

Reroll of #144 for 11.3.1