Duplicated field handlers in field UI for some base table fields

Its kinda what core/modules/node/src/NodeAccessControlHandler.php:135 was built for. I guess we need to add the node type there as well to check for 'administer nodes'?

Dom: in Drupal 8, views of entities must obey the entity access permissions, so yes they can look different for admin and non-admin users. That is not a regression, that is a security decision.

But I think the node type is a field that should be visible to everyone, not just administrators... Let's see... Hm, it looks like NodeAccessControlHandler::checkFieldAccess is only special-casing Edit operations... so I don't know why regular users would be blocked from View on the Node Type field?

Bumping to normal, but the duplicate naming definitely looks confusing.

By looking at the checkbox HTML code, I determined that one of these is the database field node.type. The other is the database field node_field_data.type.

We also have more duplicates:
- both node.nid and node_field_data.nid showing up as "Content: Node ID".
- both node.vid and node_field_data.vid showing up as "Content: Revision ID".

I thought that on another issue we got rid of the database table fields from the base table, so I'm not sure why this is still there. @dawenher, any idea? Is that issue still open? Did its patch actually get committed? Am I hallucinating?

I just tested this on simplytest.me using the latest -dev...

Talked to @dawehner on IRC about this.

On #2429447: Use data table as views base table, if available., we changed Views to use the entity data table as the base table in Views. I thought we had also removed the duplicate fields there, and made it so the base table isn't joined unless it's actually needed, but apparently we didn't?

I guess this is not something I can work on as per a Novice tag ?

You can certainly try to work on it. The novice tag is just an indicator for issues which are good for getting started.

The code which defines those fields is in core/modules/views/src/EntityViewsData.php:209.
There you would have to exclude the fields on the base table, which also exists on the data table,
not sure how to exactly do that.

We do not have any rules on who can work on which issues! If you would like to work on it, and think you can solve the problem, please do. But this issue is probably a bit complex, so it may not be the best choice if you are new to Drupal 8 development.

Hm, I think this is major.

However, isn't therefore this issue related ? (see todo in code)

Yeah I think its related but not necessarily the same issue.

I'll give that issue a try.

Hm, I think this is major.

It is certainly a user experience improvement, but I don't really see yet, why the initial report claims that one of the two ones works different than the other one.

Retitling to the scope that I think is major.

Yeah, and given it is not just Node that presumably has them...

This removes the duplicates for now, let's see whether this causes some failures.

I guess this is it.

Looking good! A few comments:

a) Suggested rewrite of the comment in EntityViewsData about skipping fields... currently says:

+          // Some fields, like for example the entity ID appear on both the base
+          // table and data table. Given that we want to skip the field on
+          // the bsae table, as otherwise the user has duplicate entries in the
+          // UI.
+          // On top of that we have to special case the langcode, because on the
+          // base table it is the default langcode and on the data table, it is
+          // the langcode of the

My suggestion:

Some fields (for example, Entity ID) appear in both the base table and data table, or in both the revision table and revision data table. With the exception of the langcode field (which is different in the two tables -- base language vs. translation language), these fields have the same values in both tables, corresponding to an entity base field. To avoid confusing duplication, only add them in the data tables.

And just below that:

          if ($field_name !== 'langcode' && $data_table && $table === $base_table && array_search($field_name, $field_names_per_table[$data_table]) !== FALSE) {
+            continue;
+          }
+          if ($field_name !== 'langcode' && $revision_data_table && $table === $revision_table && array_search($field_name, $field_names_per_table[$revision_data_table]) !== FALSE) {
+            continue;
+          }

b) I think you can use in_array instead of array_search -- should be a bit more efficient and it just returns TRUE/FALSE which is what you need here.

c) To me the if() statements would be clearer (and possibly slightly more efficient) if they were reordered and multi-line. I'd go: if ($data_table && table === $base_table && [field name is not langcode and is in the field names array]) rather than the order here, and similar for the other one. This seems slightly more organized. But, not a big deal.

d) A UI test that verifies there aren't duplicates in the Field or Filter or Sort lists would make this patch more convincing. Not sure we need that, but if we don't have that, we need to look at the UI for Node and other entities manually and verify there are not any remaining duplicates to be addressed.

Some fields (for example, Entity ID) appear in both the base table and data table, or in both the revision table and revision data table. With the exception of the langcode field (which is different in the two tables -- base language vs. translation language), these fields have the same values in both tables, corresponding to an entity base field. To avoid confusing duplication, only add them in the data tables.

This indeed sounds way more fluent to read. Thanks a lot!

b) I think you can use in_array instead of array_search -- should be a bit more efficient and it just returns TRUE/FALSE which is what you need here.

Ha, while working on it I stumbled upon the return value of array_search() and wondered why I never had problems with it.

c) To me the if() statements would be clearer (and possibly slightly more efficient) if they were reordered and multi-line. I'd go: if ($data_table && table === $base_table && [field name is not langcode and is in the field names array]) rather than the order here, and similar for the other one. This seems slightly more organized. But, not a big deal.

Fair point! It was more of a organic growing of the conditions.

d) A UI test that verifies there aren't duplicates in the Field or Filter or Sort lists would make this patch more convincing. Not sure we need that, but if we don't have that, we need to look at the UI for Node and other entities manually and verify there are not any remaining duplicates to be addressed.

Sure, let's do that.

Regarding the test, I was thinking about something more systematic. Would it be possible to do something like this, conceptually:

foreach entity in (node, user, taxonomy term, ...) {
  foreach add_type in (fields, filters, sorts, arguments) {
     generate the Add add_type dialog for entitity that you'd get in Views UI, or its list of options at least
     foreach item in list {
       calculate the displayed item title and help that the user would see 
       verify that it doesn't match one previous calculated in this loop
       throw a fail if it does match
     }
   }
}

Just faced that user:id is duplicated too in #2456691-3: User email field need to use Field-Entity-aware formatters in Views

Regarding the test, I was thinking about something more systematic. Would it be possible to do something like this, conceptually:

Well, possible is everything, but the question is whether we want to do that. For me tests are about catching problems ... but these problems will be always catched already by the unit test, which have a way better developer experience due to its faster feedback cycle.

Rerolled ...

