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

The fix look good to me. I'd suggest to add a comment to the condition.

--- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php

--- a/core/modules/entity_reference/src/Tests/EntityReferenceItemTest.php
+++ b/core/modules/entity_reference/src/Tests/EntityReferenceItemTest.php

The fix is in core folder and tests are in ER. Which component should we choose? :D

This needs @fago's approval

This is the kind of problems I feared would show up when we were discussing the change to ER's autocreate architecture in Ghent :(

One of the problems with the patch in #15 is what happens if a validation is triggered between "get the field value" and "then set it"? I expect the validation to fail because target_id will be NULL.

I think we need to revisit that decision and try to see if we can bring back the "old way" (e.g. 'target_id' => NULL, 'entity' => $entity_object) for autocreate somehow.

Maybe my proposal to skip validation at some point and bring it back later was not so good, but the real case here is that the 'target_id' property might not pass the NotNull constraint on its own, but it should pass it if combined with the 'entity' property. So we need a way to validate a property with the context of its related properties. Does that make sense?

Added the check for if target id values['target_id'] is defined.

Added inline condition.

hm, that patch meanas $entity->reference->first()->target_id is not the same as $entity->reference->first()->getValue()['target_id'] then - that's unexpected behaviour imo.

Maybe better, we should allow writing the value back in combination with the unsaved entity?

Reroll after #2404021: entity_reference formatters should be in Core

@fago, did you see #20? IMO, the solution here is really simple, we just need a custom NotNull constraint for the entity reference item which takes the context into account (i.e. the 'entity' property) when it determines if 'target_id' is NULL or not.

