Views rendering ignores entity language, eg: Identical nodes rendered in views when nodes have translations

This is a known behavior, see #1498674-213: Refactor node properties to multilingual. We need some kind of language filtering in the front page view. This is the quick fix. The real fix is making Views using EFQ and handle language filtering from there.

Drupal core did this forever. Depending on what you want on your front page this might or might not be an issue. The frontpage did not use to be language filtered in any Drupal version so far :)

Yes but then again, Drupal core didn't used to be multilanguage capable/aware before D8 and nor was the front page a View ;)

Being able to (optionally and configurably - is this a real word btw or did I just invent it?) filter by language is a thing that D8 needs to do (Views, Search results etc).

The difference with what happended in the past is that now you have two identical nodes (due to content language negotiation), where before you had two different translations. This is not something we can leave as-is.

I agree it is more likely a default assumption that posts in the same language should show up. Either way it sounds like something we should set up in the default view and people will be able to modify :)

Yep, I think a quick and easy fix would be using node_modules_enabled() to add a node language filter set to "current language" or "language not specified" when entity translation is enabled.

In the long run when Views will use EFQ this won't be needed as EFQ already selects unique entity ids. Language filtering could be added by those actually needing it.

Sounds like to me a language filter is useful even if no entity translation? To filter for nodes in specific language. So make it go right into default view?

This will slow down the query on monolingual sites. Maybe we can react to Language module being enabled instead?

The problem actually originates from either a filter or a sort handler in views that joins a translatable field in a given entity. Given that there will be a row for each translation of that entity in a translatable field, the query will return multiple results. The only way to fix this is to add a WHERE condition to the field table JOIN in the field filter/sort handler to filter out results where the language column doesn't match the desired language.

Updated issue summary, title and added VDC tag.

