EntityViewsData adds entity type to multi-value base tables, conflicts with QueryPluginBase::getEntityTableInfo

Its IMHO certainly better than not adding the flag to the multivalue base fields, but we should think about all the places where we use it maybe first.

I've spotted a bug in TermViewsData. We changed the base table for node to node_field_data and that was breaking the test.

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

We need tests for this.

@jibran, I spoke to @alexpott yesterday, and that change is quite difficult to test. Also there is an issue to replace the term index relationship with a generic entity relationship, so as long as we have test coverage for the change in @berdir's patch that should be enough.

Added test to cover changes to getEntityTableInfo().

Moving to 8.2.x

I talked to @yanniboi in person asking these questions (and included some of his nice answers here.)

1. filling in the ...changes sections in the summary will help a committer evaluate this issue.

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

   so this was already wrong, but we didn't know cause we weren't using the name of the base field to check things. And this new test does. So this fix was needed. See #5 interdiff (fixing previous patch fails)

3. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   --- a/core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php
   +++ b/core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php
   
   +++ b/core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php
   +++ b/core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php
   @@ -183,24 +183,34 @@ protected function setupEntityTypes($entities_by_type = [], $entity_revisions_by
   
   @@ -183,24 +183,34 @@ protected function setupEntityTypes($entities_by_type = [], $entity_revisions_by
            'entity type' => 'first',
            'entity revision' => FALSE,
          ],
   +      'id' => [],
        ]);
        $views_data->get('entity_first__revision')->willReturn([
          'table' => [
            'entity type' => 'first',
            'entity revision' => TRUE,
          ],
   +      'vid' => [],
   

   these added id's and vid's are needed now, cause without the fix to core, they were not being skipped (which is what this test wanted) but with the fix, they were being skipped. So explicitly set them so they are not skipped and this test works as intended. See #8 interdiff (fixing previous patch fails)

4. +++ b/core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php
   @@ -371,6 +381,54 @@ public function testLoadEntitiesWithRelationship() {
   +  public function testLoadEntitiesWithNonEntityRelationship() {
   +    // We don't use prophecy, because prophecy enforces methods.
   

   this test is copy and pasted from other similar tests. unknown if we want to repeat that here, or ... add a comment to the similar tests so they are related.

5. +++ b/core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php
   @@ -371,6 +381,54 @@ public function testLoadEntitiesWithRelationship() {
   +    $entity_information = $query->getEntityTableInfo();
   +
   +
   +    $this->assertSame($entities['first'][1], $result[0]->_entity);
   

   nit (double blank line)

6. +++ b/core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php
   @@ -371,6 +381,54 @@ public function testLoadEntitiesWithRelationship() {
   +    $this->assertTrue(in_array('first', array_keys($entity_information)));
   +    $this->assertFalse(in_array('entity_first_field_data__entity_first_field_data', array_keys($entity_information)));
   +  }
   

   these asserts seem to be the key. And it makes sense to test before that first is there, so that entity_first_field_data__entity_first_field_data not being there actually is meaningful. (if first wasn't there, then testing if the base field was not there wouldn't mean much)

So I guess the only real question about this... is if we want so much of the duplicated test here, that might not be strictly relevant to this particular test condition.
I kinda feel like the test could use some more comments about what the set up is... but it might also be that others wont need comments (and if the code is copy and pasted form another test, than commenting might be out of scope).

Updated issue summary

Removed @YesCT's nitpick and added some comments to the test.

+++ b/core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php
@@ -414,14 +414,15 @@ public function testLoadEntitiesWithNonEntityRelationship() {
+    // This is an entity table and should be in $entity_information.
     $this->assertTrue(in_array('first', array_keys($entity_information)));
+    // This is not an entity table and should not be in $entity_information.

Thanks!

uh, I thought first was a table, and entity_first_field_data__entity_first_field_data was a base field?

Also, why is this bug major? https://www.drupal.org/core/issue-priority#major-bug

+++ b/core/modules/taxonomy/src/TermViewsData.php
--- a/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
+++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php

+++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
@@ -285,6 +285,14 @@ public function getEntityTableInfo() {
+        // If the table does not have the necessary base field, skip it.

This is the reason I'm asking about wording.

Also, why is this bug major?

This bug is major because it basically breaks any view with a relationship to an non entity table.

So according to @dawehner, we don't just want to exclude entity base tables, we want to exclude:

• Entity base table (eg entity_name)
• Entity revision table (like entity_name_revision)
• Entity data table (like entity_name_field_data)
• Entity data revision table (like entity_name_field_data_revision)

So while our current fix sort of works it feels a bit *hacky*.

Maybe we can think of a better way of doing this?

We're looking for the presence of the ID or revision ID. All of the 4 tables you listed have those

Well, we are talking about potentially arbitrary tables, which just happen to have the same columns. Just imagine maybe an entity type that has a 'entity_id' as 'id' key.

Thanks a ton @berdir!

I still like my idea of not adding this information to those tables, will see about uploading a patch for that tomorrow.

Its good to see whether alternative approaches also work.

1. +++ b/core/modules/taxonomy/src/TermViewsData.php
   @@ -157,7 +157,7 @@ public function getViewsData() {
          'relationship' => array(
            'id' => 'standard',
   -        'base' => 'node',
   +        'base' => 'node_field_data',
            'base field' => 'nid',
   

   Can we create its own issue for this change? The most recent approach should still support it. ... The problem here is that we probably need an update path for it.

2. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -285,6 +285,14 @@ public function getEntityTableInfo() {
   +
   +        // If this is not one of the entity base tables, skip it.
   +        $entity_type = \Drupal::entityTypeManager()->getDefinition($table_data['table']['entity type']);
   +        $entity_base_tables = [$entity_type->getBaseTable(), $entity_type->getDataTable(), $entity_type->getRevisionTable(), $entity_type->getRevisionDataTable()];
   +        if (!in_array($relationship->definition['base'], $entity_base_tables)) {
   +          continue;
   +        }
   

   IMHO this approach is one with the least possible BC problems.

Just some quick nitpick ...

+++ b/core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php
@@ -371,6 +377,55 @@ public function testLoadEntitiesWithRelationship() {
+    $this->assertTrue(in_array('first', array_keys($entity_information)));
...
+    $this->assertFalse(in_array('entity_first_field_data__entity_first_field_data', array_keys($entity_information)));

Let's use $this->assertContains and $this->assertNotContains

Nitpick addressed.

I tested @yanniboi's patch from #27 on Payment, of which the view on /admin/content/payment is broken because of this problem. I can confirm the patch fixes the problem. To reproduce:

1. Go to a clean Drupal 8.2.x install.
2. Include Plugin 8.x-1.x, Payment 8.x-2.x, Currency 8.x-3.x, and Currency's Composer dependencies in the code base.
3. Install Payment and Views.
4. Go to /admin/content/payment.
5. Confirm you see an uncaught exception or a WSOD.
6. Apply the patch from #27
7. Go to /admin/content/payment again.
8. Confirm you see a working view with an empty tabular list of payments.

I will leave the RTBC to someone else, because I do not know if this solution is the best possible one for this issue.

Thank you @yanniboi

In general I'm quite convinced of this solution. It provides the minimal chance of a BC break.

See #28 for the reason this is a blocker.

Here is the patch from #27 for 8.1.x.

This should fix the last remaining test failure on 8.1.x.

#33's interdiff is correct, but its patch does not contain the changes.

Oh hello, weekend!

#27 contains the latest patch for 8.2.x.
#33 contains the latest patch for 8.1.x.

I am RTBC'ing this per #29, because all I did was fix an erroneous code comment, as proven by the tests passing.

Given what @dawehner says in #29 only going to commit the 8.2.x - I don't want to take any chance of BC break in 8.1.x.

Committed c7cc44e and pushed to 8.2.x. Thanks!

Hm, I'm not sure that's what @dawehner meant with that comment :). We are not aware of any BC break and I can't imagine what could break it.

Right, the solution we went with is the one which has no known BC break yet, unless some of the earlier ones. If you join to the base table, adding the entity IDs should be safe.

Committed 126d2a1 and pushed to 8.1.x. Thanks!