Can only intentionally re-render an entity with references 20 times

@Spokje looks like the tests from #47 (test-only at #42) got lost on the way?

Furthermore, I think it might be better to incorporate changes into the MR again for better review, instead of having X patches and rerolls? This is very confusing.

I'm using a layout builder with inline blocks and seeing the same error
after applying patch #105 all inline blocks content disappear
I don't know if I miss something

re roll the changes from comment #92

@alexdoma thank you, could you perhaps also re-add the tests as said in #107?

I added the tests, please review.

NW per #103 @catch:

Still think we need dedicated documentation and testing in the render system itself given we're adding a new API to it.

Hi, the patch from #109 did not work well for me.
When applied I'm receiving a huge ammount of errors when indexing content to Elasticsearch:

 [error]  Recursive rendering attempt aborted for entity_view:node:32478:search_index:122652. In progress: Array
(
    [entity_view:node:32478:search_index:122652] => entity_view:node:32478:search_index:122652
)

I am facing the same issue with entity_block and raised issue https://www.drupal.org/project/entity_block/issues/3382983 🐛 Recursive rendering detected when rendering entity Active
If there is any workaround to this issue please suggest, that would be really helpful.

As discussed in ✨ Add setting to disable per-user rendering Active this bug causes problems for sending simplenews newsletters. If the recipients are registered users then the newsletter entity (node) will not be cached, leading to a limit of 20 emails in a single cron run.

Rebased against latest 11.x and put up a new MR with a slightly different approach. Instead of checking for recursion in the Renderer, I used #pre_render and #post_render hooks in the EntityViewBuilder to track entity render arrays.

Advantages of this approach:

• No changes to Renderer or Render API necessary.
• The render array #cache keys can be altered after EntityViewBuilder::getBuildDefaults, in hook_entity_build_defaults() implementations, so capturing the keys later to use as an index is more accurate

Disadvantages:

• #pre_render and #post_render hooks can potentially get clobbered by other contrib or custom code
• As seen in Drupal\Tests\EntityViewTrait::buildEntityView(), running the #pre_render callbacks without actually rendering can cause an issue, since the #post_render callback would need to be run before rendering the array

Was previously tagged for issue summary update, which if a new approach/solution is going to be used is 100% needed
Tests appear to be added so removing that tag.

But #115 mentions

NW per #103 @catch:

Still think we need dedicated documentation and testing in the render system itself given we're adding a new API to it.

As that been completed?

@smustgrave

But #115 mentions

NW per #103 @catch:

Still think we need dedicated documentation and testing in the render system itself given we're adding a new API to it.

As that been completed?

The new solution I put up does not change the render system API, so I don't know that there's still a need for new documentation or testing. The IS can be updated if this approach is preferred over the other one.

Thanks @godotislate. I just tested your MR in the context of #118 provided by @AdamPS.
1300 emails were sent without getting recursion problem.
Good job. I'll test it more.

so I don't know that there's still a need for new documentation or testing. The IS can be updated if this approach is preferred over the other one.

Could you @smustgrave make a note what other tests do we need? Do we still need to write documentation, because:

The new solution I put up does not change the render system API

The IS can be updated if this approach is preferred over the other one.

I think it is not necessary right now, we need more manual tests to confirm approach.

Updated IS for solution in MR 4994

left some small comments in MR

Mainly around the change record, the attach one to this ticket could use some updates too. Examples are always very very useful.

Updated MR per review comments and updated CR at https://www.drupal.org/node/3316878 →

Thanks!

Attaching the patch from 4994 MR to use with composer as .diff link is not recommended.

@carolpettirossi The .diff is not recommended due to security implications, but you can download the .diff on a patches folder locally instead of adding a patch on the issue that duplicates the information of the MR.

If all MR threads are solved, and the issue is on RTBC, what is left to fix the issue?

This issue gets this week's "funniest bug" award. Magic numbers are bad, kids.

The issue summary includes both patches and multiple merge requests. There should be only one canonical patch or merge request listed.

Please close all non-canonical merge requests and hide non-canonical patches. If you don't have permission to close merge requests, please hide any non-canonical patches and then document which merge request(s) should be closed in an issue comment and under a separate header in the issue summary. This will allow a committer to close them for you. Thanks!

MR 4994 is canonical.

Please close and hide these PRs: 4050, 1897, 1896. (Any MR before 4994, in case any are missing from this list.)

Pushing back to RTBC, since there are no code changes.

xjm → closed merge request !4050

xjm → closed merge request !1897

Thanks @godotislate! Done.

