Need to join fields to the entity field data tables, not entity tables, or filtering increases number of results

This issue applies to field contains filters, not just language, but it's related to having translated nodes.

And I think it's pretty much major... I am going to verify it is still a problem though, and then probably write a test.

I just tested it again, and have added Steps to Reproduce to the issue summary. It is still a problem.

OK, here's a patch with a failing test.

At this point... Having written a test to illustrate the problem, I am not sure how to fix it.

As I pointed out when I originally added this issue, the joins set up for fields in Views are generic to the Field module, not specific to the Node module. So the way the Field module does its Views integration, it is ignoring the fact that individual field entries in field tables are associated with particular translations of entities, and it is just joining to the base table for the entity (node), not the correct table (node_field_data).

This problem may be specific to nodes... I am not sure if other entities have two tables like this, one for the base item and one for the translation information? Anyway, I really don't know how to fix it, so I'm unassigning it.

But Views on Nodes with any field filters on them are currently unusable on multilingual sites, as it is, because instead of just filtering (reducing the number of results), they also multiply the number of results due to the bad join.

Well, the test failed the way it should have. Setting this back to "Active" since there is no patch yet to fix the issue.

This has been added as a child issue of #2313159: [meta] Make multilingual views work. Since it is now part of the master plan there, updating title and summary (it is still the same issue, it's just that we've worked out how to make fixing it part of the master plan).

Also postponed until #1498664: Refactor user entity properties to multilingual is finished.

The last patch on #3 is *only* a failing test, illustrating the problem with Node views. It doesn't test views with other entities, and has no code fix.

I talked to Gabor today in IRC and I think what we need to do on this issue is:
- If an entity has a data table (all Core fieldable ones do now), join field data to the data table.
- If an entity does not have a data table, join the way we do now. Contrib entities intending to be multilingual that don't use data tables do so at their own risk!

I think I'm ready to take this on!

OK, here's another failing test added, for Taxonomy; have tested in my local environment and both Node and Taxonomy tests in this patch fail in the right way to demonstrate the bug.

We also need tests for Comment and User.

I haven't worked on the fix yet either, but I actually think that should be straightforward.

OK, here's a patch with an additional (failing as I'd expect) test for Comment.

I've made a test for User as well, but I'm running into problems, because neither the Entity view mode display choice nor the Field view mode display choice for User in Views is working correctly. So I'm going to upload that as a separate patch; we can add this test later if/when the underlying issues are fixed:
#2321645: Default entity view for User should show user name
#2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language"

Anyway, three tests is pretty good, right? Should be able to move towards making a patch for the actual issue now. So what we want to do here is patch the code and make the Node, Comment, and Taxonomy tests pass. Getting User to pass is not going to be possible right now.

OK, I think making the tests is probably about as far as I can take this. The Views join syntax is inscrutable and undocumented.

What needs to happen is in core/modules/field/field.views.inc in field_views_data(), or rather in the function it calls: field_views_field_default_views_data().

There it's doing:

  if (!$entity_table = $entity_type->getBaseTable()) {
    return $data;
  }
...
  $data[$table_alias]['table']['join'][$entity_table] = array(
    'left_field' => $entity_type->getKey('id'),
    'field' => 'entity_id',
    'extra' => array(
      array('field' => 'deleted', 'value' => 0, 'numeric' => TRUE),
    ),
  );

What it needs to be doing instead is joining to the $entity_type->getDataTable() (if it exists), and instead of just matching the entity ID, it also needs to match the language code (if it exists).

I tried to do this, but what I tried only succeeded in not only breaking all views with fields, but also getting some kind of weird error when it tried to output an exception:

Call to a member function getTrace() on a non-object in ....../core/lib/Drupal/Core/Controller/ExceptionController.php

Alternatively, we could patch the hook_views_data() docs so they explain how to do complex joins, which they totally don't now. I also looked at the Join handler plugin in Views but it was not much more help.

Anyway... The tests in 2218065-field-filters-FAIL-comment-node-taxonomy-tests.patch need to pass... that is as far as I can take it right now. Any takers?

Um. No. This is not right.

-- a/core/modules/node/src/NodeViewsData.php
+++ b/core/modules/node/src/NodeViewsData.php
@@ -41,6 +41,9 @@ public function getViewsData() {
       'type' => 'INNER',
       'left_field' => 'nid',
       'field' => 'nid',
+      'extra' => array(
+        array('left_field' => 'langcode', 'field' => 'langcode'),
+      ),
     );

This is adding to the auto-join between node data and the node_field_data. But this is wrong. The langcode column in node is "the original language of the node". The langcode column in node_field_data is "the language of this translation". They absolutely should not ever be joined. That will completely break everything. The joins in the *ViewsData classes are correct as they are -- we do want the joins there to just join to the field data tables on the entity ID, which is what they are doing.

So now I see the syntax of 'extra'. Let me take this on again... and I'll file a separate issue to document 'extra' better in hook_views_data(). :)

OK, after some hair pulling and help from dawehner in IRC, I got this working, at least locally!

Some notes:
- There's a change in the Join handler -- to let it support 'left_field' syntax in 'extra' in joins.
- The default field join uses the data table if available, and base table if not. It does assume there is a langcode field in the data table; I think/hope this will be OK.

Anyway... The interdiff here is the code changes; full patch is code + tests. In other words, the interdiff is to my test-only patch in #17, not to dawehner's in #21, which wasn't my starting point.

Hopefully this won't break anything else...

Oh and regarding #23, there is no automatic language filtering in my patch. The tests check whether filtering by "contains" on fields work, and there is no language filtering.

Oh, and just to clarify: the JoinHandlerBase code came from dawehner's code in his patch. :)

Hah! That fixed most of it. Down to two fails in Drupal\field\Tests\Views\ApiDataTest -- I'll take a look shortly.

So there were some tests in Field module looking for the exact format of the field views data, which was changed in this patch, so had to update the patch. This should pass everything now hopefully!

Follow-up to add docs to hook_views_data():
#2321995: More info needed in hook_views_data docs/sample body