Add a EntityReferenceField::referencedEntities() method

Tagging

I think this is another one that should be postponed on #2047229: Make use of classes for entity field and data definitions as I don't see an easy way to get the 'target_type' setting in a list class..

Too bad though, I wanted to write a patch here and use it in #1605290: Enable entity render caching with cache tag support.

The setting is on the field item class (\Drupal\Core\Entity\Plugin\DataType\EntityReferenceItem) and the list class (Drupal\Core\Entity\Field\Field) has only:

  public function getFieldDefinition() {
    // @todo https://drupal.org/node/1988612
    return NULL;
  }

And yes, I agree that it should be named getReferencedEntities().

I think this means a public getTargetType() method on EntityReferenceItem, and then then EntityReferenceField::getReferencedEntities() can do $this->list[0]->getTargetType() ?

That's what I meant with "I don't see an easy way" above :) Sure, we could do that, or wait until \Drupal\Core\Entity\Field\Field::getFieldDefinition() returns something useful..

Well color me convinced :) This is what I have so far, but of course we can't have nice things like this because of PHP and the way we currently do base fields and configurable fields.

This only works for base fields (which are singe-valued anyway), because the list class for the configurable entity reference field must extend \Drupal\field\Plugin\Type\FieldType\ConfigField, so it can't also extend the new EntityReferenceField class that I'm adding here :( The only solution seems to have two list classes, what a waste..

ARGH!

We should probably add some sort of interface for these methods, so that a) code that uses it can easily check if a field has referenced entities (there could be a different implementation, e.g. a computed back-referenced field) and b) we can check if the item class implements that interface and fail with an exception instead of a fatal error (if something is mis-configured/defined).

getReferencedEntities() seems to break the naming pattern a bit (everything else uses target) but as outlined above, it could be used for different use cases so I think it makes sense.

I used ::targetEntities() in #2073033-8: Optimize file usage updates in file/image fields. See @webchick comment from #2062573-37: Add method defaultConfiguration() in ConfigurablePluginInterface why the pattern "getFooBar" should be used mostly on getters.

EDIT: And, yes, I propose to use the same name here as it's shorter, simpler and contains the "target" word.

Let's see with this...

@yched, on #15.5

Does this work for ConfigurableEntityReferenceFieldItems too ? or does that class need its own implementation of the method ?

As I can see ConfigurableEntityReferenceFieldItem inherits getTargetType() from EntityReferenceItem via ConfigEntityReferenceItemBase. I admit that I'm lost a little bit in "reference" jungle :)

@yched,

Sure, what I mean is I'm not sure the inherited method implementation works for config fields. Those will need $this->getFieldSettting('target_type') (the "definition" received by base fields and config fields is not unified yet)

Does the attached test answer your concerns?

Other than that, if both this issue and the "multiple file load" issue reach RTBC at the same time, we do need to settle on whether the method returns entities keyed by delta or entity id. I'd tend to think by delta, but I'm curious about what the code in the other issue would look like.

No doubt here. We need to preserve deltas. Even this will not be used in Drupal 8 core but for sure will be a useful API tool for contrib modules development.

That error is raised because EntityReferenceField (set as list class of ConfigurableEntityReferenceItem) extends \Drupal\Core\Entity\Field\Field but that one has no valid ::getFieldDefinition() method:

  public function getFieldDefinition() {
    // @todo https://drupal.org/node/1988612
    return NULL;
  }

I see that #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title deals with this. Shouldn't we postpone this till #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title?

Postponing on #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface. And we kinda agreed to name it consistently with what was introduced in #1605290: Enable entity render caching with cache tag support, so updating the title as well.

@amateescu,

And we kinda agreed to name it consistently with what was introduced in #1605290: Enable entity render caching with cache tag support, so updating the title as well.

