Dangling entity references in entity reference field with multiple possible target bundles: results in exception/500 response

Thanks for reporting this!

I wonder why $target_entity === NULL ever happens for this though. Can you also post steps to reproduce for creating content entities that trigger this problem?

I ran into the same problem when using the recipe from umami.

This content model has 4 references to taxonomy terms:

• Ingredients
• Recipe categories (starter, main ...)
• Cusine (italien, ...)
• Tags

This error message is really confusing. I would suggest to maybe create a type exception and throw a message which actually helps in the context of Drupal.
One possible better message could be: "JSONAPI doesn't support having multiple entity reference to the same entity type in the same bundle", or well anything which is actually helpful.

Thanks for reporting that @dawehner, that makes it easier to reproduce :)

[…] or well anything which is actually helpful.

This was added in #2940339: Port reference field support for non-empty entity reference fields not pointing to an entity from #2543726 to check a pretty fundamental assumption in the logic that was being added there to support the <root> Taxonomy term parent. It was thought to be an impossible case. Clearly, as #3 and #4 show, that is not at all impossible! I'm glad we added that exception, because it made this bug report already much more precise than "fatal error in some weird place" :)

This error message is thrown in case an entity reference items' ->entity property is NULL. How is it possible to have an entity reference that doesn't point to any entity? Understanding that is key to making sure nobody has to ever see this exception again.

If somebody has time to debug this (I don't have time right now, and will be going on vacation next week):

1. \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer::normalize() does $entity = $item->get('entity')->getValue();, and passes this to
2. \Drupal\jsonapi\Normalizer\Relationship::__construct() which passes it to
3. \Drupal\jsonapi\Normalizer\RelationshipItem::__construct(), which throws the exception.

I fear weakly typed languages will always have an infinite amount of edge cases :(

@dawehner I … agree. Not sure why exactly you're making that statement here though, I'm guessing because you are a bit sad by seeing this bug being reported here? :)

Now that 2.0-beta1 is released, I really would like to see this bug fixed at last :)

I tried to reproduce this with just Drupal core:

… but it normalizes just fine:

@dawehner reported to be able to reproduce this with just Drupal core. But @haihoi2 reported to be using https://www.drupal.org/project/video_embed_field. So I'll install that and try again.

I installed video_embed_media (which requires video_embed_field). I created an "embedded media" media type, just like in the screenshots in #3. And I changed the settings of my media reference field to allow "embedded media" to be referenced.

Results:

1. Without actually pointing to a "embedded media" media item, the output remained exactly the same, and still no errors.
2. I then linked to an "embedded media" media item, the output … was updated, but works just fine:
   

That's what I was referring to when I wrote:

This error message is thrown in case an entity reference items' ->entity property is NULL. How is it possible to have an entity reference that doesn't point to any entity? Understanding that is key to making sure nobody has to ever see this exception again.

The code that is triggering the error only is executed for references to entities that cannot be loaded, because either they never existed (which is the case for Taxonomy Terms' virtual "root" term), or because they no longer exist.

@dawehner and @haihoi2, are you in your cases dealing with an entity reference field pointing to non-existing data?

If I delete a media item (any of them, not video_embed_media-related necessarily), then I can indeed reproduce this, because Drupal core does not clean up its dangling entity references … you need something like https://www.drupal.org/project/entity_reference_integrity to fix that, weirdly.

This means the logic introduced by #2940339: Port reference field support for non-empty entity reference fields not pointing to an entity from #2543726 in \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer::normalize() is too naïve.

This fixes it.

I thought we already considered this and decided that it was best to not omit those values because it misrepresents the field data and could lead a JS client to accidentally delete field data via a PATCH based on our response.

Better title & IS.

This is technically a BC break, because in the 1.x any entity reference field which has a single target bundle will have its dangling references still show up in the JSON API normalization. With this patch in, they won't anymore, for the sake of consistency: see the comment that #12 adds.

#13: I think you're referring to #2968972: Cannot PATCH an entity with dangling references in an ER field? That's not what that patch did.

You wrote in #2968972-5: Cannot PATCH an entity with dangling references in an ER field:

I'd like to shy away from "correcting" the data. Instead, I think we can simply filter the constraint violations by the submitted fields, so only PATCHed fields return a 422 and associated errors.

This patch does amount to "correcting" the data.

Failing test-only patch is also the interdiff.

Something about this just feels wrong. I can see why it's needed, but I don't like it.

I would feel better about this if there were a core issue to fix this "correctly".

Did you consider a "missing" resource identifier? ^{Sorry, I know that explodes the size of the patch /me runs away.}

Did you consider a "missing" resource identifier?

I'd be fine with this, actually.

Sorry, I know that explodes the size of the patch /me runs away.

It's not that bad at all :)

+++ b/src/JsonApiResource/EntityCollection.php
@@ -55,7 +55,8 @@ class EntityCollection implements \IteratorAggregate, \Countable {
-      return $entity === NULL
+        return $entity === NULL

Nit: D'oh, stupid reformatting.

+++ b/tests/src/Functional/JsonApiRegressionTest.php
@@ -310,4 +310,131 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
+        'type' => 'node--?',

This is my only nit. I wish this could be 'type' => 'unknown'.

It'd be great if both the typeand the id were static and then could be more easily hardcoded into a client.

I guess that would mean making $type_name an argument to ResourceType.

Wow, only one nit!?

I solved it in a different way than you suggested. Somewhat too clever, but requiring far fewer infra changes. And since it's @internal anyway, we can change it later if we want.

Wow, only one nit?!

😞you're right, I lied. I'm sorry!

+++ b/src/Normalizer/RelationshipItem.php
@@ -74,6 +76,27 @@ class RelationshipItem {
+      $field_definition = $parent->getHostEntity()
+        ->get($parent->getPropertyName())
+        ->getFieldDefinition();
+      $target_entity_type_id = $field_definition->getFieldStorageDefinition()
+        ->getSetting('target_type');
+      $target_bundles = $field_definition
+        ->getSetting('handler_settings')['target_bundles'];
+      $target_bundle = count($target_bundles) === 1
+        ? reset($target_bundles)
+        : '?';

This logic already exists in the resource type repository. Something like this will avoid it:

$target_resource_types = $host_resource_type->getRelatableResourceTypesByField($relationship_field_name);
$target_resource_type = count($target_resource_types) > 1
  ? new ResourceType(...)
  : reset($target_resource_types);

✔️

👏

Needs https://www.drupal.org/docs/8/modules/json-api/core-concepts#missing to exist :P

Needs work per #26.

CR published: https://www.drupal.org/node/2996568

Still needs documentation.

Docs added: https://www.drupal.org/node/2803133/revisions/view/10989525/11107488

Moving to Drupal core's issue queue.

I'm working on https://www.drupal.org/project/drupal/issues/3122113