IMO the proper solution for this issue should be exposing node_field_data.langcode as a "Translation language" views field and add an optional filter based on it which would filter stuff based on Langcode::LANGUAGE_NOT_SPECIFIED/***current language***.

@plach: I think that may be a stop-gap, but generally Views should be aware that it may display an entity in different languages, and not always use the page negotiated language. I guess Views needs a setting to apply the negotiated language (and then DO FILTER) or not apply that and then do render in different languages. By default it should probably filter, but that is not an overall solution for the views cases it needs to cover.

I think we should avoid automagic as much as possible: we should give people the way to implement whatever use case they need without getting in the way.

What about having an option on the node row plugin where the user could choose which language to use to render the entity? For instance 'italian', 'current language', 'translation language', 'node language' an so on.

My suggestion of applying an optional filter was just related to the node front page and the content admin.

Yeah an option on the node row plugin works as well.

Coming from D7 we have inherited the concept of field language.
On each view you could select one of the three options:

$languages = array(
        '***CURRENT_LANGUAGE***' => t("Current user's language"),
        '***DEFAULT_LANGUAGE***' => t("Default site language"),
        Language::LANGCODE_NOT_SPECIFIED => t('Language neutral'),
    );

This information is used to filter the fields, when they are displayed.
@Gabor There was one issue which added some of the new language types there, do you remember the issue?

What about having an option on the node row plugin where the user could choose which language to use to render the entity? For instance 'italian', 'current language', 'translation language', 'node language' an so on.

I wonder whether the entity row plugin should just use the option which is currently called field language to figure out which language to use
for loading? Maybe we should then also rename the option.

@dawehner: that would be #2037979: "Current user's language" views filter label is named very misleading.

@dawehner: I think filtering is not the only solution here. While that solved the problem itself, if you want to filter for more than one language, then the rendering would still happen in the page negotiated language for all of them, so the rendering of the node itself also needs to be potentially different based on views settings. For a view that displays entities from multiple languages, the same entity may need to be displayed in multiple languages.

Considering the use case in #20 we would need an additional option, that is the translation language (node_field_data.langcode), which would allow us to render each row in its own language.

And I think filtering needs to be explicit :)

@plach: in fact I think given what we explored above #21 is what should be done. Then IF the view is filtered, you would get node's where node_field_data.langcode is the one being filtered for, so naturally you would see that displayed. If you filter for multiple languages, the right ones would display. If you filter for the page negotiated language, only nodes for that language would display (rendered in their proper language), etc. So views should take the language of the entity variant found and NOT the page's language. If the page's language was used in a filter, that will have the proper effect already.

Am I missing something?

Am I missing something?

No, it's ok, I was complementing your answer to Daniel :)

I do not think this is normal.

@dawehner:

I wonder whether the entity row plugin should just use the option which is currently called field language to figure out which language to use
for loading? Maybe we should then also rename the option.

Right, views should render the node in the language it found it :) Then filtering will probably be yet another (different) issue. We already have #2037979: "Current user's language" views filter label is named very misleading which deals with naming the filters and then we can open yet another issue to add filters to some views when translation is enabled (and debate the validity of that magic there).

I think the full scope of this issue is the rendering of the node in the language views found it, so that should be a small enough scope to not drive lots of bikeshedding :)

So basically we would restore the D7 behavior and all node translations would be shown?

This is just the first work on the issue.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
@@ -140,23 +141,52 @@ public function summaryTitle() {
+    $this->alias$query->addField($base_table, 'langcode');

langcode should be picked from the data table (if it exists).

Looks like some random PHP ended up in the code...

Adressed the feedback of plach.

The error seems to be Undefined property: Drupal\views\ResultRow::$langcode :/

Instead of using the langcode property on the row (which is not there), we should use the langcode alias :) Thanks @dawehner for pointing that out :)

Another misused variable :D

Now only actual fails :) In node translation. I'll not have time to look into this anymore this week unfortunately, so @plach / @dawehner feel free to pick it up :)

Attached patch solves duplicate display but not sure whether it is right approach as we are not doing anymore >1 language check.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
@@ -182,34 +182,29 @@ public function preRender($result) {
-        // Prepare the render arrays for all rows.
-        $this->build = $view_builder->viewMultiple($entities, $this->options['view_mode'], current($langcodes));

:( I would like to keep the viewMultiple functionality.

So what about doing something like that?

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -182,21 +182,30 @@ public function preRender($result) {
   +          $entity_translation = $entity_translation[$langcode]>getTranslation($langcode);
   

   missing pointer

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -182,21 +182,30 @@ public function preRender($result) {
   +        $this->build = $view_builder->viewMultiple($$langcodes, $this->options['view_mode'], $langcode);
   

   $$?

Updating...

Tested and the language rendering behaviour looks good (ignore that the body is always the same that seems to be a current core bug, heh).

However, the links on all nodes lead to the node in the language of the page instead of the language of themselves. That may not be a bug in Views I suppose. Either way, we should back this up with some tests :)

Opened #2148795: Configurable field translatability is not properly switched about the body problem. Not caused by or related to this issue. Not sure if the links should be / possible to be fixed here or not.

Regarding the links I guess we need something like the following? This feels a little bit out of scope for that issue to be honest.

@dawehner: indeed that seems to be its own issue. In fact I opened #2149649: Entity rendering/theming does not use the active entity language to render links and put your patch there :) So this only needs tests then? :)

ok, Patch in #45 is still applies and issue that #48 tries to fix has it is own issue (ref: #50)

I think this would be good to go as soon as we have tests :) I think this is a very important piece of functionality for multilingual, so people can create views that make sense for them :)

Updating the test case to cover this issue.

That looks sweet and simple if it works :)

Yay, amazing, thanks!

I don't fully grok the patch yet but it seems like a weird solution. It looks like the problem is that viewMultiple() is not capable of handling multiple languages and so we have to work around that. Wouldn't it make more sense to move this logic to viewMultiple() / entity_view_multiple() so users of that API don't have to use similar workarounds? Can't help but feel we're fixing this in the wrong place.

Additionally, the patch doesn't seem to work. :\ I applied the patch, followed the instructions in the OP, and still see duplicate nodes.

61: 2006606-language-view-duplicate-61.patch queued for re-testing.

I've applied your patch. But it does not work on my side.
I see it does not filter language items. I though entity translation almost in core. I see Different translation use the same nid. It is right. But why views do not filter it?

t is very interesting. entity_view_multiple returns the right quantity of entities. But Views ask renders duplicates.

Entity is an atom of a new Drupal system. Here we split atom into subatoms :) I see how the last patch works. It looks like on the old Drupal way. When we had a node translation by creating another related node. They existed as separated nodes. As for me I do not like this way. I can not find a way how views split it.

@likin: *you* can add a language filter to your front page view. Drupal does not have this filter. This bug is about fixing the case when the view does not have such a language filter. We cannot just assume all views will have a language filter of some kind and if they don't, then you get this bug. If you do filter down your view to a language, then you'll not notice this bug because this bug is about the same node matching the view in different languages and then rendered improperly. Clearer?

Looks like this may have been @webchick's misunderstanding as well? Maybe we need an issue summary update because the stop-gap in the issue summary is NOT what is implemented in this module based on extensive discussion above.

@YesCT can you help with a clear issue summary? :)

For my case this hack works for any multilingual "views". It look on a workaround way.

function spage_views_query_alter($view) {
  if ($view->storage->id == 'frontpage') {
    $view->query->distinct =TRUE;
  }
}

I guess that I am wrong because I do not see a whole picture. But I looked over the code, the query object. I found out that it used LEFT joins with the node_revision table. It multiplied nodes.

I wait for a right solution. :)

@linkin: right, if you *never* *ever* *under any circumstances* want to *ever* display the same node in multiple languages in the same view, then your workaround "works". If in any case, you need to display an entity translated to more than one language in the same view, then you'll hit this bug, so we are trying to resolve it. :) As an added bonus, the front page view as-is is exposing this bug big time, so it is pretty visible.

An issue summary that explains what the actual problem is we're trying to solve here and how would definitely help, but I don't understand why we wouldn't make #68 the default (it's certainly my expectation when I filter to "french" i will see french content and not english content on my site), so maybe include that in the issue summary as well. :) Or, ideally, fix it, to match user expectations.

Yeah, but there are actually two cases of "filtering" depending on what meaning one might give to that:

1. filtering French so that French content is shown and untranslated content gets hidden. IOW: show only French - hide the rest.
2. filtering French so that French content is shown and content not yet translated in French is shown in the language it was originally created. IOW: prefer French but show all (a.k.a. language fallback).

IMHO, because this is a matter of per-site and some times per-view preference, we should provide an option to define what people want to do with untranslated content. So, two options: "Hide it" and "Show untranslated original"

If we wanted to make it clear(er) why non-French content is shown when people choose to show untranslated too, we could append (optionally again) "not translated in [selected_language] yet" next to titles. That text could become a link for users with appropriate permissions to take them to the translation page for that piece of content for the specific language. Sounds like a reasonable workflow to me.

@webchick
We will show all translations, unless someone adds a filter in views. Views in general went from magic in D6 to more explicit behavior in D7 and the response was really positive. What this issue solves is just the rendering in the right language. Please please don't be confused by comments of people.

You bother about case when some content is not translated. So it shows all articles with default language. If an article is not translatted comletly it can use a translation from predefined source. Multilingual Entities work well . it does not reduce nodes if they do not have.
I call views - visual entity query manager. Why we modify views lets modify entity api.

@klonos views does not hide untranslated nodes. I've checked. Views uses entity-load-multy it works well. It always returns nodes without duplicates. Every entity has a getTranslation method. Look the last patch. It works but it looks on the workaround method.

There are two "problems" with the frontpage view, as the title already says. It displays duplicate *and* identical nodes. This issue currently only solves the problem that *identical* nodes are displayed.

This is because views completely ignores the language and renders any entity in the current content language. This is a simple problem, and you best ignore the current frontpage view configuration, or multiple translations of the same entity use case for now. Consider the use case where you want to display a list of french nodes, then they should be shown in french. However, if you do this right now, you would get them in your current language, if available. This patch changes them to respect the language but currently doesn't consider the fallback, I think, see blow. The result is that entities are in the right language now, but we still have th second part of the problem:

The second "problem" is the duplicated part. And while it's true that there are many use cases for it and that views should absolutely support it, I think it would really make sense to have a better default configuration for the default frontpage view, this issue is proof that the current default confuses people. And those that want to see them all can easily re-configure the view. It's also easier to remove the condition rather than add it?

Maybe we should open a separate issue for that and re-title this issue to something more simple, like "Views rendering does not respect/ignores entity language"? Because while I perfectly understand the arguments made by @dawehner, @GaborHoitsy and so on, the patch does *not* solve the problem described in the issue title.

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -140,23 +148,65 @@ public function summaryTitle() {
   +            $entity = $entity->getTranslation($langcode);
   

   This doesn't consider language fallback. Should we use EntityManager::getTranslationFromContext() here to support the use case "show current language of all entitites or best available fallback"? Or does views already have the correct langcode? I think not?

   Should we name this $translation or $entity_translation? Re-using $entity is a bit weird within a foreach loop.

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -140,23 +148,65 @@ public function summaryTitle() {
   +        // @todo It should be possible to use viewMultiple even if you get
   +        //   more than one language.
   

   Entities in viewMultiple() are keyed by the entity ID, while #2073217: Remove the $langcode parameter from the entity view/render system would allow to set a language per entity, the real problem is that we don't support to render the same entity in multiple languages, even with that. Not sure what to do about that.

#75++

Entities in viewMultiple() are keyed by the entity ID, while #2073217: Remove the $language parameter from the entity view/render system would allow to set a language per entity, the real problem is that we don't support to render the same entity in multiple languages, even with that. Not sure what to do about that.

@YesCT: Thanx!

I totally agree with @Berdir that the current behavior is not the most sensible default, although we need to support it. If we do not want to postpone this to #2073217: Remove the $langcode parameter from the entity view/render system, to address @Dries' feedback we need to remove the ::viewMultiple() call and go with just ::view() (sucks).

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
@@ -143,20 +151,65 @@ public function summaryTitle() {
+    if (isset($this->entityInfo['data_table'])) {
+      $data_table = $this->entityInfo['data_table'];
+      $data_table_alias = $query->ensureTable($data_table);
+      $this->langcodeAlias = $query->addField($data_table_alias, 'langcode');
+    }
+    else {
+      $base_table = $this->entityInfo['base_table'];
+      $this->langcodeAlias = $query->addField($base_table, 'langcode');
+    }

This needs to take into account also the revision table. Fixed in the attached patch. Setting to needs review for the bot, although this is still needs work.

Also, we should make the render language configurable, otherwise no kind of fallback based on the current language would be possible, I am afraid.

Updated the remaining task to say "Figure out if we can make this behavior work in viewMultiple()." only. That sounds like would require other changes in the entity system (if at all possible). Eg. so we can pass in that we want the entities to be rendered in the language found (instead of taking the language from elsewhere). Is that in scope for this issue and if not, are the changes here useful regardless? I mean the actual line changes. I think the bugfix as-is is very valuable.

@plach: I'm not sure what you mean there? Fallback in the query? In rendering? What would the options for that setting be?

#83: I think changing the entity render system here is definitely out of scope and not trivial. There's #2073217: Remove the $langcode parameter from the entity view/render system, the parent issue it relates to (which still need to be updated) but as I mentioned, the problems for this specific issue (rendering the same entity in different languages) go even deeper, as mentioned above (it assumes that the list of entities is keyed by entity id, which would no longer be possible)

Which makes the current patch RTBC again, no? @plach?

A follow-up issue was added to introduce support for language fallback: I don't think we can implement that at query level in an efficient way. if I were to implement such a view today, I'd just create it with a distinct clause and I'd be done: entities would be rendered in the current language or would fall back to another language, if no translation for the current one existed (I manually tested it).

This issues currently makes supporting that use case impossible, as it hardcodes the translation language as rendering language. If, as discussed above, we had a selector in the entity row plugin to choose the rendering "strategy" we could support both. The available options might be:

• Current language: all entities are rendered in the current language or fall back to an existing one (the current behavior).
• Default language: all entities are rendered in the default (original) language.
• Translation language: all entities (translations) are rendered in their translation language (what this patch implements).

I am available to implement this ASAP if you are ok with it, but I don't think this is good to go as-is.

Agreed that enforcing the selected language without a way to change it is problematic, that's what I meant in #75.1.

@Gabor:

Are you ok with the plan outlined in #87?

...entities would be rendered in the current language or would fall back to another language, if no translation for the current one existed (I manually tested it).

This issues currently makes supporting that use case impossible, as it hardcodes the translation language as rendering language.

I don't see how *this* patch makes supporting that use case impossible. It is not possible to support now either. The rendering does not pass on any information to tell if the entity was a duplicate render, or if its rendered in views or even a suggested langcode for rendering... So I don't see how that use case would be possible *without* this patch either. I contest this patch makes it any harder to implement. Either way it requires further changes, no? The question is just keep that unsupported for a little while more while letting this bug fixed or get more features into this bugfix.

In the current HEAD you can configure the front page to perform entity translation fallback on rendering by just enabling the "Distinct" option in the query settings. You can see the result in the three "head" PNGs you can find attached. Apply this patch and you get what you can see in the "patch" PNG, that is the distinct query no longer works, as we are hardcoding the translation language field in the select.

You can find also my test db and files if you want to see this in action.

My point is that with the current patch we are fixing one use case and breaking another one, which IMHO is more common, and I strongly believe we should support both.

Sorry if I am missing something, but I really don't see the point of committing the current patch as-is.

Yeah let's add options then...

Cool, I will do it tonight, if no one else beats me to it.

This adds the selector in the row plugin settings to choose the rendering strategy. It adds a new entity renderer class that should obviously a new plugin type, but since I wasn't sure it's worth the effort, this patch instantiates them statically.

If Daniel is ok with this approach we just need to make them plugins, which should be pretty trivial, otherwise we can collapse them back into the EntityRow plugin.

I will fix tests once I have a feedback on #94.

From the pure code perspective it totally make sense to separate the different usecases of rendering into several classes. I wonder whether there is any need for contrib to override this behavior. If yes I wonder whether entity list controllers have a similar problem.

Not sure whether there are use cases to override those behaviors, but maybe adding new ones could make sense?
Gabor, what do you think?

If yes I wonder whether entity list controllers have a similar problem.

Possibly. But I think that for those a default sensible behavior could be enough, as they are not configurable.

I think adding more options/plugins like this would be potentially useful, not sure about overriding. If you can add more, you can add another option and just update your view, so you can effectively do the same thing.

@dawehner:

So, ok for plugins?

Can we look into making them plugins in an follow-up issue that is not a major bug and go with a quick fix here?

We already have a crazy amount of plugin types (which currently results in tons of cache get requests) and I'm not sure there are other options here?

Making something that is currently harcoded extensible/pluggable shouldn't be an API change for existing code, so we wouldn't need to hurry something in...

Do you mean just cleaning-up the current code and keeping separate classes or removing them and possibily introduce plugins in a followup?

This should clean up PHP docs and fix failures.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/rendering/TranslationLanguageRenderer.php
@@ -0,0 +1,63 @@
+class TranslationLanguageRenderer extends DefaultLanguageRenderer {
...
+/**
+ * {@inheritdoc}
+ */

