\Drupal\views\Plugin\views\field\Field::getValue needs to support entity reference items

+++ b/core/modules/views/src/Tests/Handler/FieldFieldTest.php
@@ -67,12 +68,14 @@ class FieldFieldTest extends ViewKernelTestBase {
   protected function setUp() {
-    parent::setUp();
+    parent::setUp(FALSE);
 
-    $this->installEntitySchema('entity_test');
     $this->installEntitySchema('user');
+    $this->installEntitySchema('entity_test');
     $this->installEntitySchema('entity_test_rev');
 
+    ViewTestData::createTestViews(get_class($this), array('views_test_config'));
+
     // Bypass any field access.
     $this->adminUser = User::create(['name' => $this->randomString()]);
     $this->adminUser->save();
@@ -551,4 +554,30 @@ public function testMissingBundleFieldRender() {

@@ -551,4 +554,30 @@ public function testMissingBundleFieldRender() {
     $this->assertEqual('', $executable->getStyle()->getField(6, 'field_test'));
   }
 

I think this needs some comments as to why you are doing the view setup yourself and not using the one from ViewKernelTestBase.

Change looks good, and should future proof fields that are implemented in similar ways. Test looks good and valid. Verified test fails w/o the patch.

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -1004,12 +1004,15 @@ public function getValue(ResultRow $values, $field = NULL) {
+      $main_property_name = $field_item_list->first()->mainPropertyName();
+      return $field ? $field_item_list->$field : $field_item_list->first()->{$main_property_name};
...
+      $main_property_name = $field_item->mainPropertyName();
+      $values[] = $field ? $field_item->$field : $field_item->{$main_property_name};

We only need to get the main_property_name for one side of the ternary so lets just use an if statement instead.

Do have test coverage of when the cardinality is 1 and when it is higher?

This patch helped me to get a value from a color field (see #2701933: Declare the main property name for related informations).

Looks good to me, maybe the title should change from "needs to support entity reference items" to "needs to support field which main value is not 'value'".

This doesn't apply against 8.1.x. Needs reroll.

Let see.

#16, I think so. You can't interdiff them b/c git output the hunks in a different order, but they all look the same. The conflict was b/c the namespace change in #2304461: KernelTestBaseTNG™ for KernelTestBase. It was an easy merge once I realized that.

FieldItemInterface::mainPropertyName() can return NULL so would be nice to add a check for that. I don't think we need to support a property named 0 so if ($main_property_name = $field_item->mainPropertyName()) { ... } should be fine.

DER has this in DynamicEntityReferenceItem.php

  /**
   * {@inheritdoc}
   */
  public static function mainPropertyName() {
    // We have two main properties i.e. target_type and target_id.
    return NULL;
  }

So this will generate an error here.

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -1031,12 +1031,22 @@ public function getValue(ResultRow $values, $field = NULL) {
     if ($field_item_definition->getFieldStorageDefinition()->getCardinality() == 1) {
-      return $field ? $field_item_list->$field : $field_item_list->value;
+      if ($field) {
+        return $field_item_list->$field;
+      }
+      /** @var \Drupal\Core\Field\FieldItemInterface::mainPropertyName $first_item */
+      // Potentially field items might have no main property. There is nothing
+      // we can do then.
+      elseif (($first_item = $field_item_list->first()) && $main_property_name = $first_item->mainPropertyName()) {
+        return $first_item->{$main_property_name};
+      }
     }

With the current logic, if the cardinality == 1 but both the if and the elseif are false, you will fall through to the cardinality > 1 logic and return an array (possibly an empty one).
Shouldn't the cardinality == 1 logic include an else, and return something other then an array, maybe the original $field_item_list->value fallback?

Does #24 and #25 mean that we have a test coverage problem? Maybe the one mentioned in #10 didn't cover this for some reason?

but if the field type doesn't return anything useful we cannot guess what to render. Decide on one: 'target_id' IMHO.

Let me create an issue in DER and discuss it with ER maintainers as well.

@dawehner oh yeah this now looks way better. Also the new test addresses @alexpotts comment in #10, so nice!

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -1030,15 +1030,29 @@ public function getValue(ResultRow $values, $field = NULL) {
+      // Potentially field items might have no main property. There is nothing
+      // we can do then.
...
+        // Try out value, which might be used, even if its not defined as main
+        // propery name.

The first comment isn't quite right anymore, there is something we can do, namely 'take a guess'. Maybe combine the two comments into one with something like:
Find the value using the main property of the field. If no main property is provided fall back to 'value'.

Otherwise, the second comment contains two grammatical errors: its => it's and propery => property

Nitpicking aside, this looks ready.

Thanks!

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

Sorry for being late here but I have a question.

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -1030,15 +1030,27 @@ public function getValue(ResultRow $values, $field = NULL) {
-      $values[] = $field ? $field_item->$field : $field_item->value;
+      /** @var \Drupal\Core\Field\FieldItemInterface $field_item */
+      if ($field) {
+        $values[] = $field_item->$field;
+      }
+      // Find the value using the main property of the field. If no main
+      // property is provided fall back to 'value'.
+      elseif ($main_property_name = $field_item->mainPropertyName()) {
+        $values[] = $field_item->{$main_property_name};
+      }
+      else {
+        $values[] = $field_item->value;
+      }
+    }
+    if ($field_item_definition->getFieldStorageDefinition()->getCardinality() == 1) {
+      return reset($values);
+    }
+    else {
+      return $values;

Instead of all this why not just use $field_item->getValue() here?

Something like this.