Multiple valued Base fields won't work in Views

Yes, I ran into the same issue with a custom entity. Looking closer into it, EntityViewsData only explicitly loads the base, revision an data tables, which doesn't include tables for multi-valued fields.

// Load all typed data definitions of all fields. This should cover each of
// the entity base, revision, data tables.

To fix this, we would need to go through the fields instead of the tables, but there's already a TODO in the code pointing to this issue. #2337511: EntityViewsData: "@todo We should better just rely on information coming from the entity storage.". I guess the alternative for now would be to extend EntityViewsData.

Steps to reproduce this issue:

1. Generate a new module with drupalconsole
2. run the command to generate a new content entity: $ drupal generate:entity:content
3. Change one of the fields to be multivalue: $fields['name'] = BaseFieldDefinition::create('string')->setCardinality(2);
4. Create a couple of entities for this new content entity
5. Create a new view for this new entity.
6. Make sure the view displays fields.
7. Try to add the name field, you can't find it in the add fields dialog

@pfrenssen and I looked at this and we figure that $tableNames didn't have all the required table names, so we added $table_mapping->getDedicatedTableNames(), locally this doesn't make a difference but at least this is a direction this could possibly go into.

I spent a car ride and a flight coming home from drupalaton playing around with this and I have absolutely no idea how to continue. I've attached a patch that conists mainly of some comments that reflect what I was doing.

I traveled from Belgium to Valencia today, I spent a couple of hours in the airport / plane working on this patch.

The patch I posted in #6 didn't apply anymore, at least this one does. That's sadly enough the only positive thing I can say about this patch because I didn't get any closer to something that actually works.

So, another method of traveling (train) doesn't make me much wiser it seems, I've been looking at why this doesn't work and I think I may have found that, I haven't found a solution though.

Posting a patch with intermediate work.

We're also running into this issue. I suppose as a workaround you can use a configurable field, but it gets tedious to make sure that you add it to each new bundle. And as a module maintainer, you probably wouldn't want to force your end users to have to maintain that field.

Took a while, but I think I may have found the culprit.

The gist is that the list of "all field definitions" ($definitions) is filtered down to just those fields that allow for shared storage. Then farther on, the list of "all field definitions" is filtered down to just those fields that require dedicated storage. But at this point, that list doesn't contain all the field definitions, just the shared storage ones, so the list gets wiped out completely because those two ideas are mutually exclusive. As a result, the table mapping for dedicated storage fields never runs. Ever. (Sorry if that explanation is confusing...the patch may make more sense. ;) )

With this patch, the fields become available to Views. There's no interdiff because this patch is not based on the other work here.

Someone with more knowledge of these systems should definitely have a look at the patch, but I think it's headed in the right direction. One question I have for someone with more familiarity would be whether the base field check belongs in DefaultTableMapping::requiresDedicatedTableStorage() or whether it should just remain in the array_filter().

Alrighty, and fixing that issue exposed a bug in the part of the code that was never being run. (Revision tables were being added even for fields/entities that aren't revisionable.) C'mon lucky number 13! :)

Hmm, I must have had some other code in play that I didn't account for, but this doesn't quite work. I swear I had it working a minute ago...

Unassigning myself in the off-chance that someone wants to pick this up over the holidays.

Third time's the charm? Looks like I goofed and forgot the changes from one file.

getDedicatedRevisionTableName() is a public method on DefaultTableMapping, but this is not part of the TableMappingInterface. This patch takes a slightly different tack to accomplish the same goal (getting the correct group for the join). It also fixes a small bug (I think) in DefaultTableMapping::getFieldTableName(), which does not currently take into account that the table might be dedicated.