Mmh, no

1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
   @@ -246,6 +246,19 @@ function testTranslationRendering() {
   +    $display['display_options']['row']['options']['rendering_language'] = '\Drupal\views\Plugin\views\row\rendering\TranslationLanguageRenderer';
   ...
   +    $display = &$view->getDisplay('default');
   ...
   +    $view = \Drupal::entityManager()->getStorageController('view')->load('frontpage');
   

   I'm concerned that by default this will not be the default behaviour of the view. Also not the default behaviour of a view created later. So by default core will still have the same "bug" that is has now :/

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -128,6 +138,19 @@ protected function buildViewModeOptions() {
   +    // @todo Consider making these plugins.
   

   Any todo added should be qualified with a URL :)

3. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/rendering/RendererBase.php
   @@ -0,0 +1,96 @@
   +abstract class RendererBase {
   

   Should have an interface for this as well, so those not implementing derivatives from rendererbase would still be forced to implement the proper interface, no?

The attached patch should fix #106, except for #3: I don't think it makes sense to introduce an interface while renderers are still hard-coded. It would just mean more boilerplate code and I/O, since we are not using type-hinting anywhere.

I added a @todo to go back to the current language renderer as default as soon as we have a translation language filter, which I think makes more sense a default behavior. Or at least we should do that for the front page view (this is why I retained the changes to the node translation test).

There is also some code clean-up since hard-coding class names made my eyes bleed :)

Qualified another @todo.

Looks good to me. I understand your point of view that we don't need an interface so far as we hardcode it and we should add it later on. I think we should get a Views OK on this and get it in :)

We can always add the interface later. Views itself though don't really leverage interfaces yet.

This code looks fine, just a small nitpick here.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
@@ -51,22 +53,32 @@ class EntityRow extends RowPluginBase {
+   * @var \Drupal\Core\Language\LanguageManager
...
+   * @param \Drupal\Core\Language\LanguageManager $language_manager
...
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, EntityManagerInterface $entity_manager, LanguageManager $language_manager) {

Should we just typehint the LanguageManagerInterface

That interface only exists since a few hours, it has been added by #1862202: Objectify the language system. So yes, we can do that *now* :)

Fixed #112 :)))

It seems all concerns were addressed or have a follow-up. Back to RTBC?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
@@ -141,22 +178,31 @@ public function summaryTitle() {
+      $class = '\Drupal\views\Plugin\views\row\rendering\\' . Container::camelize($this->options['rendering_language']);

Container::camelize seems like a bit of magic, but its internal so, ok.

Why do we need the \\ though? My only concern left :)

Because a single backslash would escape the closing single quote and you'd get a syntax error because of the unclosed string literal.

OMG right :) So looks good. Resolved the multiple view problem, has options and stuff and @dawehner and others also liked it :) What else would one need? :)

