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

A hacky workaround I use for now is to implement hook_field_formatter_info_alter() and replace the formatter's $info['entity_reference_entity_view']['class'] with \Drupal\my_module\EntityReferenceFieldFormatterProxy

.

I wouldn't necessarily call that hacky, IMO it shows really well the power given by the plugin system coupled with OO code, meaning that you can easily replace a class with something that suits your needs better, while also being able to use most of the code from the original class.

However, we could make it easier for people with advanced use cases like described in the issue summary by moving the code which generates the recursive render ID into a helper method. What do you think about that?

Hi,

Drupal core 8.6.10
Entity Reference Display 8.x-1.2
Entity Reference Revisions 8.x-1.6
Media entity 8.x-1.8

I got an issue not being able to render the same reference more than 20 times.

I have several nodes including a reference to the same node A including several medias.

Every Cron I got an entity.ERROR for each media liked to node A:

Example of one error message:

entity.ERROR: Recursive rendering detected when rendering entity media: 248, using the field_media field on the slide_media bundle. Aborting rendering. {"%entity_type":"media","%entity_id":"248","%field_name":"field_media","%bundle_name":"slide_media"}

The security avoiding recursivity blocks also the repetition:

Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceEntityFormatter::RECURSIVE_RENDER_LIMIT

This seems a huge blocker.

Can we imagine rising the priority level, and applying the latest drupal version 8.6 ?

Cheers,

Ran into this issue when Search API indexes rendered content using Layout Builder.
The article content type full content display uses LB, and the defaults layout has three block_content blocks. Each block has an image media reference field.

When search batch indexes 20 or more articles either using defaults layout or overrides layout that still have these blocks, the blocks are being re-rendered the same number of times, including the media reference field, leading to the error.

Workaround so far has been a little complex:
1) Configure the view mode for indexing rendered content to be search_index
2) Implement hook_entity_view_display_alter() to set the search_index view mode to use the configured full content display (and setting the originalMode on the display to search_index
3) Add a subscriber for the LayoutBuilderEvents::SECTION_COMPONENT_BUILD_RENDER_ARRAY event, with priority set to be greater than \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray
4) Created the search_index view mode for block_content entities and changed the configuration of the layout block in the event subscriber to use the search_index view mode for block_content (based on a list of block UUIDs)
5) Configured the search_index display on block_content not to display the image media.

This is also an issue in Drupal 7.67. I have a view that lists all nodes on a term page. For each listed node, we render the icon of the term. So on one page, one could see a list of 30 of the same term icons, when there are 30 nodes connected to the term.

Drupal 7 is very different. Not sure how you embed things, but because it doesn't have render caching, the entity_reference formatter (which is not in core and taxonomy term doesn't have a view entity formatter, so no idea what you are using) knows when rendering is finished and can decrease the recursion counter again.

If you have a problem in D7, you need to check how exactly you are rendering that information, if it's really a formatter or just a field in views and investigate there.

What @Berdir said :)

Update workaround for search indexing mentioned in #7.
Steps 1 & 2 the same.
3. Create a subclass of \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceEntityFormatter which add static getter and setter methods for $recursiveRenderDepth
4. Create a subclass of the search api plugin so that every time an entity is rendered for indexing, $recursiveRenderDepth is saved before the rendering, and then set back to the saved value after the rendering. This way any depth incrementation done per indexed entity render is undone after, and the batch continues.
5. (I also had to do something similar for the contrib entity embed filter.)

I've replaced the render count with an approach that builds the reference "path" traversed during rendering. This allows for rendering the same entity side by side for more than 20 times, but still allows detection of recursion. How do you guys feel about this?

On a side note, this allows us to make our themers happy, because they can get more information about the entity's parent(s) when preprocessing ;)

Narf, local git update changed the output of diff files. Attached is an updated patch, that hopefully applies.

I think #13 solution is pretty cool.

Review of #13:
1. 🤓This is a really cool idea.

