Use data table as views base table, if available.

Adding relevant tags from #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency.

@dawehner with all due respect, I get it what you are trying to do here but this is a huge architecture change obviously it makes sense but I am not at all comfortable with it. Perhaps we can ping @damiankloip to review this or if I may, can we ask @merlinofchaos to weight in here?
Given that I have worked with you guys on some VDC issues, I tried to follow changes in views closely, I believe, I have some(very little) understanding of the APIs in Views and I also understand the problem space here but still I am not comfortable reviewing this issue at all. IMHO

PS: Code changes seem fine to me.

you can't deny that.

Yes I agree. Can we ping someone about it for the extra set of eyes?

There are already 13 additional eyes (actually, 26) watching here ;)

I'm not a "views person" by any definition, but I think this patch makes perfect sense. Here's a small review:

1. +++ b/core/modules/comment/src/Plugin/views/field/Username.php
   @@ -43,6 +43,11 @@ protected function defineOptions() {
   +  public function query() {
   +    parent::query();
   +  }
   

   Not needed.

2. +++ b/core/modules/user/src/Tests/Views/HandlerFieldUserNameTest.php
   @@ -35,18 +35,19 @@ public function testUserName() {
   +    debug($render);
   
   @@ -55,8 +56,9 @@ public function testUserName() {
   +    debug($render);
   

   :)

3. +++ b/core/modules/views/src/ViewsData.php
   @@ -293,6 +293,9 @@ public function fetchBaseTables() {
   +        if (!isset($info['table']['base']['title'])) {
   +          debug($table);
   +        }
   

   Another one.

4. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_groupwise_term_ui.yml
   @@ -34,8 +34,8 @@ display:
   -          table: node
   -          plugin_id: node
   +          table: node_field_data
   +          plugin_id: node_field_data
   

   I think the plugin id is still node?

5. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -719,6 +791,8 @@ protected function assertEntityReferenceField($data) {
   +    $a = 123;
   

   lol.

6. +++ b/core/modules/views_ui/src/Tests/ViewEditTest.php
   @@ -29,7 +29,7 @@ class ViewEditTest extends UITestBase {
   -  public function testDeleteLink() {
   +  public function testsDeleteLink() {
   
   @@ -48,7 +48,7 @@ public function testDeleteLink() {
   -  public function testMachineNameOption() {
   +  public function testsMachineNameOption() {
   
   @@ -142,9 +142,9 @@ public function testEditFormLanguageOptions() {
   -  public function testRelationRepresentativeNode() {
   +  public function testsRelationRepresentativeNode() {
   

   Are these changes needed?

Let's compare node tables in Drupal 7 and Drupal 8.

Unless views has to show uuid it doesn't make sense to use node base table anymore.
But that is not a problem.

-    $data['node']['table']['base']['weight'] = -10;
-    $data['node']['table']['base']['access query tag'] = 'node_access';
-    $data['node']['table']['wizard_id'] = 'node';
+    $data['node_field_data']['table']['base']['weight'] = -10;
+    $data['node_field_data']['table']['base']['access query tag'] = 'node_access';
+    $data['node_field_data']['table']['wizard_id'] = 'node';

The problem is these changes. In Drupal 7 and Drupal 8(till now) we have everything in $data['node'] and now because we are changing base table so we have to change the data array structure to $data['node_field_data'] and this doesn't make sense to me.

1. At this stage of release this is a huge api change as you can see the number of changes in php files.
2. Views data array is already pretty difficult to understand this will make it even more difficult for developers.
3. Let's suppose I have module which stores nid and lock(boolean) in a table and I want to show this field in views. So should I add it to $data['node'](because it makes sense it is related to node but we don't have $data['node'] in data array) or $data['node_field_data'](it's not present on {node_field_data} table)? This create huge viewsWTF for contrib.
4. + $data['node_field_data']['table']['wizard_id'] = 'node'; it is a node wizard so why it is on $data['node_field_data']? And why plugin name is 'node' not node_field_data? viewsWTF!
5. I know adding stuff to $data['node_field_data'] or $data['node'] doesn't matter because this is a dummy array. But this is important for views field grouping in views_ui and for relationships in contrib.
6. The last big change, of this magnitude, we have done with the views code base is renaming the methods and we are still facing the consequences(no. of bugs) of that change. We still have snake properties(i.e $view->display_handler and $view->displayHandlers) in ViewExecutable class. Who knows how many bugs this change will create.

Because of the above change I am not comfortable with this patch and I think it is an architectural change. Changing the data table is not an architectural change because it's a logical thing to do now.

We can address the above concern in either of two ways:

1. We can change the data table and leave the views data array as it is. I think it is more logical thing to do at this point of release.
2. We can change the view data array to object as we(@dawehner, @damiankloip and I) discussed it at DrupalCon Amsterdam. But it's not possible at least in D8.

In the end I'd like to say let's fix this critical with the minimum code change. Let's not change the views data array. It also make sense because everything(which stores any kind of data in DB) is an entity in D8 and we are talking about entity integration with views not tables integration. In D7 data array is keyed by table name, which makes sense, and in D8 data array is keyed by entity name, which now makes sense, that is why I requested for the opinion form the other VDC team members.

PS: I don't want to be disrespectful in any way to anyone. I am just elaborating my issue with this patch.

So the moral of the story is $data['node'] is $data['node_field_data'] now and we can't fix these issues without this change.

I'm sorry, but patch size not necessarily correlates with huge API change. Its the amount of concepts, which would have to change, which is pretty straightforward here: Start from the data table,
if available, otherwise use the base table. Every change to every bit in views requires big patches, its just a sad matter of life.

I agree to everything you said here but this doesn't mean we have to change the structure of array return by \Drupal\views\Views::viewsData()->get() but then you said

... just to be clear, views always described the SQL table structure, so pointing to {node_field_data} is just IMHO the right thing to do in terms of explaining
what is going on, as its what we want to do. We want to avoid the JOIN. Its not a about the node entity type, see user as a different example, the user entity type users {users}

OK, fine. :)

Well, ... these wizards are living on base tables ... i'm sorry but this is not a viewsWTF, its rather the way how it works. A wizard is used to create a view starting from a specific base table, nothing else.

That's not the point. I was saying it was unnecessary change here we can live without this change. The matter of fact it doesn't exist on $data['node'] anymore is veiwsWTF.

Can you please explain exactly what you mean by that? Note: The problem of the other critical issue is that we don't use the data table, which is what this issue is trying to solve.

I was saying change the data table but not change the structure of views data array return by \Drupal\views\Views::viewsData()->get().

How would that help us? OT IMHO.

We can change the tables without caring the the keys in views data array.

I'm sorry but this statement is simply wrong. Its just coincidence that node uses 'node' as its base table name. There are actually way more entity types which uses a different name as base table name: taxonomy, users, aggregator item.

This is from the doc of hook_views_data

At the outermost level,
 *   the keys are the names used internally by Views for the tables (usually the
 *   actual table name).

so I was not that wrong :P

nested tables, nice!.

I am glad you liked the nested tables :D

Just to amplify what @dawenher is saying:

Views in Drupal 7 is not the same as Views in Drupal 8. In Drupal 7, unless I'm mistaken, each row in a view corresponded to one entity ID. In Drupal 8, each row corresponds to one language & entity ID -- i.e., one translation. The table that has this structure in Drupal 8 is the entity data table, not the entity base table, so it really makes more sense to use that as the Views base table for all entity views. All of the joins to other tables should also be joining using the entity data table -- you always should hopefully be joining on ID/language not just ID. So if we insist on keeping the entity base table in the view (really, we don't need it for Views unless the UUID field is needed in the view), we would need to somehow ensure that the entity data table is always joined no matter what, or we'd sometimes lose rows from the view -- the fundamental idea of "each row is an entity translation" would not be constant -- it would depend on the specific filters that were part of the view (which is the problem I identified on the parent issue that led to this issue being filed).

So from a philosophical point of view, this is the right thing to do, to make sure that the Views data model fits with the entity data model, and to preserve the idea of "one row == one translation of an entity" no matter what the particular view is doing.

Except, of course, in the case of the few entities that are not translatable and hence don't have a data table. In Core I think this is limited to File and maybe some of the Aggregator things.

So... Took a careful look at the latest patch. A few nitpicks and questions:
a)

+++ b/core/modules/file/src/FileViewsData.php
@@ -79,28 +79,28 @@ public function getViewsData() {
       ),
       // Link ourself to the {node} table
       // so we can provide node->file relationships.
-      'node' => array(
+      'node_field_data' => array(

Comment needs an update here. ;) Same for the rest of the changes in this file, where it links to comment, user, etc. Other files also needing some comment attention:
core/modules/views/src/Tests/FieldApiDataTest.php

     // The node field should join against node.
-    $this->assertTrue(isset($data[$current_table]['table']['join']['node']));
-    $this->assertTrue(isset($data[$revision_table]['table']['join']['node_revision']));
+    $this->assertTrue(isset($data[$current_table]['table']['join']['node_field_data']));
+    $this->assertTrue(isset($data[$revision_table]['table']['join']['node_field_revision']));
 

That's all I found.

b)

++ b/core/modules/user/src/UserViewsData.php

-    unset($data['users_field_data']['signature']);
-    unset($data['users_field_data']['signature_format']);
-

I do not think these lines should be removed. They're unsetting the defaults that are added by the base EntityViewsData class, and they're added back in a few lines below but only if the Filter module is enabled. That functionality should not be changed in this patch I think?

c) There seems to maybe be a problem in the Revision table setup in EntityViewsData:

-      $data[$revision_table]['table']['base'] = array(
+
+      $views_revision_base_table = $revision_table;
+      if ($revision_data_table) {
+        $views_revision_base_table = $revision_data_table;
+      }
+      $data[$views_revision_base_table]['table']['base'] = array(
         'field' => $revision_field,
         'title' => $this->t('@entity_type revisions', array('@entity_type' => $this->entityType->getLabel())),
       );
       // Join the revision table to the base table.
-      $data[$revision_table]['table']['join'][$base_table] = array(
+      $data[$revision_table]['table']['join'][$views_base_table] = array(

In that last bit, shouldn't it be joining $views_revision_base_table, not $revision_table (since $revision_table is not even necessarily defined here)?

d) Hah!!!

-   * @todo Given that the base_table is pretty much useless as you often have to
-   *   join to the data table anyway, it could make a lot of sense to start with
-   *   the data table right from the beginning.
-   * @see https://drupal.org/node/2337509
-   *

No kidding. :)

Agreed with @jhodgdon and @dawehner, not much else to say.

Agreed with the direction here - just wondering what happens if someone changes an entity type from not translatable to translatable ?

@yched: so long as that means originally the data was in a base table and then goes to a data table (or vice versa), the fields in the view will need to change anyway, no? This edited snippet from the front page view for example:

        promote:
          field: promote
          id: promote
          table: node_field_data
          value: true
          plugin_id: boolean
          entity_type: node
          entity_field: promote
        status:
          field: status
          id: status
          table: node_field_data
          plugin_id: boolean
          entity_type: node
          entity_field: status

If this entity type would have the status, promoted etc. not in a data table, the view would need updating in these areas as well (not just in terms of what is its base table).

The fields will need to change anyway, one problem is the hardcoded assumptions in the custom views data classes.

As mentioned before, maybe there should be a method that can be overridden easily to add fields to the "main" table, whatever views considers that to be?

Reviewed locally with git diff --color-words. For a 160K patch it's not actually that big, just the primary data model change. Other than that, the only actual changes are in EntityViewsData and DisplayPluginBase. Except for the relationships going away in FileViewsData. What's the deal with that?

Overall, this makes a ton of sense.

Can #2337509: Remove "@todo In theory we should use the data table as base table, as this would" from EntityViewsData be closed as a duplicate of this? Edit: as well as #2340181: Don't expose fields on the base table / revision table, if they exists on the data table as well..

Started a CR draft at: https://www.drupal.org/node/2433153
(Since the main goal is to notify D8 module authors and site owners of the change, these change records should no longer be sent to the Views queue to die.)

@dawehner: Are you thinking this is close/ready and are you looking for the final extra-careful tests/reviews to happen? Or do you think this needs more work? I plan to try it out using Views UI and make sure things are working as I'd expect, similar to what I did on the parent issue, but if it's going to change a lot I'd rather put that off.

+++ b/core/modules/views/src/EntityViewsData.php
@@ -129,20 +129,25 @@ public function getViewsData() {
+    $views_base_table = $base_table;
+    if ($data_table) {
+      $views_base_table = $data_table;
+    }

@@ -169,12 +174,17 @@ public function getViewsData() {
+      $views_revision_base_table = $revision_table;
+      if ($revision_data_table) {
+        $views_revision_base_table = $revision_data_table;
+      }

@@ -493,7 +498,7 @@ protected function processViewsDataForTextLong($table, FieldDefinitionInterface
   protected function getViewsTableForEntityType(EntityTypeInterface $entity_type) {
-    return $entity_type->getBaseTable();
+    return $entity_type->getDataTable() ?: $entity_type->getBaseTable();
   }

To address #30 and pave the way for #2432273: Make it possible for custom viewsData class to adapt to entity schema changes., we should add to the table mapping a method returning which are the current "field data" and "field revision" tables. For translatable entity types those are {entity_field_data} and {entity_field_revision}, while for untranslatable entity types those are {entity} and {entity_revision}. We should always use field data (revision) tables as views base tables.

Once we have these methods, we should key all the views data arrays with these dynamic values, this way we don't need to change the definitions if the entity table layout changes.

Looks like I will not get to this testing until later this week (today got busy) but I have it on my agenda!

Discussed with @catch, @alexpott, @webchick, and @effulgentsia. We agreed that this is critical, because it's essential for translatability and to unblock #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency. It also has the advantage of removing some unneeded joins in Views.

The patch apparently needs a new reroll. There were failures in:
core/modules/comment/src/Plugin/views/argument/UserUid.php
core/modules/comment/src/Plugin/views/filter/UserUid.php
There are also a few files that apply with "fuzz".

Marking "needs work" for this.

These pieces of the patch were small so I was able to apply manually. So I'm doing some more testing and reviewing, which I'll put in a separate comment.

OK, one emergency HEAD revert later... HEAD is not broken now, and the reroll is a bit less but still necessary.

Anyway. In this comment, I'll report on some manual testing. I'll take a look at the code in a separate comment.

So, I gave this a run-through test (manual) as follows, just to make sure nothing weird was happening that I could immediately detect. Here's what I did:

- Standard install in English
- Added a basic Text field to Article content type, and made it display in teaser.
- Also added a Text field to the User entity.
- Add two articles (promoted to front page). Put some tags on both of them, plus values for the text fields.
- Edited my user account to add a value for the text field in it.
- Verify front page view and taxonomy pages are OK [I'm sure the test bot would have caught that, hope so anyway]
- Added a new view of articles with fields. Added relationships to taxonomy term and author, and added fields from those to the view. I tested both base fields and my added fields. It all seems OK.

That was all with a one-language site. So far, so good.

Then I turned on the 4 multilingual modules, added Spanish, and made articles and users and tags translatable on the Content Language page
admin/config/regional/content-language

I was going to translate everything, but I did not get any translation links. ?!? I may have screwed up my cache by looking at the View while I was doing other stuff, so just in case I did a "drush cr" and tried again. OK, now I can translate. I think this was my fault.

OK. Now I go back to the view. Refresh the preview, nothing happens. So I tried the View Page link, and I'm getting this error:

Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'langcode' in field list is ambiguous: SELECT node_field_data.title AS node_field_data_title, node_field_data.nid AS nid, node_field_data.langcode AS node_field_data_langcode, taxonomy_term_field_data_node_field_data.name AS taxonomy_term_field_data_node_field_data_name, taxonomy_term_field_data_node_field_data.vid AS taxonomy_term_field_data_node_field_data_vid, taxonomy_term_field_data_node_field_data.tid AS taxonomy_term_field_data_node_field_data_tid, users_field_data_node_field_revision__users.uuid AS users_field_data_node_field_revision__users_uuid, taxonomy_term_field_data_node_field_data__taxonomy_term_data.uuid AS taxonomy_term_field_data_node_field_data__taxonomy_term_data, node_field_data.created AS node_field_data_created, users_field_data_node_field_revision.uid AS users_field_data_node_field_revision_uid, langcode AS langcode FROM {node_field_data} node_field_data LEFT JOIN (SELECT td.*, tn.nid AS nid FROM {taxonomy_term_field_data} td LEFT JOIN {taxonomy_index} tn ON tn.tid = td.tid WHERE (td.vid IN (:db_condition_placeholder_1)) ) taxonomy_term_field_data_node_field_data ON node_field_data.nid = taxonomy_term_field_data_node_field_data.nid INNER JOIN {node_field_revision} node_field_revision ON node_field_data.vid = node_field_revision.vid LEFT JOIN {users_field_data} users_field_data_node_field_revision ON node_field_revision.uid = users_field_data_node_field_revision.uid INNER JOIN {users} users_field_data_node_field_revision__users ON users_field_data_node_field_revision.uid = users_field_data_node_field_revision__users.uid INNER JOIN {taxonomy_term_data} taxonomy_term_field_data_node_field_data__taxonomy_term_data ON taxonomy_term_field_data_node_field_data.tid = taxonomy_term_field_data_node_field_data__taxonomy_term_data.tid WHERE (( (node_field_data.status = :db_condition_placeholder_0) )) ORDER BY node_field_data_created DESC LIMIT 10 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => tags ) in Drupal\Core\Database\Connection->query() (line 581 of core/lib/Drupal/Core/Database/Connection.php).

So... something is wrong with the patch, or possibly something is wrong with Views, relationships, and multilingual without the patch. I'm not sure I've tried this combination before.

The home page view and taxonomy pages are fine with this patch. Just this view with relationships is broken.

Let's see.

I created a second display on my view and took out the taxonomy term relationship. That didn't help, the error looks the same.

When I also removed the User relationship and its fields, the error went away.

Added back in the Taxonomy relationship. This view works.

Added back in the User relationship and UUID field from User, still works.

But when I add in the text field I had added to the user, it doesn't work.

So, in summary: A view with no relationships works, and with a Taxonomy Term relationship it works. But a view with a User relationship, which displays my text field I added to the User entity, that doesn't work.

Hm, wonder if it's text fields in general with relationships with multilingual.

So I went to the Tags vocabulary and added a text field there, and edited my tags so it had a value.

Added this new field to the view with the Taxonomy relationship but not User relationship (which was working).

Yup, that broke the view. And this field was not translatable.

So. One more conclusion:

With this patch [not sure without the patch?], if I set up a multilingual site with translatable Articles, Tags, and Users, and if Tags and Users have a text field on them... if I make a field-based view of Nodes that relates to Taxonomy or User, and add the text field from User or Taxonomy to the view, the view breaks.

At this point I do not know for a fact whether or not it's also broken without this patch. Unfortunately I don't think I can just unpatch and clear the cache, because all my views configs are wrong. :(

Any thoughts? My guess is that since it's a query error around the langcode field, it's probably introduced by this patch...

I also took a look through the patch code. Some comments/questions:

a) I'm confused why this has been added to FileViewsData -- maybe an artifact from a reroll? Doesn't seem related to this patch?

+      // Link ourself to the {taxonomy_vocabulary} table
+      // so we can provide taxonomy_vocabulary->file relationships.
+      'taxonomy_vocabulary' => array(
+        'field' => 'id',
+        'left_field' => 'vid',
+        'extra' => array(array('field' => 'type', 'value' => 'taxonomy_vocabulary')),
+      ),

And there's a bunch of other related code farther down in the same file.

b) Complete nitpick, in EntityViewsDataTest:

+    // Ensure that the base table it set to the data table.

it ==> is

Wow. That is all I see when examining the patch carefully. Must be pretty close!

I talked to @dawehner in IRC. The issue I reported in #50 is NOT due to this patch. He's writing it up as a separate issue.

So I think all that needs to be done on this issue is:

a) Reroll.

b) Address small review comments in #51. Might also check out xjm's comment in #35, which seems related to the File entity data issues.

c) plach's comment #44

And by the way I also reviewed the draft change record https://www.drupal.org/node/2433153 and I think it looks good too.

The other issue @dawehner filed: #2446681: Error "Column 'langcode' in field list is ambiguous" thrown due to TranslationLanguageRenderer not rendering a field from a relationship - not really related to this one, but I'll add it to the D8MI Views meta #2313159: [meta] Make multilingual views work

Great, FileViewsData looks much better now! I looked through the latest patch and didn't see anything else that crept in from the latest merge.

So. Assuming the test bot agrees, I think this is RTBC. I reviewed the patch very carefully yesterday, and tested it manually. We need to do this as it's blocking other work. Let's do it now!

And... Maybe if @plach thinks we need to add methods so that Views can better support entity schema updates... I think that would be a separate issue that isn't part of the purpose of this one, because Views never supported it before. This issue is about using the data table instead of the base table as the main table for Views.

Awesome testing @jhodgdon!

Thanks a lot for the really thorough testing on this!

Reading through #25, I did not understand why we keep node around when the only unique thing it offers is a UUID field; the rest of the fields are in node_field_data. One conceivable way to solve this and address Jibran's concerns as well is just to simply move UUID over there and rename the table to node. Then we don't need this patch at all, and the data model is much clearer for new people trying to figure out what's going on.

However, I asked amateescu and dawehener about this. We still need node as single canonical source of serial node IDs, because node_field_data will contain N variations for translations, etc. So given that we still need the table either way...

Committed and pushed to 8.0.x. Thanks!

Unless I am mistaken, this introduced a regression for the default language field. That SHOULD NOT warrant a rollback, the other benefits are important enough. See #2450195: Original language of entities not accessible in views anymore.

This issue have also broke entity references on Views so users are not able to set field data to be read from referenced entity. If someone manages to create View reading referenced data using CMI that will cause that view to throw sql errors.

I thought that this patch was going to only use the data table unless fields were needed on the base table, but apparently that was not part of this patch. See #2458223: Duplicated field handlers in field UI for some base table fields for follow-up.

This commit broke the Taxonomy term: Parent term filter in Views.

To reproduce, create a new Taxonomy term view, try to add a filter (or a contextual filter, or a relationship) on the parent term and you'll see that's not one of your choices anymore. Try in D7, and it's there.

Attached is a small patch which addresses this problem.

Please see #2459777: {taxonomy_index} and {taxonomy_term_hierarchy} doesn't join taxonomy data table in views for #67