Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities

Cross-posting from #2183231-316: Make ContentEntityDatabaseStorage generate static database schemas for content entities:

Re: To NULL or to NOT NULL:

As long as the columns are not part of an index, the change doesn't make a difference. However, for columns that are part of an index, allowing NULL
values should be used wisely, because affected indexes require an additional internal flag to track emptiness. That makes the index larger, and a larger index negatively affects filter/group/sort performance.

This is part of the one-pager about most basic database performance optimization rules, which everyone should know inside-out and follow by heart:

http://dev.mysql.com/doc/refman/5.6/en/data-size.html

We discussed this a bit in IRC, this is the plan that came out:

Basically the idea is the we make NOT NULL every field for which we add an index automatically, fields adding indexes in their definition are in charge to evaluate whether it's the case to make their columns NULLable or not.

So we add not null when a field is required plus fix #2274597: Not all required fields are marked as required. Or any problems I miss?

Hmm, 'required' is a per-instance property on confugurable fields, we can't put it in FieldStorageDefinition nor use it to build the schema ?

This came up again today/yesterday in some discussions, assigning to @yched to get some feedback from him :)

This issue is causing the exceptions plach found in #2347301: "Data truncated for column" exceptions thrown when adding fields to the user entity type. We set not null true but don't set a default at all.

Bumping priority and tagging.

The issue summary looks good to me :)

@tstoeckler: Thanks ! The issue summary looks pretty much inline with our discussion.

5. Add code to SqlContentEntityStorageSchema::getSharedTableFieldSchema() and SqlContentEntityStorageSchema::getDedicatedTableSchema() to set those columns to 'null' for which the field definition is required and the property definition is required

I think at some point in the discussion was the notion that 'not null = TRUE' was never ok in base tables ?

Actually, it's a little more complex than that :-/
- 'not null' columns are ok in the initial table creation, or in columns added later as long as the table is empty.
- they fatal when adding new columns the moment the table has rows

So :

1) either we never say 'not null TRUE' for columns in base tables,
Drawback: we lose index optimization in base tables - a bit unfortunate, base tables are the ones where fast indexes are most useful...

2) or we add 'not null's to columns created while the table is empty, and stop doing that the moment the table has data
Drawback: the resulting schema depends on "history", that sounds like a recipe for hairpulling "why is the schema on prod different from the one on my dev machine".

3) or we somehow find a way to support a notion of 'initial' value on 'not null' columns - but as discussed previously, I have no clue how to support that. $base_field->setInitialValue($property, $value) ?
That only makes sense for "base fields that the table mapping will decide to place in the base table, and that are added after some entities were created" (and table mappings are supposed to be swappable, right ?), so it's a bit difficult to reason on it as a generic feature...

I'd think 2) is not on the table.
We can punt and go with 1) for now, and think of ways to do 3) in a followup ?

#24 sounds like a plan to me.

3) is #2346019: Handle initial values when creating a new field storage definition, I guess.

@plach: indeed.

Meaning :
- this (critical) issue here would fix the curent (critical) bug of "Fatal when adding new fields in a base table that contains existing entities" by doing 1) ( = "no 'not null's at all in base tables, too bad for index optimization")
- and then #2346019: Handle initial values when creating a new field storage definition (wich we should demote to non-critical) would try to add back the possiblity of 'not null' in base fields , by somehow adding some support for 'initial'

Does that sound like a viable plan ?

Yep :)

Both the issue summary and splitting the issue in two look right to me.

@tstoeckler:

Are you still planning to work on this? I can take it over otherwise.

Sounds like we need a summary update for #23 - #24?

We also need a patch :)

Just a bit greedy in the previous patch.

Discussed with @alexpott, @effulgentsia, and @dawehner. Retitling to the critical bug this will solve.

This will also solve a serious migrate issue where things with parents (think comments, menu links, taxonomy terms) currently tend to fatal when we stub their parents. #2372837: menu link stubbing fails because fields are NOT NULL

@xjm, the issue summary was updated in #34 :)

Oops. :)

Here is a test-only version of #34 to check whether the bug is exposed...

Looks good to me, thanks!

(Patch to commit in #34)

Updated issue summary.

@tstoeckler:

I thought the plan was using most of it in #2346019: Handle initial values when creating a new field storage definition...

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 08d3ebb and pushed to 8.0.x. Thanks!

Let's proceed with #2346019: Handle initial values when creating a new field storage definition and move on with this to unblock other patches.

Sorry that I didn't react earlier on #34, but yes, I thought the plan discussed in Amsterdam (steps 1 to 5 in the issue summary) was still valid :
- remove all 'not null = TRUE' in FieldItem::schema()
- use setRequired() on property definitions instead, to carry that same information (non-required = "non essential column, might be empty even when the field value itself is not empty because other properties have values")
- adapt SqlContentEntityStorageSchema to assign 'not null = TRUE / FALSE' automatically depending on property->isRequired()

I missed the part where we ditched that plan :-)

