ER's "autocreate" feature is mostly broken (and untested)

Wow, lots of nice catches. "autocreate" never stops being a PITA...

1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
   @@ -25,26 +25,31 @@ public function referencedEntities() {
   -      $ids[$delta] = $item->target_id;
   +      if ($item->hasUnsavedEntity()) {
   +        $target_entities[$delta] = $item->entity;
   +      }
   +      else {
   +        $ids[$delta] = $item->target_id;
   +      }
   

   Aw, good catch. Could use a word of comment though.

2. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
   @@ -25,26 +25,31 @@ public function referencedEntities() {
   -    if (empty($ids)) {
   +
   +    if (empty($ids) && empty($target_entities)) {
          return array();
        }
    
        $target_type = $this->getFieldDefinition()->getSetting('target_type');
        $entities = \Drupal::entityManager()->getStorage($target_type)->loadMultiple($ids);
   

   Logic needs some adjustment IMO : $ids might be empty, and then we don't want to run loadMultiple($ids).

3. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
   @@ -25,26 +25,31 @@ public function referencedEntities() {
   +    ksort($target_entities);
   

   I get why, but could also use a word of comment. "Ensure the returned array is ordered by deltas" ?

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   --- a/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
   +++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
   

   Not in the patch, but EntityReferenceFormatterBase::prepareView() still has a

   if (!empty($item->target_id)) {
     $target_ids[] = $item->target_id;
   }
   

   that doesn't look OK for referencing user 0 ?
   (might belong to a different patch though ?)

5. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
   @@ -79,14 +79,14 @@ public function prepareView(array $entities_items) {
   -        if ($item->target_id !== 0 && !isset($target_entities[$item->target_id])) {
   +        if ($item->target_id !== NULL && !$item->entity && !isset($target_entities[$item->target_id])) {
              // The entity no longer exists, so empty the item.
              $item->setValue(NULL);
              $rekey = TRUE;
              continue;
            }
   

   Given that ER formatters are now supposed to use ERFormatterbase::getEntitiesToView() to retrieve the entities to display, it doesn't look like we still need this at all ? (the "set the Item to NULL and run filterEmptyItems() to remove invalid items for the subsequent view() method" dance)

   Which is a win because it would remove the "viewing an ERFieldItemList modifies its content" ugliness.

6. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
   @@ -79,14 +79,14 @@ public function prepareView(array $entities_items) {
   +        $item->originalEntity = $item->entity ? $item->entity : $target_entities[$item->target_id];
   

   Same, add a word of comment ?

   Plus - unconditionally doing $item->entity will single-load the entity on regular (non-autocreate) items ?
   The entity will already have been multiple-loaded above, but I don't think we want to rely on the fact that the entity type uses a static cache ?

   Shouldn't it be smthg like:

   if ($item->hasUnsavedEntity() || isset($target_entities[$item->target_id]) {
     $item->originalEntity = $item->hasUnsavedEntity() ? $item->entity : $target_entities[$item->target_id];
   }
   

   ?

7. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceLabelFormatter.php
   @@ -66,7 +66,7 @@ public function viewElements(FieldItemListInterface $items) {
   -      if ($this->getSetting('link') && $uri = $entity->urlInfo()) {
   +      if ($this->getSetting('link') && !$entity->isNew() && $uri = $entity->urlInfo()) {
   

   Indeed too (although, shouldn't we fix $entity->urlInfo() to return NULL if entity->isNew() ?)

   Which brings - the method to use for checking "autocreate items" is:
   - $item->hasUnsavedEntity() when working on $item / $items
   - $entity->isNew() when working with the entities that come from getEntitiesToView().
   which is a bit confusing... When you think of it, "is new" and "is unsaved" are two ways to say the same thing, but the connection is not exactly intuitive when you look at the method names.

   We're not going to rename Entity::isNew(), but maybe ERFieldItem::hasUnsavedEntity() to hasNewEntity() ?

8. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTest.php
   @@ -127,6 +127,12 @@ public function testReferencedEntitiesMultipleLoad() {
   +    // Create a new target entity that is not saved, thus using the "autocreate"
   +    // feature.
   

   Nitpick : "thus testing the autocreate feature" would be more accurate ?

9. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFormatterTest.php
   @@ -44,7 +45,14 @@ class EntityReferenceFormatterTest extends EntityUnitTestBase {
   +  /**
   +   * The entity to be referenced in this test.
   +   *
   +   * @var \Drupal\Core\Entity\EntityInterface
   +   */
   +  protected $unsavedReferencedEntity;
   

   Doc is copy/pasted form the previous member :-)

10. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFormatterTest.php
    @@ -91,6 +99,13 @@ protected function setUp() {
    +    // Create another entity to be referenced but don't save it.
    

    s/don't/do not :-)

11. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFormatterTest.php
    @@ -178,13 +174,82 @@ public function testEntityFormatter() {
    +  protected function buildRenderArray($field_name, $formatter, $formatter_options = array()) {
    +    // Create the entity that will have the entity reference field.
    +    $referencing_entity = entity_create($this->entityType, array('name' => $this->randomMachineName()));
    +    $referencing_entity->{$field_name}[0]->entity = $this->referencedEntity;
    +    $referencing_entity->{$field_name}[0]->access = TRUE;
    +    $referencing_entity->{$field_name}[1]->entity = $this->unsavedReferencedEntity;
    +    $referencing_entity->{$field_name}[1]->access = TRUE;
    

    Nitpick, but it sounds a bit weird to have a helper method that hardcodes the entities being referenced ?
    It so happens that all tests there currently use the same two referenced entities, but why not pass that helper an array of entities to reference ?

    Also, not sure the method really needs $field_name as a param, it could internally use $this->fieldName ?

12. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidgetBase.php
    @@ -116,48 +117,33 @@ public function elementValidate($element, FormStateInterface $form_state, $form)
    +    $handles_multiple_values = $this->getSetting('multiple_values');
    

    Sounds wrong, 'multiple_values' is an entry in the @Widget annotation, not a widget setting.

    Use WidgetBase::handlesMultipleValues() ?

13. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidgetBase.php
    @@ -116,48 +117,33 @@ public function elementValidate($element, FormStateInterface $form_state, $form)
    +      // The autocomplete widget outputs one entity label per form element.
    +      if ($handles_multiple_values && $referenced_delta != $delta) {
    +        continue;
    +      }
    

    I might be wrong but - if this handles the "non-tags" widget (one copy per value) , then the condition should be !$handles_multiple_values ?

    Also, this code will run getReferencedEntities() (= "multiple load the entities for all deltas") once per delta. Again, not great if no static cache...
    It's already an issue in the current HEAD code, so maybe punt on this for now, but there might be some rethinking needed around the protected getLabels($items, $delta) - I know, the API for "multiple values" widgets kind of sucks :-/

14. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidgetBase.php
    @@ -116,48 +117,33 @@ public function elementValidate($element, FormStateInterface $form_state, $form)
    -  protected function getEntityIds(FieldItemListInterface $items, $delta) {
    

    Nice :-)

Heh, and I skipped the actual question I was asked :-)

I'm torn between the fix attached vs. introducing a $return_unsaved_entities = TRUE parameter to referencedEntities()

referencedEntities() returning "autocreate" entities seems right. Are there cases where we use referencedEntities() and would *not* want them ?

Wow what a review I must say just by reading it i learnt a lot. Thanks @yched and thanks for the patch @amateescu

1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
   @@ -23,28 +23,37 @@ public function referencedEntities() {
   +    // Return early if we don't have anything else to load.
   +    if (empty($ids) && !empty($target_entities)) {
   +      return $target_entities;
        }
   

   Still broken, no ? If $ids and $target_entities are both empty, then this runs loadMultiple($ids).
   Should be just if (empty($ids)) {return $target_entities;} ?

   (although then for clarity I'd tend to do :

   if ($ids) { 
     // add more stuff to $target_entities by loading the $ids
   }
   return $target_entities;
   

   but that's a matter of taste)

2. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
   @@ -30,6 +30,14 @@ protected function getEntitiesToView(FieldItemListInterface $items) {
   +      // The "originalEntity" property is assigned in self::prepareView() and
   +      // its absence means that the referenced entity was neither found in the
   +      // persistent storage nor is it a new entity (e.g. from "autocreate").
   +      if (!isset($item->originalEntity)) {
   +        $item->access = FALSE;
   +        continue;
   +      }
   

   Yeah... not sure why we still have that unofficial/hackish ->access property on the items, since formatters are only supposed to render the entities returned by getEntitiesToView() now, and that includes access checking ?

3. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
   @@ -61,37 +69,23 @@ public function prepareView(array $entities_items) {
        // Iterate through the fieldable entities again to attach the loaded data.
        foreach ($entities_items as $items) {
   ...
          foreach ($items as $item) {
   ...
   +        if (isset($target_entities[$item->target_id]) || $item->hasNewEntity()) {
   +          $item->originalEntity = isset($target_entities[$item->target_id]) ? $target_entities[$item->target_id] : $item->entity;
            }
   ...
          }
   

   OK - so now it looks like prepareView() could just defer to getReferencedEntities(), avoiding duplicating the "load or unsaved" logic :

   foreach ($entities_items as $items) {
     foreach ($items->getreferencedEntities() as $delta => $entity) { 
       $items[$delta]->originalEntity = $entity;
     }
   }
   

   ?

4. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFormatterTest.php
   @@ -178,13 +179,84 @@ public function testEntityFormatter() {
   +    $delta = 0;
   +    foreach ($referenced_entities as $referenced_entity) {
   +      $referencing_entity->{$this->fieldName}[$delta]->entity = $referenced_entity;
   +      $referencing_entity->{$this->fieldName}[$delta++]->access = TRUE;
   +    }
   

   *If* we get rid of ->access, this could become just $referencing_entity->{$this->field_name} = array_values($referenced_entities);

Nope, that means we'll do the multiple-entity-load multiple times

I don't get this.
getReferencedEntities() does one multiple-entity-load, but so does ERFormatterBase::prepareView().
How would calling getReferencedEntities() instead result in *more* multiple-entity-loads ?

I'm just saying : we have an official method to get the referenced entities, and it handles the subtleties of "autocreate". ERFormatterBase::prepareView() needs to get the referenced entities, and thus should use that method instead of custom code that duplicates the "autocreate" logic ?

Because prepareView() operates on multiple entities? :)

Oh gee, of course, sorry. I should be removed from the maintainers list for this :-p

Yeah, would be good to have a closer look at ->access in a separate issue.
RTBC for that one here.

Just : maybe add a comment in prepareView() about why we do not call referencedEntities() and thus have to re-do its internal logic ?

Maybe overkill : we could have a static ERFieldItemList::loadReferencedEntities(ERFieldItemList[] $items), that could be used both by ERFieldItemList::getReferencedEntities() and ERFormatterBase::prepareView()...

And we won't have access to the ->list protected member anymore.

We would if the static method lives in ERFieldItemList ? But ok, possibly overkill.

Final remarks - mostly nitpicks, but one is a stale doc, so back to NW - sorry :-/

1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
   @@ -23,27 +23,33 @@ public function referencedEntities() {
        $list = array_filter($this->list, function($item) {
   -      return (bool) $item->target_id;
   +      return !$item->isEmpty();
        });
   

   We are looping through the list anyway in the next code block, so that could be removed by doing
   - else {
   + elseif ($item->target_id !== NULL) {
   $ids[$delta] = $item->target_id;
   }
   in the foreach.

2. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
   @@ -58,40 +66,29 @@ protected function getEntitiesToView(FieldItemListInterface $items) {
        // Iterate through the fieldable entities again to attach the loaded data.
   

   That comment is kind of gibberish - existing code, but we could clean that while we're in there. What this does is place the referenced entity in originalEntity for getEntitiesToView() - which is actually the intent of the whole prepareView() method.

   Related: the phpdoc for ERFBase::prepareView() method is quite stale now.

   --> Suggestion: remove that inline comment, and replace the phpdoc with:
   "Load all entities referenced by the multiple entities being viewed, and place them in a custom property for getEntitiesToView()".

3. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
   @@ -58,40 +66,29 @@ protected function getEntitiesToView(FieldItemListInterface $items) {
        $target_ids = array();
    
   -    // Collect every possible entity attached to any of the entities.
   +    // Collect every possible entity attached to any of the entities. We can not
   

   "attached" is vague / misleading. Why not "referenced" ?
   Also, striclty speaking, the initilization of $target_id is part of "load referenced entities, for which we cannot use referencedEntities()", so it should be below the comment :-)

OK - attached patch adresses #21.

Since ERFormatterBase::prepareView() is the typical (and possibly single) reason for the existence of FormatterInterface::prepareView(array $multiple_item_lists), I tried to be extra clear on what we do here.

Plus, in ERFormatterBase::prepareView() :
- renamed $target_ids to $ids for consistency with the var name in ERFieldItemList::referencedEntities()
- while we're in here, switched it from entity_load_multiple() to EM::getStorage()->loadMultiple()
- expanded the last "$item->originalEntity = ..." part to make it a bit clearer.

To be honest, I'm not sure why prepareView()/getEntitiesToView() use a custom $item->originalEntity rather than $item->entity, but we can probably try to sort this out in the same issue where we double-check the $item->access stuff.

Basically, the isset() condition is faster than hasNewEntity() because that accesses ->entity internally

I don't think that is true :

public function hasNewEntity() {
  return $this->target_id === NULL && ($entity = $this->entity) && $entity->isNew();
}

So $this->entity is accessed only if target_id is NULL, in which case:
- there is supposed to be already an (autocreate) entity in $this->entity, so no loading
- if not, the target_id is NULL anyway, so the auto-loading in $this->entity will do no actual entity_load()

So $item->hasNewEntity() never loads anything ?

Sending it to test bot.

@a 'fliptable' mateescu (I need to look for the backstory of this :-p) :
Oh, sorry - you're right, that totally makes sense.

Then, same logic also applies to EntityReferenceFieldItemList::referencedEntities(). Switched the order there too.

+ fixed hasty copy/paste around $target_type & EM::getStorage()->loadMultiple(). Self-slap.

Opened #2374019: Cleanup the use of custom item properties in EntityReferenceFormatterBase for $item->access / $item->originalEntity

Exciting! :)

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

diff --git a/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidget.php b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidget.php
index 210fced..e9a0cf8 100644
--- a/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidget.php
+++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidget.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\entity_reference\Plugin\Field\FieldWidget;
 
-use Drupal\Core\Field\FieldItemListInterface;
 use Drupal\Core\Form\FormStateInterface;
 
 /**

Minor fix on commit.