Issue #2232477 by plach, yched, tstoeckler, amateescu: Fatal when adding new...

Issue #2232477 by plach, yched, tstoeckler, amateescu: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities

core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReference.php

@@ -55,6 +55,17 @@ public function getTargetDefinition() {
     return $this->definition->getTargetDefinition();
   }
 
+  /**
+   * Checks whether the target entity has not been saved yet.
+   *
+   * @return bool
+   *   TRUE if the entity is new, FALSE otherwise.
+   */
+  public function isTargetNew() {
+    // If only an ID is given, the reference cannot be a new entity.
+    return !isset($this->id) && isset($this->target) && $this->target->getValue()->isNew();
+  }
+
   /**
    * {@inheritdoc}
    */

core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php

@@ -1019,6 +1019,7 @@ protected function processIdentifierSchema(&$schema, $key) {
     if ($schema['fields'][$key]['type'] == 'int') {
       $schema['fields'][$key]['type'] = 'serial';
     }
+    $schema['fields'][$key]['not null'] = TRUE;
     unset($schema['fields'][$key]['default']);
   }
 
@@ -1386,27 +1387,36 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
 
     $field_name = $storage_definition->getName();
     $field_description = $storage_definition->getDescription();
+    $base_table = $this->storage->getBaseTable();
+
+    // A shared table contains rows for entities where the field is empty
+    // (since other fields stored in the same table might not be empty), thus
+    // the only columns that can be 'not null' are those for required
+    // properties of required fields. However, even those would break in the
+    // case where a new field is added to a table that contains existing rows.
+    // For now, we only hardcode 'not null' to a couple "entity keys", in order
+    // to keep their indexes optimized.
+    // @todo Revisit once we have support for 'initial' in
+    //   https://www.drupal.org/node/2346019.
+    $not_null_keys = $this->entityType->getKeys();
+    // Label fields are not necessarily required.
+    unset($not_null_keys['label']);
+    // Because entity ID and revision ID are both serial fields in the base and
+    // revision table respectively, the revision ID is not known yet, when
+    // inserting data into the base table. Instead the revision ID in the base
+    // table is updated after the data has been inserted into the revision
+    // table. For this reason the revision ID field cannot be marked as NOT
+    // NULL.
+    if ($table_name == $base_table) {
+      unset($not_null_keys['revision']);
+    }
 
     foreach ($column_mapping as $field_column_name => $schema_field_name) {
       $column_schema = $field_schema['columns'][$field_column_name];
 
       $schema['fields'][$schema_field_name] = $column_schema;
       $schema['fields'][$schema_field_name]['description'] = $field_description;
-      // Only entity keys are required.
-      $keys = $this->entityType->getKeys();
-      // The label is an entity key, but label fields are not necessarily
-      // required.
-      // Because entity ID and revision ID are both serial fields in the base
-      // and revision table respectively, the revision ID is not known yet, when
-      // inserting data into the base table. Instead the revision ID in the base
-      // table is updated after the data has been inserted into the revision
-      // table. For this reason the revision ID field cannot be marked as NOT
-      // NULL.
-      unset($keys['label'], $keys['revision']);
-      // Key fields may not be NULL.
-      if (in_array($field_name, $keys)) {
-        $schema['fields'][$schema_field_name]['not null'] = TRUE;
-      }
+      $schema['fields'][$schema_field_name]['not null'] = in_array($field_name, $not_null_keys);
     }
 
     if (!empty($field_schema['indexes'])) {
@@ -1596,6 +1606,7 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
 
     // Check that the schema does not include forbidden column names.
     $schema = $storage_definition->getSchema();
+    $properties = $storage_definition->getPropertyDefinitions();
     $table_mapping = $this->storage->getTableMapping();
     if (array_intersect(array_keys($schema['columns']), $table_mapping->getReservedColumns())) {
       throw new FieldException(format_string('Illegal field column names on @field_name', array('@field_name' => $storage_definition->getName())));
@@ -1605,6 +1616,10 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
     foreach ($schema['columns'] as $column_name => $attributes) {
       $real_name = $table_mapping->getFieldColumnName($storage_definition, $column_name);
       $data_schema['fields'][$real_name] = $attributes;
+      // A dedicated table only contain rows for actual field values, and no
+      // rows for entities where the field is empty. Thus, we can safely
+      // enforce 'not null' on the columns for the field's required properties.
+      $data_schema['fields'][$real_name]['not null'] = $properties[$column_name]->isRequired();
     }
 
     // Add indexes.

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->target_id !== NULL) {
-        $ids[$delta] = $item->target_id;
-      }
-      elseif ($item->hasNewEntity()) {
+      if ($item->hasNewEntity()) {
         $target_entities[$delta] = $item->entity;
       }
+      elseif ($item->target_id !== NULL) {
+        $ids[$delta] = $item->target_id;
+      }
     }
 
     // Load and add the existing entities.

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

@@ -159,13 +159,21 @@ public function isDisplayConfigurable($display_context);
   public function getDisplayOptions($display_context);
 
   /**
-   * Returns whether at least one non-empty item is required for this field.
+   * Returns whether the field can be empty.
    *
-   * Currently, required-ness is only enforced at the Form API level in entity
-   * edit forms, not during direct API saves.
+   * If a field is required, an entity needs to have at least a valid,
+   * non-empty item in that field's FieldItemList in order to pass validation.
+   *
+   * An item is considered empty if its isEmpty() method returns TRUE.
+   * Typically, that is if at least one of its required properties is empty.
    *
    * @return bool
    *   TRUE if the field is required.
+   *
+   * @see \Drupal\Core\TypedData\Plugin\DataType\ItemList::isEmpty()
+   * @see \Drupal\Core\Field\FieldItemInterface::isEmpty()
+   * @see \Drupal\Core\TypedData\DataDefinitionInterface:isRequired()
+   * @see \Drupal\Core\TypedData\TypedDataManager::getDefaultConstraints()
    */
   public function isRequired();
 

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

@@ -28,6 +28,9 @@ interface FieldItemInterface extends ComplexDataInterface {
   /**
    * Defines field item properties.
    *
+   * Properties that are required to constitute a valid, non-empty item should
+   * be denoted with \Drupal\Core\TypedData\DataDefinition::setRequired().
+   *
    * @return \Drupal\Core\TypedData\DataDefinitionInterface[]
    *   An array of property definitions of contained properties, keyed by
    *   property name.
@@ -67,10 +70,12 @@ public static function mainPropertyName();
    *   following key/value pairs:
    *   - columns: An array of Schema API column specifications, keyed by column
    *     name. The columns need to be a subset of the properties defined in
-   *     propertyDefinitions(). It is recommended to avoid having the column
-   *     definitions depend on field settings when possible. No assumptions
-   *     should be made on how storage engines internally use the original
-   *     column name to structure their storage.
+   *     propertyDefinitions(). The 'not null' property is ignored if present,
+   *     as it is determined automatically by the storage controller depending
+   *     on the table layout and the property definitions. It is recommended to
+   *     avoid having the column definitions depend on field settings when
+   *     possible. No assumptions should be made on how storage engines
+   *     internally use the original column name to structure their storage.
    *   - unique keys: (optional) An array of Schema API unique key definitions.
    *     Only columns that appear in the 'columns' array are allowed.
    *   - indexes: (optional) An array of Schema API index definitions. Only

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

@@ -43,7 +43,8 @@ public static function defaultStorageSettings() {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['value'] = DataDefinition::create('boolean')
-      ->setLabel(t('Boolean value'));
+      ->setLabel(t('Boolean value'))
+      ->setRequired(TRUE);
 
     return $properties;
   }
@@ -57,7 +58,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
         'value' => array(
           'type' => 'int',
           'size' => 'tiny',
-          'not null' => TRUE,
         ),
       ),
     );

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

@@ -41,7 +41,8 @@ public static function defaultStorageSettings() {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['value'] = DataDefinition::create('string')
-      ->setLabel(t('Decimal value'));
+      ->setLabel(t('Decimal value'))
+      ->setRequired(TRUE);
 
     return $properties;
   }
@@ -56,7 +57,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
           'type' => 'numeric',
           'precision' => $field_definition->getSetting('precision'),
           'scale' => $field_definition->getSetting('scale'),
-          'not null' => FALSE
         )
       ),
     );

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

@@ -32,7 +32,8 @@ class EmailItem extends FieldItemBase {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['value'] = DataDefinition::create('email')
-      ->setLabel(t('E-mail'));
+      ->setLabel(t('E-mail'))
+      ->setRequired(TRUE);
 
     return $properties;
   }
@@ -46,7 +47,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
         'value' => array(
           'type' => 'varchar',
           'length' => Email::EMAIL_MAX_LENGTH,
-          'not null' => FALSE,
         ),
       ),
     );

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

@@ -11,8 +11,8 @@
 use Drupal\Core\Entity\EntityTypeInterface;
 use Drupal\Core\Entity\TypedData\EntityDataDefinition;
 use Drupal\Core\Field\FieldDefinitionInterface;
-use Drupal\Core\Field\FieldStorageDefinitionInterface;
 use Drupal\Core\Field\FieldItemBase;
+use Drupal\Core\Field\FieldStorageDefinitionInterface;
 use Drupal\Core\TypedData\DataDefinition;
 use Drupal\Core\TypedData\DataReferenceDefinition;
 
@@ -36,6 +36,13 @@
  */
 class EntityReferenceItem extends FieldItemBase {
 
+  /**
+   * Marker value to identify a newly created entity.
+   *
+   * @var int
+   */
+  protected static $NEW_ENTITY_MARKER = -1;
+
   /**
    * {@inheritdoc}
    */
@@ -73,7 +80,9 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
       $target_id_definition = DataDefinition::create('string')
         ->setLabel(t('@label ID', array($target_type_info->getLabel())));
     }
+    $target_id_definition->setRequired(TRUE);
     $properties['target_id'] = $target_id_definition;
+
     $properties['entity'] = DataReferenceDefinition::create('entity')
       ->setLabel($target_type_info->getLabel())
       ->setDescription(t('The referenced entity'))
@@ -109,7 +118,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
           'description' => 'The ID of the target entity.',
           'type' => 'int',
           'unsigned' => TRUE,
-          'not null' => TRUE,
         ),
       );
     }
