Cannot use relationship for rendered entity on Views

@lauriii
Are you sure this is not a duplicate of #2451789: Entity reference joins to the wrong base table in views. ?

Choose to use rendered entity

So, do we talk about header or footer? I seriously, tried to rebuild the view, but I don't see how you are able to select a relationship for header/footer

Feel free to atttach a exported configuration of a view.

I totally forgot that this feature existed. Btw. did you hacked the config directly? I got some failures while doing that.

Let's see how much less we have.

1. +++ b/core/modules/views/src/Entity/Render/RendererBase.php
   @@ -85,15 +85,17 @@ public function query(QueryPluginBase $query) {
   -   * @param $result
   +   * @param array $result
   

   @param \Drupal\views\ResultRow[] $result?

2. +++ b/core/modules/views/src/Entity/Render/RendererBase.php
   @@ -85,15 +85,17 @@ public function query(QueryPluginBase $query) {
   +      $entity = $relationship === 'none' ? $row->_entity : $row->_relationship_entities[$relationship];
          $entity->view = $this->view;
          $this->build[$entity->id()] = $view_builder->view($entity, $this->view->rowPlugin->options['view_mode'], $this->getLangcode($row));
   

   If the relationship is not required, the $entity might not be available.

3. +++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
   @@ -47,12 +47,12 @@ public function query(QueryPluginBase $query) {
   -      $entity = $row->_entity;
   +      $entity = $relationship === 'none' ? $row->_entity : $row->_relationship_entities[$relationship];
   

   same as above

4. I think we need to make a similar change to *Renderer::render()?
5. How about adding ResultRow::getEntity($relationship)?

Thank you for you review!!

@param \Drupal\views\ResultRow[] $result?

Good idea. I still love that syntax.

How about adding ResultRow::getEntity($relationship)?

Good idea!

IMHO this is at least major

+++ b/core/modules/views/src/Entity/Render/RendererBase.php
@@ -85,17 +85,19 @@ public function query(QueryPluginBase $query) {
+      if ($entity = $this->getEntity($row, $relationship)) {

I was thinking about ResultRow::getEntity() which would make this if ($entity = $row->getEntity($relationship)) { but this works too. =)

Needs work for the render methods (RendererBase::render(), TranslationLanguageRenderer::render()) that are using $row->_entity->id(). I guess the test passes because both entities there have the same id.

Do you think TranslationLanguageRenderer::query() and DefaultLanguageRenderer::getLangcode() should also use the relationship? Maybe pass it to constructor?

Needs work for the render methods (RendererBase::render(), TranslationLanguageRenderer::render()) that are using $row->_entity->id(). I guess the test passes because both entities there have the same id.

Good point! Do you want to fix that?

Do you think TranslationLanguageRenderer::query() and DefaultLanguageRenderer::getLangcode() should also use the relationship? Maybe pass it to constructor?

You are absolutely right, they both need it as well.

Added relationship parameter to render and getLangcode methods.

I will review this patch this weekend.

Simplytest me came back with an error while applying the patch. Retesting.

So it looks like I will reroll that patch this weekend

I'll try to reroll it.

Rerolled.

Not sure what happened there, seems like there were edits to issue during upload and somehow it attached duplicate?

However, i guess we can ignore the reroll since some other patch was submitted?

Is this still broken? I was unable to reproduce it.

I created two content types
article
page

I added an entity reference field to the page that referenced the article.
I added article content and then I created a page that referenced that article content.

I created a view that adds the article content through the relation and then I set it to render full content and it worked.

So I am not sure what the steps are to reproduce.

I could totally imagine that we fixed that problem with some of our field rendering related work ... Does some mind to post a test only patch, to see whether its still broken?

Removed RowEntityTest and 'entity' from modules. This fails locally so removing novice tag.

Another attempt to reroll #13. The interdiff is for #26.

I asked back in #31 if anyone was still able to reproduce this bug after some of the other changes that where done. I am still unable to reproduce. I would be able to help if someone said more about what the problem is and if it is still happening on the latest dev.

Well, I guess @lauriii should be able to tell us, whether the problem still exists ...

@lauriii, Do you have any cm yaml files that we can use to reproduce this? I think tests would also be good for this issue.

I can also confirm that Patch #31 is fixing this issue.

@Cyberschorsch, can you confirm that the error happens before you applied the patch.

+++ b/core/modules/views/src/Entity/Render/EntityTranslationRendererBase.php
@@ -33,27 +35,62 @@ public function query(QueryPluginBase $query, $relationship = NULL) {
+
+  /**
+   * Gets the entity assosiated with a row.
+   *
+   * @param \Drupal\views\ResultRow $row
+   *   The result row.
+   * @param string $relationship
+   *   (optional) The relationship.
+   *
+   * @return \Drupal\Core\Entity\EntityInterface|null
+   *   The entity might be optional, because the relationship entity might not
+   *   always exist.
+   */
+  protected function getEntity($row, $relationship = 'none') {
+    if ($relationship === 'none') {
+      return $row->_entity;
+    }
+    elseif (isset($row->_relationship_entities[$relationship])) {
+      return $row->_relationship_entities[$relationship];
+    }
+    return NULL;
   }

I'm curious whether we should move this function into a trait as \Drupal\views\Plugin\views\field\FieldPluginBase::getEntity is the exact same code

Simple reroll for now.

I'm curious whether we should move this function into a trait as \Drupal\views\Plugin\views\field\FieldPluginBase::getEntity is the exact same code

Should that be something like RelationshipHandlerTrait? Or should it not be dedicated to Relationships and maybe be added in some existing trait (not finding a decent one).

1. index 697ae8a..ce96c49 100644
   --- a/core/modules/views/src/Entity/Render/ConfigurableLanguageRenderer.php
   
   --- a/core/modules/views/src/Entity/Render/ConfigurableLanguageRenderer.php
   +++ b/core/modules/views/src/Entity/Render/ConfigurableLanguageRenderer.php
   
   +++ b/core/modules/views/src/Entity/Render/ConfigurableLanguageRenderer.php
   @@ -44,7 +44,7 @@ public function __construct(ViewExecutable $view, LanguageManagerInterface $lang
   
   @@ -44,7 +44,7 @@ public function __construct(ViewExecutable $view, LanguageManagerInterface $lang
      /**
       * {@inheritdoc}
       */
   -  public function getLangcode(ResultRow $row) {
   +  public function getLangcode(ResultRow $row, $relationship = 'none') {
        return $this->langcode;
   

   Given that the language renderers cannot be extended this is much less of a BC break

2. +++ b/core/modules/views/src/Entity/Render/EntityTranslationRendererBase.php
   @@ -33,27 +35,62 @@ public function query(QueryPluginBase $query, $relationship = NULL) {
   -  public function preRender(array $result) {
   +  public function preRender(array $result, $relationship = 'none') {
   

   Just curious, is it really valid to extend the list of parameters from an interface?

Reroll, and want to see the test only patch.

+++ b/core/modules/views/src/Tests/Plugin/EntityRowTest.php
@@ -0,0 +1,101 @@
+class EntityRowTest extends ViewKernelTestBase {

+++ /dev/null
@@ -1,71 +0,0 @@
-class RowEntityTest extends ViewKernelTestBase {

Are we renaming this existing test? If so can we not do that in this patch? It makes it difficult to review, which I'm trying to do to figure out how to reproduce this. See https://www.drupal.org/core/scope for more information about scoping core issues.

Also, exact steps to reproduce through the UI would really help here.

NW for #56 though.

Steps to reproduce (based on Standard profile):

1. Add an entity reference field named "field_content" to the "page" content type. Allow content > article references.
2. Create and edit a content view.
3. Add a relationship for "Content using field_content."
4. Change row style plugin to "Content" and hit "Apply".
5. An AJAX error occurs.

The AJAX error is caused by fatal error triggered when RowPluginBase::buildOptionsForm() calls RelationshipPluginBase::init() while providing a wrong argument type (Array instead of DisplayPluginBase).

+++ b/core/modules/views/src/Plugin/views/row/RowPluginBase.php
@@ -95,7 +95,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+          $relationship_handler->init($executable, $this->displayHandler, $relationship);

For the record: As far as I can tell this change alone fixed the problem caused by the steps listed in the previous comment.

Are we renaming this existing test? If so can we not do that in this patch? It makes it difficult to review, which I'm trying to do to figure out how to reproduce this. See https://www.drupal.org/core/scope for more information about scoping core issues.

Sure, but sometimes its sad when this was really the only reason this blocked the patch. Always remember that fixing bug delivers actually value to users and well, this change was made because it was right. The class we test is called EntityRow and not RowEntity. It is always hard to treat one value against the other.

There we go.

Discussed with @xjm, @tim.plunkett and @dawehner - we decided that this issue is a major issue which you are able to configure a view through the UI that renders unexpected results.

I had the same issues as #59, the patch from #63 solved my issue!

Is there a reason why 'none' (an arbirtary string) was chosen over a NULL or a constant?

@esolitos
'none' is the default value provided in \Drupal\views\Plugin\views\HandlerBase::defineOptions

I can confirm, the patch from #63 also fixed an issue I was having with relationships in a view.

@esolitos
Ha, I thought I've totally seen you online before. Cool to meet you last week. Do you think the patch looks alright?

@dawehner
Since my team switched to Slack I always forget to open irc... It was nice to meet you as well, it's nice to put a face and a voice to the d.o usernames. :)

Back on the topic, the patch looks good and fixes the issue in all tests I did and I'm using it in production on a couple of sites w/o issues.
The only thing I still find weird is that magic 'none' string in the relationship, I'll open an issue to get the magic string into a constant, I believe it makes more sense.

I did not review the code but the patch in #63 fixes the problem. Do we need tests?

The only thing I still find weird is that magic 'none' string in the relationship, I'll open an issue to get the magic string into a constant, I believe it makes more sense.

A constant is a good idea! Do you mind opening up the followup?

I did not review the code but the patch in #63 fixes the problem. Do we need tests?

This patch adds a test, but I'm not sure if provides coverage for all changes added by this patch.

Back to RTBC, this seemed to be a random failure

Are these fails still random?

Yes they are

This is an API break no? https://3v4l.org/2v5JI

Well, we actively haven't made these entity renderers swappable ... so we can change internals.

@dawehner fair enough - the classes involved here are all hard coded in \Drupal\views\Entity\Render\EntityTranslationRenderTrait and \Drupal\views\Plugin\views\field\Field::getEntityFieldRenderer. It is possible to swap this out at the higher level, since Drupal\views\Plugin\views\field\Field is plugin and you can alter it. But I think it is okay to consider this internal implementation to the plugin.

+++ b/core/modules/views/src/Plugin/views/row/RowPluginBase.php
@@ -90,7 +90,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-          $relationship_handler->init($executable, $relationship);
+          $relationship_handler->init($executable, $this->displayHandler, $relationship);

This is a bug fix right? Is this never called ATM? As far as I can this is just broken in HEAD

This is a bug fix right? Is this never called ATM? As far as I can this is just broken in HEAD

Exactly, this is what this issue is all about.

DisplaySuite Plugin is breaking if you apply the patch.

@ayalon
Can you explain a bit more how its breaking?

As soon as I open a page based on Display Suite I get this error after applying the patch:

Fatal error: Declaration of Drupal\ds\Plugin\views\Entity\Render\TranslationLanguageRenderer::getLangcode(Drupal\views\ResultRow $row) must be compatible with Drupal\views\Entity\Render\EntityTranslationRendererBase::getLangcode(Drupal\views\ResultRow $row, $relationship = 'none')

The attached patch is needed to make #63 patch compatible with Display Suite.

It seems DS does exactly what is being described in #77-#79, so would this be considered an API/BC break after all with examples available in contrib?

Retested the patch in #63 and its still good, so if this is still not considered an API change this can go back to RTBC I'd say.

But still, every page that is using Display Suite will have a white screen of death if the patch I posted is not used.

Comment 84 adds a patch for Display suite, not core. Please open a new issue with that patch, for Display Suite. This core ticket is RTBC, as of comment 76, so I'm setting it back to that.

#84 could be added to DS before this patch landing. That sounds like an okay way.
To be honest these entity renders where meant to be purely internal.

I created a critical DS issue: #2806729: Entity field renderers are adding relationships.

+++ b/core/modules/views/src/Entity/Render/EntityTranslationRendererBase.php
@@ -15,11 +15,13 @@
-  abstract public function getLangcode(ResultRow $row);
+  abstract public function getLangcode(ResultRow $row, $relationship = 'none');

This is pain and potentially could break BC for more than just display suite. I think we should consider a fix that does not break contrib. Maybe we could make the method not abstract and provide a default implementation. That way we can avoid the hard break. Putting back to needs review to get sub-system maintainer opinions.

I.e do the attached patch...

I'd be honestly totally happy about this particular change. In general I really like that we haven't made these renderes another plugin type as it saved us from overgenerialization.

@ayalon any chance you can confirm that the new approach taken allows display suite to continue to work?

+++ b/core/modules/views/src/Entity/Render/EntityTranslationRendererBase.php
@@ -28,27 +34,62 @@ public function query(QueryPluginBase $query, $relationship = NULL) {
+   * Gets the entity assosiated with a row.

associated - can be fixed on commit.

Otherwise looks great but yeah let's get confirmation that display suite is happy.

@alexpott:
Thanks for the non breaking patch. I removed my display suite patch and reverted Display Suite to 8.2.6 version. Then I applied your latest patch. Generallyit works but there are strict warnings on every page:

Strict warning: Declaration of Drupal\ds\Plugin\views\Entity\Render\RendererBase::preRender() should be compatible with Drupal\views\Entity\Render\EntityTranslationRendererBase::preRender(array $result, $relationship = 'none') in Composer\Autoload\includeFile() (line 412 of htdocs\xx\vendor\composer\ClassLoader.php).

Strict warning: Declaration of Drupal\ds\Plugin\views\Entity\Render\TranslationLanguageRenderer::getLangcode() should be compatible with Drupal\ds\Plugin\views\Entity\Render\DefaultLanguageRenderer::getLangcode(Drupal\views\ResultRow $row, $relationship = NULL) in include() (line 11 of modules\contrib\ds\src\Plugin\views\Entity\Render\TranslationLanguageRenderer.php).

So this means that additional patching of Display Suite is necessary anyway.

So is this ds's problem to fix the warning, or does this imply something needs to be changed here?

The issue has to be fixed here. The notice is coming from the latest patch of @alexpott. With php strict you cannot do what has been done.

Well, there is no way to fix that properly without changing the signature IMHO. Here is a suggestion, let's fix this bug just in 8.3.x

So has a decision been made here about the patch and display suite?

Does anyone know if there are other modules out there besides display suite that create view content displays in views that this patch could impact?

It would be great to hear back from the maintainers from Display Suite and get this one patched As soon as possible because its a critical issue.

At this point am I supposed to just apply the patches separately?

thank

@ayalon I'm not seeing any strict warnings with Display Suite 8.2.6 (unpatched) with Drupal core 8.2.4 using the following patches:

        "Fix views_query_views_alter() - https://www.drupal.org/node/2744069#comment-11557945": "https://www.drupal.org/files/issues/2744069-5-views-in-queries.patch",
        "Fix issue with quickedit - https://www.drupal.org/node/2661880#comment-11443453": "https://www.drupal.org/files/issues/quickedit-undefined-attr-2661880-14.patch",
        "Fix module installer - https://gist.github.com/Decipher/8a7dc379e0b7a8fc0e64ec584b309008": "https://gist.githubusercontent.com/Decipher/8a7dc379e0b7a8fc0e64ec584b309008/raw/0b7d4ea90480aef060176bb88ee49a476e0a80b0/fix_module_installer.patch",
        "Exposed date filter notice - https://www.drupal.org/node/2811887#comment-11695313": "https://www.drupal.org/files/issues/2811887-2-exposed-date-restore-type-ealier.patch",
        "Views Argument Tokens fail validation in More Link - https://www.drupal.org/node/2780891#comment-11503811": "https://www.drupal.org/files/issues/views-ignore_invalid_tokens_in_replace-2780891-6-D8.1.x.patch",
        "Cannot use relationship for rendered entity on Views - https://www.drupal.org/node/2457999#comment-11681019": "https://www.drupal.org/files/issues/2457999-93.patch"

@realityloop if I get ds 8.x-2.x and apply the patch to core 8.2.x and then run the Drupal\ds\Tests\BlockFieldPluginTest test I see the warnings that @ayalon is reporting. Not sure what to do here.

My two cents: go ahead and "break" the API, the result is a warning "only" after all. This will let contrib developers fix their code quite conveniently. Essentially what @dawehner is saying in #100.

Note that at least Search API will also be affected by this change. I'm about to add an issue there much alike the one in DS.

@alexpott
So what happens if we fix this just in the next minor release, but keep the current patch release as it is. Would that be a safe change?

When this will be fixed on 8.3.x? I'm using the 8.3.0-beta1 and I'm experiencing the related issue #2806015: Recoverable fatal error: Argument 2 passed to Drupal\\views\\Plugin\\views\\relationship\\RelationshipPluginBase::init() must be an instance of Drupal\\views\\Plugin\\views\\display\\DisplayPluginBase

I think it is worth breaking the BC for next minor release. We can create an announcement for the change.

The full BC version (with thanks to @Xano for helping me wrap my head around this). Adds methods for the 'with relationship' versions and leaves the original methods intact.

Ran this against \Drupal\ds\Tests\ViewsTest and it ran without exceptions.

The #93 is an array() => [] reroll, the interdiff is against the reroll (the interdiff is hard to read I thought).

This probably needs work for at least some docblocks that need updates (and maybe some @depricated in some places), but the question is, is this preferable to a BC break?

And now with the actual patch.....

This should clear up some fails.

+++ b/core/modules/views/tests/src/Kernel/Plugin/RowEntityTest.php
@@ -34,27 +34,55 @@ class RowEntityTest extends ViewsKernelTestBase {
+    // Ensure entities have different ids.
+    if ($entity_test->id() == $user->id()) {
+      $entity_test->delete();
+      $entity_test = EntityTest::create([
+        'user_id' => $user->id(),
+        'name' => 'test entity test',
+      ]);
+      $entity_test->save();
+    }

Is it really possible that entities could have same id?

@lokapujya they are different entity types, so I would have thought that is quite possible, yes?

Yeah, I didn't notice that they were different entity types.

This patch fixes another problem, not sure if everyone is aware of it:

Without this patch,I created a view. Added a relationship. Then, clicked on Format->Show: Content. That brings up the "Page: How should each row in this view be styled" screen. Then, when I click "settings", nothing happens.

After this patch, the "Page: Row style options" screen will show up after clicking "settings".

I like that.

Discussed this direction with @dawehner at DevDays.

Made the 'byRelationship' methods so that the relationship is non-optional and made the original methods use the 'byRelationship' methods to reduce code duplication.

Great progress! Thanks lendude for your work. #119 works for me.

+++ b/core/modules/views/src/Plugin/views/row/EntityRow.php
@@ -166,7 +166,13 @@ public function summaryTitle() {
+      $relationship = NULL;

@@ -175,7 +181,7 @@ public function query() {
+      $this->getEntityTranslationRenderer()->preRenderByRelationship($result, isset($this->options['relationship']) ? $this->options['relationship'] : 'none');

Once its none and once NULL, this is confusing

@dawehner would something like this be better? They are actually 2 different things. Most of EntityRow is referring to the relationship handler, but query is referring to the relationship table or alias. Hence the different defaults.

This would make the variable name in the calling function different then that used in the called function, which is not great either, but updating query() seems a little out of scope :)

How about this?

+1 for $relationship_table and the simplification introduced by @jibran

+++ b/core/modules/views/src/Entity/Render/EntityTranslationRendererBase.php
@@ -22,6 +22,23 @@
   /**
+   * Returns the language code for the given row based on the relationship.
+   *
+   * @param \Drupal\views\ResultRow $row
+   *   The result row.
+   * @param string $relationship
+   *   The relationship to be used.
+   *
+   * @return string
+   *   A language code.
+   */
+  public function getLangcodeByRelationship(ResultRow $row, $relationship) {
+    if ($entity = $this->getEntity($row, $relationship)) {
+      return $entity->getUntranslated()->language()->getId();
+    }
+  }

As far as I can tell, this is overridden by every single child class, so I'm not sure what the value of this is.

I only stumbled on this because of the getUntranslated() call which seems very suspect to me as there is no comment there to justify it and as far as I can tell no test for this particular line either.

So I think we can either just drop this function altogether or I would prefer some justification (i.e. code comment and/or test) for this.

What does the relationship have to do with language? Yes, entity has to be rendered in a particular language, but what does the relationship have to do with selecting the language?

Moved the steps to repro (from @ciss in #59) into the issue summary.

We have been using the patch in #93. After a security update we tried using the new patch in #123, but it causes fatal errors with Views Parity Row

Fatal error: Call to undefined method Drupal\views_parity_row\Plugin\views\Entity\Render\TranslationLanguageRenderer::preRenderByRelationship() in Drupal\views\Plugin\views\row\EntityRow::preRender()

The patch in #93 does not cause this problem. It seems that this might be an API change and the

preRenderByRelationship()

method either needs to be added to Drupal\views\Entity\Render\RenderBase or Drupal\views\Entity\Render\EntityTranslationRendererBase. Maybe even add a check in Drupal\views\Plugin\views\row\EntityRow::preRender() to check that function exists or that it is a subclass of the correct super class; which would mean that Views Parity Row would need to be updated.

Is it okay to ask what is going on with this issue? It seems to me that the main problem is gone, or at least I could get the view to work properly but as stated here in#41 and #117, when you have a relationship for entity reference in place, you can't change the view mode that you want your content to appear, it gives an Ajax error. If you get it right the first time, no problem, but if you want to change it, you have to remove the relationship, change the view mode and put the relationship back. So it's not a major flaw, but pretty annoying...

@tstoeckler how about this? just provide the minimum we need to? make getLangcodeByRelationship() default to getLangcode()

We can't take the method out, because that would break things when modules extend EntityTranslationRendererBase but not implement the method, right?

@jojonaloha Views Parity Row \Drupal\views_parity_row\Plugin\views\Entity\Render\TranslationLanguageRenderer should extend \Drupal\views\Entity\Render\TranslationLanguageRenderer and maybe not Drupal\views\Entity\Render\RendererBase like it currently does. That should clean things up. But don't know the reason behind that setup, so that might not work.

We can't take the method out, because that would break things when modules extend EntityTranslationRendererBase but not implement the method, right?

Should it be declared abstract?

Looked at this in DrupalCon Vienna with @Lauriii and this issue needs an updated issue summary. In comment #41 there is a explanation about what is still left from the original issue.

Re-rolled. #132 didn't apply anymore.

Looks like #133 hasn't been answered yet; also maybe another review from @tstoeckler could be helpful? Thanks!

+++ b/core/modules/views/src/Entity/Render/ConfigurableLanguageRenderer.php
@@ -43,4 +43,11 @@ public function getLangcode(ResultRow $row) {
+  /**
+   * {@inheritdoc}
+   */
+  public function getLangcodeByRelationship(ResultRow $row, $relationship) {
+    return $this->getLangcode($row);
+  }

+++ b/core/modules/views/src/Entity/Render/DefaultLanguageRenderer.php
@@ -16,4 +16,11 @@ public function getLangcode(ResultRow $row) {
+  /**
+   * {@inheritdoc}
+   */
+  public function getLangcodeByRelationship(ResultRow $row, $relationship) {
+    return $this->getLangcode($row);
+  }

+++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
@@ -107,6 +124,13 @@ public function getLangcode(ResultRow $row) {
+  /**
+   * {@inheritdoc}
+   */
+  public function getLangcodeByRelationship(ResultRow $row, $relationship = 'none') {
+    return $this->getLangcode($row);
+  }

As far as I can tell, these can now be removed, as they are the same as the parent implementation.

This would also alleviate #133.

(If not, I have a question: Why does TranslationLanguageRenderer have a default value for the $relationship parameter, while the others do not?)

Setting to needs work for that, will try to do a reroll.

Just run into this myself. It's worth pointing out that this results in a fatal error and prevents you from chosing a view mode even if you don't want to actually use a relationship, just having one on your view triggers this.

Confirming the problem still exists.

Addresses #141 which is supposed to fix #133. If the tests are green and interdiff show good implementation of #141, then someone can set to RTBC per #124 and #136.

Nice simplification!

Perfect, confirming RTBC. Can we get this into the next stable release?

Patch looks really great now, thanks for all the effort!

Sorry, but found one more thing:

+++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
@@ -80,14 +80,22 @@ protected function getLangcodeTable(QueryPluginBase $query, $relationship) {
   public function preRender(array $result) {
+    $this->preRenderByRelationship($result, 'none');

@@ -95,9 +103,18 @@ public function preRender(array $result) {
   public function render(ResultRow $row) {
...
+    $this->renderByRelationship($row, 'none');
+  }

This can be removed now, I think, as it's identical to the parent implementation.

Fixed #150

Sorry, hit limit of my git patch fu. Can't wait for the developer tooling initiative for drupal.org ...

Encoding weirdness. Interdiff same as #150.

@tstoeckler
Nice find!

Thanks for the quick fix! RTBC++

Silly bot, saying it failed when it didn't

Patch 2457999-153.patch working for me, thank you @jonathanshaw !

Drupal version: 8.4.3
PHP 7.1.8

Can confirm that #154 works brilliantly.

Patch in #154 working well for me, many thanks.

Drupal 8.4.5
PHP 7.1.11

+++ b/core/modules/views/src/Entity/Render/EntityTranslationRendererBase.php
@@ -21,6 +21,21 @@
+  /**
+   * Returns the language code for the given row based on the relationship.
+   *
+   * @param \Drupal\views\ResultRow $row
+   *   The result row.
+   * @param string $relationship
+   *   The relationship to be used.
+   *
+   * @return string
+   *   A language code.
+   */
+  public function getLangcodeByRelationship(ResultRow $row, $relationship) {
+    return $this->getLangcode($row);
+  }

Maybe this is old code from an alternative fix but I don't understand why we're adding this method as getLancode() is public too. And none of the calls are from another class so even if it was protected that wouldn't matter. Ah yeah this is because we started removing stuff after #141.

Is there anything more we can trim here?

Also we need a change record here for the changes to \Drupal\views\Entity\Render\EntityTranslationRendererBase. It'd be great though if we could somehow come up with a way of fixing that was less disruptive.

I can confirm the patch works perfectly with Drupal Core 8.5

patch worked for me in 8.5.1 as well

Patch also worked for me on 8.5.1

CR added https://www.drupal.org/node/2959032

Went through this again, and I can't find anything more we can trim.

It'd be great though if we could somehow come up with a way of fixing that was less disruptive.

Agreed, but I think we went through quite a number of options here and this was the only way we found not to break major contrib modules.

I gave it another review. +1 for all of it

Resetting to RTBC. This seems to be a testbot mistake...
How can we ensure this important fix will be part of 8.6?

Failed because of broken HEAD: #2960013: i18n menu links translation in wrong directory.