The discussion in #23 / #24 only pointed that, until we have support for 'initial', base tables have to force 'not null = FALSE' for all properties, independently of the isRequired() state - something like :
$not_null = ($base_table) ? FALSE : $property->isRequired();

Moving from "field types specify 'not null' in their schema()" to "field types specify isRequired() in their propertyDefinitions()" is not related to a (still hypothetical) support for "initial", so has nothing to do with #2346019: Handle initial values when creating a new field storage definition ?

Sorry, I should have read again previous comments before reviewing the patch, given that in Amsterdam I was pretty sick and obviously I didn't retain the details of our discussion. Should we reopen this (maybe demoting it)?

Oops, sorry as well. Since I didn't participate in this particular discussion in Amsterdam, I took #23, #26 and #28 as the way forward because nothing else was documented in the issue.

We shouldn't re-use the issue unless we roll back the patch that's already gone in - makes it much harder to track history etc. (i.e. if we demote this to major it'll drop out of fixed criticals stats etc.). A new issue to continue this is fine. I don't think a rollback is required but given it's a small-ish patch that went in I'd also be fine doing that if we really want to continue here?

@amateescu :

nothing else was documented in the issue

@tstoeckler put it in the issue summary in #21 :-)

Anyway - then yeah, I guess new issue now. Although the original plan (see bottom of the IS or #48) would now be an API change that doesn't fix a critical anymore...

The idea behind that plan was to avoid the situation where the field type schema() says one thing but the final DB schema might turn out different. It was cleaning the fact that the information currently provided by FieldItem::schema() 'not_null' is actually not about the schema, it's about the property - "is this property essential/required to constitute a field value or not". Also makes it clearer for field type authors to figure out how to fill that information ("why should I put not_null = TRUE or FALSE ?" is quite obscure at the moment).

Not sure whether that move to better API conceptual consistency is still an acceptable change now that the critical symptom has been fixed in a simpler way, though. @catch ? ;-)

@yched I remember that discussion (or some of it) from Amsterdam and it also bothers me a bit about the patch (but I didn't review the final committed patch before it was committed).

I actually think we should roll back and continue the discussion here. If we get stuck, we can always re-commit #39 but schema definition not matching the schema feels like a normal bug at least and this is a real continuation of the issue, not just a follow-up.

Moving back to active for now - @alexpott what do you think?

Please no patches on this issue until after a revert decision though.

schema definition not matching the schema feels like a normal bug at least and this is a real continuation of the issue, not just a follow-up.

My bad, this was not my end goal too, obviously. I thought we would just fix the critical issue to unblock the stuff depending on it and go on with the plan in #2346019: Handle initial values when creating a new field storage definition. Again, sorry for the confusion.

I'm happy with going either way here - I see the merits of rolling back - but I also see the merits of doing the simple thing to unblock and handling the complexity in #2346019: Handle initial values when creating a new field storage definition.

OK I went ahead and rolled this back.

@Alex #2346019: Handle initial values when creating a new field storage definition would allow for not null field schemas.

This issue is essentially preventing not null field schemas, but without updating the field definitions, and I think that's more or less all we need to do here. We could possibly throw an exception rather than forcing the definition change once core is converted, so it's clear that's not possible at the moment too.

Here's a patch for the approach that was discussed in AMS.
- uses $property->setRequired() in FieldItem::propertyDefinitions() to denote required properties
- uses that to determine 'not null' in tabe schemas in SqlContentEntityStorageSchema
- removes all 'not null's in FieldItem::schema() - a lot of them were moot / inconsistent.
Let's see what breaks...

Side notes:
- DataDefinitionInterface contains isRequired() but not setRequired() - well, it doesn't contain any setter, for that matter...
- 'path' field type (PathItem) : WTF ? no schema ? nothing is ever stored ?

A lot of those fails seem to be around NULL format for text fields. Current code doesn't seem to treat that property as "required", let's see how many tests this fixes.

Also - damn, setting entity_ref's target_id property to "required" breaks autocreate, of course : the target_id receives an actual ID when the "autocreate" entity is saved, during the parent entity's preSave(), which means after validation. That's a problem.

Leaving it out for now, just to see.

Just a reroll and a small fix (probably not the right one).

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1407,10 +1407,11 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
+      // In order to optimize indexes, forbid NULL values in Key fields. Other
+      // columns necessarily allow NULL values in order to support adding a
+      // base field definition to an entity type with non-empty base tables.
+      // @todo Revisit in https://www.drupal.org/node/2346019.
+      $schema['fields'][$schema_field_name]['not null'] = in_array($field_name, $keys);

Can we do this instead by either requiring code that creates a key field to call setRequired(TRUE), or else doing that in an appropriate central place, but not special-casing a difference between required and 'not null' here?

Ignore #66. I understand now why that's being punted to the @todo. So I think "all" that's needed here is fixing the test failures.

Interdiff #64 :

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1622,7 +1622,9 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
-      $data_schema['fields'][$real_name]['not null'] = $properties[$column_name]->isRequired();
+      if (isset($properties[$column_name])) {
+        $data_schema['fields'][$real_name]['not null'] = $properties[$column_name]->isRequired();
+      }

Testbot seems to like that. Would be interesting to determine what triggers that, though - a column name that doesn't match a property looks like something is wrong with some field type.

The column names defined in schema() should be a subset of the properties defined in propertyDefinitions() - even through we don't strictly check and enforce that atm.

I created #2390495: Support marking field storage definitions as required per our discussion. It's not needed to resolve this critical.

I'm working on this.

This should be a better version of #64, which should give us passing PHP Unit tests. Let's see whether I broke anything in the process.

Still on this :)

tagging

Let's clean up things a bit.

This should fix most remaining failures

Mmh, not sure why #71 was lost, restoring it + reapplying EntityReferenceItem changes.

Discussed with @yched and @amateescu a fix for entity references having the autocreate option enabled. This is a start but it's working well while manual testing, let's see how many failures are left.

Slightly different approach. I'm not completely satisfied about it yet but let's see whether it's working better.

Interdiff is with #77.

The number of failures seems to indicate that we're on a better path :)

This should be green, now I'm going to clean-up EntityReferenceItem to centralize the new logic and add more test coverage.

reroll

Yay! Very happy to see this green. "Needs work" per #86.

It would help simplifying things if we could get #2137309: Typed data does not handle set() and onChange() consistently in first, but it's not required to block this blocking issue on it.

This cleans-up ERI taking inspiration from #2137309: Typed data does not handle set() and onChange() consistently, let's see whether it's still green.

oops, wrong patch, thanks :)

