Problem/Motivation

You cannot create a view and try to list rendered entities using relationship.

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." or "Content referenced from field_content."
  4. The row style plugin is "Content" by default.
  5. Click on "Teaser" in order to change the view mode, nothing happens.
  6. You can't choose a view mode even if you don't want to actually use a relationship, just having one on your view triggers this.

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

Argument 2 passed to Drupal\views\Plugin\views\relationship\RelationshipPluginBase::init() must be an instance of Drupal\views\Plugin\views\display\DisplayPluginBase, array given, called in /app/web/core/modules/views/src/Plugin/views/row/RowPluginBase.php on line 93

Proposed resolution

Fix it, by passing along the relationship as needed and using it.

Remaining tasks

None.

User interface changes

None

API changes

None

 

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions Extract the test code + yml file out of the patch in #26
CommentFileSizeAuthor
#154 2457999-153.patch14.63 KBjonathanshaw
#151 2457999-150.patch14.99 KBjonathanshaw
#151 interdiff-2457999-145-150.txt950 bytesjonathanshaw
#145 interdiff-2457999-138-145.txt1.93 KBjonathanshaw
#145 2457999-145.patch14.85 KBjonathanshaw
#138 interdiff-2457999-132-138.txt2.42 KBdagmar
#138 2457999-138.patch16.45 KBdagmar
#132 2457999-132.patch16.47 KBLendude
#132 interdiff-2457999-123-132.txt618 bytesLendude
#123 cannot_use_relationship-2457999-123.patch16.5 KBjibran
#123 interdiff-0542f8.txt788 bytesjibran
#122 2457999-122.patch16.52 KBLendude
#122 interdiff-2457999-119-122.txt898 bytesLendude
#119 2457999-119.patch16.5 KBLendude
#119 interdiff-2457999-113-119.txt5.81 KBLendude
#113 2457999-113.patch15.74 KBLendude
#113 interdiff-2457999-109-113.txt2.28 KBLendude
#110 2457999-109.patch14.62 KBLendude
#110 2457999-93-array-reroll.patch16.33 KBLendude
#110 interdiff-2457999-93-109.txt8.85 KBLendude
#93 2457999-93.patch16.34 KBalexpott
#93 63-93-interdiff.txt1.48 KBalexpott
#84 display_suite_renderer.patch2.92 KBayalon
#63 2457999-63.patch16.29 KBdawehner
#61 2457999-61.patch16.29 KBdawehner
#51 2457999-51.patch18.06 KBlokapujya
#51 2457999-51-test-only.patch4.86 KBlokapujya
#48 2457999-48.patch18.12 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,461 pass(es). View
#37 interdiff.txt15.17 KBolli
#37 2457999-37.patch18.16 KBolli
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2457999-37.patch. Unable to apply patch. See the log in the details link for more information. View
#35 interdiff.txt3.11 KBolli
#35 2457999-35-tests.patch5.94 KBolli
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,116 pass(es), 2 fail(s), and 4 exception(s). View
#33 cannot_use_relationship-2457999-test-only-33.patch3.58 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,928 pass(es), 3 fail(s), and 3 exception(s). View
#26 cannot_use_relationship-2457999-26.patch8.2 KBNitesh Sethia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,065 pass(es), 961 fail(s), and 995 exception(s). View
#23 2457999-20.patch31.25 KBbfr
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#23 2457999-20.patch31.25 KBbfr
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
#13 interdiff.txt7.99 KBolli
#13 2457999-13.patch15.86 KBolli
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2457999-13.patch. Unable to apply patch. See the log in the details link for more information. View
#9 interdiff.txt3.32 KBdawehner
#9 2457999-9.patch10.5 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,066 pass(es). View
#7 2457999-7.patch9.37 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,170 pass(es). View
#7 interdiff.txt671 bytesdawehner
#5 2457999-5.patch9.32 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,621 pass(es), 372 fail(s), and 477 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner’s picture

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

lauriii’s picture

Yes. This existed before that other issue existed. I have also tested this with the patch from that other issue and it's still broken.

dawehner’s picture

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.

lauriii’s picture

