"TranslationLanguageRenderer" uses the wrong table for the "langcode" column with entity revision views

Not actually a duplicate, see #2446681-26: Error "Column 'langcode' in field list is ambiguous" thrown due to TranslationLanguageRenderer not rendering a field from a relationship.

Here's an attempt to fix this properly.

Took me a while to understand what's going on, and still not 100% sure, but this seems to make sense.

+++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
@@ -28,13 +28,47 @@ public function query(QueryPluginBase $query, $relationship = NULL) {
+      switch ($base_table) {
+        case $revision_table:
+          $langcode_table = $revision_table;
+          break;
+        case $revision_data_table:
+          $langcode_table = $revision_data_table;
+          break;
+      }

That's a creative way of using a switch :)

This should at least have a comment that explains this I think.

One confusing thing is that we are mixing terminoly here. an "entity base table" is never the revision or revision data table. But this isn't the entity base table, it's the views base table, so maybe we should use $views_base_table to make that distinction a bit clearer?

@Berdir - this is the part of the patch that took me longest to understand, so yes, commenting and updating variable names is a good suggestion.

Good points :)

Addressed the review above.

Forgot the comment, sorry.

Lovely!

I'm glad to see this was so easy to fix. I just found a couple of points to address:

1. +++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
   @@ -28,13 +28,48 @@ public function query(QueryPluginBase $query, $relationship = NULL) {
   +   * @param string $relationship
   +   *   (optional) The relationship, used by a field.
   ...
   +  protected function getLangcodeTable($relationship) {
   

   By looking at the method definition, it seems that the $relationship param is not really optional?

2. +++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
   @@ -28,13 +28,48 @@ public function query(QueryPluginBase $query, $relationship = NULL) {
   +   * @return string|bool
   +   *   A table name or FALSE if none could be found.
   

   We should break out of this habit of returning FALSE when we actually can (and should) return NULL :)

Actually the method will always return a string.

Great, that reads much better now ;)

TranslationLanguageRenderer changes look good to me.

+++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
@@ -28,13 +28,48 @@ public function query(QueryPluginBase $query, $relationship = NULL) {
+    // If the entity type is revisionable, we need to take into account views of
+    // entity revisions. Usually the view will use the entity data table as the
+    // query base table, however, in case of an entity revision view, we need to
+    // use the revision table or the revision data table, depending on which one
+    // is being used as query base table.

This comment convinced me.

Just one questions: This is just adding the correct table to the query. It wouldn't affect the existing view in the storage. It would affect only their output. Right?

This is just adding the correct table to the query. It wouldn't affect the existing view in the storage. It would affect only their output. Right?

Yes, it's just ensuring the query is generated correctly at runtime.

Thanks, RTBC +1

This is blocking #2862041: Provide useful Views filters for Content Moderation State fields so hopefully we can get it committed for better Views support in Content Moderation.

@plach: curious about the !$relationship part of:

if (!$relationship && $this->entityType->isRevisionable())) {
  $query_base_table = $this->view->storage->get('base_table');
  ...
}

is this meant to limit the scope ? (there are two other issues affected by the same part of code and showing similar symptoms),
would you see an advantage of doing it like this instead:

if ($this->entityType->isRevisionable()) {
  // Determine the primary table to seek
  if (!$relationship || empty($query->relationships[$relationship])) {
    $query_base_table = $this->view->storage->get('base_table');
  }
  else {
    $query_base_table = $query->relationships[$relationship]['base'];
  }
  ...
}

to cover both cases.

@tacituseu:

Courious about the !$relationship part of [...] is this meant to limit the scope ?

Well, actually, the reason is there is no way to define a revision relationship AFAIK. But that's a good suggestion, if it is introduced then the code should work automatically.

@plach: Indeed, I'm so used to running with the ERR patch adding it that I totally forgot about it, thanks.
Edit: Manually tested #30 with the ERR patch and it works just fine.

Cool, can you point me to the issue?

It's #2799479: Views doesn't recognize relationship to host, I referenced this one from it already.

@tacituseu:

Thanks, any other concern or can we move this back to RTBC?

None from me, setting back.

This is required by #2862041: Provide useful Views filters for Content Moderation State fields which is required for Content Moderation to become stable, so moving back to 8.4.x

Since this blocks a Major should-have it should be Major too.

1. +++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
   @@ -28,13 +28,52 @@ public function query(QueryPluginBase $query, $relationship = NULL) {
   +      /** @var \Drupal\views\Plugin\views\query\Sql $query */
   

   Why is this necessary when $query is typehinted?

Everything else looks good, the inline comments in the patch are useful.

Uploading a test-only patch for the bot since I can't see one on the issue. The

Good fail.

Re #40.1: That's because the method type hints the generic QueryPluginBase class and we're using some methods that are only in \Drupal\views\Plugin\views\query\Sql, so basically for better IDE autocompletion :)

OK, those aren't my favourite comments, but fair enough.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!