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.
Comment | File | Size | Author |
---|---|---|---|
#26 | 2664880-26.patch | 11.51 KB | clemens.tolboom |
#25 | interdiff.txt | 505 bytes | vasi |
#25 | 2664880-25.patch | 11.48 KB | vasi |
#23 | interdiff.txt | 1.66 KB | vasi |
#23 | 2664880-23.patch | 11.47 KB | vasi |
Comments
Comment #2
dawehnerWhat about something like this:
Comment #3
vasi CreditAttribution: vasi at Evolving Web commentedThat doesn't quite work, the query needs munging too.
I'll see if I can figure out how to test this.
Comment #4
dawehnerOh good catch!
Maybe you could even extend
\Drupal\views\Tests\Entity\RowEntityRenderersTest
in a rest specific test class.Comment #5
vasi CreditAttribution: vasi at Evolving Web commentedOk, wrote a test! First adding just the test, it should fail.
Comment #6
vasi CreditAttribution: vasi at Evolving Web commentedAnd now the tests + code, should pass.
Comment #9
vasi CreditAttribution: vasi at Evolving Web commentedAnd that, my friends, is what happens when you forget to "git add" a new file. Trying again.
Comment #10
vasi CreditAttribution: vasi at Evolving Web commentedAnd the should-succeed.
Comment #11
vasi CreditAttribution: vasi at Evolving Web commentedComment #13
jibranWe have test now so removing the tag. Nice work @vasi. Here is some feedback.
getBaseEntityType() can return FALSE so we need a check here.
Why do we need these?
Comment #14
vasi CreditAttribution: vasi at Evolving Web commentedRe: 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?
Comment #15
dawehnerYeah 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.
Comment #16
vasi CreditAttribution: vasi at Evolving Web commentedAdding 2135829 as a parent. It has the same steps to reproduce, but it seems to be discussing mostly other parts of REST translation.
Comment #17
vasi CreditAttribution: vasi at Evolving Web commentedCreated the issue about DataEntityRow on non-entity views: https://www.drupal.org/node/2666226 . Thanks jibran for catching that!
Comment #18
alexpottThis looks good - just a couple of very minor nits.
Needs a new line here.
Missing return comment.
Comment #19
alexpottI 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.Comment #20
vasi CreditAttribution: vasi at Evolving Web commentedThanks for the quick review.
Comment #22
vasi CreditAttribution: vasi at Evolving Web commentedI 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?
Comment #23
vasi CreditAttribution: vasi at Evolving Web commentedOk, changed ViewExecutable so it doesn't discard viewData on destroy, just like storage, user, routeProvider...
Comment #24
dawehnerThe 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.
Comment #25
vasi CreditAttribution: vasi at Evolving Web commentedOk, changed it to use EntityTypeInterface.
We can't maintain caching of $baseEntityType unless we allow both NULL and FALSE to be values.
Comment #26
clemens.tolboomThis 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?
Comment #28
clemens.tolboomNot sure why the test fails now on (int) == (string) tests
Comment #29
Wim LeersThis looks very sensible.
(And completely analogous to
\Drupal\views\Plugin\views\row\EntityRow::query
.)Comment #30
Wim LeersThis belongs on 8.1.x, since this is a bug fix.
Comment #31
alexpottLooking 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.
Comment #32
alexpottHowever given #31 I think we should get release manager sign off before committing to 8.1.x.
Comment #33
alexpottCommitted 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.
Comment #35
Wim Leers+1
Comment #36
catchAgreed 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.
Comment #37
alexpottThanks @catch - let's get a CR written so we can cherry-pick after 8.1.1
Comment #38
Wim LeersCR created: https://www.drupal.org/node/2715953.
Comment #40
Wim LeersPatch 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.
Comment #41
alexpottCommitted 11b897f and pushed to 8.1.x. Thanks!