So I mean rendered views row. You are able to select the entity be used as relationship but it will still render the entity without relationship

dawehner’s picture

Status: Active » Needs review
FileSize
9.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,621 pass(es), 372 fail(s), and 477 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 5: 2457999-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
671 bytes
9.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,170 pass(es). View

Let's see how much less we have.

olli’s picture

Issue tags: +VDC
  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)?
dawehner’s picture

FileSize
10.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,066 pass(es). View
3.32 KB

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!

dawehner’s picture

Priority: Normal » Major

IMHO this is at least major

olli’s picture

Status: Needs review » Needs work
+++ 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?

dawehner’s picture

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.

olli’s picture

Status: Needs work » Needs review
FileSize
15.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2457999-13.patch. Unable to apply patch. See the log in the details link for more information. View
7.99 KB

Added relationship parameter to render and getLangcode methods.

frob’s picture

Assigned: Unassigned » frob

I will review this patch this weekend.

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

frob queued 13: 2457999-13.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2457999-13.patch, failed testing.

frob’s picture

So it looks like I will reroll that patch this weekend

lauriii’s picture

Issue tags: +Needs reroll

Tutorial how to do a reroll: https://www.drupal.org/patch/reroll

bfr’s picture

Assigned: frob » bfr

I'll try to reroll it.

Nitesh Sethia’s picture

Assigned: bfr » Nitesh Sethia
Nitesh Sethia’s picture

Nitesh Sethia’s picture

Assigned: Nitesh Sethia » Unassigned
bfr’s picture

Status: Needs work » Needs review
FileSize
31.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
31.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Rerolled.

The last submitted patch, 23: 2457999-20.patch, failed testing.

bfr’s picture

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?

Nitesh Sethia’s picture

FileSize
8.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,065 pass(es), 961 fail(s), and 995 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 26: cannot_use_relationship-2457999-26.patch, failed testing.

The last submitted patch, 23: 2457999-20.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 26: cannot_use_relationship-2457999-26.patch, failed testing.

frob’s picture

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.

dawehner’s picture

Issue summary: View changes
Issue tags: +Novice

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?

lauriii’s picture

Status: Needs work » Needs review
FileSize
3.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,928 pass(es), 3 fail(s), and 3 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 33: cannot_use_relationship-2457999-test-only-33.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
5.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,116 pass(es), 2 fail(s), and 4 exception(s). View
3.11 KB

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

Status: Needs review » Needs work

The last submitted patch, 35: 2457999-35-tests.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
FileSize
18.16 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2457999-37.patch. Unable to apply patch. See the log in the details link for more information. View
15.17 KB

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

frob queued 37: 2457999-37.patch for re-testing.

frob’s picture

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.

dawehner’s picture

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

lauriii’s picture

Seems to be still problem at least on some level since I'm not even able to configure which display mode should be printed after adding a relationship for the view. Patch attached to this issue fixes that and also makes it possible to print referenced rendered entities

frob’s picture

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

Cyberschorsch’s picture

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

frob’s picture

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

dawehner’s picture

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

dawehner queued 37: 2457999-37.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 37: 2457999-37.patch, failed testing.

geertvd’s picture

Status: Needs work » Needs review
FileSize
18.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,461 pass(es). View

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

Status: Needs review » Needs work

The last submitted patch, 48: 2457999-48.patch, failed testing.

dawehner’s picture

  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?

lokapujya’s picture

Status: Needs work » Needs review
FileSize
4.86 KB
18.06 KB

Reroll, and want to see the test only patch.

The last submitted patch, 51: 2457999-51-test-only.patch, failed testing.

no_angel’s picture

The last submitted patch, 51: 2457999-51-test-only.patch, failed testing.

dawehner’s picture

Issue summary: View changes
xjm’s picture

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

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs steps to reproduce

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

NW for #56 though.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ciss’s picture

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

ciss’s picture

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

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs steps to reproduce
FileSize
16.29 KB

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.

Status: Needs review » Needs work

The last submitted patch, 61: 2457999-61.patch, failed testing.

dawehner’s picture

There we go.

alexpott’s picture

