Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.EntityInterface defines a referencedEntities() method :
/**
* Returns a list of entities referenced by this entity.
*
* @return \Drupal\Core\Entity\EntityInterface[]
* An array of entities.
*/
public function referencedEntities();- The exact semantics are a bit fuzzy ("referenced by" - in what way ?)
- It was introduced for cache tags two years ago in #1605290: Enable entity render caching with cache tag support, but it is currently not called anywhere in Core anymore.
- The basic implementation in Entity just returns [], and no config entity ever overrides that
- The basic implementation for ContentEntities loops through all properties of all deltas of all fields to look for values that are DataType\EntityReference :
foreach ($field_items as $field_item) {
// Loop over all properties of a field item.
foreach ($field_item->getProperties(TRUE) as $property) {
if ($property instanceof EntityReference && $entity = $property->getValue()) {
$referenced_entities[] = $entity;
}
}
}That is widely inefficient (single-loads all entities).
- Pretty much all reference-like field types in core are now subclasses of EntityReference fields, which now have a more efficient method :
foreach ($this->getFields() as $field_items) {
// If the field type extends entity_reference, take advantage of the
// multiple-load capabilities
if ($field_items instanceof EntityReferenceFieldItemListInterface) {
$referenced_entities = array_merge($referenced_entities, $field_items->referencedEntities());
}
}The difference being that it wouldn't catch "properties that happen to hold a entity in a random field type that is not EntityReference". If we still want those (not sure, since the semantics of the method is not too clear :-)) we could add an "else { current HEAD code}" after that.
But do we really still need that Entity::referencedEntities() method to begin with ?
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 2471801_ContentEntityBase_referencedEntities-1.patch | 1.89 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedPatch tries to improve the performance of the existing code.
Still not sure we really need that method to begin with :-)
Comment #2
amateescu CreditAttribution: amateescu for Drupal Association commentedI think we should just remove the method, it's slow without this optimization, unused, probably untested and it doesn't serve its initial purpose anymore. We need to ask a core committer about this :)
Comment #3
yched CreditAttribution: yched commentedFWIW, the original implementation of referencedEntities() in #1605290: Enable entity render caching with cache tag support only took EntityReference field type into account.
Then it was rewritten as part of #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface (comment #108 over there (/cc @Berdir :-)), to account for "any property that contains an entity, whatever the field type".
Comment #5
larowlanFwiw default content module uses this in export and entity pilot does too.
Comment #6
effulgentsia CreditAttribution: effulgentsia at Acquia commentedGiven #5, I think we should keep EntityInterface::referencedEntities(), but make it work correctly, efficiently, and add test coverage.
Now that config has a 'dependencies' property with 'content' and 'config' subkeys, we should be able to implement ConfigEntityBase::referencedEntities() as returning those, right?
+1 to the increase in efficiency here. But is it still inefficient to call getFields() if most fields aren't entity reference fields? Should we loop on getFieldDefinitions() instead and only instantiate the fields that are references?
I'm not clear on the use case of a non-ER field holding an ER property, but if we can clarify what a legitimate use case of that is, then yes, it makes sense to have this fallback, and +1 to implementing the @todo.
Comment #7
yched CreditAttribution: yched commentedI'm fine with keeping it. But given we have no actual use case in core, it's a bit hard to define the exact behavior we want here :-)
repeating @effulgentsia's question in #6 :
- For ConfigEntities,
Do we want "a config dependency counts as a reference" ? Maybe, dunno - ConfigEntities don't really have a notion of "reference" :-)
- For ContentEntities,
Right, no clue either :-)
@larowlan, what do you think ?
Comment #8
larowlanThe current behavior is adequate. We should just document that it only considers ER items.
Comment #9
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAs an example, let's consider core.entity_form_display.comment.comment.default.yml, which has the following lines that are relevant for this issue:
IMO, the config entities listed as the dependencies are equivalent to references. This config references
comment.type.commentvia the targetEntityType and bundle properties, and it referencesfield.field.comment.comment.comment_bodyvia the targetEntityType, bundle, and content.comment_body properties. For config, I see dependencies and references as equivalent, because:Another interesting example though is block.block.bartik_account_menu.yml, the relevant lines of which are:
Here, we depend on
system.menu.accountthough don't hold a direct reference to it. Instead we reference the system menu block plugin, and that plugin references the menu. But I think it's still desirable for it to be returned byreferencedEntities(), because logically, the block entity is referencing the menu entity: that it's doing it via a plugin as an intermediary is an implementation detail that I don't think the caller of$block_entity->referencedEntities()would care about.Comment #12
tstoecklerJust stumbled upon this and wanted to point everyone to #2824097: Deep serialization for content entities to make an exact snapshot of an entity object's structure based on its current state as that will in some form need to collect the referenced entities and might make use of that method.
Comment #19
geek-merlinThe referenced issue might change the assumptions of the IS.