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
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff-11-16.txt | 2.03 KB | Anonymous (not verified) |
#16 | 2966137-16.patch | 4.93 KB | Anonymous (not verified) |
#12 | interdiff-reroll-2-11.txt | 2.77 KB | Anonymous (not verified) |
#11 | 2966137-11.patch | 4.85 KB | Anonymous (not verified) |
#11 | 2966137-11-test-only.patch | 2.79 KB | Anonymous (not verified) |
|
Comments
Comment #2
tim.plunkettHere's one attempt at preventing the recursion.
Comment #4
tim.plunkett"CI aborted" is about the best I can think of, since it gets stuck in an infinite loop
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry, @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.
Comment #6
tim.plunkettI 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.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm sure we can agree ;)
Follow-up done #2966692: Provide limit on max nesting level
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedRTBC, if no one has an objection
Comment #9
larowlanDoesn't apply anymore?
Comment #10
larowlanComment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedIndeed. But still "successfully applied" via phpstorm. Nit reroll after #2849669: Fix \Drupal\Component\Utility\Unicode() because of the Symfony mbstring polyfill.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #14
tim.plunkettThanks!
Comment #15
alexpottDo 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.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commented#15 nice idea! Done.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #19
alexpottCommitted and pushed 89a7f66bc8 to 8.6.x and 197e45aacd to 8.5.x. Thanks!
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedThank 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
Comment #23
tim.plunkettI 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
Comment #24
markhalliwellWhy wasn't
$field_definition->getUniqueIdentifier()
used here?Comment #25
tim.plunkettWe'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
Comment #27
arakwar CreditAttribution: arakwar commentedI 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.
Comment #28
jhedstromThis 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.
Comment #29
jhedstromAfter 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.