Don't care really if is targerEntities() or referencedEntities() but it would be great, for consistency reasons, to unify the naming. Want to use the term "reference", "referenced"? OK, then let's unify the naming: target_id will become reference_id, $target_type => $reference_type, ::getTargetType() would be ::getReferencedType() and so on.

What you think, guys? Should I fill a followup?

I'm not sure we want to go that far.. I think target_type and target_id are fine as they are, it's just targetEntities() that doesn't really make sense to me because it implies "possible target entities", while referencedEntities() clearly describes entities that are already referenced.

#2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface is in :)

Lots have changed in the meantime, but we still need two item list classes with duplicate code for that method. Also added an interface and replaced targetEntities() from the file item list class. I think this is the best we can do for now, sans traits or more unification of base and configurable list classes.

P.S. patch didn't apply so I have no interdiff, sorry :/

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileFieldItemList.php
@@ -7,12 +7,12 @@
-class FileFieldItemList extends ConfigFieldItemList {
+class FileFieldItemList extends ConfigurableEntityReferenceFieldItemList {

Except that this will never work because file does not depend on entity_reference.

The only possibility that I see is to have ConfigurableEntityReferenceFieldItemList provided by the field system instead of the ER module. Which doesn't sound so bad after all, looking as how the base EntityReference item is provided by Core already and D8 is all about entities and references :)

The problem there is that FileItemList needs to extend ConfigFieldItemList, because it's a configurable field, so there is code duplication whatever way you look at it.

The best approach would be to merge ConfigFieldItemList with core's FieldItemList, then everyone can go wild, but I'm not sure if that's possible at the moment :)

Definitely. I picked this up a couple of days ago but it's still ugly no matter which direction you take on duplicating code. Let's wait a bit for #2047229: Make use of classes for entity field and data definitions to land and another followup of it which I can't easily find right now.

Here is a re-roll from #27. Not sure about the new namespaces, but I think at least it works now.

I had to include a new setting about target_type in FileItem and ImageItem classes to keep working ConfigurableEntityReferenceFieldItemList::referencedEntities().

If works, then we can start to polish it :)

Regards.

New round, test failed because old targetEntities() returned results using entity id as key, however referencedEntities() returns results using delta as index. New patch processes it to match with the entity id.

Regards.

Adding reference to targte_type setting in field.schema.yml and image.schema.yml files.

About the existence of the fixed target_type setting or not in file and image field types, IMHO a fixed setting could be converted into a property. However, if a field type have to inherit all the settings from its parent, we should proceed on this way.

Regards.

I think we should be able to ship a single EntityReferenceFieldItemList class extending ConfigFieldItemList, and use it for both core EntityReferenceItem & entity_ref ConfigurableEntityReferenceItem (would just require a small adjustment to getDefaultValue() so that it doesn't assume it *will* find 'target_uuid' entries).
Probably best left for another patch though.

I would go even a bit further and say that I'd prefer this patch to happen only after we get rid of ConfigFieldItemList completely..

What we're doing here is needlessly ugly and almost all the code from the current patch will need to be updated for that, so I'm not sure why we need to rush it for this single method.

41: 2073661-41.patch queued for re-testing.

Since the changes mentioned in #43 happened in the meatime (no more ConfigFieldItemList), this patch actually makes sense now :)

Should be better now.

1. Sure thing, fixed.
2. That setting for file/image/term ref fields was actually introduced quite some time ago and this patch just kept the schema changes for it. I think we should open a different issue to discuss this point because it feels a bit out of scope here. I removed the schema changes from the patch so we can focus only on what we want to do in this issue :)

(FWIW, I agree with you that it feels wrong to have 'target_type' as a setting on those field types, that why I hardcoded it initially in their propertyDefinitions() method, but, if my memory serves well, @fago or @Berdir didn't like that very much)

Yup, that cleans things up a bit :)

Now with less #fail.

Reroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

57: 2073661-psr4-reroll.patch queued for re-testing.

Committed 3ffcebc and pushed to 8.x. Thanks!