Issue tags: +Triaged D8 major

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.

esolitos’s picture

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?

dawehner’s picture

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

vivianspencer’s picture

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

dawehner’s picture

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

esolitos’s picture

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

davidparedes21’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

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

dawehner’s picture

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 63: 2457999-63.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC, this seemed to be a random failure

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 63: 2457999-63.patch, failed testing.

jonathanshaw’s picture

Are these fails still random?

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

Yes they are

alexpott’s picture

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

dawehner’s picture

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

alexpott’s picture

@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

dawehner’s picture

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.

ayalon’s picture

DisplaySuite Plugin is breaking if you apply the patch.

dawehner’s picture

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

ayalon’s picture

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')

ayalon’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 84: display_suite_renderer.patch, failed testing.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

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.

ayalon’s picture

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

james.williams’s picture

Status: Needs work » Reviewed & tested by the community

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.

dawehner’s picture

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

jonathanshaw’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review
+++ 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.

alexpott’s picture

I.e do the attached patch...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

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.

alexpott’s picture

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

catch’s picture

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

ayalon’s picture

Status: Reviewed & tested by the community » Needs work

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

jonathanshaw’s picture

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

ayalon’s picture

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.

dawehner’s picture

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

kclarkson’s picture

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

realityloop’s picture

Status: Needs work » Reviewed & tested by the community

@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"
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

kekkis’s picture

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.

dawehner’s picture

@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?

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

rcodina’s picture

jibran’s picture

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

Lendude’s picture

Status: Needs work » Needs review

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?

Lendude’s picture

The last submitted patch, 110: 2457999-93-array-reroll.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 110: 2457999-109.patch, failed testing.

Lendude’s picture

This should clear up some fails.

lokapujya’s picture

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

james.williams’s picture

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

lokapujya’s picture

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

lokapujya’s picture

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

dawehner’s picture

I like that.

Lendude’s picture

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.

ayalon’s picture

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

dawehner’s picture

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

Lendude’s picture

@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 :)

jibran’s picture

FileSize
788 bytes
16.5 KB

How about this?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

tstoeckler’s picture

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

lokapujya’s picture

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?

mikeker’s picture

Issue summary: View changes

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

mikeker’s picture

Issue summary: View changes
jojonaloha’s picture

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.

LauraRocks’s picture

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

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

lokapujya’s picture

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?

LauraRocks’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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.

vilepickle’s picture

Issue summary: View changes
vilepickle’s picture

Status: Needs work » Reviewed & tested by the community

Updated issue summary to be a bit better. Let's not let minor stuff like that hold up fixing a major issue for site builders.

The patch works for me as well. Unless there are other major concerns with #132 it looks like a good patch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 132: 2457999-132.patch, failed testing. View results

dagmar’s picture

Status: Needs work » Needs review
FileSize
16.45 KB
2.42 KB

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

vilepickle’s picture

Status: Needs review » Reviewed & tested by the community

Cool, that one passes.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

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

tstoeckler’s picture

+++ 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?)

Berdir’s picture

Status: Needs review » Needs work

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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anybody’s picture

Confirming the problem still exists.

jonathanshaw’s picture

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.

jonathanshaw’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
jonathanshaw’s picture

Issue summary: View changes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice simplification!

Anybody’s picture

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

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

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.

jonathanshaw’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
950 bytes
14.99 KB

Fixed #150

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 151: 2457999-150.patch, failed testing. View results

jonathanshaw’s picture

Issue tags: +Needs reroll

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

jonathanshaw’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
14.63 KB

Encoding weirdness. Interdiff same as #150.

dawehner’s picture

@tstoeckler
Nice find!

tstoeckler’s picture

Thanks for the quick fix! RTBC++

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 154: 2457999-153.patch, failed testing. View results

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

Silly bot, saying it failed when it didn't

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 154: 2457999-153.patch, failed testing. View results

jonathanshaw’s picture

Status: Needs work » Reviewed & tested by the community
landsman’s picture

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

Drupal version: 8.4.3
PHP 7.1.8

joum’s picture

Can confirm that #154 works brilliantly.