+      if (substr_count($entity->_render_path, $recursive_render_id) > static::RECURSIVE_RENDER_LIMIT) {

2. This is repeatedly calling $items->getEntity(), I think it would be better to set a variable $entity and reuse it this code block.

+    $render_path = $items->getEntity()->getEntityTypeId() . ':' . $items->getEntity()->id();
+    if (isset($items->getEntity()->_render_path)) {
+      $render_path = $items->getEntity()->_render_path . '/' . $render_path;
+    }
+

I've added a reusable variable as of @oknate's suggestion.

$recursive_render_id = $items->getName() . ':' . $entity->id();
if (substr_count($entity->_render_path, $recursive_render_id) > static::RECURSIVE_RENDER_LIMIT) {

We might run into false positives, if field names and entity ids are identical across different entity types within the reference chain, or if a field is a prefix of another. Since most custom fields automatically have the field_ prefix, this should be quite rare: field_foo:123/foo:123 or field_foo:123/field_bar_field_foo:123. Is this a problem we should handle, or can we ignore it because it's rare?

Possibly add the separator slash to the lookup and use >= to retain the correct count?

$recursive_render_id = $items->getName() . ':' . $entity->id();
if (substr_count($entity->_render_path, '/' . $recursive_render_id) >= static::RECURSIVE_RENDER_LIMIT) {

I ran into the same Problem :-( My Solution is based on your well described hook_field_formatter_info_alter():
In my own class I add

protected static $actual_uid;

and clear static::$recursiveRenderDepth if user changes (happens on every new mail):

public function viewElements(FieldItemListInterface $items, $langcode) {

    $uid = Drupal::currentUser()->id();
    if ($uid != static::$actual_uid) {
      static::$actual_uid = $uid;
      static::$recursiveRenderDepth = [];
    }

    return parent::viewElements($items, NULL);
  }

So i do not break the mechanism for a single outgoing mail but have a workaround for the problem...

Rerolling for 8.9.x.

Similar to a couple other posts, my use case is sending a message with images out in email. The email is queued and run on cron. Cron runs as the anonymous user, so I can't fix this by clearing the cache when the user changes, because the user never changes. In addition, these images are media entities embedded in the body field, so I can see no way to use hook_field_formatter_info_alter() to intervene in the way I could if these were separate fields. I'm not sure how to render the body differently when sent out an email vs when it's viewed on the web site. The code doing the rendering has no knowledge about which situation applies.

For example, I have an email that has two images in it. The first 20 recipients get mail with images, everyone else has missing images (and I have dozens of log messages, one for each missing image).

At the moment I see no workaround for my use case.

Hi KarenS, just look in my code-example above.
This works in a big project with nested paragraphs and images very well.

I see no way to get an embedded image in the body to use a custom formatter. This is a media item embedded using core media library.

That shouldn't be using entity reference, but seems to. Maybe I've got something funky going on. I'll play around.

Aha, there is ANOTHER recursive setting in Drupal\media\Plugin\Filter\MediaEmbed. Which also needs to be overridden. That seems to be the one I'm hitting.

It's hard-coded to pull the limit from EntityReferenceEntityFormatter, so any extensions of that class are ignored, and the above fix doesn't help

FWIW I found a workaround. I installed https://www.drupal.org/project/queue_throttle and set it up to reduce the number of queue items executed at one time. I had to take it back to 9 items at a time to avoid errors even though you would think I could do 20 items. But that seems to work for my situation.

Hi KarenS: so much pain :-( Great you found a "crazy" workaround. The "9" instead of 20 can result of double-rendering of content (Drupal is sometimes mysterious). Greetings !

I made a separate issue for the Drupal\media\Plugin\Filter\MediaEmbed issue. It is the same idea to prevent infinite recursion by keeping a static counter, but it is a fundamentally different code base. Let's focus here on the entity reference, and once it is fixed we can fix the MediaEmbed bug in #3151977: Error "recursive rendering detected" when rendering media 20+ times.

Assigning, I plan to write a test and provide backwards compatibility by retaining and deprecating the current $recursiveRenderDepth static variable. It is depended on by a number of contrib modules (e.g. dynamic_entity_reference, entity_reference_revisions, field_pager etc).

I wrote a test for this but it was not successful. The test is passing without the patch, so I am apparently testing the wrong thing.

I seem to misunderstand the cause of the issue. My understanding was that the current recursion protection is insufficient because it only takes into account the referenced entity and the referencing entity. So I designed the test to have 3 levels of references:

1. $referencing_entity->$referenced_entity
2. $referencing_entity_1->$referencing_entity_2->$referenced_entity
3. $referencing_entity_1->$referencing_entity_2->$referencing_entity_3->$referenced_entity

Then I build a render array that will contain 30 referencing entities (10 of each depth), and repeat this 30 times, so there are in fact 900 instances of the referenced entity at 3 different depths. This test passes without the patch.

So I am clearly missing something, what exactly are the conditions for this bug to manifest itself? Can someone provide an example setup that allows this to be replicated?

Updated the patch to restore and deprecate the $recursiveRenderDepth static counter. We cannot remove it without breaking backwards compatibility because this is used in contrib modules like Dynamic Entity Reference.

The problem I had and some of the other reports had, is that we're sending out email with images in the mail. In my case, I'm sending out an email with the teaser view of the node to notify a subscriber list that there is new or changed content.

Drupal's "recursive" test is actually just a "multiple" test. How many times are you rendering the same image.

Email is typically batched and queued and sent out on cron. So if my cron run sends out 50 emails, it will need to render the same image 50 times. Which Drupal won't allow. After 20 times, the image is just missing from the mail. In fact in my case, after 9 times the image is missing, for reasons I'm not sure about.

Another example above was repeating an icon multiple times on the same page.

There is also an issue about a problem when indexing search results that I haven't sorted out.

Anyway just finding other ways to count how many times the same image is rendered while limiting it to 20 will not help the problem. The point is that there are times when you actually intend to re-render an image more than 20 times. I'm actually not sure of the best solution. Somehow we need to tell Drupal when to reset that counter, or provide a way to override it temporarily.

It would probably help to figure out what problem this was supposed to solve. I assume it's a situation where you have a circular reference where you reference an item which eventually references the original and you would have an endless loop of rendering the same thing over and over. If so, maybe there's a better way of detecting that than just arbitrarily stopping after 20 repeats.

I found the issue where this constant was added, #2073753: Fix and add tests for the recursive rendering protection of the 'Rendered entity' formatter. The test added at that time was for an entity reference with a self reference, the situation I suspected above. So whatever solution we come to has to still solve that problem, but without arbitrarily preventing an image from being repeated for any other reason.

I tried to replicate it in #29 by rendering the same entity 900 times, but it appears to work, at least in the way that the test is set up: I used a render array with 900 entity view modes with a reference to a single entity. This probably doesn't match to how it works when rendering emails. Possibly the same render array will be executed in a loop?

You might hit the render cache of the parent entity if you reuse that. Try creating 21 new entities referencing the same child in a row and render them.

Looking at related item #3081703: Add setting to disable per-user rendering , another email-related problem, the failing test could look like this.

- Create 21 users
- Create a node with a reference to something else, like an image
- Render the node 21 times, switching users each time using \Drupal::service('account_switcher')->switchTo().
- Check that the node is rendered correctly each time.
- The referenced item should be missing by the end of this process.

The fix I think is to change the logic. Ignore the counter. Keep track of which entities reference which other entities. As soon as a reference is detected, create an item that describes the reverse reference that must be prohibited and stack the prohibited references in an array. Then instead of checking the count of references, check each new reference to see if it is in the prohibited list. That would actually check recursion.

Note that there could be multiple layers of references, which is why we need an array. Each new reference must not refer to its parent, nor to any other entity all the way up the chain.

For bonus points, move the recursion logic into a service because the same logic is needed elsewhere, like in media module, see #3151977: Error "recursive rendering detected" when rendering media 20+ times.

I just want to confirm that the patch from #30 resolved my issue. For us this issue appeared under the following circumstances:

• An entity of type "politician" has a reference to the entity "politician"
• Another entity of type "question" has a reference to the politician entity
• In an overview created by views we list all questions. In this view the questions are rendered directly
• In circumstances where there are more than 20 questions to one politician the issue appeared for us because the party of the politican is rendered more than 20 times in following way in the preprocess function for each of the question entities:
  

  $politician = $question->getPolitician();
   $variables['politician_party'] = $politician->party->view('public_profile');
  

Smartsheet hit this when we had nodes tagged with terms and terms were referring media (icons). We were listing nodes from views and mysteriously after a time the nodes didn't have icons.

I have successfully reproduced this with the above structure. I couldn't reproduce with anything simpler. I created a node type, a vocabulary, a media type, a node field referring the vocabulary, a term field referring the media type, a view listing nodes, 21 nodes, 1 term, 1 media. The 21th node misses the media. It still has the term but the media is gone. Edit: but only in preview.

I will take a stab at writing a test.

I updated the issue summary. My mind is clearer now // all too well // I can see ...

The patch is just #30, together with my test.

The "fail" patch should be with .patch extension, to show that it can catch the bug

Reuploading 40 -- this we don't get the issue yanked to CNW by the bot...

1. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php
   @@ -230,6 +231,42 @@ public function testEntityFormatter() {
   +   * Tests the recursive rendering protection failing over single entity N times.
   

   Nitpick: This goes over 80 chars :)

2. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php
   @@ -230,6 +231,42 @@ public function testEntityFormatter() {
   +    $count = 21;
   

   How about we use EntityReferenceEntityFormatter::RECURSIVE_RENDER_LIMIT + 1 here instead?

The patch in #40 seems to work for me.
Thanks!

Re-rolled patch from #46 against Drupal 8.7...

Patch #46 solved the issue for me.
But I faced a similar issue in the entity_embed module #3067452: Recursive Limit Breaks Normal, Repeated Use of a Media item on a page
and added logic to reuse the render_path in process text from patch #46 when possible, otherwise set the parent entity at least as context.

Patch #46 solved the issue for me.

Rerolled the excellent work in 2940605_46.patch against 9.3.x in the new MR.

Re-rolled patch from #46 against Drupal 8.9

This is the patch file from the merge request in #56 in case this is helpful for others reviewing.

The patch applied to core 9.2.4 with no errors. I no longer get the recursive error when attempting to edit a layout, but I'm getting a WSOD instead. I haven't figured out how to recreate my specific issue, so I may have had some other issues, which means my review of this patch may not be very useful.

I have a node of type Location that uses layout builder. The node has an entity reference field for all Locations. The entity reference field displays as the teaser view mode for the location node, which includes media. A user reported that when they when to the layout /node/NID/layout, they got the error page instead. I was able to go directly to /node/NID/layout/discard-changes, and then I was able to edit the layout again.

Rerolled fail-test patch from #42 #2940605-42: Can only intentionally re-render an entity with references 20 times

Fail test failing, nothing to see here, move along...

Thank you all for working on this.

I reviewed the code, it looks clean and understandable to me. My only gripe is that I couldn't understand the description of the test, and the $entity variable could have a more meaningful name.

Also I set up a test site to do some manual testing:

• Configure a node type with a node reference field, unlimited values, displayed with the rendered entity formatter.
• Create two nodes. Node 1 contains 21 references to node 2. Viewing node 1, node 2 is shown only 20 times: this is the bug.
• Install the patch taken from the MR.
• Reload node 1 created earlier. It now shows node 2 all 21 times: the bug is fixed.
• Just for fun: create node 3 which references node 1 three times. When viewing node 3, node 2 is rendered 63 times, as it should.
• Create two nodes in a circular reference to make sure the recursive rendering protection still works. It does.

During these tests I also used a step debugger to inspect how the render path is being built and see if I understand what is happening.

TL;DR I would call this RTBC, except for the minor points about the test mentioned above. These new patches fix that.

In our case we have content with more than 20 translations, so when search_api tries to render the node for each translation reach the limit.
The patch provided by @marcvangend works on our case.

@Eduardo thanks for the feedback. If you have tested the patch, found that it works, and maybe even looked at the code to see if it makes sense to you... Feel free to change the status to RTBC! That is the only way we can move issues forward and get this committed to Drupal core.

Okey @marcvangend, I change the status to RTBC, patch on #63 tested and working on our prod environment.

If you're hitting this issue with a queue or custom rendering, an alternative workaround to #25 is to do this in your queue job or custom render code:

    $reflection = new \ReflectionProperty('\Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceEntityFormatter', 'recursiveRenderDepth');
    $reflection->setAccessible(true);
    $reflection->setValue(null, []);

This isolates each rendering job.

I don't believe in the fix here. I find #50 concerning and I don't think we should proceed with the current patch without knowing how we want to adress #50. It's a core issue with the media filter as well a a contrib issue with entity_embed.

Fundamentally we're hacking around with little recursive render solutions here and there at multiple places in the system. It's not DRY, it's got poor maintainability, and it's bug prone.

Let's put in place a single neat fix that solves this in one place for all the use cases:
- the entity reference entity formatter
- media
- entity embed
- entity reference revisions

I suggest:
1. In Renderer:doRender() we check for a #recursion_keys property in the render array being processed. We track these keys in the renderer and use them to detect and halt recursive rendering.
2. In EntityViewBuilder::getBuildDefaults() add the appropriate '#recursion_keys'.
3. Remove existing recursion prevention code from EntityReference formatters and Media.

This would be enhanced by #2529438: Inject renderer service into ThemeManager, disuse drupal_render() but I think it works ok without it.

MR !1896 is against 9.2 by mistake, MR !1987 is the one to see.

If we go this way, then all these recursion tests need to be done both with and without render caching.

@jonathanshaw Thanks for your work on this. I am testing this fix on a project. So far it looks good.

I rebased the MR1897 to 9.4.x

I've reviewed this patch and tested it with a REST call that is rendering multiple entities and was triggering the issue.

The code looks good and after apply the merge request diff as it currently stands, I stop seeing the errors and everything renders fine.

Hiding all patches, since the MR has just been RTBC-ed.

Merge branch 9.4.x to fix problems with the merge

There's an unresolved comment from @alexpott on the MR

I fixed the implode in response to @alexpott's comment, looking at it now it seems to have been unecessarily complex. Imploide will return an zero-length string if the array passed to it is empty, but a zero-length string seems harmless here.

I also created the follow-up issue.

Putting to NR, but this should be a straightforward RTBC.

Added standalone patch for 9.4.x.
Change in MR works well for me and solved problem.
In my case issue was reproducible on a multilingual site (with more than 20 languages) and enabled multilingual support (and emidiate indexing) for search_api_solr module. On each saving of node (translated to more than 20 languages) with reference (by field) to media (translatable)) I had a record related to recursion in watchdog.
I could confirm that the fix works.

This is an easy to put back to RTBC for someone who can confirm that #82 is addressed by my last commit

Last comment from @alexpott is addressed.

+1 for patch #84, tested on D 9.4.5 / PHP 7.4

Does the solution landed on here account for rendering an entity in multiple languages? I have a 40+ language website. When I send a node to search_api to index, I consistently reach the render limit right now due to the number of languages. The render limit (previously) didn't shard based on langcode. Given the design choices here, is that still a concern? Do we need a test scenario to cover this edge case?

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -213,6 +215,15 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode) {
+      $build['#recursion_keys'][] = $entity->language()->getId();

Yes, we do account for language. Great.

I don't see any test coverage of this though. Can/should we add it?

Also, the MR/patch references 9.4 for deprecation. I think that needs to move to 9.5 or 10.0 at this point? Back to NW for (hopefully) a quick turn around.

Needs a new MR targeting 9.5.

Technically #88 is a separate bug. We do fix it here - it's trivial to do so - but requiring the test coverage may stall this issue.

Re rolling the changes from above MR https://git.drupalcode.org/project/drupal/-/merge_requests/1897 to a patch

Reroll the patch #84 with Drupal 10.1.x

Anecdotally, another way to reproduce the issue is to reference the same media embed from 21+ node bodies, then try to index the rendered "Search Index" view mode in Solr.

1. Drupal 9.4.5
2. Search Api Solr Indexation task.
3. Error: "During rendering of embedded media: Recursive rendering..."
4. Setting a breakpoint at the error message uncovered entity UUID: d3b61a7c-86e4-44c5-bdcc-e7c7e22c8e95
5. Searching for the UUID, we found 28 different nodes containing a reference to the same media entity embed in the body field: select entity_id from node__body where body_value like "%d3b61a7c-86e4-44c5-bdcc-e7c7e22c8e95%";
6. This was the context: <drupal-media data-entity-uuid="d3b61a7c-86e4-44c5-bdcc-e7c7e22c8e95" data-entity-type="media" ></drupal-media>

The patch in #84 solved the issue.

Ran into this problem where we're using ECK entities for a complex page layout architecture, when we'd reindex content in Search API this error would constantly show up. Patch #94 resolves the problem and we can reindex all of the content again, this time without all of the "Recursive rendering detected" problems.

Marking this RTBC based upon #95 and #96 (my experience on a client project with complex ECK & Paragraphs structures).

This is adding a new API to the render system, so the new #recursion_keys property should probably be documented somewhere, should it also have dedicated test coverage in the render tests? Right now there's implicit testing from the entity tests, but nothing in the render tests themselves. Could also use an issue summary update since what's there doesn't reflect what's in the patch.

Added draft CR and made deprecations in 10.1.0, removal in 11.0.0 and added link to CR.

Added raw diff, since a reroll was also needed.

Confirmed that patch #94 solves the recursive rendering warning while indexing complex nodes with paragraph and media references using search_api on on Drupal 9.4.5. Patch #100 works for Drupal 10. Thanks!

Resetting the issue tags (didn't mean to change them, just had the tab open for many days before commenting). Resetting previous values.

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

Updating tags

Re-upping patch #100 to make clear what the latest patch is. Somehow can't seem to find the patch in the files block to re-enable its visibility.

I am seeing this issue, and have tested it extensively. Here is some information that might be useful.

The most important thing I have discovered, at least in my situation, is that I am only seeing this when using Drush commands. Specifically, it first happened after a migration because the Search API was set to index content immediately. Even when that was turned off, it was still happening when running drush search-api:index.

Limiting the number of records to 20 per batch did not help with Drush commands. The record of what has already been indexed seems to persist across batch calls.

The error messages from Drush were logged to watchdog. When indexing through the UI, either by running cron from the Status Report page or telling Search API to index everything, there were no messages in watchdog.

Just ran into this in a view with multiple media entities, partially referencing each other.

@Spokje thank you! 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 amount 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
)

---
edit:
The patch from @94 (or #84) did not work either - the result is the same

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

As discussed in #3081703: Add setting to disable per-user rendering 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.

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, but this may be fine since I can't think of any way that a new entity would be recursively rendered.)

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

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

I tested that the 10.x patches don't seem to apply against 10.2.3. It might be another patch causing a conflict but the patch was applying against 10.2.2 but not anymore 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. A max of 20 documents was displaying on a media field.

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 we 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 and crashed with the recursive rendering.

In the current project we have been experiencing 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. At least based on our site logs, we haven't encountered the issue anymore because there are no log items for rendering being aborted.

In our case the problem seems to be mostly also related to deeply nested paragraphs, meaning that we might have this kind of structure
- Section (main container)
-- Columns (used to split the content in multiple columns)
--- Accordion (parent element)
---- Accordion item (child element)

Also there might be entity relationships rendered in the third level like entity reference link lists and media entities which don't get rendered.

In this case it seems to still render the column level but the accordions get lost for anonymous users.

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 #3183249: Nested paragraph support 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.

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 #3396166: Convert entity type discovery to PHP attributes 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.

I can report that I don't experience the webform problem when using MR 4050.

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 recursion 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 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. 4994 can be extended or 4050 can be extended with the other MR.

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?

We see that maybe $this->renderCache->set($elements, $pre_bubbling_elements); can help.

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.

Hi, after applying MR 4050 we are getting exception for menu item content besides there are no real recursion (using menu item extras & with two main menu block instances at block structure level, one for responsive, other for desktop). For now we are disabling into preprocess for menu item content recursion keys to avoid it. Maybe this leads to some scenario that may be also taken into account like printing same entity (menu item) at different contexts (in this case different menu block instances) or maybe menu item extras should adapt code for this changes...

Perhaps it would help to use the Context API. For situations like running drush sapi-i, it might help in determining that rendering isn't recursive?

I encountered such issues with Media but this could also solve issues such as #3456722: [regression] Entity view block not displayed after #2962166 since it applies to all entity types.
Finally a true recursion detection and using pre/post_render callbacks to do it is very interesting.
We tested it on one of our large projects and it works great, did not see any negative/side effect, thanks!

Update: we also hit the webform submission issue (thankfully from our tests) as mentioned in comments 163 - 179
I created an issue in webform and proposed a fix there #3492193: Incorrect use of 'render element' for webform_submission_data
Is my assumption correct that this is an incorrect usage of render element?

Here is a static patch for 10.3.x, based on MR 8334.

The webform submission issue can be solved by applying #3492193: Incorrect use of 'render element' for webform_submission_data on webform 6.2.x, or updating to 6.3.x.
Adding revision and language to the key like MR 4994 does would make sense I think.

I've been hitting this lately where a specific content type has a media field that's an icon, after search_api indexes a few nodes with the same icon it starts hitting the ye olde "Recursive rendering detected when rendering entity media: [media_id]" error. Changing the number of items per index batch from the default of 50 to 5 or 10 doesn't resolve the problem, it still runs into it after it hits 20 occurrences per referenced media entity.

#189 I also encountered this issue during search_api indexing on a project with lots of languages (>20).
Because search_api processes each translation 1 by 1 and renders the rendered_item (Rendered HTML output) field which renders a view mode that contains a media.

Rerolled the 11.1.x MR changes against 11.1.7.

MR for 11.x is 2000+ commits behind and unmergable

MR for 11.x is 2000+ commits behind and unmergable

MR probably hasn't been touched in a year or so and has needed work since before that. It's a difficult challenge and open for anyone to implement a good solution.

Amending attribution.

Been having this issue.

I can recreate by creating a queue, adding all nodes, and rendering them. Most of my content types use a taxonomy vocab and probably like 90% use the default term. That default term also has a media attached to it.

Code isn't too bad,

- a large amount of nodes
- load them,
- then,

        $view_builder = \Drupal::entityTypeManager()->getViewBuilder('node');
        $render_array = $view_builder->view($node, 'default');
        $output = \Drupal::service('renderer')->renderInIsolation($render_array);

After like 25 nodes, it starts throwing errors. After banging my head, found https://www.drupal.org/project/search_api/issues/2913931 which lead me to this issue.

@jonathanshaw in #183 has the best, most succinct description,

> we don't have a recursive limit, we have a repeated rendering limit.

Languages is one thing, but definitely not the only thing to consider. We need a better mechanism to map the references.

The MR4994 is re rolled for 11.x and it's green.
I hope we can merge it soon and back port it to Drupal 10 (there is a MR for that but it's targeting 10.0.x).

Not sure about this but I've changed the deprecation notice to removals happening in Drupal 12 (after reading #177 and because Drupal 11.0.0 that was being mentioned is now old).

I rerolled the patch from #155 for Drupal 10.5.2

Latest patch for 10.5.3 works for me.

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.

Test coverage is in place. Here is output of test-only test:

PHPUnit 11.5.22 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.12
Configuration: /builds/issue/drupal-2940605/core/phpunit.xml.dist
...............F........                                          24 / 24 (100%)
Time: 00:38.859, Memory: 8.00 MB
There was 1 failure:
1) Drupal\Tests\media\Kernel\MediaEmbedFilterTest::testRecursionProtection
Failed asserting that actual size 0 matches expected size 1.
/builds/issue/drupal-2940605/core/modules/media/tests/src/Kernel/MediaEmbedFilterTest.php:387
FAILURES!
Tests: 24, Assertions: 654, Failures: 1, PHPUnit Deprecations: 150.
Exiting with EXIT_CODE=1

Just added code comments in a review. Not sure why the other tests are not appearing in the output of the test-only test? The code looks good at first glance, but I am investigating why the tests not failing. Any ideas?

I think there are still outstanding previous comments (don't remember them all offhand) about possible issues with revisions, languages, and some specific contrib entity types such as webform submissions. Those likely need to be investigated before putting back into NR.

@godotislate Okay, thank you for the info.

So, I gave it some time and here are my updates:

• Webform is no longer an issue, actually it never was. There was a bug in theme hook implementation, which has been fixed in #3492193: Incorrect use of 'render element' for webform_submission_data
• Some tests were NOT failing with the existing approach. However they should have. Apparently, tests were affected by the caching. I have changed tests to use a non cacheable view mode. So they correctly fail with the existing approach and are green with the new one
• While running test locally I saw that the new protection allowed for a single recursion, but should not have. After hours of trials and errors I figured out that Drupal might instantiate more than one view builder class, i.e. with separate $recursionKeys property. In order to avoid that, I've changed it to static one

There is behavior change in recursive rendering: the old method allowed up to 20 levels of recursion, while the new one does not allow a single one. It would be super weird if anyone has relied on that side effect.

In the end: the new approach correctly detects recursion and does not allow for it, while allowing rendering of the same entity multiple times and other similar things, or the same entity in different languages/revisions etc

@taran21 which MR were you reviewing. Per #181 there's two competing approaches here.

@jonathanshaw I was working/looking at 4994 (I guess you have MR activity turned off so you do not see any new commits )

it requires a manual rebase now, and one more test is not optimal. I plann to take a look today and hopefully move to needs review

I believe we should add language and revision id to the recursion keys.

I've added revision ID + language to the keys (not sure those are needed actually, but they don't harm I think)

I think this is ready. Even if it's not perfect, it's a big step up from the current approach which is badly broken for use cases like bulk emailing or search indexing that can require multiple rendering of the same entity in a single request.

Added a comment to the MR - I think this MR has a decent chance of introducing a random rendering issue.

Just looking at how object ID's are re-used - see https://3v4l.org/cCB6r#v8.4.14

I think we're okay here as we would not expect an object that we're rendering to be destructed while we are rendering.

@alexpott, yeah - you are 100% right. I thought that those IDs are large numbers .. for whatever reason.

Anyways, thanks for the review. Would be greta to have it in soon(ish)

Discussed this issue with @catch. I think this change is too late for 11.3.x. Given we're altering the behaviour of entity rendering I want to be a bit cautious here and commit this early to 11.x to give it sometime for people to find any problems the change causes. I can't see anything problematic but with rendering unexpected side-effect are quite common so this will be give it time for us to find them.

I've added a couple of MR comments to address before we can commit this to 11.x. Once they've been addressed this can go back to rtbc.