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

Command icon 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

taran2l created an issue. See original summary.

taran2l’s picture

Issue summary: View changes
taran2l’s picture

taran2l changed the visibility of the branch 3580855-entity-recursive-rendering to hidden.

taran2l changed the visibility of the branch 3580855-entity-recursive-rendering to active.

taran2l’s picture

Status: Active » Needs review
taran2l’s picture

Issue tags: +Needs tests
godotislate’s picture

Coming here from #3583187-8: [random test failure] DuplicateContextualLinksTest::testSameContextualLinks:

However, while trying to do that, I discovered why this test suddenly started to fail so often, and it's because of #2940605: Can only intentionally re-render an entity with references 20 times. Since that issue, the node in our test is no longer being rendered in both views blocks, it now appears only the first block rendered on the page, which (funnily enough) is the _second_ views block, that's why my initial MR asserted that one :)

Not sure if these are the same issue, but can be investigated, and if not, we can open a separate issue.

amateescu’s picture

Assigned: Unassigned » amateescu
Priority: Normal » Major

I 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.

amateescu’s picture

Assigned: amateescu » Unassigned

Pushed 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.

amateescu’s picture

Issue tags: -Needs tests
amateescu’s picture

Test-only pipeline fails as expected: https://git.drupalcode.org/project/drupal/-/jobs/9273587

taran2l’s picture

hi @amateescu, great job on that test, looks solid to me! Basically it has everything now.

taran2l’s picture

Status: Needs review » Reviewed & tested by the community

I think I can RTBC the test part, as @amateescu has confirmed that the fix part work as well