Issue #2404331 by larowlan, jibran, amateescu, RavindraSingh: Can't serialise...

Issue #2404331 by larowlan, jibran, amateescu, RavindraSingh: Can't serialise an entity with an entity reference to an unsaved entity (remove EntityReferenceItem::$NEW_MARKER in favour of a validation constraint)

core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraint.php

@@ -28,4 +28,11 @@ class ValidReferenceConstraint extends Constraint {
    */
   public $message = 'The referenced entity (%type: %id) does not exist.';
 
+  /**
+   * Validation message when the target_id is empty.
+   *
+   * @var string
+   */
+  public $nullMessage = 'This value should not be null.';
+
 }

core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php

@@ -19,10 +19,18 @@ class ValidReferenceConstraintValidator extends ConstraintValidator {
    * {@inheritdoc}
    */
   public function validate($value, Constraint $constraint) {
-    /* @var \Drupal\Core\Field\FieldItemInterface $value */
+    /** @var \Drupal\Core\Field\FieldItemInterface $value */
+    /** @var ValidReferenceConstraint $constraint */
     if (!isset($value)) {
       return;
     }
+    // We don't use a regular NotNull constraint for the target_id property as
+    // a NULL value is valid if the entity property contains an unsaved entity.
+    // @see \Drupal\Core\TypedData\DataReferenceTargetDefinition::getConstraints
+    if (!$value->isEmpty() && $value->target_id === NULL && !$value->entity->isNew()) {
+      $this->context->addViolation($constraint->nullMessage);
+      return;
+    }
     $id = $value->get('target_id')->getValue();
     // '0' or NULL are considered valid empty references.
     if (empty($id)) {

core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php

@@ -27,12 +27,12 @@ public function referencedEntities() {
     // "autocreate" entities that are already populated in $item->entity.
     $target_entities = $ids = array();
     foreach ($this->list as $delta => $item) {
-      if ($item->hasNewEntity()) {
-        $target_entities[$delta] = $item->entity;
-      }
-      elseif ($item->target_id !== NULL) {
+      if ($item->target_id !== NULL) {
         $ids[$delta] = $item->target_id;
       }
+      elseif ($item->hasNewEntity()) {
+        $target_entities[$delta] = $item->entity;
+      }
     }
 
     // Load and add the existing entities.

core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php

@@ -14,8 +14,8 @@
 use Drupal\Core\Field\FieldItemBase;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
 use Drupal\Core\StringTranslation\TranslatableString;
-use Drupal\Core\TypedData\DataDefinition;
 use Drupal\Core\TypedData\DataReferenceDefinition;
+use Drupal\Core\TypedData\DataReferenceTargetDefinition;
 
 /**
  * Defines the 'entity_reference' entity field type.
@@ -39,13 +39,6 @@
  */
 class EntityReferenceItem extends FieldItemBase {
 
-  /**
-   * Marker value to identify a newly created entity.
-   *
-   * @var int
-   */
-  protected static $NEW_ENTITY_MARKER = -1;
-
   /**
    * {@inheritdoc}
    */
@@ -82,12 +75,12 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
     }
 
     if ($target_id_data_type === 'integer') {
-      $target_id_definition = DataDefinition::create('integer')
+      $target_id_definition = DataReferenceTargetDefinition::create('integer')
         ->setLabel(new TranslatableString('@label ID', ['@label' => $target_type_info->getLabel()]))
         ->setSetting('unsigned', TRUE);
     }
     else {
-      $target_id_definition = DataDefinition::create('string')
+      $target_id_definition = DataReferenceTargetDefinition::create('string')
         ->setLabel(new TranslatableString('@label ID', ['@label' => $target_type_info->getLabel()]));
     }
     $target_id_definition->setRequired(TRUE);
@@ -171,19 +164,23 @@ public function setValue($values, $notify = TRUE) {
       parent::setValue($values, FALSE);
       // Support setting the field item with only one property, but make sure
       // values stay in sync if only property is passed.
-      if (isset($values['target_id']) && !isset($values['entity'])) {
+      // NULL is a valid value, so we use array_key_exists().
+      if (is_array($values) && array_key_exists('target_id', $values) && !isset($values['entity'])) {
         $this->onChange('target_id', FALSE);
       }
-      elseif (!isset($values['target_id']) && isset($values['entity'])) {
+      elseif (is_array($values) && !array_key_exists('target_id', $values) && isset($values['entity'])) {
         $this->onChange('entity', FALSE);
       }
-      elseif (isset($values['target_id']) && isset($values['entity'])) {
+      elseif (is_array($values) && array_key_exists('target_id', $values) && isset($values['entity'])) {
         // If both properties are passed, verify the passed values match. The
         // only exception we allow is when we have a new entity: in this case
         // its actual id and target_id will be different, due to the new entity
         // marker.
         $entity_id = $this->get('entity')->getTargetIdentifier();
-        if ($entity_id != $values['target_id'] && ($values['target_id'] != static::$NEW_ENTITY_MARKER || !$this->entity->isNew())) {
+        // If the entity has been saved and we're trying to set both the
+        // target_id and the entity values with a non-null target ID, then the
+        // value for target_id should match the ID of the entity value.
+        if (!$this->entity->isNew() && $values['target_id'] !== NULL && ($entity_id !== $values['target_id'])) {
           throw new \InvalidArgumentException('The target id and entity passed to the entity reference item do not match.');
         }
       }
@@ -216,10 +213,10 @@ public function onChange($property_name, $notify = TRUE) {
     // Make sure that the target ID and the target property stay in sync.
     if ($property_name == 'entity') {
       $property = $this->get('entity');
-      $target_id = $property->isTargetNew() ? static::$NEW_ENTITY_MARKER : $property->getTargetIdentifier();
+      $target_id = $property->isTargetNew() ? NULL : $property->getTargetIdentifier();
       $this->writePropertyValue('target_id', $target_id);
     }
-    elseif ($property_name == 'target_id' && $this->target_id != static::$NEW_ENTITY_MARKER) {
+    elseif ($property_name == 'target_id') {
       $this->writePropertyValue('entity', $this->target_id);
     }
     parent::onChange($property_name, $notify);
@@ -252,6 +249,9 @@ public function preSave() {
       // react properly.
       $this->target_id = $this->entity->id();
     }
+    if (!$this->isEmpty() && $this->target_id === NULL) {
+      $this->target_id = $this->entity->id();
+    }
   }
 
   /**
@@ -277,7 +277,7 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
    *   TRUE if the item holds an unsaved entity.
    */
   public function hasNewEntity() {
-    return $this->target_id === static::$NEW_ENTITY_MARKER;
+    return !$this->isEmpty() && $this->target_id === NULL && $this->entity->isNew();
   }
 
   /**

core/lib/Drupal/Core/TypedData/DataReferenceTargetDefinition.php

+<?php
+/**
+ * @file
+ * Contains \Drupal\Core\TypedData\DataReferenceTargetDefinition.
+ */
+
+namespace Drupal\Core\TypedData;
+
+/**
+ * A typed data definition class for the entity reference target_id property.
+ *
+ * The target_id property differs from other data definitions in that it is
+ * required at the storage level, but not at the validation level. This is
+ * because its value can be set just-in-time using the preSave() method.
+ *
+ * Validation for the target_id property is provided by the 'ValidReference'
+ * validation constraint.
+ *
+ * @see \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::preSave()
+ */
+class DataReferenceTargetDefinition extends DataDefinition {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getConstraints() {
+    $constraints = parent::getConstraints();
+    // If this data definition is marked as required for the sake of schema
+    // definitions, we don't enforce it using the NotNull constraint. Instead
+    // \Drupal\Core\Field\EntityReferenceItem is validated by the
+    // 'ValidReference' constraint that operates at the field-item level. This
+    // constraint takes into consideration that the target_id property can
+    // be derived from the entity property.
+    unset($constraints['NotNull']);
+    return $constraints;
+  }
+
+}

core/modules/field/src/Tests/EntityReference/EntityReferenceItemTest.php

@@ -12,8 +12,8 @@
 use Drupal\Core\Field\FieldItemInterface;
 use Drupal\Core\StringTranslation\TranslatableString;
 use Drupal\Core\Language\LanguageInterface;
-use Drupal\entity_reference\Tests\EntityReferenceTestTrait;
 use Drupal\entity_test\Entity\EntityTest;
+use Drupal\entity_reference\Tests\EntityReferenceTestTrait;
 use Drupal\entity_test\Entity\EntityTestStringId;
 use Drupal\field\Entity\FieldConfig;
 use Drupal\field\Entity\FieldStorageConfig;
@@ -236,6 +236,30 @@ public function testConfigEntityReferenceItem() {
     $entity->save();
   }
 
+  /**
+   * Tests entity auto create.
+   */
+  public function testEntityAutoCreate() {
+    // The term entity is unsaved here.
+    $term = Term::create(array(
+      'name' => $this->randomMachineName(),
+      'vid' => $this->term->bundle(),
+      'langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED,
+    ));
+    $entity = EntityTest::create();
+    // Now assign the unsaved term to the field.
+    $entity->field_test_taxonomy_term->entity = $term;
+    $entity->name->value = $this->randomMachineName();
+    // This is equal to storing an entity to tempstore or cache and retrieving
+    // it back. An example for this is node preview.
+    $entity = serialize($entity);
+    $entity = unserialize($entity);
+    // And then the entity.
+    $entity->save();
+    $term = \Drupal::entityManager()->loadEntityByUuid($term->getEntityTypeId(), $term->uuid());
+    $this->assertEqual($entity->field_test_taxonomy_term->entity->id(), $term->id());
+  }
+
   /**
    * Test saving order sequence doesn't matter.
    */
@@ -250,6 +274,12 @@ public function testEntitySaveOrder() {
     // Now assign the unsaved term to the field.
     $entity->field_test_taxonomy_term->entity = $term;
     $entity->name->value = $this->randomMachineName();
+    // Now get the field value.
+    $value = $entity->get('field_test_taxonomy_term');
+    $this->assertTrue(empty($value['target_id']));
+    $this->assertNull($entity->field_test_taxonomy_term->target_id);
+    // And then set it.
+    $entity->field_test_taxonomy_term = $value;
     // Now save the term.
     $term->save();
     // And then the entity.
@@ -289,4 +319,41 @@ public function testSelectionHandlerSettings() {
     $this->assertTrue($field->getSetting('handler') == 'views');
   }
 
+  /**
+   * Tests validation constraint.
+   */
+  public function testValidation() {
+    // The term entity is unsaved here.
+    $term = Term::create(array(
+      'name' => $this->randomMachineName(),
+      'vid' => $this->term->bundle(),
+      'langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED,
+    ));
+    $entity = EntityTest::create([
+      'field_test_taxonomy_term' => [
+        'entity' => $term,
+        'target_id' => NULL,
+      ],
+    ]);
+    $errors = $entity->validate();
+    // Using target_id of NULL is valid with an unsaved entity.
+    $this->assertEqual(0, count($errors));
+    // Using target_id of NULL is not valid with a saved entity.
+    $term->save();
+    $entity = EntityTest::create([
+      'field_test_taxonomy_term' => [
+        'entity' => $term,
+        'target_id' => NULL,
+      ],
+    ]);
+    $errors = $entity->validate();
+    $this->assertEqual(1, count($errors));
+    $this->assertEqual($errors[0]->getMessage(), 'This value should not be null.');
+    $this->assertEqual($errors[0]->getPropertyPath(), 'field_test_taxonomy_term.0');
+    // This should rectify the issue, favoring the entity over the target_id.
+    $entity->save();
+    $errors = $entity->validate();
+    $this->assertEqual(0, count($errors));
+  }
+
 }

core/modules/system/src/Tests/Entity/EntityReferenceFieldTest.php

@@ -238,16 +238,15 @@ function testAutocreateApi() {
     });
     $this->assertUserAutocreate($entity, function(EntityInterface $entity, UserInterface $user) {
       $entity->user_id[0]->get('entity')->setValue($user);
-      $entity->user_id[0]->get('target_id')->setValue(-1);
     });
     $this->assertUserAutocreate($entity, function(EntityInterface $entity, UserInterface $user) {
-      $entity->user_id->setValue(array('entity' => $user, 'target_id' => -1));
+      $entity->user_id->setValue(array('entity' => $user, 'target_id' => NULL));
     });
     try {
       $message = 'Setting both the entity and an invalid target_id property fails.';
       $this->assertUserAutocreate($entity, function(EntityInterface $entity, UserInterface $user) {
         $user->save();
-        $entity->user_id->setValue(array('entity' => $user, 'target_id' => -1));
+        $entity->user_id->setValue(array('entity' => $user, 'target_id' => $this->generateRandomEntityId()));
       });
       $this->fail($message);
     }
@@ -273,16 +272,15 @@ function testAutocreateApi() {
     });
     $this->assertUserRoleAutocreate($entity, function(EntityInterface $entity, RoleInterface $role) {
       $entity->user_role[0]->get('entity')->setValue($role);
-      $entity->user_role[0]->get('target_id')->setValue(-1);
     });
     $this->assertUserRoleAutocreate($entity, function(EntityInterface $entity, RoleInterface $role) {
-      $entity->user_role->setValue(array('entity' => $role, 'target_id' => -1));
+      $entity->user_role->setValue(array('entity' => $role, 'target_id' => NULL));
     });
     try {
       $message = 'Setting both the entity and an invalid target_id property fails.';
       $this->assertUserRoleAutocreate($entity, function(EntityInterface $entity, RoleInterface $role) {
         $role->save();
-        $entity->user_role->setValue(array('entity' => $role, 'target_id' => -1));
+        $entity->user_role->setValue(array('entity' => $role, 'target_id' => $this->generateRandomEntityId(TRUE)));
       });
       $this->fail($message);
     }