Taxonomy 'related content' views relationship links to wrong base table.

+++ b/core/modules/taxonomy/src/TermViewsData.php
@@ -157,10 +157,10 @@ public function getViewsData() {
-        'base' => 'node',
+        'base' => 'node_field_data',

Instead of hardcoding, this should call getViewsTableForEntityType(), like in the code that sets up general reference field relationships:

        $views_field['relationship'] = [
          'base' => $this->getViewsTableForEntityType($entity_type),
          'base field' => $entity_type->getKey('id'),
          'label' => $entity_type->getLabel(),
          'title' => $entity_type->getLabel(),
          'id' => 'standard',
        ];

Hi there, I'm here at the contribution weekend and I'm going to make the suggested changes!

Adding tags

New patch with suggested changes. #ContributionWeekend2019 @mecmartini, @jlbellido

1. +++ b/core/modules/taxonomy/src/TermViewsData.php
   @@ -152,15 +152,17 @@ public function getViewsData() {
   -        'skip base' => 'node',
   

   This item's been lost.

2. +++ b/core/modules/taxonomy/src/TermViewsData.php
   @@ -152,15 +152,17 @@ public function getViewsData() {
   +        'title' => $entity_type->getLabel(),
   ...
   +        'base field' => $entity_type->getKey('id'),
   +        'label' => $entity_type->getLabel(),
   

   These are good changes, but they're technically scope creep!

   They're clean-up tasks, rather than bug fixing. I'm nitpicking, yes, but this is an important principle to keep a patch focussed on the issue.

Tested patch from #2. Worked as expected! (Drupal Contrib Weekend!)

Tested with @StephanieLuz, #2 works! Thanks!

@joachim I understand your point, we were using as reference the code on CommentViewsData

$data['comment_field_data'][$type] = [
          'relationship' => [
            'title' => $entity_type->getLabel(),
            'help' => $this->t('The @entity_type to which the comment is a reply to.', ['@entity_type' => $entity_type->getLabel()]),
            'base' => $entity_type->getDataTable() ?: $entity_type->getBaseTable(),
            'base field' => $entity_type->getKey('id'),
            'relationship field' => 'entity_id',
            'id' => 'standard',
            'label' => $entity_type->getLabel(),
            'extra' => [
              [
                'field' => 'entity_type',
                'value' => $type,
                'table' => 'comment_field_data',
              ],
            ],
          ],
        ];

Anyway here it's the updated patch with suggested changes.

#ContributionWeekend2019 @karma0321 @jlbellido @Max1muss

We mistaked this line:

'base' => $this->getViewsTableForEntityType($entity_type),

with this:

'base' => $entity_type->getDataTable() ?: $entity_type->getBaseTable(),

It should be the same. Anyway, if needed, we'll upload a new patch to fix it.

Also, since the tests didn't pass, I guess we need to update the failed tests adding the "node" entity type. We'll try to work on it next week.

Updated patch with tests.

+++ b/core/modules/taxonomy/src/TermViewsData.php
@@ -152,12 +152,15 @@ public function getViewsData() {
+        'title' => $this->t('node'),

The test failure might be because this title property is going to change the UI.

@joachim I don't see any errors in the build, why the testing failed?

About the #23, the issue there is the title field or the fact that we used $this->t() as value?

> About the #23, the issue there is the title field or the fact that we used $this->t() as value?

Don't add the title field. That's changing the UI, and not a part of this issue.

Updated patch without the title field.

Looks good. Thanks everyone!

Unrelated test failure. Back to RTBC.
#3035318: `DateFormatter()` assumes 30 days per month, while February only has 28 days. Causes fails in tests.

Drupal\Tests\taxonomy\Kernel\Views\TaxonomyFieldTidTest needs fixing.

Fixed the test

Let's not use a deprecated property

So given that the base table is not dynamic I don't think we should be using the entity type manager for this. We don't, for example, in \Drupal\file\FileViewsData, book_views_data(), \Drupal\user\UserViewsData. The only time we use the entity type definition to get the base table name is when it is dynamic. That means we don't need to fix the tests which make this change less disruptive for contrib or custom testing.

Also whilst reviewing this I realise we've not update the 'skip base' setting and I think we should.

Also whilst checking the rest of the code base I've discovered that forum has the same problem. We should look to see if there is a similar issue - I've not fixed it here because views based on forum_index are more broken than just this.

Furthermore I think we should add some automated test coverage of this change - which is done in the attached patch.

#37 Tested patch works for me. Looks Good!

That sounds like an RTBC to me!

Committed/pushed to 9.0.x and 8.9.x thanks!

Wondering if there's a theoretical chance that this could break hook_views_query_alter() implementations or similar, so not backporting to 8.8.x for now.

duplicated https://www.drupal.org/project/drupal/issues/2457999 ?