*Sigh*, whack-a-mole continues. The fact that DefaultTableMapping::getFieldTableName() gave an incorrect value for fields with dedicated storage masked an issue with updates to field storage definitions. The storage tries to issue a count query to figure out if there is any existing data in that field (and stop if there is), but in crafting the query, it uses table mapping info that is not correct. (At that point in processing, the table mapping represents what things will look like after the update, not what they currently look like, so it crafts the query against a table that doesn't exist yet.)

Okay, I thought it was going to be a quick fix for DefaultTableMapping::getFieldTableName(), but that's turning out to be way more complex than I had hoped, so I broke that out into #2644088: DefaultTableMapping::getFieldTableName does not report table for fields with dedicated storage. Unless someone can see another way to determine the group for the Views join, this is postponed on that. The TabbleMappingInterface doesn't give you much to work with.

Now that #2644088: DefaultTableMapping::getFieldTableName does not report table for fields with dedicated storage is very close to being committed, here's a patch with just the EntityViewsData changes.

#2644088: DefaultTableMapping::getFieldTableName does not report table for fields with dedicated storage has landed, yay!

If nothing else, we'll want to make sure this part:

+            'extra' => array(
+              array('field' => 'deleted', 'value' => 0, 'numeric' => TRUE),
+            ),

uses short array syntax before commit.

Trying to write some test coverage now

Here is a test which IMHO would make sense, but it showed a clear bug in core/modules/views/src/EntityViewsData.php:264 as we don't yet deal with the revision tables.

The patch could use some KernelTests as well. Over all patch looks good to me.

The patch code is still a little bit over my head on this one -- although glad to see it's underway! But just wanted to add to the thread that an interim fix is to implement a class extending `EntityViewsData` -- Drupal Console does this already -- and manually add the table joins for the multivalue field tables in the `getViewsData()` method. So it looks like:

class TestEntityViewsData extends EntityViewsData implements EntityViewsDataInterface {

  /**
   * {@inheritdoc}
   */
  public function getViewsData() {
    $data = parent::getViewsData();

    $data['test_entity__multivalue_field_name']['table'] = array(
      'join' => array(
        'test_entity' => array(
          'title' => $this->t('fieldname'),
          'field' => 'test_entity_id',
          'left_field' => 'entity_id',
        ),
      ),
      'group' => $this->entityType->getLabel(),
    );
  }

}

@jibran
Totally agree with you ...

Here is a slightly different implementation which fixes issues

I would say this is major as it is both a regression from Drupal 7 and a major dev WTF - you expect using core entity reference would just provide this.

What is needing to happen to progress this, I will see if I can do anything!

@andrewbelcher
You could for example manually test the patch, with revision etc. Get crazy with it and try to break it. This would be amazing. On top of that you could of course review the patch as well, but this is up to you.

PS: I don't know about the regression from D7. D7 didn't even generated those views integrations automatically.

So unfortunately my use case has already demonstrated this not working. Namely a multi-cardinality entity reference base field. I didn't get any fields/filters/arguments/relationships.

Looking at what this does, I'm not sure it would for any other field type anyway? Although this adds the implicitly joined table, it doesn't add any of the actual information about the field. I tried tweaking it by adding a call to mapFieldDefinition() on the joined table, but that method makes a whole bunch of assumptions about the data structure (ie uses the field name rather than FIELD_NAME_COLUMN_NAME). That I was able to solve by making the following adjustment:

     $multiple = (count($field_column_mapping) > 1);
+    $dedicated_storage = $table_mapping->requiresDedicatedTableStorage($field_definition->getFieldStorageDefinition());
     $first = TRUE;
     foreach ($field_column_mapping as $field_column_name => $schema_field_name) {
-      $views_field_name = ($multiple || $dedicated_storage) ? $field_name . '__' . $field_column_name : $field_name;
+      $views_field_name = $field_name;
+      if ($dedicated_storage) {
+        $views_field_name .= '_' . $field_column_name;
+      }
+      elseif ($multiple) {
+        $views_field_name .= '__' . $field_column_name;
+      }

That then gave me my fields/filters/relationships etc. However, my ER relationship only joins to the field table once even if I add multiple relationships. That leads me to think that actually implicit joins aren't the right way to solve it for at least ER fields (not sure about others - think they may be fine as long as you don't need to do things on 2 different items of the same field).

I wonder if we actually need to explicitly exclude entity reference fields from the bits this patch is currently doing and deal with them differently...

On the patch itself, I didn't get into the test but the bits in EntityViewsData:

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -235,11 +242,22 @@ public function getViewsData() {
   +      $dedicated_data_tables = [];
   +      foreach ($field_definitions as $field_name => $field_definition) {
   +        $storage_definition = $field_definition->getFieldStorageDefinition();
   +        if ($revisionable && $storage_definition->isMultiple()) {
   +          $dedicated_data_tables[] = $table_mapping->getFieldTableName($field_name);
   +        }
   +      }
   

   $dedicated_data_tables never gets used after it's built, so not sure what this is doing?

2. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -257,6 +275,39 @@ public function getViewsData() {
   +          $table = $table_mapping->getDedicatedDataTableName($field_definition->getFieldStorageDefinition());
   +  ¶
   +          $data[$table]['table']['group'] = $this->entityType->getLabel();
   

   Whitespace error here.

I had a chat with berdir on IRC and he gave me some helpful pointers for the ER specific side of things:

20:11:52 <berdir> andrewbelcher: IMHO, what that issue needs is a variant of hook_field_views_data() that works for base fields. the entity reference specific logic is in core_field_views_data()
20:12:41 <berdir> andrewbelcher: the problem of course is that that hook is type hinted on FieldStorageConfigInterface, for no good reason. And we can't change that.
20:14:57 <berdir> andrewbelcher: I think that was also discussed in that issue. we might need a handler-like concept for field type, so we can call something for each entity type, if supported
20:15:15 <andrewbelcher> berdir: so that's not currently called for base fields because they don't implement FieldStorageConfigInterface right?
20:15:25 <berdir> andrewbelcher: correct
20:16:31 <andrewbelcher> berdir: am I right in thinking that as EntityViewsData is only dealing with base fields, we could either implement that logic directly in there (obviously adapted for the different storage)?
20:16:45 <andrewbelcher> berdir: but the handler is a means of making it so other non core fields can also do similar?
20:17:07 <berdir> andrewbelcher: yes, we could add something there, but exactly, only for known field types
20:17:58 <andrewbelcher> berdir: ok, in that case what I might do is start by implementing in in there for ER to figure out the logic/prove the concept and then we can figure out how to make it so that other things can do the same
20:19:07 <berdir> andrewbelcher: yes. makes sense to me. tests would also help. the logic would really be the same as core_field_views_data(), should be a 1:1 copy of that
20:20:24 <andrewbelcher> berdir: thanks - that's really helpful. I may shortcut and just use a regular field to finish this sprint and make moving back to a base field (and therefore this issue) something for next sprint ;)
20:20:29 <andrewbelcher> berdir++
20:20:58 <berdir> andrewbelcher: what you could try too is just introduce a new hook. we can't change the old one for BC, but we could implement a new one called hook_field_views_data2 (we can still fight on the name later). it's not really a hook but a callback for the provider of the field type. so it would be easy to check if it implements the new one, call that, otherwise the old
20:21:25 <andrewbelcher> berdir: cool, sounds good!
20:22:17 <berdir> andrewbelcher: then you just need to change the type hint to FieldStorageDefinitionInterface and rename core_field_views_data(). afk now, you might want to copy this discussion into the issue

I will see if I can get some time to look at this in the next couple weeks.

Thank you for testing the patch!

$dedicated_data_tables never gets used after it's built, so not sure what this is doing?

Good point. This was a temporary thing.

That then gave me my fields/filters/relationships etc. However, my ER relationship only joins to the field table once even if I add multiple relationships. That leads me to think that actually implicit joins aren't the right way to solve it for at least ER fields (not sure about others - think they may be fine as long as you don't need to do things on 2 different items of the same field).

I wonder if we actually need to explicitly exclude entity reference fields from the bits this patch is currently doing and deal with them differently...

I don't get this point. You need the implicit join to have the ER fields joined in and based upon that the relationships to the foreign entities.

20:16:31 berdir: am I right in thinking that as EntityViewsData is only dealing with base fields, we could either implement that logic directly in there (obviously adapted for the different storage)?

I would love to unify views_views_data() + EntityViewsData into one class, but I don't think this has to happen as part of this issue. This issue is already solving a lot of problems for people, just maybe not yours.

This issue is already solving a lot of problems for people, just maybe not yours.

I think you're definitely right that it doesn't solve my issue and the current direction wont (and shouldn't). From what I can see and my testing (which is limited - I'll try and put some more work into that next week), it only configured the implicit join but didn't add any fields etc to the newly joined table, which means it doesn't actually solve the current scope of the issue. That's what the bits with mapFieldDefinition() were about (both adding a call to it for the newly joined tabled and adjustments in mapFieldDefinition() to support non base table fields names - both of which may not actually be the right solution).

My broader question is whether the current approach is actually the right way to solve it base on:

I don't get this point. You need the implicit join to have the ER fields joined in and based upon that the relationships to the foreign entities.

So I may well be wrong, but I think that an implicit join wont provide a particularly nice solution to multi value fields. The implicit join will mean that you will always expand to show a row for each field value. Although this is sometimes desired behaviour, D7 allowed you to:

• with fields choose whether to show all items on a single record or show one per row along with options like range/offset of the items. An implicit join wont allow this as we can't add further conditions to it - it would need to happen as part of a JoinPluginInterface, which implies this should be solved at a field plugin level rather than table level.
• with filters (and probably arguments) you could choose to include the value in the join allowing you to do AND/OR without duplicate rows. Again, this requires a JoinPluginInterface rather than an implicit join and would have to be dealt with at a filter plugin level.
• with relationships (which only really applies to ER) you could optionally filter by delta.

In at least relationships and I think the others also, each one use a new join and therefore allow you do things like my use case where I want to join to 2 different referenced entities. By using the implicit join, you exclude that as a possibility as your two relationships will both be from the same row of the field table so will always join to the same item.

Again - very happy to be wrong about this assessment, but I wonder if we actually need to move this into the plugins themselves (be it field/filter/argument/relationship)... On that note, I think non base fields also suffer from the same issue as I think the logic in core_field_views_data() results in the same problem.

I'll try and get some concrete tests built next week that demonstrate test the actual problems and provide the solutions I would expect, that may help communicate it better!

with fields choose whether to show all items on a single record or show one per row along with options like range/offset of the items. An implicit join wont allow this as we can't add further conditions to it - it would need to happen as part of a JoinPluginInterface, which implies this should be solved at a field plugin level rather than table level.

Did you had a look at the actual SQL query which is executed? While there is a join in the views data definition, this is first about describing the "relationships" between tables. On runtime the actual field still determines whether it wants to execute the joining, or whether it can figure out its field value in some other way. Any base field and ALSO configurable field (since d7) basically use the field values from a loaded entity. In HEAD we already use the same kind of code for normal base fields and configurable fields already: \Drupal\views\Plugin\views\field\Field, especially have a look at core/modules/views/src/Plugin/views/field/Field.php:256

I'm wondering whether splitting this work into #2337515: Allow @FieldType to customize views data or even a followup for that would do it.

Here are small bits. I think though we have test coverage here, don't we?

We agreed on some KTB in #31 and #33.

Here is a test, finally. This interdiff also removes some code, which isn't needed anymore.

Let's start from 8.2.x.

The patch is ready to go. I've cleaned up some nitpicks along the way.

This is still a pure bugfix

Now with the test class name renamed as well.

Just another random failure.

+1 for RTBC and yeah it is a pure bug fix.

I think we need an empty update function for a cache clear because views data is cached - no?

Yeah you are right about that point.

There we go.

I think its fair to move this back to RTBC. The update path really don't need test coverage :)

Ah, so this doesn't apply to 8.1.x but it does for 8.2.x

Queued a 8.2.x test.

Can be fixed on commit:

}


/**
 * Clear caches due to updated entity views data.
 */
function views_update_8007() {

Double newline before views_update_8007().

Wanted to commit this but ran into #2743297: Mis-named update function in views.

The update here needs to be in the 8.1.x section and start with 81. Re-rolled for that.

Since this is a one-liner plus code move I think I could still commit this, but moving to CNR so I'm not committing my own patch without review though.

+1

Committed/pushed to 8.2.x and cherry-picked to 8.1.x, thanks!

#2743309: Flush caches after post updates would have solved that particular problem.

Hi,
As suggested here : https://www.drupal.org/node/2842933, I'd like to reopen this issue because I just ran into this bug.

I've got this issue with Drupal core 8.3.7 and brightcove module's version 8.x.1-2.

Steps to reproduce :
1 - install the brightcove video module and its dependencies
2 - Configure brightcove api client credentials
3 - Sync video with drupal
4 - create a view for the type brightcove video
5 - add the name of the video as field
6 - add a filter on the tag field with a not null value (when no value is found, the field will not be used in the query)

7 - You should have now :

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'brightcove_video__tags.tags' in 'where clause': SELECT brightcove_video.bcvid AS bcvid FROM {brightcove_video} brightcove_video LEFT JOIN {brightcove_video__tags} brightcove_video__tags ON brightcove_video.bcvid = brightcove_video__tags.entity_id AND brightcove_video__tags.deleted = :views_join_condition_0 WHERE brightcove_video__tags.tags = :db_condition_placeholder_1 LIMIT 11 OFFSET 0; Array ( [:db_condition_placeholder_1] => 4 [:views_join_condition_0] => 0 ) in Drupal\views\Plugin\views\query\Sql->execute() (line 1488 of core/modules/views/src/Plugin/views/query/Sql.php).
instead of a list of results

Thanks in advance for your help.

Best regards,
David

I confirm @muldos's issue.

Defined in my custom entity Class the following field:

$fields['receivers'] = BaseFieldDefinition::create('entity_reference')
      ->setLabel(t('Receivers'))
      ->setDescription(t('The user IDs to whom the notification is addressed.'))
      ->setSetting('target_type', 'user')
      ->setSetting('handler', 'default')
      ->setRequired(TRUE)
      ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED)
      ->setRevisionable(FALSE)
      ->setTranslatable(FALSE)
      ->setDisplayOptions('view', [
        'label' => 'hidden',
        'type' => 'author',
        'weight' => 0,
      ])
      ->setDisplayOptions('form', [
        'type' => 'entity_reference_autocomplete',
        'weight' => 5,
        'settings' => [
          'match_operator' => 'CONTAINS',
          'size' => '60',
          'autocomplete_type' => 'tags',
          'placeholder' => '',
        ],
      ])
      ->setDisplayConfigurable('form', TRUE)
      ->setDisplayConfigurable('view', TRUE);

When I use this field as a contextual filter I get the following error:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'itrack_notification__receivers.receivers' in 'where clause': SELECT itrack_notification.created AS itrack_notification_created, itrack_notification.id AS id FROM {itrack_notification} itrack_notification LEFT JOIN {itrack_notification__receivers} itrack_notification__receivers ON itrack_notification.id = itrack_notification__receivers.entity_id AND itrack_notification__receivers.deleted = :views_join_condition_0 WHERE (itrack_notification__receivers.receivers = :itrack_notification__receivers_receivers ) ORDER BY itrack_notification_created DESC LIMIT 21 OFFSET 0; Array ( [:itrack_notification__receivers_receivers] => 27 [:views_join_condition_0] => 0 )

An important followup was #2846614: Incorrect field name is used in views integration for multi-value base fields, which fixed the problem from #66.