Some doc nitpicks

1. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
   @@ -28,6 +28,9 @@
   +   * Properties that are required to constitue a valid, non-empty item should
   

   constitute ?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -178,15 +203,35 @@ public function getValue() {
   +   * @param string $property_name
   +   * The name of the property to be written.
   +   * @param $value
   +   * The value to set.
   

   Comments of the params need indentation

Added explicit test coverage for the new code and fixed #95. We should be done here.

Small clean-up

I looked over the patch and the parts in EntityReferenceItem look good to me. I will have to leave the RTBC to @fago or @yched though..

Trying to unblock the patch

Just a reroll

The whole patch looks great to me. Very minor nits:

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -38,6 +37,13 @@
   +  const NEW_ENTITY_MARKER = -42;
   

   Cute. But really? Can we use 0 or -1 instead?

2. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteTagsWidget.php
   @@ -74,7 +75,7 @@ public function elementValidate($element, FormStateInterface $form_state, $form)
              $value[] = array(
   -            'target_id' => NULL,
   +            'target_id' => EntityReferenceItem::NEW_ENTITY_MARKER,
                'entity' => $this->createNewEntity($input, $element['#autocreate_uid']),
              );
   ...
   +++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidget.php
   @@ -71,7 +72,7 @@ public function elementValidate($element, FormStateInterface $form_state, $form)
   -          'target_id' => NULL,
   +          'target_id' => EntityReferenceItem::NEW_ENTITY_MARKER,
              'entity' => $this->createNewEntity($element['#value'], $element['#autocreate_uid']),
   ...
   +++ b/core/modules/taxonomy/src/Plugin/Field/FieldWidget/TaxonomyAutocompleteWidget.php
   @@ -156,7 +157,7 @@ public function massageFormValues(array $values, array $form, FormStateInterface
   -        $item = array('target_id' => NULL, 'entity' => $term);
   +        $item = array('target_id' => TaxonomyTermReferenceItem::NEW_ENTITY_MARKER, 'entity' => $term);
   

   With the changes in EntityReferenceItem that keep target_id and entity in sync, can we remove these 'target_id' lines entirely?

3. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListFloatItem.php
   @@ -28,7 +28,8 @@ class ListFloatItem extends ListItemBase {
   +      ->setRequired(TRUE);
   

   There's also a 'not null' in this class that needs to be removed. Also some in ShapeItem.

Also, this will need a change record instructing contrib field types to:
- Remove their 'not null' lines.
- Add a setRequired() where needed, which for a single-property field should always include that property.

NW as per #101

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -206,12 +251,13 @@ public function isEmpty() {
+      $this->target_id = $this->entity->id();
...
       $this->target_id = $this->entity->id();

Why are we not using $this->set('target_id', $this->entity->id(), FALSE); here? AFAIK save is always called on parent i.e. entity then why set $notify TRUE here?

This should address the reviews above. It also expands test coverage to actually cover the main bug here, because that was not the case previously.

Working on the CR right now.

Here you can find a failing patch.

Spoke with @fago about #103 and we agreed there are cases where the parent needs to be notified the target id column has changed, e.g. to invalidate caches. Reverting that change.

Draft CR at https://www.drupal.org/node/2392807

Thanks!

Reverted the issue summary to the actual approach we use for the fix ($property->setRequired(TRUE)).

Other than that, I'm just a little worried that the changes in ERItem::setValue() / set() / onChange() clash with or anticipate the cleanup being done in that exact area in
#2386559: ERItem::setValue(array('entity' => $entity) produces broken Items
and
#2137309: Typed data does not handle set() and onChange() consistently
(the former is actually waiting for the latter being committed)

It might be better to get those in first ?

The proposed solution was outdated.

@yched:

This is a prioritized critical, so I don't think it should wait for those: if the changes here are good the other issues just need a reroll. This was written with an eye to #2137309: Typed data does not handle set() and onChange() consistently, btw.

Discussed with @fago, he agreed it would be best to get #2137309: Typed data does not handle set() and onChange() consistently and #2386559: ERItem::setValue(array('entity' => $entity) produces broken Items in first. This area (TypedData notifications & sync between properties in an ER item) is quite confused at the moment, and untangling it is a bit hairpulling.

The 1st one is RTBC already, I'll bug @alexpott to try to get it committed asap.
@fago said he'd update the 2nd one today with a patch rolled on top of the 1st one, for a quicker RTBC.

So this makes #1916790: Convert translation metadata into regular entity fields postponed not only this one issue but now three issues. Considering this issue as an endpoint of things would be incorrect. See #1916790-98: Convert translation metadata into regular entity fields. Is that intended?

Spoke with @yched and he's ok with unpostponing this now that #2137309: Typed data does not handle set() and onChange() consistently landed, #2386559: ERItem::setValue(array('entity' => $entity) produces broken Items will be rolled on top of this instead.

The reroll was a bit tough so I'm providing a diff of EntityReferenceItem (the only part that was involved in the merge) for reviewer's convenience.

Agreed, it should be easy to merge #2386559: ERItem::setValue(array('entity' => $entity) produces broken Items in later on.

Reviewing the patch with @yched we found a small issue.

So my worry is :

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -141,16 +148,25 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   public function setValue($values, $notify = TRUE) {
     if (isset($values) && !is_array($values)) {
-      // If either a scalar or an object was passed as the value for the item,
-      // assign it to the 'entity' property since that works for both cases.
-      $this->set('entity', $values, $notify);
+      $values = $values instanceof EntityInterface ? array('entity' => $values) : array('target_id' => $values);
     }
-    else {
-      // Make sure that the 'entity' property gets set as 'target_id'.
-      if (isset($values['target_id']) && !isset($values['entity'])) {
-        $values['entity'] = $values['target_id'];
-      }
-      parent::setValue($values, $notify);
+    // Make sure that the 'entity' property gets set as 'target_id'.
+    if (!isset($values['entity']) && isset($values['target_id'])) {
+      $values['entity'] = $values['target_id'];
+    }
+
+    // Update any existing property objects.
+    foreach ($this->properties as $name => $property) {
+      $value = isset($values[$name]) ? $values[$name] : NULL;
+      $this->writePropertyValue($name, $value);
+      // Remove the value from $this->values to ensure it does not contain any
+      // value for computed properties.
+      unset($this->values[$name]);
+    }
+
+    // Notify the parent of any changes.
+    if ($notify && isset($this->parent)) {
+      $this->parent->onChange($this->name);
     }
   }

@@ -169,17 +185,26 @@ 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);
+  protected function writePropertyValue($property_name, $value) {
+    if ($property_name == 'entity') {
+      parent::writePropertyValue('entity', $value);
+      $target_id = $value instanceof EntityInterface && $value->isNew() ? static::$NEW_ENTITY_MARKER : $this->properties['entity']->getTargetIdentifier();
+      parent::writePropertyValue('target_id', $target_id);
     }
-    elseif ($property_name == 'entity') {
-      $this->writePropertyValue('target_id', $this->get('entity')->getTargetIdentifier());
+    elseif ($property_name == 'target_id' && $value !== static::$NEW_ENTITY_MARKER) {
+      parent::writePropertyValue('target_id', $value);
+      parent::writePropertyValue('entity', $value);
+    }
+    else {
+      parent::writePropertyValue($property_name, $value);
     }
-    parent::onChange($property_name, $notify);
   }

The first hunk largely reverts what was done in #2137309: Typed data does not handle set() and onChange() consistently (keep the main logic in Map::setValue() and have child implementations be mere wrappers around that).

The second hunk directly contradicts the approach for bringing sanity to the 'target_id' / 'entity' sync dance that @fago defined in #2386559: ERItem::setValue(array('entity' => $entity) produces broken Items.

It seems the unique point of those changes is to ensure that "wehenever 'entity' received to an unsaved entity, 'taget_id' needs to be set to NEW_ENTITY_MARKER".

Given that :
- Such syncing is currently over complicated, I'm honestly lost in it myself.
- #2386559: ERItem::setValue(array('entity' => $entity) produces broken Items has a plan to make those "sync" simpler
- Meanwhile, this patch here needs convoluted code to acount for its own, new syncing needs,
I'd be way more confortable with having #2386559: ERItem::setValue(array('entity' => $entity) produces broken Items clarify things first (it's green), and then use it here to express our new sync behavior using simple code", rather than committing the current convoluted code here and have to undo it later.

But well, I'm officially lost in that code, so I'll leave it to @fago to decide.

Fair enough, unpostponed #2386559: ERItem::setValue(array('entity' => $entity) produces broken Items.

I think I got it right this time. I was not able to succesfully reroll this exactly because #2386559: ERItem::setValue(array('entity' => $entity) produces broken Items. The attached patch is rolled upon it so it should be an easy reroll once that lands.

Setting NR for the bot, but still actually postponed.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -185,26 +191,20 @@ public function getValue() {
+      $property = $this->get('entity');
+      $target = $property->getTarget();
+      $target_id = $target && $target->getValue()->isNew() ? static::$NEW_ENTITY_MARKER : $property->getTargetIdentifier();

Why can't we use $this->get('entity')->getTargetIdentifier() here?

Not sure what you mean: the ternary operator is needed because in the case of a new entity the ID is NULL or a value different from static::$NEW_ENTITY_MARKER. If the entity is new we need to use the marker value to populate target_id, so that it will pass not null validation even if the entity actual ID is NULL.

Yeah, now I see you are right please ignore my comment and thanks for the explanation.

This should be green again.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -164,7 +164,7 @@ public function setValue($values, $notify = TRUE) {
         // If both properties are passed, verify the passed values match.
-        if ($this->get('entity')->getTargetIdentifier() != $values['target_id']) {
+        if ($this->get('entity')->getTargetIdentifier() != $values['target_id'] && ($values['target_id'] != static::$NEW_ENTITY_MARKER || !$this->entity->isNew())) {

This comment needs update cuz it doesn't make sense with current condition.

Right, thanks :)

d.o.--

Patch looks good to me. The only questionable thing after #121 is addressed so RTBC.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1022,6 +1022,7 @@ protected function processRevisionDataTable(ContentEntityTypeInterface $entity_t
      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']);
   

   Shouldn't we ensuring this for all identifiers not only those of type int (that we convert to serial)?

2. I feel that the difference between configurable field instance requiredness and data definition schema requiredness is not really explained.

     /**
      * Returns whether at least one non-empty item is required for this field.
      *
      * Currently, required-ness is only enforced at the Form API level in entity
      * edit forms, not during direct API saves.
      *
      * @return bool
      *   TRUE if the field is required.
      */
     public function isRequired();
   

   From FieldDefinitionInterface - is this statement true?

   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1609,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;
   +      // Properties marked as 'required' do not allow NULL values, others do.
   +      // Note: this works because dedicated tables contain no row for empty
   +      // fields.
   +      $data_schema['fields'][$real_name]['not null'] = $properties[$column_name]->isRequired();
        }
   

   This is also ringing alarm bells.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -164,7 +164,7 @@ public function setValue($values, $notify = TRUE) {
+        if ($this->get('entity')->getTargetIdentifier() != $values['target_id'] && ($values['target_id'] != static::$NEW_ENTITY_MARKER || !$this->entity->isNew())) {

Can we please add a quick test of ($values['target_id'] != static::$NEW_ENTITY_MARKER || !$this->entity->isNew()) part of the condition in EntityReferenceItemTest?

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -155,8 +163,11 @@ public function setValue($values, $notify = TRUE) {
   +        if ($this->get('entity')->getTargetIdentifier() != $values['target_id'] && ($values['target_id'] != static::$NEW_ENTITY_MARKER || !$this->entity->isNew())) {
   

   Nitpick : any way we could split that on several lines using temp vars ?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -215,13 +229,15 @@ public function isEmpty() {
        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) {
   +      // Handle the case where an unsaved entity was directly set using the
   +      // public 'entity' property and then saved before this entity. In this
   +      // case the referenced entity will already be saved so no need to save it
   +      // again.
   +      if ($this->entity->isNew()) {
   +        $this->entity->save();
   +      }
   

   Nitpick : IMO we can simplify the comment to "Save the entity if it has not already been saved by some other code".

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -187,11 +198,14 @@ 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 = $property->getTarget();
   +      $target_id = $target && $target->getValue()->isNew() ? 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);
      }
   

   Awesome, much simpler than in the previous iterations !

   Although : when assigning '2' to the 'entity' property, this code here will now trigger an entity load ? Any way we could avoid that ?
   That's a perf impact, since "assign '2' to the 'entity' property" is what happens on $node->field_ref = 2 / $node->field_ref->setValue(2) - see the comment at the beginning of ERItem::setValue().

   @fago, do you think that is problematic ?

This should address #136 and #137 (plus a couple of other cases I came up with meanwhile).

@alexpott:

Shouldn't we ensuring this for all identifiers not only those of type int (that we convert to serial)?

Yep :)

Actually that line is redundant in the default implementation because the same 'not null' clause is set in ::getSharedTableFieldSchema(). Anyway subclasses may make find this useful, so better making it work correctly for all possible cases.

+++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
@@ -161,11 +161,28 @@ public function getDisplayOptions($display_context);
+   * validation. However if the list contains at list one non-empty item, the

"at least"

Not sending the patch to the testbot as there is a big queue already: this is green if the previous one is (only a PHP doc a change).

Awesome tests thanks @plach

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTest.php
@@ -230,6 +230,24 @@ function testAutocreateApi() {
+      $entity->user_id[0]->get('target_id')->setValue(-1);

@@ -247,6 +265,24 @@ function testAutocreateApi() {
+      $entity->user_role[0]->get('target_id')->setValue(-1);

Can we replace -1 with proper variable here?

D'oh, missed #138.

Can we replace -1 with proper variable here?

Nope, it's protected :)

Please, tell me this is RTBC now :)

re: @alexpott #136.2 and the interdiff in #139 :

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1616,9 +1616,11 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
-      // Properties marked as 'required' do not allow NULL values, others do.
-      // Note: this works because dedicated tables contain no row for empty
-      // fields.
+      // Properties marked as "required" do not allow NULL values, others do. If
+      // a not required field has a non-empty value its required properties must
+      // be populated. Note: this works because dedicated tables contain no row
+      // for empty fields, instead shared tables allow "not null" columns only
+      // for required fields.

I'm not sure I understand that new comment, and IMO it unnecessarily muddies the water. We are in the code that deals with per-field tables, for which *field* requiredness doesn't matter.
It matters upstream at the entity validation level, but storage-wise, it is orthogonal to the reasoning about the structure of a per-field table.

@alexpott : if the comment here said :
"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.",
would that adress your concerns from #136.2 ?

+++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
@@ -161,11 +161,28 @@ public function getDisplayOptions($display_context);
-   * Currently, required-ness is only enforced at the Form API level in entity
-   * edit forms, not during direct API saves.
+   * If the defined field is required, empty item lists will fail Type Data
+   * validation. However if the list contains at list one non-empty item, the
+   * item will pass validation.
+   *
+   * A single item in the list is considered empty if at least one of its
+   * required properties is empty. For instance in the default implementation a
+   * String item containing an empty string is not considered empty, while the
+   * same item containing a NULL value is considered empty.
+   *
+   * Since in the default widget implementation a required field definition
+   * determines a '#required' form element, a non-empty String item list
+   * containing empty string values will fail form validation even if Typed Data
+   * validation will pass.

Hmmm - that new text contains a couple inaccuracies. We could work to adjust them, but I don't get why we bother adding that in this issue here, since, as mentioned in the previous comment, this is not the issue where we do anything with *field*-requiredness ?

Field-requiredness is going to matter for allowing 'not null' = TRUE in shared/base tables, and that's #2346019: Handle initial values when creating a new field storage definition.

@plach #142 : awesome, thanks !
I'd feel more confortable to have @fago validate that interdiff (the hasTarget() part), but that does adress my concerns in #138.3

@yched yes the suggestion in #143 sounds better. I'm just concerned that the meaning of requiredness is different and there is a lack of documentation explaining this.

@yched:

Can we just fix the new docs? They are describing the current status, we can always update them after adding support for required storage definitions. If you tell me what's wrong with them it should be a quick fix...

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -155,8 +163,12 @@ public function setValue($values, $notify = TRUE) {
              throw new \InvalidArgumentException('The target id and entity passed to the entity reference item do not match.');
   

   The second clause would be easier to parse if we apply the de-morgan rule once and make it:

   if ($entity_id != $values['target_id'] && !($values['target_id'] == static::$NEW_ENTITY_MARKER && $this->entity->isNew())) {

2. +++ b/core/lib/Drupal/Core/TypedData/DataReferenceBase.php
   @@ -31,6 +31,13 @@
   +  public function hasTarget() {
   +    return isset($this->target);
   

   That generally loads the reference - imo the default should be to check the target identifier so it avoids loading it unncessarily.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -187,11 +199,16 @@ public function getValue() {
   +      $property = $this->get('entity');
   +      $has_new_entity = $property->hasTarget() && $property->getTarget()->getValue()->isNew();
   

   Given the marker, there should be no need any more to load entities when an entity reference is set by id. However, this loads it. As discussed, I'd suggest adding a isNew() method to the EntityReference property class, which can assume an entity is not new if there is no object but only an id given.

That generally loads the reference

Nope, ::hasTarget() just checks a plain object property value, just to be sure there was no magic getters involved I debugged this with the following code:

    $entity = $this->entityManager
      ->getStorage($this->entityType)
      ->create(array('name' => $this->randomString()));
    $storage = $this->entityManager->getStorage('user');
    $user_id = $this->generateRandomEntityId();
    $user = $storage->create(array('uid' => $user_id, 'name' => $this->randomString()));
    $user->save();
    $storage = $this->entityManager->getStorage('user');
    $storage->resetCache();
    $entity->user_id = $user->id();
    $user = User::load($user_id);

The last line fetches the user entity from the DB, hence it was not loaded while setting the value in the previous line, otherwise it would be in the static cache.

imo the default should be to check the target identifier so it avoids loading it unncessarily.

Checking whether the ID is defined does not help here, we need to know whether the target is actually loaded.

Given the marker, there should be no need any more to load entities when an entity reference is set by id. However, this loads it.

Nope, this code is written exactly with the goal of not loading the entity object. This is the place where the marker is set, hence we cannot rely on it, we just need to know whether the entity is new.

As discussed, I'd suggest adding a isNew() method to the EntityReference property class, which can assume an entity is not new if there is no object but only an id given.

I don't agree: if in the EntityReference object we have only the id set we cannot automatically assume it's a new entity. That's precisely the use case tested in the code above. It's in the parent object that we can safely make this assumption because the entity property cannot be set to a new entity without providing the related object.

Sorry, I misread the sentence above. I'm fine with the ::isNew() method, it can be implemented exactly as ::hasTarget(), although it provides less information (not sure whether that's good or bad).

Once the documentation issue is sorted out I'll provide a new patch.

I'll let @plach & @fago disccus #148 / #149.

Meanwhile, here are some proposed doc adjustments for my remarks in #143 / #144.
Patch needs a reroll, so I just post them as an interdiff.

This includes #150 (thanks) and should fix #148. I tried to implement ::isTargetNew() (I think the name is more consistent this way) as:

return !isset($this->id) && isset($this->target);

but that is not enough, because we can populate the item property with an already saved entity. The current code is basically the same of the previous patch, just wrapped in the ::isTargetNew() method. Performance concerns should be still addressed as the entity will be accessed only if already populated, so no load operation is ever triggered.

Just a reroll

@fago:

Any feedback on the interdiff posted in #151?

Just keeping this fresh...

I re-reviewed the entire patch, and it looks great to me, but assigning to fago for RTBC to confirm his concerns in #148 have been addressed.

Some non-RTBC-blocking nits:

1. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
   @@ -251,9 +255,33 @@ public function testBaseFieldCreateDeleteWithExistingEntities() {
   +    // Add a base field with a 'not null' column in the schema and run the
   +    // update. Ensure 'not null' is not applied and thus no exception is thrown.
   

   'not null' isn't in the ShapeRequired field's schema(). Should we change the comment to "Add a base field with a required property..."? Or were you actually wanting to test the situation of someone adding a 'not null' explicitly in the schema() method?

2. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
   @@ -282,9 +312,31 @@ public function testBundleFieldCreateDeleteWithExistingEntities() {
   +    // Test that required columns are created as 'not null'.
   +    $this->addBundleField('shape_required');
   +    $this->entityDefinitionUpdateManager->applyUpdates();
   +    $message = 'The new_bundle_field_shape column is not nullable.';
   +    try {
   +      $this->database->insert('entity_test_update__new_bundle_field')
   +        ->fields(array(
   +          'bundle' => $entity->bundle(),
   +          'deleted'=> 0,
   +          'entity_id' => $entity->id(),
   +          'revision_id' => $entity->id(),
   +          'langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED,
   +          'delta' => 0,
   +          'new_bundle_field_color' => $this->randomString(),
   +        ))
   +        ->execute();
   +      $this->fail($message);
   +    }
   +    catch (DatabaseExceptionWrapper $e) {
   +      $this->pass($message);
   +    }
   

   There can be other reasons for the exception to be thrown, so to make this test more robust, how about also doing an insert with the same data and the new_bundle_field_shape column and make sure that passes?

Additionally, should something like #149 be added as an automated test, so we know if some future patch creates an unnecessary entity load, since it seems like it's not so easy to tell just by looking at the code?

Addressed #155. This should prove that @fago's concerns were addressed too.

Wrong patch...

Thanks @plach. A couple nitpicks on the last interdiff :

1. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTest.php
   @@ -343,4 +345,54 @@
   +  public function testTargetEntityNoLoad() {
   

   Nice - however, this tests Core stuff, right ?
   Drupal\Core\Entity\Plugin\DataType\EntityReference and Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem ?

   So it isn't really for modules/entity_reference/src/Tests ?

2. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTest.php
   @@ -343,4 +345,54 @@ protected function assertUserRoleAutocreate(EntityInterface $entity, $setter_cal
   +    $entity_type->setHandlerClass('storage', '\Drupal\entity_test\Entity\EntityTestNoLoadStorage');
   

   Could use a word of comment ?

3. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
   @@ -320,21 +320,26 @@ public function testBundleFieldCreateDeleteWithExistingEntities() {
   +    $values = array(
   +      'bundle' => $entity->bundle(),
   +      'deleted'=> 0,
   +      'entity_id' => $entity->id(),
   +      'revision_id' => $entity->id(),
   +      'langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED,
   +      'delta' => 0,
   +      'new_bundle_field_color' => $this->randomString(),
   +    );
        try {
          $this->database->insert('entity_test_update__new_bundle_field')
   -        ->fields(array(
   -          'bundle' => $entity->bundle(),
   -          'deleted'=> 0,
   -          'entity_id' => $entity->id(),
   -          'revision_id' => $entity->id(),
   -          'langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED,
   -          'delta' => 0,
   -          'new_bundle_field_color' => $this->randomString(),
   -        ))
   +        ->fields($values)
            ->execute();
          $this->fail($message);
        }
        catch (DatabaseExceptionWrapper $e) {
   +      $values['new_bundle_field_shape'] = $this->randomString();
   +      $this->database->insert('entity_test_update__new_bundle_field')
   +        ->fields($values)
   +        ->execute();
          $this->pass($message);
   

   OK, but could use a couple comments too ?

4. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNoLoadStorage.php
   @@ -0,0 +1,25 @@
   +class EntityTestNoLoadStorage extends SqlContentEntityStorage {
   

   Could be more interesting to have a test storage that extends SqlStorage but logs actual loads (from storage / from cache) in an internal property that can be fetched in tests.

   Arguably outside the scope of this patch here, though. Just sayin, feel free to ignore :-)

Thanks, plach. Looks like I mixed up the both entity reference classes - sry. However, having test coverage for that is best anyway. (Looks like this would have been a good fit for a phpunit test and mocking, but nevermind.)

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReference.php
@@ -56,6 +56,18 @@ public function getTargetDefinition() {
+    // We assume that if the target entity is not set we cannot have a new
+    // entity.

That sentence was a bit a hard to parse, maybe rephrase to something like: "If only an ID is given, the reference cannot be a new entity."

Agreed this is ready, should we address the few nitpicks and move the test to core before commit?

@yched:

This should address #159 except for 4, which I shamelessly ignored :)

@fago:

Looks like this would have been a good fit for a phpunit test and mocking, but nevermind.

Yep, initially I wanted to provide a PHP unit test, but then it turned out there is no such test for field items so this would be a bit inconsistent.

Yay :-)

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 057b0ca and pushed to 8.0.x. Thanks!

Y A H O O O O !

Finally. Thanks Entity API team for fixing it and @alexpott for committing it.

Awesome ! @plach++++++

Now we can do this : #2401221: FieldItem::isEmpty() should rely on $property->isRequired()

Two follow ups from this

#2403793: EntityReferenceItem uses a static, but it was most likely supposed to be a constant - the static can't be accessed elsewhere and can be changed

and

#2403873: FileFormatterBase does not retain unsaved entities (files) - which I think was just missed

Finally published the change record.

Another follow up
#2404331: Can't serialise an entity with an entity reference to an unsaved entity (remove EntityReferenceItem::$NEW_MARKER in favour of a validation constraint)