Revisiting my MR, I realize there are a couple issues:

• No recursion protection if $build['#cache']['keys'] is not set, which can happen if the view mode is not cacheable, the entity is not the default revision, or the entity type is not render cacheable
• Cache contexts on the render element are not taken into account when building the recursive render protection ID

The previous approach from MR 4050 (which is also preserved in the git history of MR 4994) does try to account for recursion protection for non-default revisions and situations where the render element is not cacheable. (Incidentally, it does not check for whether an entity is new before trying to get its ID.)

However, if the entity being rendered is a referenced entity, neither approach accounts for the referencing entity and referencing field name which the current recursion protection in Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceEntityFormatter::viewElements() does. Arguably, if render of the referenced entity needs to vary based on what entity is referencing it, then there probably should be cache keys added in a hook_entity_build_defaults_alter() implementation or similar, so using cache keys to build the recursion ID could be fine in this case. But some more thought might needed on how to determine whether an uncacheable entity render element is being recursively rendered multiple times.

Moving back to Needs Work.

No recursion protection if $build['#cache']['keys'] is not set, which can happen if the view mode is not cacheable, the entity is not the default revision, or the entity type is not render cacheable

This is causing most paragraphs to not be shown on our site, because Paragraph entity type is not render cacheable. What can be done about this? Should it be doing anything if $build['#cache']['keys'] is empty? Using an empty string as the $recursion_key seems to be a mistake.

@Rob230: Correct, the reason I moved the issue back to needs work is that the MR has issues. I have some ideas on how to address, but I haven't had time to come back to this.

Alternatively, you can try getting a patch from the MR diff for the previous approach MR 4050 and see if that works for you. I have a couple reservations about it, but it does account for uncacheable entity types. That approach was stuck waiting on documentation and approval for API changes it introduced, but you can evaluate whether it works well enough for your project for now.

Thank you, @Rob230 - your note on Rendering of paragraphs broken - recursive rendering attempt aborted 🐛 Rendering of paragraphs broken - recursive rendering attempt aborted Closed: duplicate which highlighted the error Recursive rendering attempt aborted for . In progress: Array ( [] => ) led me back here where - thank you, @godotislate - I learned that MR 4050 works better for us than MR 4994.

I was only seeing that error on some Paragraphs, not all, but having MR 4994 applied and assuming my problem wasn't related to this branch had me banging my head against my code for a good while.

In case it's helpful, I'll note that the erroring paragraph type for us was meant to render a taxonomy term in a display mode showing some different paragraph types on the term. So there was supposed to be a node rendering a paragraph rendering a taxonomy term rendering some other paragraphs which is a lot but shouldn't have been recursive AFAIK. MR 4050 allows this chain of rendering to work.

#136

However, if the entity being rendered is a referenced entity, neither approach accounts for the referencing entity and referencing field name which the current recursion protection in Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceEntityFormatter::viewElements() does.

I believe the current approach you describe is not optimal. I think the current entity id and view mode are the only information that matters, they are necessary and sufficient to define a recursion.

More generally, I'm not sure that MR4994 is preferable to MR4050. It seems to have brought complications of its own.

It's true that MR4994 could prevent a false positive recursion detection in some weird weird case where a render of an entity in the same view mode was not identical and so an additional cache key had been added. But this is such an edge case that I find it difficult to take seriously.

It's true that MR4994 could prevent a false positive recursion detection in some weird weird case where a render of an entity in the same view mode was not identical and so an additional cache key had been added. But this is such an edge case that I find it difficult to take seriously.

I agree this seems like an edge case, but I have worked on at least three projects, including one just recently and why this is top of mind, with business requests to vary some element of an entity display based on what entity was referencing.

That being said, I definitely agree it seems extremely unlikely that the same entity in the same view mode will appear multiple times in a nested/recursive render chain and intended to be displayed differently in separate instances in the chain (whether because of referencing entity, an alternate revision, or a different language). So generally entity type ID, entity ID, and view mode should suffice.

Separately, I did think of a use case with new entities to account for: if you are creating a node with a node reference field, and allow creating new nodes in the widget, whether through an autocomplete widget or something like IEF, previewing the node before saving requires generating temporary unique IDs, so I've accounted for that.

Updated MR 4994 with changes mentioned, resolved conflicts, and rebased.

godotislate → changed the visibility of the branch 9.4.x to hidden.

godotislate → changed the visibility of the branch 2940605-renderer to hidden.

Left some small comments, but overall change seems good. Definitely saw this on a site we inherited where they embedded an icon. Bad content editing but definitely was a seen bug.

