Problem/Motivation
The fix for recursive rendering committed here #2940605: Can only intentionally re-render an entity with references 20 times is not compatible with placeholders rendering via Fibers, because recursion keys are being mixed between different Fibers
Steps to reproduce
Not sure if this is possible with Drupal core itself, my steps are:
- Enable BigPipe
- Install Menu items extras
- Configure menu with multiple nodes being rendered as menu items
- Place 2+ menu blocks on the same page
Proposed resolution
Add Fiber IDs to recursion keys
Remaining tasks
Review the patch
User interface changes
None
Introduced terminology
None
API changes
None
Data model changes
None
Release notes snippet
None
Issue fork drupal-3580855
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
taran2lComment #3
taran2lComment #7
taran2lComment #8
taran2lComment #9
godotislateComing here from #3583187-8: [random test failure] DuplicateContextualLinksTest::testSameContextualLinks:
Not sure if these are the same issue, but can be investigated, and if not, we can open a separate issue.
Comment #10
amateescu commentedI checked this change on the test that was failing randomly and it does fix it! Working on a test that doesn't require a view to trigger the problem.
Comment #11
amateescu commentedPushed the test! It was quite tricky to determine all the conditions required to hit this problem: we need a block that's placeholdered so it renders in a Fiber, and the entity being rendered needs a formatted text field to create filter placeholders that cause the Fiber to suspend mid-render, allowing the other block's Fiber to start and hit the collision.
Comment #12
amateescu commentedComment #13
amateescu commentedTest-only pipeline fails as expected: https://git.drupalcode.org/project/drupal/-/jobs/9273587
Comment #14
taran2lhi @amateescu, great job on that test, looks solid to me! Basically it has everything now.
Comment #15
taran2lI think I can RTBC the test part, as @amateescu has confirmed that the fix part work as well
Comment #16
quietone commentedTriaged and I didn't find anything amiss. I have updated credit.
Comment #17
alexpottCommitted a2ee249 and pushed to main. Thanks!
Committed b8c65c4 and pushed to 11.x. Thanks!
Is the code in prior to #2940605: Can only intentionally re-render an entity with references 20 times also vulnerable to this? I'm not sure. I.e do we need to work on backporting a fix to 11.3.x for the different apporach to entity render recursion protection?
Comment #21
taran2l@alexpott, afaik 11.3.x is not affected; the issue has been introduced in #2940605: Can only intentionally re-render an entity with references 20 times which not yet available in a stable release
I've found this issue only because we were using the patch from #2940605: Can only intentionally re-render an entity with references 20 times on 11.3.x
Comment #22
alexpott@taran2l that is great - thanks for testing so thoroughly and the early adoption - it is great to fix stuff before a stable release!