Provide a relationship from the revision table to the main table

Thanks for working on this. This is a huge problem for contrib so it's at least major.

+++ b/core/modules/views/src/ViewsData.php
@@ -125,12 +125,12 @@ public function __construct(CacheBackendInterface $cache_backend, ConfigFactoryI
+      if (TRUE || !isset($this->storage[$key])) {
...
+        if (FALSE && $data = $this->cacheGet($cid)) {

This can't be correct.

@alexpott, @effulgentsia, @webchick, and I discussed this and agreed it makes sense for this to be marked major. We all thought it was more of a task though.

Finally, I think this is best targeted against 8.1.x. I think we need an upgrade path?

I think we need an empty update function to clear cache - unless already have an 8.1.x update function in views which there's not... looks like views_update_8003() and views_update_8004() are going to be the same :)

+++ b/core/modules/views/src/EntityViewsData.php
@@ -233,6 +234,23 @@ public function getViewsData() {
+      $data[$views_revision_base_table][$this->entityType->getKey('id')]['relationship'] = [
...
+        'base field' => $this->entityType->getKey('id'),
...
+      $data[$views_revision_base_table][$this->entityType->getKey('revision')]['relationship'] = [
...
+        'base field' => $this->entityType->getKey('revision'),

Maybe replace $this->entityType->getKey('revision') and $this->entityType->getKey('id') with $revision_field and $base_field, these variables are already set earlier in the method anyway and would make this a little easier to read.

+++ b/core/modules/views/src/EntityViewsData.php
@@ -233,6 +234,23 @@ public function getViewsData() {
+        'label' => $this->t('Get the actual @label from a @label revision', ['@label' => $entity_type_label]),

This looks more like a help text, and with the 'Get' look a bit weird as a label and could probably be shortened to just 'Content' since everything from the revision is marked 'Content revision'. See screenshots for how this looks now.

No help makes it unclear what the difference between these two thing is:

The label looks weird and way to long:

Manually tested the patch and I got:
Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'langcode' in field list is ambiguous: SELECT node_field_data_node_field_revision.langcode AS node_field_data_node_field_revision_langcode, node_field_revision.vid AS vid, node_field_data_node_field_revision.nid AS node_field_data_node_field_revision_nid, langcode AS langcode FROM {node_field_revision} node_field_revision LEFT JOIN {node_field_data} node_field_data_node_field_revision ON node_field_revision.nid = node_field_data_node_field_revision.nid WHERE (( (node_field_revision.status = :db_condition_placeholder_0) )) LIMIT 11 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 )

Didn't dig any further yet to find out if this also happens without this patch applied with the original relationships.

Attached an export with a failing View. Basic steps to reproduce, multiple languages enabled, View showing node revisions, add relationship to content -> fail. But....this is just as broken without the patch, so no new fails get introduced and maybe we should open a separate issue for this.

But to the patch:
The new label is definitely shorter then it was, but this might be a bit too short :)

The 'add' dialog is still a bit ambiguous (but definitely better then it was). Any way to to make the difference between these two options clearer?

@Lendude: I created #2714717: Content (nid) relationship joins to wrong nodes, which may capture the bug you refer to creating a separate issue for in #18.

We're using this patch against 8.3.x if anyones interested.

I added a simple example of this error here #2446681-18: Error "Column 'langcode' in field list is ambiguous" thrown due to TranslationLanguageRenderer not rendering a field from a relationship. Perhaps in the wrong issue?

[EDIT] Patch #22 does not fix the error.

Reroll against 8.4.x.

I have a bunch of views that were affected by The 'content_revision_tracker' table from Content Moderation has been removed. This patch is now essential for displaying any non-revisionable field on a custom entity with the revision table as the base.

I do agree with #18 however, the label is a little confusing in the list.

I botched the reroll. Fixing.

+++ b/core/modules/views/src/EntityViewsData.php
@@ -243,6 +247,22 @@ public function getViewsData() {
+      $data[$views_revision_base_table][$entity_revision_key]['relationship'] = [
+        'id' => 'standard',
+        'base' => $views_base_table,
+        'base field' => $entity_revision_key,
+        'title' => $this->t('@label revision', ['@label' => $entity_type_label]),
+        'help' => $this->t('Get the actual @label from a @label revision', ['@label' => $entity_type_label]),
+      ];

Can I ask what this actually does? Why would we need a relationship from the revision table to the base table based on the revision ID? Does that mean the relationship would fail/not-exist if the given revision wasn't default? Because vid in the base table doesn't match vid in the revision?

If that's the reason we're adding it, we have @ViewsFilter("latest_revision") instead and should probably rely on that instead of joining a table to access the same data?

Uploaded wrong patch in #28.

I was going to propose removing a chunk of the patch in #29, do you know if that part is still relevant? If there is a use case for it or I'm misunderstanding, happy to put back to RTBC.

Reroll of #31.
Manually fixed conflicts on:

• core/modules/views/src/EntityViewsData.php
• core/modules/views/views.install

Ow, the bot runs .diff files as well.. sorry.

<        $views_field_name = ($multiple) ? $field_name . '__' . $field_column_name : $field_name;
< -      $table_data[$views_field_name] = $this->mapSingleFieldViewsData($table, $field_name, $field_definition_type, $field_column_name, $field_schema['columns'][$field_column_name]['type'], $first, $field_definition);
< +
---
> -      $table_data[$schema_field_name] = $this->mapSingleFieldViewsData($table, $field_name, $field_definition_type, $field_column_name, $field_schema['columns'][$field_column_name]['type'], $first, $field_definition);

This part of the reroll I've realized is not correct, since $views_field_name is no longer set previously.

This should hopefully be a correct reroll of #31.

Diff against #35:

diff --git a/core/modules/views/src/EntityViewsData.php b/core/modules/views/src/EntityViewsData.php
index 028687f..34de2e5 100644
--- a/core/modules/views/src/EntityViewsData.php
+++ b/core/modules/views/src/EntityViewsData.php
@@ -409,8 +409,8 @@ protected function mapFieldDefinition($table, $field_name, FieldDefinitionInterf
     foreach ($field_column_mapping as $field_column_name => $schema_field_name) {
       // The fields might be defined before the actual table.
       $table_data = $table_data ?: [];
-      $table_data += [$views_field_name => []];
-      $table_data[$views_field_name] = NestedArray::mergeDeep($table_data[$views_field_name], $this->mapSingleFieldViewsData($table, $field_name, $field_definition_type, $field_column_name, $field_schema['columns'][$field_column_name]['type'], $first, $field_definition));
+      $table_data += [$schema_field_name => []];
+      $table_data[$schema_field_name] = NestedArray::mergeDeep($table_data[$schema_field_name], $this->mapSingleFieldViewsData($table, $field_name, $field_definition_type, $field_column_name, $field_schema['columns'][$field_column_name]['type'], $first, $field_definition));
       $table_data[$schema_field_name]['entity field'] = $field_name;
       $first = FALSE;
     }

Re: #29 - I agree this relationship doesn't add much value. Instead, let's add the missing relationship to the $revision_table (node_revision for nodes), so as to give access to the revision_log, revision date, etc.

I attach the interdiff that would get this working, please let me know what you think :)

This has been requested here #2674518: Add revision log field to views, and I think it would be very useful for content editors =)

It is also not clear what the difference is between these two in the UI (see #29):

I'm having a problem that I think is related to this issue. In my standard 'Content' view, I'm trying to add the column "Last Updated By". When I add a User relationship, I should be able to put in a User:Name field that references the User relationship, which ultimately should supply the user that last updated the node... right? Instead, it's supplying the original author of the node, as if it were referencing the Author relationship.

#39 patch could not be applied to 8.6.x anymore... sorry

Reroll of #38 (three-way merge did the trick).

now it's not compatible with 8.5.x

+++ b/core/modules/views/src/EntityViewsData.php
@@ -250,6 +254,22 @@ public function getViewsData() {
+      // Add a relationship from the revision table back to the main table.

As found and fixed in #2977276: Node views integration that joins revisions to the default entity fails to consider langcode, resulting in duplicate rows, we should include the langcode as an "extra" join argument if the entity types are translatable.

NW for #50.

+++ b/core/modules/views/views.install
@@ -532,3 +532,10 @@ function views_update_8500() {
+function views_update_8501() {

This should be converted into a post-update hook.

@amateescu also helpfully marked #3001146: EntityViewsData does not provide a relationship from the revision data table to a data table as a duplicate! 🙏

Straight reroll.

#51 is still pending.

+1 for #41, adding a relation to the revision table will give us access to date and log message.

Just a recap. Current patch needs work for:

• #29 and #41 (relationships)
• #50 (langcode)
• #51 (post-update hook)
• #16 and #42 (labels)

It's a little bit confusing... which patch should i use to get a solution for #44 with translated nodes/entities?
I use 8.6.14

After testing patch from #46 (applying with problems) i was able to get the revision user name - but only when checked "create new revision" in node edit. The "last updating user" is not stored in the DB without creating a new revision... the uid in "node_revision" table is still the author of the node.

Is this the way to go in Drupal 8, save every change in a new revision? Maybe this should be discused in a new issue.

Rerolling.

Thanks for the reroll @Sam152

Setting to needs work because although its very close, we still have a bit of work to do, see #56

Rerolling #61 for 9.1.

Updated the patch for Drupal 9.2.x

Looks like they moved one of the test files and two of the test modifications are already part of the new test file.

Patch appears to apply to 9.3 and 9.4. Figure run the tests and see what it says. I guess not then. That's weird cause it applied locally. Well maybe I'll take a shot at updating it again...

I added

      if (($uid_key = $entity_keys['uid'] ?? '')) {
        $data[$data_table][$uid_key]['filter']['id'] = 'user_name';
      }
      if ($revision_table && ($revision_uid_key = $this->entityType->getRevisionMetadataKeys()['revision_user'] ?? '')) {
        $data[$revision_table][$revision_uid_key]['filter']['id'] = 'user_name';
      }

currently user_name is added in Node and Media, this is likely unnecessary after slamming it like this. Not a lot remained of MediaViewsData which I consider a good thing. NodeViewsData also sees a healthy trim.

Now with more langcode. All the previous versions didn't take langcode into account.

Wow, this issue is still open. I reviewed it again. Code changes and test coverage looks good. Only one observation but other than that this is RTBC.

+++ b/core/modules/views/tests/src/Kernel/Entity/EntityViewsDataTest.php
@@ -483,7 +512,8 @@ public function testDataTableFields() {
-      'extra' => [[
+      'extra' => [
+        [

Seems like an unwanted change.

Removed that. No other change.

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -132,7 +133,9 @@ public function getViewsData() {
   +    $entity_id_key = $this->entityType->getKey('id');
   +    $base_field = $entity_id_key;
   

   I think that rather than have both $entity_id_key and $base_field we should have only one. Changing the existing usages of $base_field to $entity_id_key seems fine to me.

2. +++ b/core/modules/views/tests/src/Kernel/Entity/EntityViewsDataTest.php
   @@ -256,6 +256,13 @@ public function testRevisionTableWithoutDataTable() {
   +    $this->assertTrue(!empty($revision_data['id']['relationship']));
   ...
   +    $this->assertTrue(!empty($revision_data['revision_id']['relationship']));
   

   $this->assertNotEmpty() - but also given the following assertions - not sure these are required.

3. +++ b/core/modules/views/tests/src/Kernel/Entity/EntityViewsDataTest.php
   @@ -294,16 +301,27 @@ public function testRevisionTableWithRevisionDataTableAndDataTable() {
   +    $this->assertTrue(!empty($revision_field_data['id']['relationship']));
   ...
   +    $this->assertTrue(!empty($revision_field_data['revision_id']['relationship']));
   

   As above.

4. +++ b/core/modules/views/tests/src/Kernel/Entity/EntityViewsDataTest.php
   @@ -341,15 +359,26 @@ public function testRevisionTableWithRevisionDataTable() {
   +    $this->assertTrue(!empty($revision_field_data['id']['relationship']));
   ...
   +    $this->assertTrue(!empty($revision_field_data['revision_id']['relationship']));
   
   @@ -583,6 +612,13 @@ public function testRevisionTableFields() {
   +    $this->assertTrue(!empty($data['entity_test_mulrev_property_revision']['id']['relationship']));
   ...
   +    $this->assertTrue(!empty($data['entity_test_mulrev_property_revision']['revision_id']['relationship']));
   

   Same again...

5. +++ b/core/modules/views/views.install
   @@ -18,3 +18,10 @@ function views_install() {
   +/**
   + * Clear views data cache.
   + */
   +function views_update_8501() {
   +  // Empty update function to rebuild the views data.
   +}
   

   Use hook_post_update_NAME and not a schema numbered update. It's not necessary to use hook_update_N here.

I've made changes @alexpott recommended to the patch from #77.

Updated the patch for 9.4.x and removed the unneeded AssertNotEmpty() tests that were in patch #79.

This concludes everything Alex asked for and it was RTBC before. Thanks!

The code looks great. We need a CR to tell developers about this change and the fact that they can remove code from contrib / custom entity types that provide this relationship because core views will now add it automatically.

Created https://www.drupal.org/node/3252836

Committed and pushed 45347fa580 to 10.0.x and d9bd98f6ed to 9.4.x. Thanks!

Thanks @jibran

As this issue is merged into Drupal 9.4, it fixes the adding the relationship into view automatically for all entities.

But the above patch is not adding filter user_name for node_revision, media_revision and block_content_revision table.

Eg,

$data['node_revision']['revision_uid']['help'] = $this->t('The user who created the revision.');
$data['node_revision']['revision_uid']['relationship']['label'] = $this->t('revision user');
$data['node_revision']['revision_uid']['filter']['id'] = 'user_name';
$data['node_revision']['table']['join']['node_field_data']['left_field'] = 'vid';
$data['node_revision']['table']['join']['node_field_data']['field'] = 'vid';

Similarly for media_revision and block_content_revision as well.

Before Drupal 9.4 if the content view is using the "Revision User" field was showing the username for the current revision now showing User ID instead of Username and it is breaking the display.

Drupal 9.3 - Revision User Field:

Drupal 9.4 - Revision User Field:

There are related issues with "Revision User" field to show the author name it was added into Drupal core 8.9 onwards and now it got removed again.

https://www.drupal.org/project/drupal/issues/2769289
https://www.drupal.org/project/drupal/issues/3113986

Closed #3086916: Add Views Relationship back to original Term in Taxonomy Term Revision view as a duplicate