Appears all feedback has been addressed

Updated tests for new deprecation after rebasing. Also added a test for the use case mentioned in #141 🐛 Can only intentionally re-render an entity with references 20 times RTBC : avoiding false positives when previewing a new node referencing a new node.

Additional changes appear fine. For us this did fix the problem we were seeing with an entity icon rendered many times.

Rebased MR for merge conflict.

Since using MR as a patch is a security issue, I am providing a patch based on it https://git.drupalcode.org/project/drupal/-/merge_requests/4994 for the Drupal 10.2.2 release.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased.

godotislate → changed the visibility of the branch 11.x to hidden.

I tested that the 10.x patches don't seem to apply against 10.2.3.

As mentioned in #150, applying patches against MR's is a security issue and the patch from that comment doesn't anymore apply against core 10.2.3, I will attach a newer patch from the MR here.

This patch applies against 10.2.3.

Patch on 10.1.x at comment #100 working for me.

Happy to see this RTBC'd! Should the version here stay 11.x or be set to 10.3.x or even 10.2.x now with https://www.drupal.org/about/core/blog/drupal-11-is-now-open-for-develop... → ?

This should go into 10.3.x at least, 10.2.x if possible as it affects many use-cases and batch operations.

Added some review comments to the MR.

MR updated per review feedback.

Believe all feedback has been addressed. There was one thread wasn't 100% but saw there was a code change and response from @godotislate

Just want to comment that glad this issue is now RTBC. We have been experiencing also mysterious situations where paragraphs are not rendered on the page. We originally installed this patch because had some cases where the Paragraph Library submodule was causing issues where it was rendering the same element in multiple places at the same time.

In the current project we have been having this issue, we don't have Paragraph library yet enabled but we installed this patch because the feature was planned to be implemented.

In our case we were also thinking the reason could be Quick node clone module which can also clone the same paragraph content multiple times.

In our case the symptom was that the paragraph content went missing from anonymous users but it was visible for logged in users. So it definitely seemed like a cache issue. The issue gets solved when you save the node again.

I am happy with the RTBC situation and I can report back if we still experience the issue with paragraphs with the latest patch. But I also want to point out that there might be edge cases that contrib modules are cloning or rendering the same element many times in different places (Paragraph library and Quick node clone for one). In Quick node clone there is an open issue 🐛 Nested paragraph support RTBC where the cloning doesn't revision the cloned parent paragraph correctly which in part might explain some edge case bugs.

We are currently using the patch from #155. I compared that patch to the latest MR and there seems to be just differences in tests.

What's the impact on contrib code like https://git.drupalcode.org/project/entity_reference_dynamic_display/-/bl...?

I think we should consider pushing the deprecations out to D12 because I think code like that and other listed on https://git.drupalcode.org/search?group_id=2&page=2&repository_ref=&scop... need a bit of work.

I also think we need to update the change record https://www.drupal.org/node/3316878 → to specifically address what a formatter that extends EntityReferenceEntityFormatter should do.

I found a bug with the latest patch with Webform Submissions. It causes a PHP error Recursive rendering attempt aborted for webform_submission1html. In progress: Array ( [webform_submission1html] => webform_submission1html ).

Reproduction steps:

