ERItem::setValue(array('entity' => $entity) produces broken Items

Since the assignment of the target_id is taken care of by EntityReferenceItem::onChange(), which isn't triggered by Item::setValue(), maybe there should also be some special code in EntityReferenceItem::setValue() to populate the 'target_id' if the 'entity' property was part of the passed value ?

As a proof-of-concept fail in "drush php" :

$term = \Drupal\taxonomy\Entity\Term::create(['vid' => 'tags', 'name' => 'foo']);
$term->save();
$node = \Drupal\node\Entity\Node::create(['type' => 'article']);
// Assign the term by 'entity' in setValue():
$node->field_tags = [ [ 'entity' => $term ] ]; // or $node->field_tags[0] = [ 'entity' => $term ];
$node->field_tags->entity;  // OK, returns the term
$node->field_tags->target_id; // NOK, returns null
// Assign the 'entity' property directly:
$node->field_tags->entity = $term;
$node->field_tags->target_id; // OK, returns the term id.

Attached patch seems to fix this - this would really need to be approved by @fago though, because the sync between 'target_id' and 'entity' goes through a notify dance that loses me a bit :-).

Also, this will need tests, once the fix is agreed.

Yep, the tests look great, thanks @amateescu :-)

Patch from #4 + tests from #5

Indeed - even with current patch :

$term1 = \Drupal\taxonomy\Entity\Term::create(['vid' => 'tags', 'name' => 'foo']);
$term1->save();
$term2 = \Drupal\taxonomy\Entity\Term::create(['vid' => 'tags', 'name' => 'bar']);
$term2->save();
$node = \Drupal\node\Entity\Node::create(['type' => 'article']);
$node->field_tags = [ [ 'entity' => $term1, 'target_id' => $term2->id() ] ];
$node->field_tags->entity->id() == $term1->id(); // TRUE, 'entity' contains $term1
$node->field_tags->target_id == $term2->id(); // TRUE, 'target_id' points to $term2

Also, looks like LanguageItem will need the same fixes, it is likely to have the same issues for its 'value' and 'language' properties.

Yay, that approach looks way easier to follow :-)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -150,12 +150,19 @@ public function setValue($values, $notify = TRUE) {
   +      parent::setValue($values, FALSE);
   +      // Make sure values stay in sync if only one is passed.
          if (isset($values['target_id']) && !isset($values['entity'])) {
   -        $values['entity'] = $values['target_id'];
   +        $this->synchronizePropertyValues('target_id');
   +      }
   +      elseif (!isset($values['target_id']) && isset($values['entity'])) {
   +        $this->synchronizePropertyValues('entity');
   +      }
   +      if ($notify) {
   +        $this->notifyParent();
          }
   

   We should comment this better IMO :

   // Apply the specified values without notifying the parent yet. 
   ...
   // Make sure the 'target_id' and 'entity' properties stay in sync.
   ...
   // Notify the parent if needed.
   ...
   

   Also, we should raise an exception if $values contain both 'entity' and 'target_id' that contradict each other. That could go in that if / elsif group ? We'd still call parent::setValue() for nothing, but that's not too important ?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -173,16 +180,26 @@ public function getValue($include_computed = FALSE) {
   +   * Synchronizes the property values of the target_id and entity properties.
   

   Nitpick : "Synchronizes the values of the 'target_id' and 'entity' properties" ?

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -173,16 +180,26 @@ public function getValue($include_computed = FALSE) {
   +   * @param  string $changed_property
   

   nitpick : double space