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

quietone’s picture

Triaged and I didn't find anything amiss. I have updated credit.

alexpott’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 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?

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed b8c65c49 on 11.x
    fix: #3580855 Entity recursive rendering protection is not compatible...

  • alexpott committed a2ee2493 on main
    fix: #3580855 Entity recursive rendering protection is not compatible...
taran2l’s picture

@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

alexpott’s picture

@taran2l that is great - thanks for testing so thoroughly and the early adoption - it is great to fix stuff before a stable release!

Status: Fixed » Closed (fixed)

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