DataEntityRow doesn't respect translations

What about something like this:

Oh good catch!

Maybe you could even extend \Drupal\views\Tests\Entity\RowEntityRenderersTest in a rest specific test class.

We have test now so removing the tag. Nice work @vasi. Here is some feedback.

1. +++ b/core/modules/rest/src/Plugin/views/row/DataEntityRow.php
   @@ -23,16 +27,96 @@
   +    return $this->view->getBaseEntityType()->id();
   

   getBaseEntityType() can return FALSE so we need a check here.

2. +++ b/core/modules/rest/src/Plugin/views/row/DataEntityRow.php
   @@ -23,16 +27,96 @@
   +  protected function getEntityManager() {
   ...
   +  protected function getLanguageManager() {
   ...
   +  protected function getView() {
   

   Why do we need these?

May I suggest we file that as a separate issue?

Yeah let's do that. There are totally ways to solve that properly.

This is some good first test coverage. In general I disagree with using assertIdentical in all places, but meh.

This looks good - just a couple of very minor nits.

1. +++ b/core/modules/rest/src/Plugin/views/row/DataEntityRow.php
   @@ -23,16 +27,96 @@
   +  }
    }
   

   Needs a new line here.

2. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -924,6 +931,26 @@ public function getBaseTables() {
   +   * @return \Drupal\Core\Entity\EntityType|false
   +   */
   

   Missing return comment.

+++ b/core/modules/views/src/ViewExecutable.php
@@ -924,6 +931,26 @@ public function getBaseTables() {
+      $views_data = Views::viewsData()->get($view_base_table);

I was going to fix #18 and commit but whilst doing I noticed that we should be using $this->viewsData. So I think we should roll a new patch and retest.

+++ b/core/modules/views/src/ViewExecutable.php
@@ -434,6 +434,13 @@ class ViewExecutable implements \Serializable {
+   * @var \Drupal\Core\Entity\EntityType|false
...
+  protected $baseEntityType;

The thing is, by default this variable is NULL and then maybe switches to FALSE at some point. Let's be consistent with those two.

Also let's use EntityTypeInterface instead, you never know.

This does not apply on 8.2.x ... guess that should be its place now.

After following steps to reproduce I got my sites default language 'nl' node

After applying patch I only get the 'en' translation.

Did I re-rolled ok?

Not sure why the test fails now on (int) == (string) tests

doTestAuthoringInfo

fail: [Other] Line 314 of core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php:
Translation date correctly stored.
Value '1459616233' is equal to value 1459612633.

fail: [Other] Line 328 of core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php:
Translation date correctly kept.
Value '1459616233' is equal to value 1459612633.

+++ b/core/modules/rest/src/Plugin/views/row/DataEntityRow.php
@@ -23,16 +27,97 @@
+  use EntityTranslationRenderTrait;
...
+  public function query() {
+    parent::query();
+    $this->getEntityTranslationRenderer()->query($this->view->getQuery());
   }

This looks very sensible.

(And completely analogous to \Drupal\views\Plugin\views\row\EntityRow::query.)

This belongs on 8.1.x, since this is a bug fix.

Looking at the applicability of this patch to 8.1.x - it adds methods to ViewExecutable and DataEntityRow. At least for DataEntityRow there is no way we can use the underscore trick for 8.1.x backporting since the method names are mandated by the trait. So then it becomes a question of how internal do we consider these classes - from the point of view of whether there are classes that extend and are likely to implement methods with the same name. I think in these cases that is very unlikely and it is okay to add these methods in 8.1.x.

However given #31 I think we should get release manager sign off before committing to 8.1.x.

Committed 5c386ed and pushed to 8.2.x. Thanks!

Leaving as rtbc against 8.1.x. We might cherry-pick after discussion with release managers.

Also considering the nature of the bug I think this is at least major as mutlilingual rest views are essentially borken.

Also considering the nature of the bug I think this is at least major as mutlilingual rest views are essentially borken.

+1

Agreed this looks like a relatively straightforward bug fix. If we commit it shortly after tagging a patch release, add it to release notes, do a change record I'd be fine with it going into a patch release.

Thanks @catch - let's get a CR written so we can cherry-pick after 8.1.1

CR created: https://www.drupal.org/node/2715953.

Patch was committed to 8.2, the fail in #39 is solely the 8.2 patch that failed. The 8.1 patch in #26 is green. So, back to RTBC.

Committed 11b897f and pushed to 8.1.x. Thanks!