Added another test. Also created a similar issue for DER #2407123: Can't access autocreated entity after serialization

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -290,4 +290,22 @@ public static function calculateDependencies(FieldDefinitionInterface $field_def
+    if ($name == 'target_id' && $value == static::$NEW_ENTITY_MARKER) {
+      return NULL;
+    }

I think we should add comment for this.

Awesome, perfactly implemented. moving to RTBC

I would still like to hear a definite NO from @fago and/or @yched for the proposal in the last paragraph of #20 / #34, because only @larowlan replied so maybe no one else saw it :)

Thanks. The patch looks solid to me, however I must say I'm not so fond of making the -1 an implementation detail because it requires us to massage incoming/outgoing values all the time. Usually, that's a sign of bad architeture, i.e. if it's not suppoed to be in target_id, don't put it there but e.g. add another internal object property instead.
But as of now, we have to put it there because of validation. I'd agree with amateescu (#20/#34) that fixing validation instead seems to be better option to me, but I'm not sure how realistic or simple that would be. Given that, I'd be fine with moving on with this patch unless someone else can come up with a patch for #20? ;)

But yeah, I'd love to hear yched's and berdir's thoughts on this also. Thus, assigning to the next one.

Please have a look at these two points.

1. Can we explain API change $this->getTargetId() and $this->target_id in IS?
   

   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -297,6 +301,26 @@ public static function calculateDependencies(FieldDefinitionInterface $field_def
   +   * Returns the target id, even if the value is static::$NEW_ENTITY_MARKER.
   

   And maybe explain the difference here as well.

2. I added the new test in #35 but it didn't fail for ER it should because it failed for DER #2407123: Can't access autocreated entity after serialization . Can you please have a look at it?

From #20

Maybe my proposal to skip validation at some point and bring it back later was not so good, but the real case here is that the 'target_id' property might not pass the NotNull constraint on its own, but it should pass it if combined with the 'entity' property. So we need a way to validate a property with the context of its related properties. Does that make sense?

Can we at least explore that? We can create ERNotNull constraint?

The failed test was testEntitySaveOrder not testEntityAutoCreate which was added in #35.

Just uploading test only patch and testEntityAutoCreate test only patch to make sure fails exists. Other then that this looks RTBC to me.

Which patch is rtbc #64? It's best when the last patch on the issue is the rtbc patch since the rtbc retest uses this one.

Also @Berdir or @yched are yet comment despite people asking for it.

I can't speak for @yched but I don't feel like being able to provide a useful review for this, I haven't been involved in the recent refactorings that happened around this. @fago, @amateescu and @yched were.. and if they're OK with this then I'm fine with it too ;)

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceItemTest.php
@@ -228,6 +227,30 @@ public function testConfigEntityReferenceItem() {
+  public function testEntityAutoCreate() {

@larowlan we talked about updating this test last time around in IRC.

Sorry for the delay, I'm traveling without much internet access until tomorrow evening :/ I'm going to try and poke a bit at the approach of using a custom validation during this long train ride.

Just two little nitpicks:

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceItemTest.php
@@ -11,8 +11,8 @@
-use Drupal\entity_reference\Tests\EntityReferenceTestTrait;
 use Drupal\entity_test\Entity\EntityTest;
+use Drupal\entity_reference\Tests\EntityReferenceTestTrait;

This change is not necessary.

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceItemTest.php
@@ -228,6 +227,30 @@ public function testConfigEntityReferenceItem() {
+    $entity = serialize($entity);
+    $entity = unserialize($entity);

Can we change this to:

+    $entity_serialized = serialize($entity);
+    $entity = unserialize($entity_serialized);

New patch doesn't include old changes of testEntitySaveOrder

+  /**
    * Test saving order sequence doesn't matter.
    */
   public function testEntitySaveOrder() {
@@ -243,6 +266,12 @@ public function testEntitySaveOrder() {
     $entity->name->value = $this->randomMachineName();
     // Now save the term.
     $term->save();
+    // 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;
     // And then the entity.
     $entity->save();
     $this->assertEqual($entity->field_test_taxonomy_term->entity->id(), $term->id());

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/EntityReferenceNotNullConstraintValidator.php
@@ -0,0 +1,38 @@
+class EntityReferenceNotNullConstraintValidator extends NotNullValidator {

We should also test this.

Changing the title as well perhaps now @yched will review it. :P

NIce, this looks very similar to my wip patch from London :) It had a few failures locally and it seems that I lost the last commit on my branch, but I'll post it anyway to see if I can spot any major differences.

Ok, the only important difference is that we need to bring back the optimization from EntityReferenceFieldItemList::referencedEntities(), which used to check the 'target_id' property first in order to delay loading the computed 'entity' property as much as possible.

Added that optimization and a fixed a few nitpicks in @larowlan's patch (interdiff is to #89), this looks ready to me now.

Really, really sorry for keeping everyone waiting so long :-(
I'll try to have a closer look in the next few days.

However :

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -89,7 +82,6 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
-    $target_id_definition->setRequired(TRUE);

That was the reason why we introduced the NEW_ENTITY_MARKER in #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities - being able to set the property as "required", so that its SQL column gets a 'NOT NULL' = TRUE and keep the indexes optimized.

If we remove that and abandon the NOT NULL for target_id, then it seems we're basically reverting the e_r part of #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities, and then I'm not sure why we need to add new dedicated validators, they weren't needed back then ?

Thank you very much @yched for your insightful reply. :)

We should add explicit tests to make sure that target_id get 'NOT NULL' true. TBH we are not aware of this problem at all and I agree target_id field should be required.

AFAIK we are introducing validator to remove the dirty workaround to this problem (EntityReferenceItem::$NEW_MARKER). Maybe we are addressing this problem on a wrong layer? There should have to be some other way around this.

Heh, it's been so long that even I forgot what my proposal for a fix here was about :/

Looking back through the comments, what we would have to do in the patch is add the custom NotNull constraint to the 'target_id' property, not to the field item, and change SqlContentEntityStorageSchema::getDedicatedTableSchema() to also account for properties that have a 'not null' constraint (or an extension to it) instead of just checking isRequired():

      // 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();

Ok so this means ERItem property target_id can be NULL but actual target_id will never be NULL in schema or in theory . If this is correct then we need some documentation for this on ERItem class.

1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
   @@ -27,12 +27,12 @@ public function referencedEntities() {
   -      if ($item->hasNewEntity()) {
   -        $target_entities[$delta] = $item->entity;
   -      }
   -      elseif ($item->target_id !== NULL) {
   ...
            $ids[$delta] = $item->target_id;
          }
   +      elseif ($item->hasNewEntity()) {
   +        $target_entities[$delta] = $item->entity;
   +      }
   

   Aren't both the conditions(new one and old one) same? Change seems unwanted.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -33,19 +33,12 @@
   - *   constraints = {"ValidReference" = {}}
   + *   constraints = {"ValidReference" = {}, "EntityReferenceNotNull" = {}}
   

   I like that we have two constraint but NULL ref is a valid ref so perhaps they should belong together.

3. +++ b/core/lib/Drupal/Core/TypedData/DataReferenceTargetDefinition.php
   @@ -0,0 +1,27 @@
   +class DataReferenceTargetDefinition extends DataDefinition {
   

   Class docs missing.

#101.1: See #93.

Thanks @amateescu for clearing that out.
@yched & @fago any thoughts on the current patch.
@larowlan can we use EntityReferenceNotNull in DER if we keep it separate? Or do we need a DynamicEntityReferenceNotNull validator anyway?

Minor fixes.
@larowlan I have uploaded the DER patch in #2407123: Can't access autocreated entity after serialization . DynamicEntityReferenceItemTest::testEntityAutoCreate() and DynamicEntityReferenceItemTest::testEntitySaveOrder() still fails with this patch.

Hm, smart :-) Haven't done a line-by-line review, but I think I like the approach.

I hate to say that, but... might be good to have @fago vet that too :-/

Let's add a change notice as well or update Field schema no longer uses 'not null' entries.

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
@@ -19,7 +19,12 @@ class ValidReferenceConstraintValidator extends ConstraintValidator {
+    if (!$value->isEmpty() && $value->target_id === NULL && !$value->entity->isNew()) {
+      $this->context->addViolation($constraint->nullMessage);
+      return;
+    }
     if (!isset($value)) {
       return;
     }

If we've just called $value->isEmpty() it doesn't make sense to be checking isset($value) right after?

OK, fago isn't responding, so I'm biting the bullet :-)

I really like this approach - good work! :) The patch looks pretty good already also, here some small remarks related to documentation:

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -19,10 +19,15 @@ class ValidReferenceConstraintValidator extends ConstraintValidator {
   +    if (!$value->isEmpty() && $value->target_id === NULL && !$value->entity->isNew()) {
   

   Let'S add a pointer here why this is not handled by a regular NotNull constraint. E.g. point to the explanation in the definition class?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -230,10 +227,7 @@ public function onChange($property_name, $notify = TRUE) {
   -    if ($this->target_id !== NULL) {
   -      return FALSE;
   -    }
   -    if ($this->entity && $this->entity instanceof EntityInterface) {
   +    if ($this->target_id !== NULL || (($entity = $this->entity) && $entity instanceof EntityInterface)) {
   

   This seems to change nothing?

not so "fast" :P

@fago : HAH ! ;-)

#125 adresses @fago's remarks. Back to RTBC then ?

yeah, changes look good - agree on RTBC. Thanks!

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

Thanks @yched, @fago and @amateescu for all the help. @larowlan it is finally fixed great work.