1. Be on Core 10.2.4 and the latest stable release of Webform. Apply the patch as of the 15.03.2024 commits.
2. Setup a basic Webform, submit a form (will be submission ID #1)
3. Go to /admin/structure/webform/submissions/manage and go to View the submission you just put in.
4. Instead of seeing a list of fields that you filled in, you just see the "Submission information"
5. If you have dblog enabled, you can see the PHP error in the latest log messages

Confirming @kmonty's report in #163 using patch in #155.

Adding a check that count($this->recursionKeys) !== 1 allows webform submissions to render without logging an error, but I don't pretend to understand this issue well enough to know what else that might do. Attaching the patch I made and the interdiff from the patch in #155 for review.

Sorry made the patch from Drupal default branch this time and this one applies against Drupal 10.2.6.

The change that made my webform submissions work again was adding (count($this->recursionKeys) !== 1) to line 569 in core/lib/Drupal/Core/Entity/EntityViewBuilder.php but starting from the default Drupal branch added some other changes to the patch. Interdiff against the patch in #155 attached.

I also came across issue with webform submissions, but I suppose the patch from 165/166 is going to far, probably rendering the whole checking for recursion useless.

I suppose webform is really doing here something that is to be considered recursive rendering, see

https://git.drupalcode.org/project/webform/-/blob/03b6e07416480ad802f453...

This is ending up in using #theme => 'webform_submission_data' and then having this again in $variables['elements']['#theme'].

So I think the code here is still valid for checking for recursion, but we can maybe provide a possibility to circumvent the webform issue by providing a possibility to vary the render recursion key by something else than entity type, entity id and view mode. This would allow "intentionally" recursive rendering.

With this change it would be possible to solve the webform issue by using hook_preprocess_hook like this:

/**
 * Implements hook_preprocess_HOOK().
 *
 * fix regression (no submission data shown) due to patch from:
 * Can only intentionally re-render an entity with references 20 times -
 * https://www.drupal.org/project/drupal/issues/2940605
 */
function yourmodule_preprocess_webform_submission_data(&$variables) {
  if (isset($variables['elements']['#theme']) && $variables['elements']['#theme'] === 'webform_submission_data') {
    $variables['elements']['#recursion_key_variator'] = 'wrapped';
  }
}

The name of the variator can be anything, it just distinguishes the wrapped render element from the outer one.

Providing an interdiff to #155. (the patch from 155 is made of multiple commits, so when comparing the patch files you will note a lot more changes, but this interdiff is showing the real difference by comparing the effective changes via two git branches → ).

Not changing the MR right now. Not sure if maintainers agree on this, so letting MR out for the moment.

andre.bonon → made their first commit to this issue’s fork.

andre.bonon → changed the visibility of the branch 2940605-10.0.x to hidden.

andre.bonon → changed the visibility of the branch 2940605-10.0.x to active.

andre.bonon → changed the visibility of the branch 2940605-10.0.x to active.

andre.bonon → changed the visibility of the branch 2940605-10.0.x to hidden.

Before we make a solution, we need to understand why the recursive rendering in paragraphs library and webform is OK. Why is it not creating an infinite regression, when our regression prevention is detecting the start of a regression?

Sometimes, Drupal is rendering a paragraph many times but it isn't technically recursive. For instance, you may have a paragraph that embeds a call-to-action block on lots of different pages. If you are indexing pages for search, you may end up rendering that block more than 20 times in the same PHP process. That is what happened to me while migrating hundreds of pages. A similar thing can happen if you have the same webform on many pages.

I am sure there are other scenarios where you can trip the recursive rendering failsafe, but you aren't technically caught in a loop.

Yeah I can confirm what #174 reported and I think that's the worst thing. I remember we ran into the same issue.
But all complaining doesn't help, we have to get this fixed asap.

I took a quick look at Webform submission view building/rendering code, and nothing stood out to me, but I haven't had a chance to debug what could be going on. Regardless, I think we can preserve BC and prevent false positives in cases like Webform submissions by making the new recursive rendering protection an opt-in process per entity type. Idea would be like this:

• Create a new entity type handler definition property of recursive_render_protection or similar
• Create a new EntityRecursiveRenderProtection handler class that does the tracking (essentially the #pre_render/#post_render callback methods in MR 4994 would be moved to this class)
• Only add the the pre_render/post_render callbacks in EntityViewBuilder::getBuildDefaults() if the recursive_render_protection handler is defined for the entity type
• In EntityReferenceEntityFormatter::viewElements(), check whether the referenced entity is of an entity type that has the recursive_render_protection handler. If not, keep the 20 count limit in place. Trigger a deprecation that the 20 count limit will go away altogether in Drupal 12
• Add the recursive_render_protection to all core entity type annotations/attributes)
• Create CR and document that contrib/custom entity types should add the recursive_render_protection property to their definitions by Drupal 12
• There should also probably be a way to alter the recursive tracking key, so custom/contrib code could possibly use that instead of implementing/overriding EntityRecursiveRenderProtection class if they need special logic for recursive rendering protection

If this idea is acceptable, I think it'd be best to wait for 📌 Convert entity type discovery to PHP attributes Needs review to go in, so that the handler definition can be added to entity type classes as attributes, instead of annotations, because that MR is such a beast to rebase. But I think I can provide a PoC of this in a couple weeks.

Before we make a solution, we need to understand why the recursive rendering in paragraphs library and webform is OK. Why is it not creating an infinite regression, when our regression prevention is detecting the start of a regression?

Another scenario is sending a simplenews newsletter. The same entity is rendered repeatedly for each recipient, possibly hundreds of times in a single batch. If the recipient are Drupal users, then the entity is rendered each time in that specific users context, meaning that it cannot be cached. Clearly in this case there is no recursion.

#161, #174, #175, #178 are beside the point I think. We know that there can be valid uses for rendering the same element multiple times in the page: that's the motivation of this issue from the beginning, and why we are building key based recursive rendering detection mechanisms. Both MR 4050 and 4994 try to prevent genuinely recursive rendering but still allow for these use cases like paragraphs, paragraphs library and simplenews.

#164/#165 is probably wrong solution as suggested in #167, they simply allow double rendering but not more. It's kind of a hack - we allow something we think shouldn't happen to happen once, but stop it after that to limit the damage. Maybe this is a clever hack, but it smells like ducking the real bug to me.

I don't like the proposal in #177 to make the recursion prevention opt-in. This is supposed to be a guard rail to help developers not shoot themselves in the foot. It should be on by default. It doesn't particularly seem right to me either to make it opt-out at the entity level. If people need to opt out, they can just use a custom EntityViewBuilder or whatever.

The immediate question is why MR 4994 is incorrectly detecting recursive rendering in webform as reported in #163, #164, and #167.

It would help if someone posted a log here: MR 4994 is supposed to log an error when it detects recursive rendering that describes why it thinks this is recursion.

#167 suggests a possible cause:

I suppose webform is really doing here something that is to be considered recursive rendering, see
https://git.drupalcode.org/project/webform/-/blob/03b6e07416480ad802f453...
This is ending up in using #theme => 'webform_submission_data' and then having this again in $variables['elements']['#theme'].

But I can't really understand what's going on there, can any one explain more?

Possibly there are similar problems with paragraphs as reported in #137 & #139. I'm not sure: has this been resolved for paragraphs in the latest version of MR 4994 or not?

One possible issue is that MR 4994 has not used revision and language keys in the way that MR 4050 did. This might explain the paragraphs problems as revisions are important in paragraphs.

More generally, the discussion in #140 / #141 about MR 4050 vs MR 4994 are still with me. I do wonder whether MR 4994 is creating fragility here and we need to move back to MR 4050. For example, does anyone have any idea whethere 4994 will work with a lazy builder? #141 raises an issue with MR 4050, but maybe there's a simpler solution to that than MR 4994.

In our case with a multilingual website with more than 20 countries, the MR4994 does not fit because it does not take into account the language on the recursion keys, on our case the MR 4050 fits better.

Differences between 4994 and 4050:

4994:
* The keys are the entity type, entity id and view mode:

 
    return $entity->getEntityTypeId()
      . $entity_id
      . $build['#view_mode'];

 
* The recursion protection is executed on the pre-render and the post-render:

    // Add callbacks to protect from recursive rendering.
    $build['#pre_render'] = [[$this, 'setRecursiveRenderProtection']];
    $build['#post_render'] = [[$this, 'unsetRecursiveRenderProtection']];

4050:
* The keys are a key on the render array and contain the revision and the language:

...
    $keys = [
      'entity_view',
      $this->entityTypeId,
      $entity->id(),
      $view_mode,
    ];
.... 
    // Add keys for the renderer to use to identify recursive rendering.
    $build['#recursion_keys'] = $keys;
    if ($entity instanceof RevisionableInterface) {
      $build['#recursion_keys'][] = $entity->getRevisionId();
    }
    if ($entity instanceof TranslatableDataInterface && count($entity->getTranslationLanguages()) > 1) {
      $build['#recursion_keys'][] = $entity->language()->getId();
    }

* The recursion protection is executed on the method do render:

protected function doRender(&$elements, $is_root_call = FALSE) {
....
    // Guard against recursive rendering now that all other early return
    // possibilities are exhausted and it's time to render children.
    $recursion_key = implode(':', $elements['#recursion_keys'] ?? []);
    if ($recursion_key) {
      if (isset($this->recursionKeys[$recursion_key])) {
        \Drupal::logger('render')
          ->error('Recursive rendering attempt aborted for %key. In progress: %guards', [
            '%key' => $recursion_key,
            '%guards' => print_r($this->recursionKeys, TRUE),
          ]);
        $elements['#printed'] = TRUE;
      }
      else {
        $this->recursionKeys[$recursion_key] = $recursion_key;
      }
    } 

So 4050 is more complete because it considers the revision IDs and the languages and allows adding more keys using #recursion_keys on the render array.

Why remove the recursive limit?
Before the MR the limit was 20, why remove it?
We should detect the real recursions but is there any way to render an element multiple times without recursions?

Why remove the recursive limit?
Before the MR the limit was 20, why remove it?

Because without the MR we don't have a recursive limit, we have a repeated rendering limit. That does limit recursion, but it also blocks other valid use cases of non-recursive repeated rendering.