EntityReferenceFieldItemList::referencedEntities might not return up-to-date entity objects

This applies to DER as well.

This issue was the random bugsmash bug to triage a few days ago, adding tag.

It looks like this is waiting on a decision. I'll see if I can ping @hchonov in Slack.

What can be done to make a decision here?

@hchonov is the one who purposed the solutions so we need @berdir, @amateescu, or @catch's opinion on the issue.

EntityReferenceFieldItemList::referencedEntities should just call $this->getValue($include_computed=TRUE) and then from the result leave only the entity objects.

Drupal\Core\TypedData\Plugin\DataType\ItemList::getValue() doesn't have an $include_computed parameter, so that option is off the table.

I'll create a MR implementing the first option.

There are 2 options it appears in the issue summary proposed solution. Can it be highlighted which was chosen?

Also could use a test case showing the issue.

It seems like the current implementation of the MR causes a performance problem. $item->entity !== NULL triggers DataReferenceBase::getValue(), which triggers EntityReference::getTarget(), which triggers EntityStorageInterface::load(). This means that every entity is now loaded separately. I'll look for a different solution.

I found a fix. In a custom API endpoint with a bunch of referencedEntities() calls this makes a huge performance difference: -5.62 s (-75%) and -2 219 database queries. Hopefully anyone using a patch from this issue sees this.

Yes, that's the problem with accessing the property, it auto-initializes, so that condition is always true if it can be loaded.

I don't think your change is correct either. getValue() will never return computed values, so the result is the same as HEAD.

I don't think it is possible at all to do this, because even accessing the EntityReference property directly with get('entity') there simply is no API to detect whether it has been set or not. There is no public method to access $this->target and \Drupal\Core\Entity\Plugin\DataType\EntityReference::getTarget() always loads it if not set, that's the same as ->entity.

That's why \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase::prepareView + \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase::getEntitiesToView() use the magic _loaded property to track that.

So to change this, we'd first need to extend the underlying entity reference property to add a method to check if the refernce is loaded or not.

I don't think your change is correct either. getValue() will never return computed values, so the result is the same as HEAD.

No, that does seem to work. Because EntityReferenceItem::getValue() looks like this:

/**
 * {@inheritdoc}
 */
public function getValue() {
  $values = parent::getValue();
  // If there is an unsaved entity, return it as part of the field item values
  // to ensure idempotency of getValue() / setValue().
  if ($this->hasNewEntity()) {
    $values['entity'] = $this->entity;
  }
  return $values;
}

$this->entity also triggers EntityReference::getTarget(), but since isset($this->target) it doesn't trigger a new entity load and returns the existing entity object instead.

I think that means that the previous approach would have also worked, if we had just replaced $item->entity !== NULL with $item->hasNewEntity().

By the way, I just tested this with code like this:

$this->set('field_event_type', [EventType::create()]);
$types = $this->get('field_event_type')->referencedEntities();

$types contained the newly created, unsaved entity.

It works for *new* entities, but it doesn't work for existing, changed entities, which is what this issue is trying to fix. \Drupal\Core\Field\EntityReferenceFieldItemList::referencedEntities() in HEAD already accounts for this, because it falls back to that if target_id is null. That's consistent with hasNewEntity(), which also explicitly checks for target_id === NULL, so it doesn't matter whether that's checked first or not.

Your code will also work in HEAD. What is the bug that you are trying to fix?

We can not solve this and keep multiloading without exposing something like isTargetLoaded() on the reference property.