Unassign from me

+1 to the RTBC

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -141,22 +178,31 @@ public function summaryTitle() {
   +      $class = '\Drupal\views\Plugin\views\row\rendering\\' . Container::camelize($this->options['rendering_language']);
   

   If this is not a plugin, it should not be living in the plugin namespace. Also 'rendering' is a strange sub namespace, why not just render?

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -141,22 +178,31 @@ public function summaryTitle() {
   +      $this->renderer = new $class($this, $this->entityType);
   

   Seems like a better idea to pass in the view instead of the row plugin?

3. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -141,22 +178,31 @@ public function summaryTitle() {
   +   * Returns the current renderer.
   

   Needs some sort of @return

4. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/rendering/RendererBase.php
   @@ -0,0 +1,96 @@
   +  public function query(QueryPluginBase $query) {
   

   It's weird that a 'renderer' has a query method on it.

5. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/rendering/RendererBase.php
   @@ -0,0 +1,96 @@
   +    $entity_id = $row->{$this->rowPlugin->field_alias};
   
   +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/rendering/TranslationLanguageRenderer.php
   @@ -0,0 +1,63 @@
   +    $entity_id = $row->{$this->rowPlugin->field_alias};
   

   Why not just use the entity again? if a row plugin decides to not call the parent query method, it will break this I think?

6. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -128,6 +152,19 @@ protected function buildViewModeOptions() {
   +    $options = array(
   ...
   +    return $options;
   

   Could just return array() here.

7. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -128,6 +152,19 @@ protected function buildViewModeOptions() {
   +   * Returns the available rendering strategies for language-aware entities.
   

   @return

8. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
   @@ -112,6 +127,15 @@ public function buildOptionsForm(&$form, &$form_state) {
   +      '#title' => t('Rendering language'),
   
   @@ -128,6 +152,19 @@ protected function buildViewModeOptions() {
   +      'current_language_renderer' => t('Current language'),
   +      'default_language_renderer' => t('Default language'),
   +      'translation_language_renderer' => t('Translation language'),
   

   $this->t()

9. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
   @@ -246,6 +246,19 @@ function testTranslationRendering() {
   +      $this->assertText($values[$langcode]['title'][0]['value']);
   ...
   +    foreach ($this->langcodes as $langcode) {
   ...
   +    }
   

   All of this code but just this test coverage in a web test? Seems a bit scant... Also all this code is added to views, but this little bit of coverage is in node module.

Also, do we always assume an entity langcode will always be 'langcode' in its schema?

Thanks for the review, this should address #120, except for:

• 2: Well, by passing the EntityRow plugin we ensure we are dealing with an entity row plugin, this wouldn't work/make sense with, for instance, a field row plugin. Not sure whether there are better approaches to do this.
• 4: Unless I am missing something, we definitely need that method so I guess the only alternative is picking a different name, any suggestion?
• 5: I think I implemented the suggestion but I really didn't understand your concern :) Can you elaborate, please?
• 6: I have not a big experience with writing views tests, what strategy would you recommend? Unit tests or maybe DUTB?

Setting needs review for the bot.

+++ b/core/modules/views/lib/Drupal/views/Entity/Render/TranslationLanguageRenderer.php
@@ -0,0 +1,63 @@
+        $this->langcodeAlias = $query->addField($table_alias, 'langcode');

This is only an assumption from entities that extend ContentEntityBase?

2. But it's the EntityRow plugin that is using this. So that's not really possibly. Unless we are expecting people to try and do crazy things with this?!

4. It is just strange having something that works kind of like the query() method of views handlers, but not quite.

5. If for some reason a row plugin using this does not call the current code in RowPluginBase::query (which adds field_alias for the base field) then that code break straight away.

6. I'm not sure what sort of dependencies the language part of this patch has? We have base test classes for web tests and DUTB tests in views :) As usual, a DTUB test would be good.

@damiankloip: I think the assumption from entities that extend ContentEntityBase is to be resolved with #2143729: Entity definitions miss a language entity key, entities lack a key for language, so we need to hardcode it for now.

This fixes also #120.2 and #120.6, not sure what to do about #120.4.

+++ b/core/modules/views/lib/Drupal/views/Tests/Entity/RowEntityRenderersTest.php
@@ -0,0 +1,174 @@
+  /**
+   * A string for assert raw and text helper methods.
+   *
+   * @var string
+   */
+  protected $content;

Removed unused variable.

@damiankloip started working on #2143729: Entity definitions miss a language entity key there, not to be solved here :)

Some more code clean-up :)

Good points from damian before, thank you for the review.

1. +++ b/core/modules/views/lib/Drupal/views/Entity/Render/RendererBase.php
   @@ -17,11 +17,11 @@
   +   * The top object of a view.
   

   Let's call it "The view executable" or something else. The top object does not describe anything. What about "The view exeucutable which is set onto the entity"

2. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/RowEntityRenderersTest.php
   @@ -0,0 +1,174 @@
   +
   +  public static function getInfo() {
   

   You can also just put on there {@inheritdoc}

1: I tried to make it as explict as I could and fixed another PHP doc.

#2 was already fixed in #127, I forgot to mention it, sorry.

Hope this looks ok now :)

+++ b/core/modules/views/lib/Drupal/views/Entity/Render/RendererBase.php
@@ -17,7 +17,7 @@
+   * The view executable wrapping the view storage entity.

nice!

Tested and added some screenshots.

And immediately using the description/screenshot while looking at #2149649, thanks :)

Just a reroll

139: 2006606-language-view-duplicate-139.patch queued for re-testing.

Updated the new test after #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit.

The change was really minor so I think i'ts fair to move this back to RTBC.

+1

error: patch failed: core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php:51
error: core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php: patch does not apply

Rerolled

146: 2006606-language-view-duplicate-146.patch queued for re-testing.

I tested by patch in comment 146. But I got a php error on the front page:
Fatal error: Call to a member function get() on a non-object in core/modules/views/lib/Drupal/views/Entity/Render/TranslationLanguageRenderer.php on line 34

Going to take a look at this.

Updated the patch:

--- a/core/modules/user/user.module
+++ b/core/modules/user/user.module
@@ -174,7 +174,7 @@ function user_attach_accounts(array $entities) {
   }
   $uids = array_unique($uids);
   $accounts = user_load_multiple($uids);
-  $anonymous = drupal_anonymous_user();
+  $anonymous = entity_create('user', array('uid' => 0));
   foreach ($entities as $id => $entity) {
     if (isset($accounts[$entity->getOwnerId()])) {
       $entities[$id]->setOwner($accounts[$entity->getOwnerId()]);

In user_attach_accounts() if an anonymous user was being attached to an entity, it was using a UserSession object rather than a User object. Apparently there was no test coverage for this. This is now indirectly covered in RowEntityRenderersTest, but we might want to open a followup to add a specific test for attaching anonymous users to entities.

--- a/core/modules/views/lib/Drupal/views/Entity/Render/RendererBase.php
+++ b/core/modules/views/lib/Drupal/views/Entity/Render/RendererBase.php
@@ -42,10 +43,10 @@
    *
    * @param \Drupal\views\ViewExecutable $view
    *   The entity row being rendered.
-   * @param string $entity_type
+   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
    *   The entity type.
    */
-  public function __construct(ViewExecutable $view, $entity_type) {
+  public function __construct(ViewExecutable $view, EntityTypeInterface $entity_type) {
     $this->view = $view;
     $this->entityType = $entity_type;

There was a confusion between using a full EntityType object, or just the entity type as a string in the rendererBase. I chose to use the object, as this can be type hinted by PHP and it will throw exceptions when a string is passed by mistake. This is more error proof than using strings.

I am currently blocked on a test failure in RowEntityRenderersTest. When it is rendering the nodes and tries to retrieve the node URL in template_preprocess_node() it throws:

RouteNotFoundException: Route "node.view" does not exist

It is not clear to me why this route should not be available. The Entity::url() method it relies on has been added recently (in #2167641: EntityInterface::uri() should use route name and not path), but I reviewed the code in that issue and it does not seem to be the cause of this.

Let's see if the bot also has trouble finding the node.view route.

This should fix the exception

This should be better as it also fixes the renderable array returned by the default language renderer (test-coverage provided by the removed lines in the interdiff test hunk).

Yay, thanks for unbreaking this. This one would be one important baby to get into core :)

Updated the issue summary. The patch also has tests for both the UI rendering of the front page and specific tests for the newly added language rendering options in views.

@YesCT:

1. CurrentLanguageRenderer is one of the views render options added by the patch, it is exposed on the UI in views. This is used.
2-5: thanks!
6. All the test views have offset: '0' already in core on their pagers. Almost 30 of them :) Should be good as-is.

Back to RTBC then :)

Ok, ran through my testing again. Before the patch: "/" shows "English marrrggh" twice, "/fr" shows "French marrrghh" twice. After the patch, both views show "English marrrgh", "French marrggh" in that order. I commented that this was not particularly an improvement. :P But Gábor correctly pointed out that what this does is restore D7's behaviour.

Therefore, committed and pushed to 8.x. Thanks! Let's move ahead with the other follow-ups we found while testing this one. :)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
@@ -142,22 +188,34 @@ public function summaryTitle() {
+      $class = '\Drupal\views\Entity\Render\\' . Container::camelize($this->options['rendering_language']);
+      $this->renderer = new $class($this->view, $this->entityType);

Is there a follow up to clean up this lurker? This is kind of nasty (which was mentioned already), plus no one can add their own 'renderer'.

I don't think there's a follow-up yet. I'm not sure if there is a useful different way to render an entity, not sure if it really needs to be pluggable, but happy to discuss that :)

I wanted to clarify one more *why* we did this change. It's not about making this work the same way as it did in 6.x, that was quite a different situation and is only a side effect of this change and we have a follow-up in place to not display the same content multiple times by default.

There are at least 4 different ways to display translatable nodes (or any content) in views.

1. Display all translations even if that means nodes are displayed multiple times. (which is what we have now)
2. Display all nodes in the current content language or best available fallback)
3. Display all nodes that are translated into the current content language, without fallbacks. (by adding a filter for "current content language"
4. Display all translations for a specific language that might be the current content language or not. (by adding a filter for language X)

Before this patch, only option 2 and 3 were possible, because no matter what language you actually selected, entity rendering in views then always displayed in the current content language or best fallback. Now, you can configure views to work with all those options.

The fact that we right now do option 1. is IMHO not a deliberate decision, it's also just a side affect. The reason is that we filter on the promoted flag, this is now a "possibly translatable field", which means it's stored in a separate table for each available translation. So views adds a join to that table, and because we don't have any further conditions, we get a SQL result for every translation.

Now, we just need to decide what the default should be for core in the mentioned follow-up issue, 2 (by making the query distinct, so that each nid is only returned once) or 3 (by adding a condition on the current language).

Hope that helps :)

Yay, thanks!