Optional empty relationship on view fails with fatal error

This is at least Major according to the standards:

Trigger a PHP error through the user interface, but only under rare circumstances or affecting only a small percentage of all users, even if there is a workaround.

This sounds like a duplicate of at least one (maybe all) of these :

#2731433: Fatal error when using group by on views
#2715483: [regression] getEntity() doesn't always return entity, which results in add comment field not working in Views
#2319005: Notice: Undefined index: [field name] is thrown if relationship entities are missing
#2457999: Cannot use relationship for rendered entity on Views

d.o seems to have issues with me downloading the patch here at the moment, so can't check if the fix overlaps with any of the fixes proposed in those issues.
@Andre-B sounds like you might be in a good position to look at the existing fixes and maybe consolidate all the efforts into one issue? If we have 5 issues open for it, it sounds like something that people run into with some frequency.

@lendude+++++++

Thank you for working on this quite tough problem space. Groupby, especially in the context of the generic field handler is always challenging.
I think your setup is a little bit borked.

You group by both name and field_data_test_unlimited and the same time, which results in effectively no grouping. Maybe you want to group just by the second field?
The attached patch just fixes some minor bits.

@Andre-B yeah lets open this up and maybe close #2731433: Fatal error when using group by on views, this has a fix and test proposed. Setting to needs review to see what the testbot thinks.

Looking good. Bit of nitpicking:

1. +++ b/core/modules/field/tests/src/Kernel/EntityReference/Views/EntityReferenceRelationshipTest.php
   @@ -276,4 +281,50 @@ public function testDataTableRelationshipWithLongFieldName() {
   +  public function testGroupByWithEmptyRelationships() {
   

   This needs a docblock

2. +++ b/core/modules/field/tests/src/Kernel/EntityReference/Views/EntityReferenceRelationshipTest.php
   @@ -276,4 +281,50 @@ public function testDataTableRelationshipWithLongFieldName() {
   +    $this->assertNotEmpty($view->getStyle()->getField(0, 'name_2'));
   +    $this->assertNotEmpty($view->getStyle()->getField(1, 'name_2'));
   +    $this->assertNotEmpty($view->getStyle()->getField(2, 'name_2'));
   +    $this->assertEqual('', $view->getStyle()->getField(3, 'name_2'));
   

   This could use some explanation about what we test/expect.

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -820,10 +820,16 @@ public function getItems(ResultRow $values) {
   +      // optional relationships do not provide an entity at all. So we can't use
   

   Needs a capital at the start and should 'do not' not be 'may not'? They can provide an entity, they just don't always do.

Also a test-only patch would be nice to show the bug in the current HEAD.

The fix looks great for me as well!

@Andre-B test-only against 8.1.x is perfect

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -820,10 +820,16 @@ public function getItems(ResultRow $values) {
+      // Optional relationships may not provide an entity at all. So we can't use

If you are still in a rerolling mood, this is now over 80 characters long. But that could just be fixed on commit. Will RTBC if it all comes back green.

Nice work!

We got a test, we got a fix. Nice.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -820,10 +820,16 @@ public function getItems(ResultRow $values) {
   +      // Optional relationships may not provide an entity at all. So we can't use
   

   Longer than 80 chars.

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -820,10 +820,16 @@ public function getItems(ResultRow $values) {
   +      if (($entity = $this->getEntity($values)) && ($entity = $this->createEntityForGroupBy($entity, $values))) {
   

   Double assignment of the same variable in the same if looks a bit fragile. Also I can't see how $this->createEntityForGroupBy() can ever return a bool as its documentation claims... so the whole thing might be easier to understand if it was like this:

         // Optional relationships may not provide an entity at all. So we can't
         // use createEntityForGroupBy() for those rows.
         if ($entity = $this->getEntity($values)) { 
           $entity = $this->createEntityForGroupBy($entity, $values);
           // Some bundles might not have a specific field, in which case the faked
           // entity doesn't have it either.
           $build_list = isset($entity->{$this->definition['field_name']}) ? $entity->{$this->definition['field_name']}->view($display) : NULL;
         }
         else {
           $build_list = NULL;
         }
   

Feedback for #26 applied, looks much better indeed. Needs a separate 8.3.x patch so rolled that too.

@dawehner, @tim.plunkett, @alexpott, @cilefen, and I discussed this issue awhile back. As a fatal error that can be triggered with just the UI, this is definitely a major bug.

@tim.plunkett noted that it seems related to #2457999: Cannot use relationship for rendered entity on Views.

+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
@@ -818,10 +818,17 @@ public function getItems(ResultRow $values) {
+      else {
+        $build_list = NULL;
+      }

We could get rid of 2 lines of code when we predeclare $build_list = NULL

Turn out I missed one in #8, this is a duplicate of #2666166: View with not required relationship and aggregation enabled fails to execute. Work was done on both issues and that is the older issue, so moving this there.