Views aggregation: Grouping a field that doesn't exist on all bundles causes an error.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -817,9 +817,8 @@ public function getItems(ResultRow $values) {
   +      $entity = $this->createEntityForGroupBy($this->getEntity($values), $values);
   +      $build_list = isset($entity->{$this->definition['field_name']}) ? $entity->{$this->definition['field_name']}->view($display) : NULL;
   
   @@ -999,18 +998,28 @@ protected function getTableMapping() {
   +    $field_item_list = isset($entity->{$this->definition['field_name']}) ? $entity->{$this->definition['field_name']} : NULL;
   

   We need to document how this can happen ...

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -999,18 +998,28 @@ protected function getTableMapping() {
   +    // Get the entity.
   +    $entity = $this->getEntity($values);
   

   Pointless documentation.

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -999,18 +998,28 @@ protected function getTableMapping() {
   +    // Make sure the field exists.
   +    if (!$field_item_list) {
   +      // There isn't anything we can do without a valid field.
   +      return NULL;
   +    } else {
   +      $values = [];
   +
   +      $field_item_definition = $field_item_list->getFieldDefinition();
    
   -    $values = [];
   -    foreach ($field_item_list as $field_item) {
   -      $values[] = $field ? $field_item->$field : $field_item->value;
   +      if ($field_item_definition->getFieldStorageDefinition()->getCardinality() == 1) {
   +        return $field ? $field_item_list->$field : $field_item_list->value;
   +      }
   +      foreach ($field_item_list as $field_item) {
   +        $values[] = $field ? $field_item->$field : $field_item->value;
   +      }
        }
   +
   

   You can make the code easier to read by just using if (!isset($field_item_list)) { return NULL; } and don't deal with an else at all

Wow, I would not have expected that its that hard to reproduce the problem with a normal field, not an image field.
This is still not failing.

There we go.

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -1002,8 +1003,17 @@ protected function getTableMapping() {
+    $entity = $this->getEntity($values);
+    // Some bundles might not have a specific field, in which case the faked
+    // entity doesn't have it either.

I don't think we should call the entity loaded here 'faked', that only goes for entities created by createEntityForGroupBy right?

+++ b/core/modules/views/src/Tests/QueryGroupByTest.php
@@ -292,4 +292,45 @@ public function testGroupByFieldWithCardinality() {
+  public function testGroupByWithFieldsNotExistingOnBundle() {
...
+    $entities = [];
+    $entity = EntityTestMul::create([
+      'field_test' => [1],
+      'type' => 'entity_test_mul',
+    ]);
+    $entity->save();
+    $entities[] = $entity;
+
+    $entity = EntityTestMul::create([
+      'type' => 'entity_test_mul2',
+    ]);
+    $entity->save();
+    $entities[] = $entity;

missing docblock and maybe give some explanation as to what is being set up here.

Thank you for the review @Lendude!

missing docblock and maybe give some explanation as to what is being set up here.

Well, the method is quite self descriptive to be honest, but well, here is a one line comment.

Well, the method is quite self descriptive to be honest, but well, here is a one line comment.

Don't think the docblocks for methods are optional :)

+++ b/core/modules/views/src/Tests/QueryGroupByTest.php
@@ -25,14 +25,14 @@ class QueryGroupByTest extends ViewKernelTestBase {
+  public static $modules = array('entity_test', 'system', 'field', 'user', 'language', 'image');

sorry missed this the first time, don't think the image module is needed? Tests pass fine without it.

Don't think the docblocks for methods are optional :)

Feel free to look at some examples in \Drupal\Tests\UnitTestCase :)

sorry missed this the first time, don't think the image module is needed? Tests pass fine without it.

Nor do I, good catch!

This now looks good to me.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!