Steps to reproduce:
* Enable Content Translation.
* Add a language (eg: French).
* Create a new content type, enable translation for it.
* Add some content in your new content type, and translate it. I used a content type "rest_test", and created nodes "Cat" (en) and "Chat" (fr). See TranslatedContent.png
* Enable REST.
* Add a view over your content type, with a REST export display. Ensure the row handler is "Entity". See ViewsConfig.png
* Look at the view output. Notice that you only see the default site language, eg: here "Cat" appears twice, but "Chat" never appears.
* The correct behavior would be to show both "Cat" and "Chat", once each.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vasi created an issue. See original summary.

dawehner’s picture

Status: Active » Needs review
FileSize
5.15 KB

What about something like this:

vasi’s picture

Issue tags: +Needs tests
FileSize
5.31 KB
538 bytes

That doesn't quite work, the query needs munging too.

I'll see if I can figure out how to test this.

dawehner’s picture

Oh good catch!

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

vasi’s picture

Ok, wrote a test! First adding just the test, it should fail.

vasi’s picture

And now the tests + code, should pass.

The last submitted patch, 5: 2664880-5-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2664880-6.patch, failed testing.

vasi’s picture

And that, my friends, is what happens when you forget to "git add" a new file. Trying again.

vasi’s picture

And the should-succeed.

vasi’s picture

Status: Needs work » Needs review

The last submitted patch, 9: 2664880-9-fail.patch, failed testing.

jibran’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

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?

vasi’s picture

Status: Needs work » Needs review

Re: getView(), getLanguageManager(), etc: Those are abstract methods in EntityTranslationRenderTrait, which we are use-ing.

Re: getBaseEntityType() returning FALSE: I'm not really sure what to do here. I could just check the result of getBaseEntityType() and return NULL, but that would just cause an error immediately afterwards in EntityTranslationRenderTrait.

I think it's kinda moot though, since DataEntityRow already seems to fail with non-entity types! Try building a new view over 'Log entries', adding a REST Export display, and selecting Entity as the row handler. May I suggest we file that as a separate issue?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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.

vasi’s picture

Adding 2135829 as a parent. It has the same steps to reproduce, but it seems to be discussing mostly other parts of REST translation.

vasi’s picture

Created the issue about DataEntityRow on non-entity views: https://www.drupal.org/node/2666226 . Thanks jibran for catching that!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

alexpott’s picture

+++ 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.

vasi’s picture

Status: Needs work » Needs review
FileSize
10.03 KB
1.37 KB

Thanks for the quick review.

Status: Needs review » Needs work

The last submitted patch, 20: 2664880-20.patch, failed testing.

vasi’s picture

I think the failure is actually a problem with testLivePreview(). It's calling $view->destroy(), followed by $view->setDisplay('rest_export_1'). Is it really safe to do operations on a destroyed view?

vasi’s picture

Status: Needs work » Needs review
FileSize
11.47 KB
1.66 KB

Ok, changed ViewExecutable so it doesn't discard viewData on destroy, just like storage, user, routeProvider...

dawehner’s picture

+++ 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.

vasi’s picture

Ok, changed it to use EntityTypeInterface.

We can't maintain caching of $baseEntityType unless we allow both NULL and FALSE to be values.

clemens.tolboom’s picture

Version: 8.0.x-dev » 8.2.x-dev
FileSize
11.51 KB

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?

Status: Needs review » Needs work

The last submitted patch, 26: 2664880-26.patch, failed testing.

clemens.tolboom’s picture

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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
+++ 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.)

Wim Leers’s picture

Version: 8.2.x-dev » 8.1.x-dev

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

alexpott’s picture

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.

alexpott’s picture

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

alexpott’s picture

Priority: Normal » Major

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.

  • alexpott committed 5c386ed on 8.2.x
    Issue #2664880 by vasi, dawehner, clemens.tolboom: DataEntityRow doesn't...
Wim Leers’s picture

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

+1

catch’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs release manager review +Needs change record

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

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2664880-26.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.1.2 release notes

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

  • alexpott committed 11b897f on 8.1.x
    Issue #2664880 by vasi, dawehner, clemens.tolboom: DataEntityRow doesn't...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.