ContentEntityDatabaseStorage::mapToStorageRecord hard-codes $entity->$name->value

Looks good, just a question

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -612,7 +612,17 @@ protected function mapToStorageRecord(EntityInterface $entity, $table_key = 'bas
+        elseif (!$main_property) {
+          // If there is no main property, get all values.
+          $values[$name] = $entity->get($name)->first()->getValue();

Maybe supposed "first value"

Patch looks good, just one remark:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -612,7 +612,17 @@ protected function mapToStorageRecord(EntityInterface $entity, $table_key = 'bas
   +        if ($main_property && isset($entity->get($name)->$main_property)) {
   +          $values[$name] = $entity->get($name)->$main_property;
   

   That seems weird, why not just $entity->$name->$main_property ?
   If we go with methods we should probably do for $name and $main_property.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -612,7 +612,17 @@ protected function mapToStorageRecord(EntityInterface $entity, $table_key = 'bas
+          // If there is no main property, get all values.
+          $values[$name] = $entity->get($name)->first()->getValue();

That seems a bit weird but I guess it's required for the MapItem to work. Should we add a @todo to resolve this as part of #2232427: Allow field types to control how properties are mapped to and from storage?

I prefer to use get() because PhpStorm always adds those hints if you just do $name :) We have no method for ->$main_property that would do the same... would have to call getValue() then.

Adding a todo sounds good, but it's not enough, as we also need that special case for no main property. For example, compare MapItem and LinkItem, both need the serialization, but on a different level. We can combine it with better explaining what we are doing there to solve #1.

Something like this I think.

Minor :

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -611,7 +610,22 @@ protected function mapToStorageRecord(EntityInterface $entity, $table_key = 'bas
+          // If there is no main property, get all values from the first field

I know the method is named ->getValue() (wasn't there an issue to rename it to toArray() BTW ?), but
"If there is no main property, get all *s/values/properties/*" would be clearer IMO.

Also, that whole mapToStorageRecord() method only knows how to store single-values fields anyway, so it would be a bit clearer if we started with an unconditional $item = $entity->get($name)->first();, and then do "this or that", rather than having comments in each if () branch explain that they both happen to restrict to the first item.

Other than that, looks good. I guess most of this "reasoning on the shape of the schema" will be moot anyway when schemas for base tables get auto-generated ?

+ Posted some thoughts about the notion of "main property", on which we attach more and more behavior, but that remains worringly vague IMO : #2269599: Clarify the notion of "main property" for a Field Type

Thanks. Works for me if still green.

I came by here catching up on #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins and read through the whole patch. I noticed two tiny things (no : in @todo https://drupal.org/node/1354#todo and a sentence missing a period)
This should still be rtbc.

I think the toArray() issue @yched was looking for is #2225943: Use toArray() instead of getValue() where applicable for field items and lists of field items

Thanks, clean-up looks good.

Yep, this looks good now. RTBC+1.

Committed/pushed to 8.x, thanks!

Missed that this was retesting, reverting for now.

12: 2267753-12.patch queued for re-testing.

12: 2267753-12.patch queued for re-testing.

Rerolled after #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities.

More reroll :)

Thanks, re-roll looks good, bad timing.

Committed/pushed to 8.x, thanks!