Well, I'm not convinced that having that super crazy test coverage of #23 is worth, as explained before.
We do have now unit tests which cover that problem, which allows for a very fast feedback cycle, compared to a slow UI test.

If we had unit tests that covered this problem, why did it happen?

If we had unit tests that covered this problem, why did it happen?

Well, because we introduced the unit tests as part of this issue ...

Tested the patch and it works great.

I just redid a couple extremely minor coding style changes:

- Use same syntax for arrays at least in the same file.
- Use consistent string format (single quotes instead of double quotes).

If you think that is unnecessary (or wrong), then I won't be the one to complain. No offence taken :)

Other than that, it looks good.

I agree with #27. This is ready just a minor confirmation for sanity.

+++ b/core/modules/views_ui/src/Tests/HandlerTest.php
@@ -172,4 +185,18 @@ public function testBrokenHandlers() {
+    $this->assertEqual(1, substr_count($content, 'Content: Type'), 'Content: Type appears just once.');
+    $this->assertEqual(1, substr_count($content, 'Content: Node ID'), 'Content: Node ID appears just once.');

Can we please add a quick assert about uuid because it's the only field which we still get from base table?

Since I already had the file open.

Can we please add a quick assert about uuid because it's the only field which we still get from base table?

Added that, although that were not duplicated before the patch. On the other hand, revision ID was actually duplicated and with no tests. So I added that as well.

+++ b/core/modules/views/src/EntityViewsData.php
@@ -213,8 +213,27 @@ public function getViewsData() {
+        $field_names_per_table[$table] = $table_mapping->getFieldNames($table);
+      }
+
+      foreach ($field_names_per_table as $table => $fields) {
+        foreach ($fields as $field_name) {
+          // Some fields (for example, Entity ID) appear in both the base table
+          // and data table, or in both the revision table and revision data
+          // table. With the exception of the langcode field (which is different
+          // in the two tables -- base language vs. translation language), these
+          // fields have the same values in both tables, corresponding to an
+          // entity base field. To avoid confusing duplication, only add them in
+          // the data tables.
+          if ($data_table && $table === $base_table && $field_name !== 'langcode' && in_array($field_name, $field_names_per_table[$data_table])) {
+            continue;
+          }
+          if ($revision_data_table && $table === $revision_table && $field_name !== 'langcode' && in_array($field_name, $field_names_per_table[$revision_data_table])) {
+            continue;
+          }
+
           $this->mapFieldDefinition($table, $field_name, $field_definitions[$field_name], $table_mapping, $data[$table]);

I think we can use TableMappingInterface::getFieldTableName() for this.

I think we can use TableMappingInterface::getFieldTableName() for this.

Sounds like a good idea! I guess we still have to special case 'langcode', but the rest should be alright!

I think we can use TableMappingInterface::getFieldTableName() for this.

Like this?

Attached also interdiff and test-only patch

Heh, diffing the wrong way around. And setting it to RTBC on top of that. Sorry about that. The interdiff was correct though. Still uploading it here for better clarity :)

Yes, that's idea but TableMappingInterface::getFieldTableName() already wraps all that logic and will return the data table if defined, so I suspect that code can be simplified even more.

Btw, you aren't supposed to RTBC your own patches :)

Here is a new patch. It's a little bit shorter again.
To me it seems we do not need to take care of fields that are in "both the revision table and revision data table" as this is not duplicated in the GUI for me. (screenshot attached without patch applied). So I also removed those assertion changes from the unit tests (and updated the comment about it).

+++ b/core/modules/views/src/EntityViewsData.php
@@ -217,6 +217,16 @@ public function getViewsData() {
       foreach ($table_mapping->getTableNames() as $table) {
...
+          $field_table = $table_mapping->getFieldTableName($field_name);

That honestly is confusing, don't $table_mapping->getFieldNames($table) already ensure the table we are dealing with?

Hm, you are of course right.

However. If we simplify it to only

if ($data_table && $table === $base_table && $field_name !== 'langcode')

Then it will no longer return nid and vid and so on duplicated. But it will also not return other base fields either. Like type and uuid. Actually the last patch would in theory also exclude type if it were not in the data table.

So I am a bit at a loss here. Are you saying we should not use getFieldTableName as suggested in #34?

To me the most "robust" solution is still in #33, but I would love to simplify some more if someone could give me some additional pointers? :)

Isn't the following enough?

if ($table !== $field_table)

You mean replace

if ($data_table && $data_table == $field_table && $table === $base_table && $field_name !== 'langcode') {

with

if ($table !== $field_table) {

?

Because that does not work so well.

Sorry if I misunderstood you.

I think I meant something like this:

    // Load all typed data definitions of all fields. This should cover each of
    // the entity base, revision, data tables.
    $table_mapping = $this->storage->getTableMapping();
    $definitions = $this->entityManager->getBaseFieldDefinitions($this->entityType->id());
    $definitions = array_filter($definitions, [$table_mapping, 'allowsSharedTableStorage']);
    $langcode_key = $this->entityType->getKey('langcode');
    /** @var \Drupal\Core\Field\FieldDefinitionInterface $definition */
    foreach ($definitions as $definition) {
      $field_name = $definition->getName();
      $table = $table_mapping->getFieldTableName($field_name);
      $this->mapFieldDefinition($table, $field_name, $definition, $table_mapping, $data[$table]);
      // TODO ...
      if ($field_name == $langcode_key && $table == $data_table) {
        $this->mapFieldDefinition($base_table, $field_name, $definition, $table_mapping, $data[$base_table]);
      }
    }

Note: This potential has potential, but it seems that the support for revisions will cause more problems.

@eiriksm
So it seems to be that we should basically
$field_table = $table_mapping->getFieldTableName($field_name);
and always use $table instead of $field_table.
In case you are at the sprint, feel free to come over at the sprint at any point.

Not at the sprint, no.

Tried to apply the logic in #48 but that also fails to add the field mapping for all base type fields (for example sticky). That is, if I understood you correctly this time... :)

I will be afk until at least tomorrow, so feel free to take over at any time :)

Status update due to recent comments.

Here is a patch that also adds back the logic for revisions. I changed the unit tests so they don't expect the type to be returned from the base table, but from the data table.

This issue desperately needs an issue summary update. The current summary has very little to do with the current patch.

Updated patch that also should fix the test failures.

Will update the IS now.

+++ b/core/modules/views/src/EntityViewsData.php
@@ -217,6 +217,23 @@ public function getViewsData() {
+          if ($data_table && $table === $revision_table && !in_array($field_name, $base_table_exceptions)) {

UUID is not part of the revision table

mysql> describe node_field_revision;
+------------------+------------------+------+-----+---------+-------+
| Field            | Type             | Null | Key | Default | Extra |
+------------------+------------------+------+-----+---------+-------+
| nid              | int(10) unsigned | NO   |     | NULL    |       |
| vid              | int(10) unsigned | NO   | PRI | NULL    |       |
| langcode         | varchar(12)      | NO   | PRI | NULL    |       |
| title            | varchar(255)     | YES  |     | NULL    |       |
| uid              | int(10) unsigned | YES  | MUL | NULL    |       |
| status           | tinyint(4)       | YES  |     | NULL    |       |
| created          | int(11)          | YES  |     | NULL    |       |
| changed          | int(11)          | YES  |     | NULL    |       |
| promote          | tinyint(4)       | YES  |     | NULL    |       |
| sticky           | tinyint(4)       | YES  |     | NULL    |       |
| default_langcode | tinyint(4)       | NO   | MUL | NULL    |       |
+------------------+------------------+------+-----+---------+-------+
11 rows in set (0.01 sec)

Updated the Issue summary as description about content type and permissions was confusing.

For site-developers who would come to this issue page, I think we should add a message saying "Using either of the displayed fields should be fine and shouldn't affect the functionality as such."

That is not true golunk. Some of those fields are going away.

I meant to post this sooner, but the reminder disappeared in my inbox :)

Removed the check for uuid in revision table

I'm glad to see this making progress again!

I tested this patch using simplytest.me, and have verified that it fixes the problem. Great!

I also took a look through the entire patch, and I had some questions/comments:

1. +++ b/core/modules/block_content/tests/modules/block_content_test_views/test_views/views.view.test_block_content_revision_id.yml
   @@ -31,7 +31,7 @@ display:
              id: id_1
   -          table: block_content_revision
   +          table: block_content_field_data
              field: id
   

   This change to several views doesn't seem correct -- it was previously the ID of the *revision* and now I think it is getting the ID of the custom block instead of the revision ID? Unsure, but changing from block_content_revision to block_content_field_data doesn't seem like the right change?

2. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -217,6 +217,23 @@ public function getViewsData() {
   +          $base_table_exceptions = [
   +            'langcode',
   +            'uuid'
   +          ];
   +          if ($data_table && $table === $base_table && !in_array($field_name, $base_table_exceptions)) {
   

   Coding standards: should be a , after 'uuid' right?

3. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -217,6 +217,23 @@ public function getViewsData() {
   +          if ($data_table && $table === $base_table && !in_array($field_name, $base_table_exceptions)) {
   +            continue;
   +          }
   +          if ($data_table && $table === $revision_table && $field_name != 'langcode') {
   +            continue;
   +          }
              $this->mapFieldDefinition($table, $field_name, $field_definitions[$field_name], $table_mapping, $data[$table]);
   

   I am not a huge fan of the asymmetry here. Why not use in_array() for the revision table too? I guess it is OK as it is...

4. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -527,9 +527,18 @@ public function testDataTableFields() {
            ['entity_test_mul', ['id', 'uuid', 'type', 'langcode']],
   -        ['entity_test_mul_property_data', ['id', 'langcode', 'name', 'description', 'homepage', 'user_id']],
   +        ['entity_test_mul_property_data', ['id', 'type', 'langcode', 'name', 'description', 'homepage', 'user_id']],
   

   How come 'type' and 'id' don't have to be removed from entity_test_mul? I think the fact that they're in both the base table and the data table is precisely what this patch is supposed to be fixing? This makes me not trust the test, since it passed apparently.

5. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -527,9 +527,18 @@ public function testDataTableFields() {
   +    $table_mapping->expects($this->any())
   +      ->method('getFieldTableName')
   +      ->willReturnCallback(function($field) {
   +        if ($field == 'id') {
   +          return 'entity_test_mul_property_data';
   +        }
   +        return 'entity_test_mul';
   +      });
   +
   

   This looks wrong to me. Aren't most of the fields supposed to be on the property_data table? looks like it's just looking for 'id' there? If this passes, I think we have a problem.

6. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -547,13 +556,10 @@ public function testDataTableFields() {
   -    $this->assertNumericField($data['entity_test_mul']['id']);
   -    $this->assertField($data['entity_test_mul']['id'], 'id');
   +    $this->assertFalse(isset($data['entity_test_mul']['id']));
        $this->assertUuidField($data['entity_test_mul']['uuid']);
        $this->assertField($data['entity_test_mul']['uuid'], 'uuid');
    
   -    $this->assertBundleField($data['entity_test_mul']['type']);
   -    $this->assertField($data['entity_test_mul']['type'], 'type');
        $this->assertFalse(isset($data['entity_test_mul']['type']['relationship']));
   

   In the first bit here, it looks good -- we take out the tests asserting the 'id' field is on the base table, and add an assertFalse instead.

   Maybe we should also add an assertFalse to the second one too, to assert that the 'type' field doesn't exist on entity_test_mul?

7. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -645,6 +654,15 @@ public function testRevisionTableFields() {
   +    $table_mapping->expects($this->any())
   +      ->method('getFieldTableName')
   +      ->willReturnCallback(function($field) {
   +        if ($field == 'id' || $field == 'revision_id') {
   +          return 'entity_test_mulrev_property_data';
   +        }
   +        return 'entity_test_mulrev';
   +      });
   +
   

   Again I don't see how this can be right.

8. Also just above there are arrays of which fields are expected to be on the revision and revision field data tables, and these were not patched?

9. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -654,14 +672,10 @@ public function testRevisionTableFields() {
   -    $this->assertNumericField($data['entity_test_mulrev']['id']);
   -    $this->assertField($data['entity_test_mulrev']['id'], 'id');
   -    $this->assertNumericField($data['entity_test_mulrev']['revision_id']);
   -    $this->assertField($data['entity_test_mulrev']['revision_id'], 'revision_id');
   +    $this->assertFalse(isset($data['entity_test_mulrev']['id']));
   +    $this->assertFalse(isset($data['entity_test_mulrev']['revision_id']));
        $this->assertUuidField($data['entity_test_mulrev']['uuid']);
        $this->assertField($data['entity_test_mulrev']['uuid'], 'uuid');
   -    $this->assertStringField($data['entity_test_mulrev']['type']);
   -    $this->assertField($data['entity_test_mulrev']['type'], 'type');
    
   

   Again should we add an assertFalse for the 'type' field here?

10. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
    @@ -672,11 +686,9 @@ public function testRevisionTableFields() {
    -    // Check the revision fields.
    -    $this->assertNumericField($data['entity_test_mulrev_revision']['id']);
    -    $this->assertField($data['entity_test_mulrev_revision']['id'], 'id');
    -    $this->assertNumericField($data['entity_test_mulrev_revision']['revision_id']);
    -    $this->assertField($data['entity_test_mulrev_revision']['revision_id'], 'revision_id');
    +    // Check the revision fields. The revision id should only appear in the data
    +    // table.
    +    $this->assertFalse(isset($data['entity_test_mulrev_revision']['revision_id']));
     
    

    and assertFalse() on the 'id' field?

Updated the patch.

This change to several views doesn't seem correct -- it was previously the ID of the *revision* and now I think it is getting the ID of the custom block instead of the revision ID? Unsure, but changing from block_content_revision to block_content_field_data doesn't seem like the right change?

The ID of the revision of stored in the 'revision_id' column, so this is not the problem, but I agree we should change just the lines we actually have to fix.

Coding standards: should be a , after 'uuid' right?

Yeah

I am not a huge fan of the asymmetry here. Why not use in_array() for the revision table too? I guess it is OK as it is...

I like that argument :)
Yeah that code doesn't run often, no need to micro-optimize

How come 'type' and 'id' don't have to be removed from entity_test_mul? I think the fact that they're in both the base table and the data table is precisely what this patch is supposed to be fixing? This makes me not trust the test, since it passed apparently.

Well, the thing is that this test tests ONE of many possible implementation of the table data ... this one is storing the type as part of the property_data table, I mean there is no requirement that has to be that way.
EVD should not depend on that internal detail ... the table mapping defines where those fields are stored, we just ensure that its not appearing twice.

But look at the code in item #4 that I was referring to in "don't type and id have to be removed"....

+++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
@@ -527,9 +527,18 @@ public function testDataTableFields() {
         ['entity_test_mul', ['id', 'uuid', 'type', 'langcode']],
-        ['entity_test_mul_property_data', ['id', 'langcode', 'name', 'description', 'homepage', 'user_id']],
+        ['entity_test_mul_property_data', ['id', 'type', 'langcode', 'name', 'description', 'homepage', 'user_id']],

It looks like type and id are in both the tables. Why is that not wrong?

I really do think this patch has problems, as per my previous reviews... no one has answered #67 yet and explained why this is not wrong. I don't trust the tests...

It looks like type and id are in both the tables. Why is that not wrong?

As written before, there is no reason for us here to mock the exact same behaviour of core. The views integration was not written against THE way we store those fields,
but rather against the interface. The interface doesn't disallow you to have the same fields in multiple tables.

Maybe the next point is more convincing, the node type is actually in both tables:

mysql> describe node;
+----------+------------------+------+-----+---------+----------------+
| Field    | Type             | Null | Key | Default | Extra          |
+----------+------------------+------+-----+---------+----------------+
| nid      | int(10) unsigned | NO   | PRI | NULL    | auto_increment |
| vid      | int(10) unsigned | YES  | UNI | NULL    |                |
| type     | varchar(32)      | NO   | MUL | NULL    |                |
| uuid     | varchar(128)     | NO   | UNI | NULL    |                |
describe node_field_data;
| langcode | varchar(12)      | NO   |     | NULL    |                |
+----------+------------------+------+-----+---------+----------------+
5 rows in set (0.00 sec)

mysql> describe node_field_data;
+-------------------------------+------------------+------+-----+---------+-------+
| Field                         | Type             | Null | Key | Default | Extra |
+-------------------------------+------------------+------+-----+---------+-------+
| nid                           | int(10) unsigned | NO   | PRI | NULL    |       |
| vid                           | int(10) unsigned | NO   | MUL | NULL    |       |
| type                          | varchar(32)      | NO   | MUL | NULL    |       |
| langcode                      | varchar(12)      | NO   | PRI | NULL    |       |
| title                         | varchar(255)     | NO   | MUL | NULL    |       |
| uid                           | int(10) unsigned | NO   | MUL | NULL    |       |
| status                        | tinyint(4)       | NO   | MUL | NULL    |       |
| created                       | int(11)          | NO   | MUL | NULL    |       |
| changed                       | int(11)          | NO   | MUL | NULL    |       |
| promote                       | tinyint(4)       | NO   | MUL | NULL    |       |
| sticky                        | tinyint(4)       | NO   |     | NULL    |       |
| revision_translation_affected | tinyint(4)       | YES  |     | NULL    |       |
| default_langcode              | tinyint(4)       | NO   |     | NULL    |       |
+-------------------------------+------------------+------+-----+---------+-------+
13 rows in set (0.00 sec)

We're going to need a hook_update_N() for this right?

Absolutely, people might have used the wrong field.

Regarding those test classes, I was under the impression that they were using the EntityViewsData classes. If they aren't, then I take back everything I said. :)

And if they are not using the base EntityViewsData classes, then I don't know why they are in this patch?

They are using this class

Humph. So I must have been looking at the interdiff or an earlier patch rather than the final patch here and gotten confused about this. Sorry for the noise.

So I think this is ready to go, except that it definitely needs an update function, and a test for the update function.

The API change here for exported views feels worth doing to minimise confusion up until RC (assuming we have an upgrade path).

Tagging RC target since I don't see the patch getting in as-is after release candidate, we'd have to provide a BC layer for existing views - but could hopefully still not show the options in the UI or allow them to be configured for new Views.

OK, this is Needs Work because there are no update functions in the patch.

Also tagging Usability, because the UI is really really really confusing at this point.

I started the upgrade path some weeks ago, will take another stab at it at the sprints today.

First pass at an update function. Works for me at least. I saw earlier someone mentioned a test for the update function. Is there any good examples of this, if this is needed?

Here is a start for an update test extending UpdatePathTestBase.

This is going to fail.

+++ b/core/modules/views/src/Tests/Update/FieldHandlersUpdateTest.php
@@ -0,0 +1,64 @@
+    // Get the expected base table name for the entity.
+    $test_entity = "";

Instead of this block of code that extracts the base table, the base table can probably just be hardcoded.

The test view can be simplified.

Tagging rc deadline. Not sure what we can do about this after rc if it's not in before.

The name of the view in the update test was wrong.
I get an error in the test locally when calling: $view->get('display').

Looking into those fails.

reroll

Should the test view needs to start with node as the base table in order to properly test the upgrade path?

Should the test view needs to start with node as the base table in order to properly test the upgrade path?

No, the _field_ should use the node table, which it does. The problem is that you can both add fields for the nid field in the base table and in the data table. And we want to convert fields that are added with the base table as a table, to using the data table as a table.

Although, pretty sure this will prove the point as well. But it is less correct IMO :)

No, the _field_ should use the node table, which it does.

Oops, I agree. Reverting that last change.

This is looking pretty good, thanks for all the work!

However, there are several problems in this patch that definitely need to be addressed:

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -217,6 +217,12 @@ public function getViewsData() {
          // @todo https://www.drupal.org/node/2337511
          foreach ($table_mapping->getTableNames() as $table) {
            foreach ($table_mapping->getFieldNames($table) as $field_name) {
   +          // Some fields (for example, Entity ID) appear in both the base table
   +          // and data table. With the exception of the uuid field. To avoid
   +          // confusing duplication, only add them in the data tables.
   +          if ($data_table && ($table === $base_table || $table === $revision_table) && $field_name != 'uuid') {
   +            continue;
   +          }
   

   It seems to me that the logic should be something like:

   If we are doing the base table or revision table, and the field exists in both the table I'm doing and the corresponding data table (data table for base, revision data table for revision), then skip this field.

   But the logic here is instead:

   If we are doing the base table or revision table, and there is a data table defined, and the field is not 'uuid', then skip it.

   This seems wrong to me. Can we fix it to actually do the right logic?

   By the way, I'm not actually sure that if there is a data table and a revision table, there is necessarily also a revision data table... is that always true?

2. in views.install:
   

    /**
   + * Verify that no fields of the previous duplicated types is used in views.
   + */
   +function views_update_8003(&$sandbox) {
   

   This comment needs a bit of adjustment. How about:

   Update views that use previously-duplicated fields that have been removed.

3. In that same update function, it looks like the code is taking something that was previously in the revision table and moving it to the data table:
   

   +            if (($table === $base_table || $table === $revision_table) && $field_name != 'uuid') {
   +              if (in_array($field['field'], $update_exceptions)) {
   +                continue;
   +              }
   +              $field['table'] = $data_table;
   

   This is wrong. It needs to move it to the revision data table instead, as you can see in some of the YML files in the patch, such as:

   --- a/core/modules/block_content/tests/modules/block_content_test_views/test_views/views.view.test_block_content_revision_id.yml
   +++ b/core/modules/block_content/tests/modules/block_content_test_views/test_views/views.view.test_block_content_revision_id.yml
   @@ -31,7 +31,7 @@ display:
              entity_field: revision_id
            id_1:
              id: id_1
   -          table: block_content_revision
   +          table: block_content_field_revision
    

   It would probably be illuminating to run the update function here on all the core Views and see what it does... right now it would not produce this output, as it would move block_content_revision to be the data table, which doesn't have a revision_id field and this would be broken.

4. And we apparently need an update test for the revision data table stuff, because the existing tests are not catching this problem. They are only testing one field being updated in a base table, not a revision table.

1.

and the field exists in both the table I'm doing and the corresponding data table

vs.

and there is a data table defined

I think what you mean is that just because the entity has a data table, the field doesn't necessarily have a data table. So we need to check if the field has a data table? Possibly, this can section can be refactored in this other issue? #2337511: EntityViewsData: "@todo We should better just rely on information coming from the entity storage."

Ah, I see what you mean. Disregard comment #102, item 1... however, I think if $table is the revision table, we should only skip the field if the revision data table exists, right?

Also, one other thing. The langcode field is actually different between the base table and the field table -- it has a different meaning. So we need to have 'langcode' in all 4 tables. It is not actually a duplicate... We had that in a patch around #62, but it apparently got lost. The tests that removed 'langcode' from various stuff probably shouldn't have either.

I think it's always true that if there is a data table and a revision table, that there is also a revision data table. At least it's supposed to be that way unless something gets messed up. But since we setting $revision_data_table, it wouldn't hurt to check it. So, what you want is the following?:

<?php
          if (($data_table && $table === $base_table) || ($revision_data_table && $table === $revision_table) && $field_name != 'uuid') {
?>

For 3,4, it's probably better to write the test first. I think the steps are: import the test view into a site. Then add a relationship to revisions and add a field from the revisions table. Export the view to replace the test view. Assert that the field gets updated to the revision data table.

RE #105, right, but also we need to make an exception for 'langcode'.

I was going to do #106, but revision table fields are not duplicated in the UI and I don't know how to create a view (using the UI) that has a field using the node_revision table. Somehow, the test_block_content_revision_id test does have one though.

If you look at the screen shot in the issue summary, it sure looks to me as though revision fields are duplicated in the UI just like base fields... oh wait, that is Revision ID but it's a field on the base/data table, right. So, let's see. You're right! The revision fields are not duplicated in the UI. Interesting. I wonder why that is?

Hm... So I looked at the Views UI for adding a new field when the base table on a view is node revisions, and it looks like the fields from node_revision are not present at all. Only the fields from node_field_revision are present.

Is that a bug?

Thanks all for the really important work on this issue!!

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -217,6 +217,12 @@ public function getViewsData() {
   +          // Some fields (for example, Entity ID) appear in both the base table
   +          // and data table. With the exception of the uuid field. To avoid
   +          // confusing duplication, only add them in the data tables.
   +          if ($data_table && ($table === $base_table || $table === $revision_table) && $field_name != 'uuid') {
   +            continue;
   +          }
   

   I'm curious how the entity system decides which fields are just in the base table. Do we have that hardcoded to be just the UUID? Otherwise we might need some more complex logic.

2. +++ b/core/modules/views/views.install
   @@ -311,5 +311,80 @@ function _views_update_argument_map($displays) {
   +  // Some special fields are not relevant here.
   +  $update_exceptions = [
   +    'user_bulk_form',
   +    'operations',
   +    'node_bulk_form',
   +  ];
   

   Urgs, that feels fragile. What would happen if any custom / contrib code adds new fields to the base table as well? What about using the views data to always ensure that the destination field also exists in the views data?

Hm. Well for one thing, there are the entity keys, like 'id' and 'uuid' and 'bundle', but any entity can assign these to be different database table names in the entity annotation (for instance, Node uses "type" not "bundle"). So the database table field name might not be 'uuid' for all entities.

So to get more dteails, I looked at SqlContentEntityStorageSchema::getEntitySchema(), which is what sets up the entity schema tables.

This calls EntityManager::getFieldStorageDefinitions [although note that technically any entity type can use its own manager instead of the default EntityManager and could override this method! In practice, in Core there are no overrides.]. This method:

a) First finds the base field definitions, which (again in the default manager) come from the baseFieldDefinitions() method on the entity type, plus any that are provided by hook_entity_base_field_info(), and then lets modules alter them via hook_entity_base_field_info_alter().

b) Then calls buildFieldStorageDefinitions(), which (again in the default entity manager) calls hook_entity_field_storage_info() to get storage definitions for any of the fields added by the hooks in (a), and then lets modules alter the storage definitions via hook_entity_base_field_info_alter().

So back to SqlContentEntityStorageSchema::getEntitySchema(), the next thing it does is to call the method getTableMapping() on the SqlContentEntityStorage class that is injected... let's see what SqlContentEntityStorage::getTableMapping() does:

a) Calls EntityManager::getFieldStorageDefinitions() to get the field storage definitions (see above). Filters this down to only non-multiple base fields that don't have custom storage defined in their definitions.

b) From this list, makes a list of all fields, key fields (defined as the entity keys for ID, revision, bundle, uuid, and language code), and revisionable fields (those marked as revisionable by the entity in its baseFieldDefinitions method presumably and/or altered to be such). Also makes a list of "revision metadata fields", which are hard-wired to be revision_timestamp, revision_uid, revision_log (if they exist on the entity).

c) Then does some logic:
- If the entity has no revisions and is not translatable, all fields go on the base table.
- If it is revisionable but not translatable, all fields except revision metadata fields go on the base table, and the revision table gets the revisionable fields plus ID and revision ID. [Hopefully the revision meta-data fields are marked as revisionable!! Otherwise they might not get added. This is most likely a logic bug in this function I think, because the revisionable/translatable case below explicitly adds the metadata fields and this case doesn't.]
- If it is translatable but not revisionable, the base table gets the key fields and the data table gets everything but UUID.
- If it is revisionable and translatable, the base table gets the key fields; the data table gets everything but UUID and the revision metadata fields; the revision table gets ID, revision ID, language code, and revision metadata fields; the revision data table gets ID, revision ID, language code, plus all revisionalbe fields except revision meta data fields.

So back to SqlContentEntityStorageSchema::getEntitySchema(), it takes this mapping and finds the column names defined for each field from the field type definitions, and adds them to the table schema for each table.

So. In short: no, we cannot assume anything. However, Views is already making some assumptions about storage I think, so assuming something here may not be much worse. But we should definitely NOT assume that the database column names are specifically uuid or langcode, because entities can easily override this in their entity key meta-data.

Hope this helps...

OK. That was kind of a long comment. Here is what I think we need to do:

a) For sure, don't assume that 'uuid' is the actual database field name. We should at least get this from the entity definition's entity keys.

b) We don't have to worry about the "not translatable" cases in my previous comment because in that case we don't have a data table or a revision data table, and there is no duplication.

c) It looks like only the "key" fields can be duplicated between base table and data table. These are defined as the entity keys for ID, revision ID, bundle, uuid, and language code. However, UUID is never on the data table, and we don't want to remove langcode because it has a different meaning on the two tables. So really all we need to remove is the base table fields for ID, revision ID, bundle. Those are all that can be duplicated between the two tables.

d) Between revision and revision data, it looks like ID and revision ID can be duplicated (plus language code, but this has different meanings and needs to be in both). I still do not understand why we are not getting this duplication in the views UI.

c) It looks like only the "key" fields can be duplicated between base table and data table. These are defined as the entity keys for ID, revision ID, bundle, uuid, and language code. However, UUID is never on the data table, and we don't want to remove langcode because it has a different meaning on the two tables. So really all we need to remove is the base table fields for ID, revision ID, bundle. Those are all that can be duplicated between the two tables.

This sounds really promising! Great research!

d) Between revision and revision data, it looks like ID and revision ID can be duplicated (plus language code, but this has different meanings and needs to be in both). I still do not understand why we are not getting this duplication in the views UI.

I think the trick is that the implicit (automatic) join from {node_revision} to anything is missing.

>>> \Drupal\views\Views::viewsData()->get('node_revision')['table']
=> [
     "group" => Drupal\Core\StringTranslation\TranslatableString {#2635}
     "provider" => "node"
     "entity type" => "node"
   ]

vs.

>>> \Drupal\views\Views::viewsData()->get('node_field_revision')['table']
=> [
     "entity revision" => true
     "base" => [
       "field" => "vid"
       "title" => Drupal\Core\StringTranslation\TranslatableString {#2649}
       "help" => Drupal\Core\StringTranslation\TranslatableString {#2653}
       "defaults" => [
         "title" => "title"
       ]
     ]
     "join" => [
       "node_field_data" => [
         "left_field" => "vid"
         "field" => "vid"
         "type" => "INNER"
       ]
       "node_revision" => [
         "left_field" => "vid"
         "field" => "vid"
         "type" => "INNER"
       ]
     ]
     "group" => Drupal\Core\StringTranslation\TranslatableString {#2650}
     "entity type" => "node"
     "wizard_id" => "node_field_revision"
     "provider" => "views"
   ]

see

      // Join the revision table to the base table.
      $data[$views_revision_base_table]['table']['join'][$views_base_table] = array(
        'left_field' => $revision_field,
        'field' => $revision_field,
        'type' => 'INNER',
      );

for the code which adds the implicit join from 'node_field_revision' to 'node_field_data'.

Hm, that seems wrong then. I think that if we are making a view of revisions, we need the base table to be the revision field data table (just like if the view is of entities, the base table is the field data table), and we also need the automatic join to the revision table to be there, so that we can reference the language code field if nothing else (which again is NOT the same between revision data and revision tables).

Lots of info in these last few comments, is this patch heading in the right direction?

woops

The patch to EntityViewsData looks like the right thing to do, yes.

now it should be ok.

Aside from the tests failing... ;)

+++ b/core/modules/views/src/EntityViewsData.php
@@ -217,10 +221,9 @@ public function getViewsData() {
+          // To avoid confusing duplication, only add duplicate fields the data
+          // tables.

reminder for next patch: missing a word here.

Its great that you all work on this!

Fails in Views_ui.Drupal\views_ui\Tests\HandlerTest were related to #1832862: Make views add field scannable.
Haven't figured out why Drupal\Tests\views\Unit\EntityViewsDataTest is failing yet (it's not failing locally)

This should fix it

This is looking pretty good! I reviewed the code carefully and I have 1 big/important thing, and a bunch of docs nipticks to report.

Also I tested it using simplytest.me and it does fix the field duplication problem. However, filters, sorts, etc. are still duplicated (see below).

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -217,6 +221,11 @@ public function getViewsData() {
   +          // To avoid confusing duplication, only add duplicate fields to the
   +          // data tables.
   

   This comment was confusing to me when I read it.

   How about:

   To avoid confusing duplication in the user interface, for fields that are on both base and data tables, only add them on the data table (same for revision vs. revision data).

2. +++ b/core/modules/views/src/Tests/Update/FieldHandlersUpdateTest.php
   @@ -0,0 +1,46 @@
   +    $this->assertEqual('node_field_data', $data['display']['default']['display_options']['fields']['nid']['table']);
   

   So... We're only testing updates on fields. Do we also need to update and/or test filters and sorts? They have the same problems, I think? [see below for more]

3. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -672,17 +691,12 @@ public function testRevisionTableFields() {
   +    // Check the revision fields. The revision id should only appear in the data
   

   id => ID [in text, "id" is like ego, superego, id; ID means identifier]

4. +++ b/core/modules/views/views.install
   @@ -311,5 +311,73 @@ function _views_update_argument_map($displays) {
   + * Verify that no fields of the previous duplicated types is used in views.
   

   is => are

   But actually, this is not *verifying*. It is *updating*.

   This text will appear in the UI when someone runs update.php, so it should be clear.

   How about:

   Update some views fields that were previously duplicated.

5. +++ b/core/modules/views/views.install
   @@ -311,5 +311,73 @@ function _views_update_argument_map($displays) {
   +        if (!empty($display['display_options']['fields'])) {
   +          foreach ($display['display_options']['fields'] as $field_name => &$field) {
   

   Yeah, so here it looks like we are only updating fields.

   I think we also need to update filters, sorts, arguments, relationships... anything else (@dawehner???)?

   And we should have tests for the UI for all of those, and tests for the update of all of them. Right?

Thanks for the review. I'll work on this later today.

fwiw considering how bad the UX issue is, we've discussed this between the core committers and decided it is probably OK to land during RC without a backwards compatibility layer for exported views.

This might break some exported views, but should be a simple update. If it turned out not to be, the main change and upgrade path here is still important to do.

1. Another option, but don't care that much (it is just a comment):
Avoid duplicate fields in the user interface. Only add the field from the data table.

edit: I meant for the text in the comment in #132.1, but NM.

RE #135, That is an interesting idea, but how would you even do that? That would require a huge overhaul of Views UI, which knows nothing about what the tables are. It just figures out all available fields from the tables involved in the view (plus automatic joined tables) and displays them. It doesn't know what "entities" are, much less base tables vs. data tables, etc.

1. Fixed with your suggestion
2. Extended the view a bit to include these duplicate fields in sorts/filters/arguments, couldn't find any other handlers that have duplicates (relationships didn't have any as far as I could see).
3. Fixed
4. Went with your suggestion
5. Didn't fix this yet, will do so in my next patch

So this patch should demonstrate the problem @jhodgdon describes in #132.5

And this fixes #132.5

Good. I think we also need to add to the UI test (in core/modules/views_ui/src/Tests/HandlerTest.php ) so that we also verify that the Fields, Sorts, etc. UIs don't have duplicates?

This covers that.

Great! We may need to add other handler types... not sure. Maybe 'relationship' as well? @dawehner, any thoughts there?

Well, I doubt that relationship really makes sense, given that those fields should not have a relationship anyway right? For now the test coverage tests the problem the issue is describing.

Thank you @geertvd to stick with that.

+++ b/core/modules/block_content/tests/modules/block_content_test_views/test_views/views.view.test_block_content_revision_id.yml
@@ -31,7 +31,7 @@ display:
-          table: block_content_revision
+          table: block_content_field_revision

+++ b/core/modules/block_content/tests/modules/block_content_test_views/test_views/views.view.test_block_content_revision_revision_id.yml
@@ -33,7 +33,7 @@ display:
-          table: block_content_revision
+          table: block_content_field_revision

I might be missing something but I don't see how the update could ever make this type of fix.

I think we need to add a test view to test updating a table which references the wrong revision table.

Is it even possible to create a view that mistakenly uses the node_revision table?

It's possible to use the node revision tables... In the UI if you create a Node view, add go to add fields, you'll be given several options from node revision tables. It's auto-joined to Node.

There is also a NodeRevision wizard class, so you should be able to create one in the UI with base table being node revisions. I guess we've probably made that be node_revision_field_data as the base table by now, but presumably the base node revision table is auto-joined? Maybe not. If not that is probably a bug... actually I vaguely remember mentioning that in one of the previous 147 comments on this issue.

Not sure if that is exactly what you are asking... Anyway the Node module provides table/field data for this table, so in principle if you did it programmatically you could definitely create a view on this table.

+++ b/core/modules/views/views.install
@@ -311,5 +311,76 @@ function _views_update_argument_map($displays) {
+          if (!empty($display['display_options'][$handler_type['plural']])) {
+            foreach ($display['display_options'][$handler_type['plural']] as $field_name => &$field) {
+              $table = $field['table'];
+              if (($table === $base_table || $table === $revision_table) && in_array($field_name, $duplicate_fields)) {
+                $field['table'] = $data_table;
+                $changed = TRUE;
+              }

So yeah I think it would be better safe and migrate revision information as well, than sorry

Tagging this as minor version target because if it's not fixed during RC we'd have to do it in 8.1.

Would of course still be great to fix during RC.

OK, seems the latest patch actually breaks quite a few different things. Since it is a good point made in #146 that we do not test why we actually have to do this change, I decided to try to replicate the view attached in the block test. It is not possible anymore with this patch attached. Also, the interface bugs out again in a different way with this patch applied.

Attached are the popup for "add relationship" for a "Custom block revision" view (before and after).

Have not tried to find out why this is yet, but just wanted to post my findings before I go home from work :)

You said "the interface bugs out" and "this patch breaks things"... ??!?

Heh, sorry about that. That was not very elaborate. And on top of that, I named the screenshots wrong (so that no_patch is really patch_applied and so on). As an excuse, I was in a hurry, and coincidentally I am not at that particular computer for the next week, so i felt had to post it quickly before I left. Anyway, enough about me :)

What I was trying to say:

Reading through comment #146 I decided to try to create one of the views touched in the latest patch (#142) manually through clicking in the interface. The particular view was a view of "Custom block revisions".

So first I did this with no patch applied (clean checkout of 8.0.x branch). Everything seems fine. Among the things I do is add a relationship (the view I wanted to "mimic" had this). Precise steps to reproduce:
- Go to admin/structure/views
- Add new view
- Select custom block revisions.
- Click "add relationship".
This is a screenshot from when I clicked "Add relationship":

That screenshot also seems fine.

Now, following this logic, I wanted to create the same view with the patch applied, and then just export the view, and inspect the differences. Among the things I need to do (again) is click "Add relationship". This is a screenshot from that:

So to reiterate: The screenshot above depicts a buggy interface, even though I applied the patch in #142.

So what I meant with "the interface bugging out in a different way" was this:

1. Yes, with the patch applied it no longer has the bug with duplicated node ids (and so on, as reported).
2. With the patch applied it suddenly has other bug(s) (duplicated relationship fields being at least one of them).

So what I meant with "this patch breaks things" was point 2 above.

So what I am trying to say is that the patch in #142 (that we previously RTBCed) indeed needs work (and not only extra tests).

Also, I have not had the time to dig into why this happens yet, just thought it might be helpful to post while it was still fresh in my memory.

Also, thanks for pointing out what an unhelpful comment my last comment was @jhodgdon :)

The screenshot in #153 doesn't show a duplicate block. The problem I see in that screenshot is that the Description says "Error:missing help." Is the patch causing that? We should confirm that.

Yeah, it looks like with/without the patch, the Relationship list is the same in both cases, with two fields having identical Names but different Help; but with the patch, the help is labeled as "missing".

That's odd... just to be sure did you apply the patch to an already-running site? If so might need to clear a cache or rebuild the something or other, just to make sure.

Let's see. No, you're right; I made a clean site on simplytest.me with the patch installed and got the same "Error: missing help" message, which does seem to be introduced by this patch.

This happens when the views data doesn't have a 'help' component on (in this case) the relationship.

I will be on a local sprint tomorrow, so I would have time to work on this, unless @eiriksm wanted to pick this up.

Go for it :)

I won't have the time needed in the next few days anyway.

Regarding #153, I think that's related to #2410275: EntityViewsData should add relationships to revision tables so I just had to make some changes BlockContentViewsData::EntityViewsData() for that, which can be removed once #2410275 is fixed.
We might need extra test coverage for this part.

For #146 I'm not actually sure if we need to make that change there, since IDs and revision IDs don't actually have duplicate handlers if used with a revision table. Running \Drupal\views_ui\Tests\HandlerTest::testNoDuplicateFields() without the patch applied should actually demonstrates that.
I'm totally not sure if that's the right thing to do so I'd love to hear someone else's opinion on that.

The update_hook was failing when it encountered a view that has a revision table as base table, since it couldn't find the entity type matching it's base table. For now I fixed this with a simple isset().

For #146 I'm not actually sure if we need to make that change there, since IDs and revision IDs don't actually have duplicate handlers if used with a revision table. Running \Drupal\views_ui\Tests\HandlerTest::testNoDuplicateFields() without the patch applied should actually demonstrates that.
I'm totally not sure if that's the right thing to do so I'd love to hear someone else's opinion on that.

Disregard this for now, the handlers mentioned don't actually have a revision table as base table. I'm going to work some more on this tomorrow.

edit: wrong ticket

Not a lot of progress today, won't be able to work on this the next couple of days.

+++ b/core/modules/block_content/src/BlockContentViewsData.php
@@ -36,25 +36,24 @@ public function getViewsData() {
-    $data['block_content_revision']['table']['base']['help'] = $this->t('Block Content revision is a history of changes to block content.');
-    $data['block_content_revision']['table']['base']['defaults']['title'] = 'info';
+    $data['block_content_field_revision']['table']['base']['help'] = $this->t('Block Content revision is a history of changes to block content.');
+    $data['block_content_field_revision']['table']['base']['defaults']['title'] = 'info';

What if we include both relationships?

Or, maybe we just need to update the tests to expect: block_content_block_content_revision_id ? Actually, it's the expected result that has block_content_block_content_revision_id; the actual result is missing that column.

Reroll + Possible solution to failing tests.

For the reroll, I needed to increment the update function to views_update_8004(). Actually, the reroll change shows up in the interdiff.

Rerolled again.