Problem/Motivation

Steps to reproduce
  • Add a taxonomy field to user
  • Add a user view
  • Add the user: field_tags relationships to the user
  • You should be able to add taxonomy term: name and other fields, but it doesn't work

Sadly we forgot to change the code of entity_reference.views.inc in #2429447: Use data table as views base table, if available.
so the relationships introduced

Proposed resolution

Add them, as well as test coverage

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
olli’s picture

Status: Active » Needs review
FileSize
3.88 KB

Here's a start.

olli’s picture

Added test for entity reference.

The last submitted patch, 4: 2451789-4-test_only.patch, failed testing.

Anonymous’s picture

Atleast on views entity references work again with this patch.

jhedstrom’s picture

Issue tags: -Needs tests

Removing the 'needs tests' tag. The fix looks solid to me, and adds test coverage.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me too

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
+++ b/core/modules/taxonomy/taxonomy.views.inc
@@ -108,7 +108,7 @@ function taxonomy_field_views_data_views_data_alter(array &$data, FieldStorageCo
-    'base' => $entity_type->getBaseTable(),
+    'base' => $entity_type->getDataTable() ?: $entity_type->getBaseTable(),

Are we not missing test coverage wrt to this change and the original bug report?

olli’s picture

Status: Needs work » Needs review
FileSize
5.9 KB
10.99 KB
  1. +++ b/core/modules/entity_reference/entity_reference.views.inc
    @@ -49,9 +51,10 @@ function entity_reference_field_views_data(FieldStorageConfigInterface $field_st
           'label' => t('@field_name', array('@field_name' => $field_storage->getName())),
    +      'entity type' => $entity_type_id,
         );
       }
    

    This is entity_type elsewhere. Added join_extra.

  2. +++ b/core/modules/taxonomy/taxonomy.views.inc
    @@ -108,7 +108,7 @@ function taxonomy_field_views_data_views_data_alter(array &$data, FieldStorageCo
    -    'base' => $entity_type->getBaseTable(),
    +    'base' => $entity_type->getDataTable() ?: $entity_type->getBaseTable(),
    

    Made the same change for file and image.

Still needs more tests.

Status: Needs review » Needs work

The last submitted patch, 10: 2451789-10.patch, failed testing.

Status: Needs work » Needs review

Berdir queued 10: 2451789-10.patch for re-testing.

dawehner’s picture

  1. +++ b/core/modules/entity_reference/entity_reference.views.inc
    @@ -39,19 +39,30 @@ function entity_reference_field_views_data(FieldStorageConfigInterface $field_st
    +      'help' => t('Relate each @entity with a @field_name set to the @label.', $args),
    

    I like the better help text.

  2. +++ b/core/modules/entity_reference/src/Tests/Views/EntityReferenceRelationshipTest.php
    @@ -151,4 +151,79 @@ public function testRelationship() {
    +    // Create reference from entity_test_mul to entity_test.
    ...
    +    // Create reference from entity_test to entity_test_mul.
    

    Its good to have these 1 line summary documentation, this makes it really easy to scan the code.

lauriii queued 10: 2451789-10.patch for re-testing.

lauriii’s picture

Title: taxonomy/entity reference joins to the wrong base table in views. » Entity reference joins to the wrong base table in views.
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 10: 2451789-10.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
13.6 KB
18.06 KB
9.12 KB

The last submitted patch, 17: 2451789-test_only.patch, failed testing.

dawehner’s picture

@olli
This is not really what I meant.
You added new tests which ensure that specific views data is generated ... which is not a bad idea, but we still haven't a proper test that an entity reference query work as expected.

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Needs work based upon my last feedback, sorry :)

jibran’s picture

+++ b/core/modules/entity_reference/entity_reference.views.inc
@@ -39,19 +39,30 @@ function entity_reference_field_views_data(FieldStorageConfigInterface $field_st
+      'join_extra' => array(
+        0 => array(
+          'field' => 'deleted',
+          'value' => 0,
+          'numeric' => TRUE,

I don't think we need this.

dawehner’s picture

I don't think we need this.

Are you sure about this? Other fields do a similar kind of join:

    $data[$table_alias]['table']['join'][$data_table] = array(
      'left_field' => $entity_type->getKey('id'),
      'field' => 'entity_id',
      'extra' => array(
        array('field' => 'deleted', 'value' => 0, 'numeric' => TRUE),
        array('left_field' => 'langcode', 'field' => 'langcode'),
      ),
    );
jibran’s picture

Yeah you are right please ignore me.

jibran’s picture

Assigned: Unassigned » jibran

Having a look at it.

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
FileSize
39.65 KB
47.12 KB

I hope this address @dawehner concerns.

  • Improve the existing code for sanity sake.
  • Re-wrote EntityReferenceRelationshipTest for fields with data table join and base table join.
  • Getting silly exceptions related to shape field. If someone can have a look that would be great.

Status: Needs review » Needs work

The last submitted patch, 25: entity_reference_joins-2451789-25.patch, failed testing.

nuwe’s picture

Issue tags: +Needs screenshots
dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs screenshots

Thank you for working on it, great work!

@nuwe
Thank you for adding the tag.
This just fixes some bug on an API level, screenshots would not really add an value, IMHO :)
But sure we should post more screenshots of stopped debuggers.

Improve the existing code for sanity sake.

You could have tried to post an interdiff just containing the new test coverage ... :)
But sure +1 for renaming variables to be more sane and moving fields around in order to make it easier to read!

  1. +++ b/core/modules/entity_reference/src/Tests/Views/EntityReferenceRelationshipTest.php
    @@ -51,103 +58,175 @@ protected function setUp() {
    +    foreach (array_keys($view->result) as $index) {
    ...
    +      $this->assertEqual($view->result[$index]->id, $this->entities[$index]->id());
    ...
    +    foreach (array_keys($view->result) as $index) {
    +      $this->assertEqual($view->result[$index]->id, 1);
    

    This is a bit odd ... why do we get the index but then just use it to access $view->result[$index}?
    Can't we just use foreach ($view->result as $row) and then use $row?

  2. +++ b/core/modules/entity_reference/src/Tests/Views/EntityReferenceRelationshipTest.php
    @@ -51,103 +58,175 @@ protected function setUp() {
    +      // Also check that we have the correct result entity.
    +      $this->assertEqual($view->result[$index]->_entity->id(), $this->entities[$index]->id());
    +
    +      // Test the forward relationship.
    +      $this->assertEqual($view->result[$index]->entity_test_mul_property_data_entity_test__field_test_data_i, 1);
    +
    +      // Test that the correct relationship entity is on the row.
    +      $this->assertEqual($view->result[$index]->_relationship_entities['field_test_data']->id(), 1);
    +      $this->assertEqual($view->result[$index]->_relationship_entities['field_test_data']->bundle(), 'entity_test_mul');
    

    These tests are looking great! Exactly what I thought we should have test coverage for

  3. +++ b/core/modules/entity_reference/src/Tests/Views/EntityReferenceRelationshipTest.php
    @@ -51,103 +58,175 @@ protected function setUp() {
    +    $view->destroy();
    

    You can skip the destroying if you reload the view anyway, just a tip.

olli’s picture

I think those exceptions are caused by entity_test_mul_default_value entity type having the same base/data table as entity_test_mul. Test installs only entity_test_mul but views tries to load entity_test_mul_default_value... Should we change entity_test_mul_default_value table names?

jibran’s picture

FileSize
7.35 KB
47.9 KB

Thanks for the review @dawehner fixed everything.
@olli you are correct. I have changed the table name of entity_test_mul_default_value now the tests are passing but I'd not be surprised seeing some other tests failing in core.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, this is clearly a bug and its just luck that things work at the moment.

jibran’s picture

Is it a views data bug?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed b58ab8c and pushed to 8.0.x. Thanks!

  • alexpott committed b58ab8c on 8.0.x
    Issue #2451789 by olli, jibran: Entity reference joins to the wrong base...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jweowu’s picture

I wonder if someone from this issue can assist with this?

https://www.drupal.org/project/drupal/issues/571548#comment-14590720