@@ -155,8 +163,12 @@ public function setValue($values, $notify = TRUE) {
         $this->onChange('entity', FALSE);
       }
       elseif (isset($values['target_id']) && isset($values['entity'])) {
-        // If both properties are passed, verify the passed values match.
-        if ($this->get('entity')->getTargetIdentifier() != $values['target_id']) {
+        // 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())) {
           throw new \InvalidArgumentException('The target id and entity passed to the entity reference item do not match.');
         }
       }
@@ -187,11 +199,13 @@ public function getValue() {
    */
   public function onChange($property_name, $notify = TRUE) {
     // Make sure that the target ID and the target property stay in sync.
-    if ($property_name == 'target_id') {
-      $this->writePropertyValue('entity', $this->target_id);
+    if ($property_name == 'entity') {
+      $property = $this->get('entity');
+      $target_id = $property->isTargetNew() ? static::$NEW_ENTITY_MARKER : $property->getTargetIdentifier();
+      $this->writePropertyValue('target_id', $target_id);
     }
-    elseif ($property_name == 'entity') {
-      $this->writePropertyValue('target_id', $this->get('entity')->getTargetIdentifier());
+    elseif ($property_name == 'target_id' && $this->target_id != static::$NEW_ENTITY_MARKER) {
+      $this->writePropertyValue('entity', $this->target_id);
     }
     parent::onChange($property_name, $notify);
   }
@@ -215,13 +229,12 @@ public function isEmpty() {
    */
   public function preSave() {
     if ($this->hasNewEntity()) {
-      $this->entity->save();
-    }
-    // Handle the case where an unsaved entity was directly set using the public
-    // 'entity' property and then saved before this entity. In this case
-    // ::hasNewEntity() will return FALSE but $this->target_id will still be
-    // empty.
-    if (empty($this->target_id) && $this->entity) {
+      // Save the entity if it has not already been saved by some other code.
+      if ($this->entity->isNew()) {
+        $this->entity->save();
+      }
+      // Make sure the parent knows we are updating this property so it can
+      // react properly.
       $this->target_id = $this->entity->id();
     }
   }
@@ -249,7 +262,7 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
    *   TRUE if the item holds an unsaved entity.
    */
   public function hasNewEntity() {
-    return $this->target_id === NULL && ($entity = $this->entity) && $entity->isNew();
+    return $this->target_id === static::$NEW_ENTITY_MARKER;
   }
 
   /**

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

@@ -29,7 +29,8 @@ class FloatItem extends NumericItemBase {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['value'] = DataDefinition::create('float')
-      ->setLabel(t('Float'));
+      ->setLabel(t('Float'))
+      ->setRequired(TRUE);
 
     return $properties;
   }
@@ -42,7 +43,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
       'columns' => array(
         'value' => array(
           'type' => 'float',
-          'not null' => FALSE,
         ),
       ),
     );

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

@@ -53,7 +53,8 @@ public static function defaultFieldSettings() {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['value'] = DataDefinition::create('integer')
-      ->setLabel(t('Integer value'));
+      ->setLabel(t('Integer value'))
+      ->setRequired(TRUE);
 
     return $properties;
   }
@@ -92,7 +93,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
       'columns' => array(
         'value' => array(
           'type' => 'int',
-          'not null' => FALSE,
           // Expose the 'unsigned' setting in the field item schema.
           'unsigned' => $field_definition->getSetting('unsigned'),
           // Expose the 'size' setting in the field item schema. For instance,

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

@@ -37,7 +37,8 @@ class LanguageItem extends FieldItemBase {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['value'] = DataDefinition::create('string')
-      ->setLabel(t('Language code'));
+      ->setLabel(t('Language code'))
+      ->setRequired(TRUE);
 
     $properties['language'] = DataReferenceDefinition::create('language')
       ->setLabel(t('Language object'))
@@ -58,7 +59,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
         'value' => array(
           'type' => 'varchar',
           'length' => 12,
-          'not null' => FALSE,
         ),
       ),
     );

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

@@ -43,7 +43,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
         'value' => array(
           'type' => 'varchar',
           'length' => (int) $field_definition->getSetting('max_length'),
-          'not null' => FALSE,
           'binary' => $field_definition->getSetting('case_sensitive'),
         ),
       ),

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

@@ -34,7 +34,8 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
     // early t() calls by using the TranslationWrapper.
     $properties['value'] = DataDefinition::create('string')
       ->setLabel(new TranslationWrapper('Text value'))
-      ->setSetting('case_sensitive', $field_definition->getSetting('case_sensitive'));
+      ->setSetting('case_sensitive', $field_definition->getSetting('case_sensitive'))
+      ->setRequired(TRUE);
 
     return $properties;
   }

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

@@ -40,7 +40,8 @@ class TimestampItem extends FieldItemBase {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['value'] = DataDefinition::create('timestamp')
-      ->setLabel(t('Timestamp value'));
+      ->setLabel(t('Timestamp value'))
+      ->setRequired(TRUE);
     return $properties;
   }
 
@@ -52,7 +53,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
       'columns' => array(
         'value' => array(
           'type' => 'int',
-          'not null' => FALSE,
         ),
       ),
     );

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

@@ -42,7 +42,8 @@ public static function defaultStorageSettings() {
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['value'] = DataDefinition::create('uri')
       ->setLabel(t('URI value'))
-      ->setSetting('case_sensitive', $field_definition->getSetting('case_sensitive'));
+      ->setSetting('case_sensitive', $field_definition->getSetting('case_sensitive'))
+      ->setRequired(TRUE);
 
     return $properties;
   }
@@ -56,7 +57,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
         'value' => array(
           'type' => 'varchar',
           'length' => (int) $field_definition->getSetting('max_length'),
-          'not null' => TRUE,
           'binary' => $field_definition->getSetting('case_sensitive'),
         ),
       ),

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

@@ -61,4 +61,5 @@ public function setValue($value, $notify = TRUE) {
   public function getString() {
     return (string) $this->getType() . ':' . $this->getTargetIdentifier();
   }
+
 }

core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php

@@ -55,7 +55,8 @@ public static function defaultFieldSettings() {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['status'] = DataDefinition::create('integer')
-      ->setLabel(t('Comment status'));
+      ->setLabel(t('Comment status'))
+      ->setRequired(TRUE);
 
     $properties['cid'] = DataDefinition::create('integer')
       ->setLabel(t('Last comment ID'));
@@ -87,7 +88,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
         'status' => array(
           'description' => 'Whether comments are allowed on this entity: 0 = no, 1 = closed (read only), 2 = open (read/write).',
           'type' => 'int',
-          'not null' => TRUE,
           'default' => 0,
         ),
       ),

core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php

@@ -51,7 +51,8 @@ public static function defaultStorageSettings() {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
     $properties['value'] = DataDefinition::create('datetime_iso8601')
-      ->setLabel(t('Date value'));
+      ->setLabel(t('Date value'))
+      ->setRequired(TRUE);
 
     $properties['date'] = DataDefinition::create('any')
       ->setLabel(t('Computed date'))
@@ -73,7 +74,6 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
           'description' => 'The date value.',
           'type' => 'varchar',
           'length' => 20,
-          'not null' => FALSE,
         ),
       ),
       'indexes' => array(

core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteTagsWidget.php

@@ -8,6 +8,7 @@
 namespace Drupal\entity_reference\Plugin\Field\FieldWidget;
 
 use Drupal\Component\Utility\Tags;
+use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem;
 use Drupal\Core\Form\FormStateInterface;
 
 /**
@@ -73,10 +74,7 @@ public function elementValidate($element, FormStateInterface $form_state, $form)
         elseif ($auto_create && (count($this->getSelectionHandlerSetting('target_bundles')) == 1 || count($bundles) == 1)) {
           // Auto-create item. See
           // \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::presave().
-          $value[] = array(
-            'target_id' => NULL,
-            'entity' => $this->createNewEntity($input, $element['#autocreate_uid']),
-          );
+          $value[] = array('entity' => $this->createNewEntity($input, $element['#autocreate_uid']));
         }
       }
     };