Problem/Motivation

Steps to reproduce:

Add an entity reference field for nodes (Content) on a Custom Block type
Add an entity reference field for Custom Blocks on a node type
Enable Layout Builder
Go to the "Manage Layout" UI for the node's content type

Expected:

The Layout Builder UI

Actual:

Fatal error: Maximum function nesting level of '256' reached, aborting!

This is reproducible in more complex ways without Layout Builder, but it's usage of sample entities is the easiest approach.

The generateSampleValue() method for entity reference creates new sample entities if none are available.
It has rudimentary recursion protection in place by not allowing one entity type to reference itself.
It does not have any protection against circular references.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 interdiff-11-16.txt2.03 KBAnonymous (not verified)
#16 2966137-16.patch4.93 KBAnonymous (not verified)
#12 interdiff-reroll-2-11.txt2.77 KBAnonymous (not verified)
#11 2966137-11.patch4.85 KBAnonymous (not verified)
#11 2966137-11-test-only.patch2.79 KBAnonymous (not verified)
#2 2966137-entity_ref-2-PASS.patch4.84 KBtim.plunkett
#2 2966137-entity_ref-2-FAIL.patch2.78 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
FileSize
2966137-entity_ref-2-FAIL.patch2.78 KB
2966137-entity_ref-2-PASS.patch4.84 KB

Here's one attempt at preventing the recursion.

The last submitted patch, 2: 2966137-entity_ref-2-FAIL.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

"CI aborted" is about the best I can think of, since it gets stuck in an infinite loop

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Priority: Normal » Major

Sorry, @tim.plunkett, but I think you made a mistake when you put the "Normal" status, it seems to be at least Major, but otherwise excellent work! I immediately remembered our issues:

Where we also have "CI aborted". Perhaps they not related. But if the test bot does not have an nesting level limit, it is worth it to introduce, to simplify the finding of problematic places.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

I don't know that I made a mistake, just a judgement call :)
In core with no custom code it can only be reproduced with an experimental module.
And even then, only with a questionable architecture...
But either way it's fine.

CI aborted only because it timed out the bot.
Introducing a nesting limit would be a reasonable follow-up.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I don't know that I made a mistake, just a judgement call :)

I'm sure we can agree ;)

Follow-up done #2966692: Provide limit on max nesting level

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

RTBC, if no one has an objection

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review

Doesn't apply anymore?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented 
error: patch failed: core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceItemTest.php:3
error: core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceItemTest.php: patch does not apply
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
2966137-11-test-only.patch2.79 KB
2966137-11.patch4.85 KB

Indeed. But still "successfully applied" via phpstorm. Nit reroll after #2849669: Fix \Drupal\Component\Utility\Unicode() because of the Symfony mbstring polyfill.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
interdiff-reroll-2-11.txt2.77 KB

The last submitted patch, 11: 2966137-11-test-only.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented 
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -41,6 +41,13 @@
+  /**
+   * An associative array keyed by the reference type, target type, and bundle.
+   *
+   * @var bool[]
+   */
+  protected static $recursionTracker = [];

Do we want to stick this on the class? It looks like it is part of the real run-time and something you might need / want to use. But actually it's just for generateSampleValue method. I'd be tempted to just make it a static inside that method. It's not as if anything outside that method should touch it - even on this class.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
2966137-16.patch4.93 KB
interdiff-11-16.txt2.03 KB

#15 nice idea! Done.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2966137-16.patch, failed testing. View results

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 89a7f66bc8 to 8.6.x and 197e45aacd to 8.5.x. Thanks!

  • alexpott committed 89a7f66 on 8.6.x 
    Issue #2966137 by vaplas, tim.plunkett: Circular entity references cause...

  • alexpott committed 197e45a on 8.5.x 
    Issue #2966137 by vaplas, tim.plunkett: Circular entity references cause...
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Thank you, @alexpott! But you suggested an idea that helped get rid of the static variable at the class level. Why are not you on the credit list?

Also is it possible to move @tim.plunkett on the first place? I just made a reroll and #15, but the merit of this fixation is entirely his

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

I straight up forgot that you could do static variables like that in PHP. Too much OO has rotted my brain!

Unless @alexpott cares about the mention, I don't care about the mention order. We all got equal credit via the d.o system

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented 
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -300,9 +304,25 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
+      // Track the generated entity by reference type, target type, and bundle.
+      $key = $field_definition->getTargetEntityTypeId() . ':' . $options['target_type'] . ':' . $bundle;

Why wasn't $field_definition->getUniqueIdentifier() used here?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

We'd still need the target type, that's the important part. Additionally, we don't care what the field name is. Just what type this is and what type it references

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

arakwar’s picture

arakwar CreditAttribution: arakwar commented

I may have found a use case where it still fails...

I was using Paragraphs in a content type, where one of them used nested Paragraphs.

I needed to switch my field to "include only" paragraphs instead of "exclude" paragraphs so I could get access to the layout builder...

I first made sure my paragraphs were not causing a circular issue, and switch them also to this approach to make sure no circular references were possible there.

It may be an issue for the Paragraphs module, but I think people in this issue will be better than me to figure it out.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Human Rights Watch commented 
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -300,9 +304,25 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
+      // Remove the indicator once the entity is successfully generated.
+      unset($recursion_tracker[$key]);

This bit of logic still potentially allows for infinite recursion. I'm seeing this coupled with the Layout Builder Restrictions module when entity reference fields are configured in a cyclic manner.

It may be this issue in the Entity Reference Revisions module: #3050473: EntityReferenceRevisionsItem::generateSampleValue() may run into recursion.

Posting this here for folks running into this issue.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Human Rights Watch commented

After further investigation, I think this still might be a core fix. I've opened #3103422: Potential for recursion in EntityReferenceItem::generateSampleValue() with circular entity references.