ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type"

This might be a duplicate of #3029090: Dynamic Entity Reference: support discovering relatable resource types.

@hchonov Thanks for all this work!

It's exactly what I proposed in #3029090: Dynamic Entity Reference: support discovering relatable resource types (Jan 30, 2019) and #3005274-17: [PP-1] Support for Paragraphs' ability to negate the entity reference selection (May 21, 2019)! (Which @jibran already linked, thanks Jibran!)

I'm fine with doing all that in this issue instead, because those two issues started from the DER and Paragraphs POV respectively, this one is more generic. Then we'd postpone those two issues on this one, and rename this one to Entity Reference fields don't have official APIs for getting their target entity types and target bundles, preventing JSON:API from supporting Paragraphs and Dynamic Entity Reference. Thoughts?

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -709,4 +709,25 @@ public static function getPreconfiguredOptions() {
+   * @return array
+   *   The referenceable entity type bundles, keyed by entity type ID.
+   */
+  public static function getReferenceableEntityTypes(FieldDefinitionInterface $field_definition) {
...
+    return [$target_type_id => $target_bundles];

This solves the challenge of both target types and target bundles in a single new API. This is much better than what I proposed in #3005274-17: [PP-1] Support for Paragraphs' ability to negate the entity reference selection. I'm +1 😀

Assigning to @Berdir for review.

I am not convinced we absolutely need an interface for this. In fact, I think we shouldn't do that. Because we've gotten this far into the Drupal 8 cycle without an interface for \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem, and we see lots of contrib modules doing MySpecialReferenceItem extends EntityReferenceItem. The only way to provide custom entity reference-like field types and have them work with the rest of the ecosystem is subclassing, so why should we start deviating from that now?

Am I missing something?

Also: would really like to hear @jibran's and @Berdir's thoughts on this — I think a +1 from both of them is required for this to reach RTBC.

#11 works for me — thanks for explaining your thinking in so much detail, it's very helpful 🙂

+1 on having a method and also having a new, optional interface for it.

\Drupal\jsonapi\ResourceType\ResourceTypeRepository::calculateRelatableResourceTypes() (which calls this) is some pretty wild code (or how the cool kids these days call it, functional) with a combined array map and filter. It filters first by field types having a main property that implements DataReferenceTargetDefinition. DynamicEntityReferenceItem doesn't have a main property, so I think this patch alone won't actually make it work with that. IMHO, we can drop that check as a separate thing and primarily go against this interface, with a fallback for the main property. We could even consider to deprecate that fallback and trigger a @deprecated() if a field type doesn't implement it?

Not quite sure about the method name yet, it says EntityTypes but actually returns bundles keyed by entity type ID. So maybe getReferenceableBundles() as a name would actually make more sense? Since regular most fields will only return one entity type anyway.

And last, the thing is that we are still hardcoding the handler settings. But those are the settings of the selection plugin and as pointed out by Wim, this won't yet support alternative selection plugins. But we could go "one level deeper" in the referenced issue and move that logic into selection plugins.

Also noticed that \Drupal\jsonapi\ResourceType\ResourceTypeRepository::isReferenceFieldDefinition() doesn't check if getMainProperty() returns anything and calls getPropertyDefinition(NULL), that only works because the implementations just return nothing, but this would fail if we'd enforce the argument with a string type hint for example.

+1 on having a method and also having a new, optional interface for it.

I'm +1 on this as well.

DynamicEntityReferenceItem doesn't have a main property

DER has a main property https://git.drupalcode.org/project/dynamic_entity_reference/blob/8.x-2.x... I discussed setting one up, at length, with you at DrupalCon Dublin ;-)

maybe getReferenceableBundles() as a name would actually make more sense?

+1

we could go "one level deeper" in the referenced issue and move that logic into selection plugins.

Yeah, this makes sense.

> DER has a main property https://git.drupalcode.org/project/dynamic_entity_reference/blob/8.x-2.x... I discussed setting one up, at length, with you at DrupalCon Dublin ;-)

Riiight... the module in my D8 dev installation was 4 years old :p I think my suggestion makes sense anyway though.

So as I understand, there's still work to be done :)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -39,7 +39,7 @@
   -class EntityReferenceItem extends FieldItemBase implements OptionsProviderInterface, PreconfiguredFieldUiOptionsInterface {
   +class EntityReferenceItem extends FieldItemBase implements EntityReferenceItemInterface, OptionsProviderInterface, PreconfiguredFieldUiOptionsInterface {
   

   I understand why this needs to be on the FieldItemBase but it makes it impossible to use EntityReferenceItemInterface as a means to detect fields that are entity reference. Would it be too far out of scope to add an hasEntityReferenceProperty method along the same lines as EntityInterface::isRevisionable()?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -708,4 +708,18 @@ public static function getPreconfiguredOptions() {
   +    $target_bundles = $has_target_bundles ? $handler_settings['target_bundles']
   +      : array_keys(\Drupal::service('entity_type.bundle.info')->getBundleInfo($target_type_id));
   

   Nit: Can ? $handler_settings['target_bundles'] be on its own line?

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItemInterface.php
   @@ -0,0 +1,24 @@
   +   * @param \Drupal\Core\Field\FieldDefinitionInterface $field_definition
   +   *   The field definition for which to retrieve the referenceable entity
   +   *   types.
   

   Nit: "The referenceable entity types and bundles."

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItemInterface.php
   @@ -0,0 +1,24 @@
   +   * @return array
   +   *   The referenceable entity type bundles, keyed by entity type ID.
   

   I think this needs some expansion. Maybe: "An array keyed by entity type IDs where the values are an indexed array of bundle IDs or ??? if the entity type does not have bundles"

5. +++ b/core/modules/jsonapi/src/IncludeResolver.php
   @@ -136,10 +136,19 @@ protected function resolveIncludeTree(array $include_tree, Data $data, Data $inc
   +            // target type stored in the settings.
   

   target type stored in the settings.

@hchonov, 🤦‍♂️🤦‍♂️🤦‍♂️ my mistake! I mistakenly read:

class FieldItemBase implements

instead of:

class EntityReferenceItem extends FieldItemBase implements

Which made me think that the interface would be on all field items, making it impossible to use for detection. I was trying to work quickly and I guess I just worked sloppily :\ Now that I read it correctly, I'm very excited to have this interface and to be able to use it for detection!

I am fine with this, but would like to know the reasoning behind it given the short size of the statement. :)

I think @e0ipso taught me this code pattern a couple years ago. He told me it was a JS best practice and I've since grown to appreciate it because the style makes it easy to spot a ternary. A indented line starting with ? just stands out. If you have ag (the silver searcher) installed, try running ag -A1 -B1 '^\s*\?' and you'll find we use that style pretty much exclusively within the jsonapi module unless the entire ternary is all on a single line.

This patch still doesn't cover everything.
I noticed today that when trying to filter by a field of an dynamic entity reference it still has an error.

I tried to fix it, but couldn't get it to work, but still uploading my current progress.

What works currently is the following json api filter (target is a DER field, thats why entity:node is required):

filter[target.entity:node.title]=hello

What currently doesn't work is:

filter[target.entity:node.type.meta.id]=article

I'm unsure how the filter should look like, but I can't get the bundle filter working.

Using taget.entity:node.type I get "detail: "Invalid nested filtering. The field `type`, given in the path `target.entity:node.type` is incomplete, it must end with one of the following specifiers: `id`, `meta.value`.""

Using target.entity:node.type.id results in "Ambiguous path. Try one of the following: target.entity:node.type.entity:node_type.id, target.entity:node.type.entity:block_content_type.id ,... in place of the given path: target.entity:node.type.id" - thats the point where I see my fixes are working as I get all possible referenced entity types

Using target.entity:node.type.entity:node_type.id results in "Filtering on config entities is not supported by Drupal's entity API. You tried to filter on a Content type config entity."

Using meta.id at the end results in a 500 with "Invalid specifier 'value'

So somehow I didn't catch the case yet. I think somehow the $candidate_definitions needs to filtered in the loop again when something like "entity:node" is specified.

Some strange side note where I'm unsure if bug or intended design of entity queries and DER.

$entity_manager = \Drupal::entityTypeManager();
$query = $entity_manager->getStorage('node')->getQuery();
$query->condition('target.entity:node.title', 'test');
$query->execute();

fails with an error to get it working I need to add the target_id property of the DER field to get it working.

$entity_manager = \Drupal::entityTypeManager();
$query = $entity_manager->getStorage('node')->getQuery();
$query->condition('target.target_id.entity:node.title', 'test');
$query->execute();

Thats why I needed to introduce $data_reference_target_property_name in the patch.

RE: #23

Some strange side note where I'm unsure if bug or intended design of entity queries and DER.

As per https://www.drupal.org/node/2835034 and https://www.drupal.org/node/2825947, this shouldn't be the case. DER and core have some tests to check this functionality see https://git.drupalcode.org/project/dynamic_entity_reference/blob/8.x-1.x... and https://git.drupalcode.org/project/drupal/blob/8.2.x/core/tests/Drupal/K....

If your DER field is a basefield then #2915512: Entity query doesn't allow relationship specifier for base entity fields might be a culprit here.

Reroll of the patch against 8.9.x. The whole FieldResolver::collectResourceTypesForReference() was removed in #3014277: ResourceTypes should know about their fields so I didn't keep this part of the patch. If it still makes sense, it should be implemented differently. Also I see no use of use Drupal\dynamic_entity_reference\Plugin\Field\FieldType\DynamicEntityReferenceItem; in FieldResolver.
Btw, doesn't the above-mentioned issue resolve this one?

Comments 2-5 from #20 addressed.

OK seems like we have a consensus here from at least two entity API maintainers (@hchonov and @Berdir), two JSON:API maintainers (@Wim Leers and @gabesullice) and DER maintainer myself.

Let's add some tests to cover issues like #23 and also keep the #3057556: Implement DERItem::getReferenceableBundles green as well in DER issue queue as PoC.

+++ b/core/modules/jsonapi/src/Context/FieldResolver.php
@@ -18,6 +18,7 @@
+use Drupal\dynamic_entity_reference\Plugin\Field\FieldType\DynamicEntityReferenceItem;

This needs to go - can't reference contrib class from core

Other than that, this approach is looking good

Reroll after #2996114: Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given

Missed some bits in the reroll

This looks good to me!

I only fixed a small grammatical nit in a comment. See interdiff.

There are a few things missing here:

• We need tests with red/green patches.
• We are adding a new interface here so a change record.
• IS needs an update esp. API change section. Might need the release note as well.
• We might be able to use new method in \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget::getAutocreateBundle()? Can be a follow up.

Re-rolling patch for 9.4.x.
The patch still applied but there were some offsets, here is my cli output.

patch -p1 < 3057545-34.patch
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
Hunk #1 succeeded at 40 (offset 1 line).
Hunk #2 succeeded at 726 (offset 18 lines).
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItemInterface.php
patching file core/modules/jsonapi/src/Context/FieldResolver.php
Hunk #1 succeeded at 346 (offset 16 lines).
Hunk #2 succeeded at 357 with fuzz 2 (offset 15 lines).
Hunk #3 succeeded at 455 (offset 16 lines).
Hunk #4 succeeded at 637 (offset 18 lines).
patching file core/modules/jsonapi/src/IncludeResolver.php
Hunk #2 succeeded at 138 (offset 2 lines).
patching file core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
Hunk #1 succeeded at 15 with fuzz 2.
Hunk #2 succeeded at 440 (offset 14 lines).

I've been using the patch from #34 in production for quite a while, and have encountered an error.
If the referenced entity has been deleted, you get a 500.

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getEntityTypeId() on null in Drupal\jsonapi\IncludeResolver->resolveIncludeTree() (line 152 of core/modules/jsonapi/src/IncludeResolver.php).

Attaching a new patch that checks that the entity is valid. This fixes the error for me.

Trying to fix test failures.

one more try to resolve test failure & removing my old patch.

Awesome work. We do need to address the comments from #35 by @jibran :)

Also;

1. I see a lot of test changes, that kinda feels like a BC break?

Related jsonapi_extras issue needs committed to be compatible with this change - it had previously overridden some methods for D8 compatibility, including the method that determines the eligible entity/bundle targets.

Converted to an MR, working to address #35:

We need tests with red/green patches.

As in, we need a test that basically provides a DER-type field in a test module?

We are adding a new interface here so a change record.

Agree, will write/link one.

IS needs an update esp. API change section. Might need the release note as well.

Agree, IS updated.

We might be able to use new method in \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget::getAutocreateBundle()? Can be a follow up.

So as to not hold this up I think that should be a follow-up, in line with the deprecation notices I mentioned in the MR review.

Per @bbrala's feedback:

I see a lot of test changes, that kinda feels like a BC break?

It wasn't immediately obvious to me why the test changes were made, but it traces back to #23. Which begs the question of, what patch is actually "under review." Hence the MR and a little bit of issue archaeology/cleanup here. The issue in #23 appears to be addressed in #24 and is likely due to (and fixed in?) #2915512: Entity query doesn't allow relationship specifier for base entity fields. So I have reverted these changes in the MR.

CR: https://www.drupal.org/node/3279140

Looks good, just some fixes needed to docs.

I'm going to mark this as NR as I'm curious as to whether we absolutely need more test coverage; this is a refactor of existing functionality and as such this does not break the existing ER field tests. Covering contrib, DER-type fields might be a little out of scope, now that I think of it? It would be "nice," but it seems like might not be in scope for core to test different use cases in the wild. Thoughts?

ContentEntityBase::referencedEntities() will need updating, as it does:

> $property instanceof EntityReference

And generally, we should look through the whole codebase for anything which references either the EntityReference or EntityReferenceItem classes, or the entity_reference field type, and update them.

That could all be done in follow-ups though.

There is precedence to test certain things contrib does. For example overriding resourcerepository I think even though the class is internal.

I don't think that is needed in the current use case. Although I would probably want to verify that with a quick contrib search for the changed method. Can't right not, I'm on mobile.

There is some of the overwritten methods in contrib: http://grep.xnddx.ru/search?text=getRelatableResourceTypesFromFieldDefinition&filename=

jsonapi_extras was already mentioned.
jsonapi_cross_bundles, commerce_api and wotapi overwrites the method.

Conclusion, i don't think we need to test. The impact on contrib should be minimal. My conclusion is we don't need to do extra testing for the custom code in contrib. Sorry for the noise :)

Note; did ping maintainers of the modules on slack, and i'm maintainer for jsonapi_extras so he is aware anyhow ;)

Per #54:

And generally, we should look through the whole codebase for anything which references either the EntityReference or EntityReferenceItem classes, or the entity_reference field type, and update them.

I can ticket a follow up, since this one is a little more nuanced.

Edit: This is basically #3262385: Add an API for general entity reference field types

EntityReference in the method you mention is referring to the typed data DataType, and DER for instance actually extends this, so it works. Also we are not touching the DataType here, so that should be OK to keep as it is. But yes, we'd need a broader look around the codebase for typehints on the field type name or EntityReferenceItem itself. This one is a little tough too since the list class doesn't have anything to typehint on, just the item class, and I know at least in my code, I mix up the type checks sometimes assuming the list class is more or less tied 1:1 to the item type/class, which isn't necessarily the case (as we have here.)

But I think all that can be handled in a follow-up as this isn't a breaking change.

I'm going to say this is RTBC.

The things I mentioned in #54 can be done in a follow-up.

Woohoo! Opened follow-up #3279986: Use EntityReferenceItemInterface::getReferenceableBundles() wherever core needs to discover reference-able entities for a field.

This overall looks good, but we can't deprecate for removal in 10.x any more.

Marking back to RTBC as this was a very small, specifically-requested change.

We need to add a test of the deprecation and that the old behaviour still works as expected.

Ok, guess i was wrong then. Will keep an eye out for this issue.

It feels like there's still a fair bit of work to do here.

Would it be worth splitting off the changes in Core/Field to a separate issue, so that contrib can benefit from this new API sooner?

There are a TON of contrib modules that really need this so they can work with generic reference fields. It's classified as a normal bug report on jsonapi module, but in core I feel this would be a major feature request.

It feels like there's still a fair bit of work to do here.

Really? Alex's suggestion for two test cases is pretty reasonable, and I'm not sure what other work is required?

+              // Support entity reference fields, which don't have the referenced
+              // target type stored in settings.

What is this comment about?

Currently, with the comma, it means that *all* entity reference fields don't have the thing stored in settings. But that's surely not true, since if that were the case, this comment wouldn't be inside an if() block, since *everything* here is an entity reference fields.

Does it mean "support some entity reference fields, specifically the ones which don't have the thing stored in settings"?

Yeah the comma seems extra.

Not just extra -- it changes the meaning to something which I suspect is incorrect.

I'm changing it to 'that', which most style guides recommend for a restrictive clause.

> Alex's suggestion for two test cases is pretty reasonable, and I'm not sure what other work is required?

Agreed, but those are not simple tests to write!

I assume we need a test module which defines a custom field type which is a reference in that it uses DataReferenceTargetDefinition for one of its field properties, but doesn't inherit from EntityReferenceItem.

That itself is tinkering about in the innards of the field system and the typed data system.

Then we need a kernel test which makes jsonapi requests with includes, and that requires an understanding of the patterns of the kernel tests in the module. I've had a brief look and there's not much documentation, so I'm really not clear on how to set up the test, or where to put it to keep things organised and not make a mess.

These tests require very specialised knowledge, and they're holding up something considerably simpler which has wide-ranging benefits.

If someone can create the field in this structure, I'd happy to prove the jsonapi part. Perhaps you eveb have code for that in your coming site Brian?

Sure, I'll make the field type -- already got the skeleton of it in place.

Awesome thanks

Pushed to a new branch `3057545-sandbox-test-module-for-deprecation` on this issues fork.

I hope I've implemented all the methods it needs -- I wrote a quick kernel test which installs the node schema and saves a node with a value in the field, and that all passes.

I still think the reference field stuff should be done in a separate issue.

The API we're adding here is incomplete -- for instance modules that work with reference field data such as https://www.drupal.org/project/entity_reference_purger or https://www.drupal.org/project/field_referenced_delete (when I've updated it to D9!) don't just need a getReferenceableBundles() method, they need to know "how do I query for the targets of fields of this type".

We're designing a new API here, and while I absolutely agree that APIs should be designed outside-in, that is, thinking of the consumers of the API and what they need, in this issue we're only looking at one consumer of this API and one with fairly limited needs.

Thanks to everyone pushing this issue along, we had been using patch #33 in production for a long time but started hitting the exception in FieldResolver when trying to filter a node's jsonapi response by a paragraph field. With the most recent MR updates everything seems to be working nicely :)

FWIW, for Googleability the now resolved exception was

Conflicting data reference definitions. The property `target_revision_id` conflicts with `target_id` property

I've rebased https://git.drupalcode.org/project/drupal/-/merge_requests/2231/diffs onto a new branch off 10.1.x, thankfully it applied with just a little fuzz :) I'll look into implementing some form of deprecation test now.

Can we please get MR 2231 closed?

I've added test coverage for ResourceTypeRepository::getRelatableResourceTypesFromFieldDefinition, thank you very much @joachim for your sandbox module. It was pretty much exactly what I needed, all I added was the mainPropertyName function to tie everything together (it took me WAY too long to figure that out).

Now all we need is a deprecation test for the include resolver.

That coverage came together a little easier than expected. This should be good to go now.

Great work! We do need a follow up issue eventually to make sure the deprecation test module is removed from 11. Although it might just be part of removing this deprecated code in 11.

We also need a change record if we want to move forward.

If possible i'd also like a review from the contrib maintainers that ran into this issue since @bradjones1 and @joachim do seem to understand this issue very well.

Thanks :) there's already a draft CR attached

See MR comment.

Added removal link and base class.

Left a comment for the deprecation message.

Feedback addressed

Changes look good and feedback addressed.

Left some comments on the MR, looks very close, keen to get this into 10.2.0 early

I've addresses the issues mentioned by Larowlan. He gave a go to RTBC. Also updated CR to point at 10.2.

Did a local rebase on 11.x without a problem, but cannot change the target of the MR to 11.x so have not pushed.

Tests failed.

Fixed the test. Checked for the wrong datadefinition. Tests are green locally.

Green, so yay.

Also openened a MR to 11.x, it could rebase without any changes, so i guess the original MR will also merge into 11.x without a problem.

Committed da5ea0a and pushed to 11.x. Thanks!

Been a long time coming, great work all 🙌

Yay! Well done eveyone!

YAY! Epic work here!

Unpostponed:

1. #3279141: Use EntityReferenceItemInterface::getReferenceableBundles() in EntityReferenceAutocompleteWidget::getAutocreateBundle()
2. #3279986: Use EntityReferenceItemInterface::getReferenceableBundles() wherever core needs to discover reference-able entities for a field.
3. #3027430: Dynamic Entity Reference normalization support
4. #3277553: JSON:API module should support POSTing dynamic entity references with only `type`, `id`/entity reference field should validate type when setting with entity property