Fix and add tests for the recursive rendering protection of the 'Rendered entity' formatter

Maybe better change the param to @target_type, and also add a space before the (@entity_id)

There's no other @target_type occurence in core, so maybe it's better to stick with @entity_type. But I agree that we should have a space between @entity_type and (@entity_id).

Looks good now, thanks.

This shows a lack of test coverage; could we add a quick test for this condition?

> could we add a quick test for this condition?

Yes.

WIP.

In the new test when I try to set the ER field, the value is not saved properly, even though it seems that the reference is correct. Furthermore, if the reference is incorrect I would expect ValidReferenceConstraintValidator to kick in, but it seems it is never called.
What am I missing here?

Test is failing due to #2078155: Access protected field items being removed, so waiting for that patch to land first.

The RecursiveRenderingException exception class has been removed in #2404021: entity_reference formatters should be in Core, now we simply abort the rendering and log a message when the limit is reached.

Here's a fresh patch with tests and everything :)

Rerolled. Any chance for a review? :)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
   @@ -122,15 +136,21 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +        static::$recursiveRenderDepth[$recursive_render_id] = isset(static::$recursiveRenderDepth[$recursive_render_id]) ? ++static::$recursiveRenderDepth[$recursive_render_id] : 1;
   

   That's a strange way to write this:

   $variable = ++$variable;

   It works but I'm not even sure exactly why and in what order it does what.

   Can't we write that as $variable = $variable + 1? That's IMHO a lot easier to understand and basically what you are doing.

   or make a proper if/else and assign 1 or do $variable++.

2. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php
   @@ -199,6 +200,34 @@ public function testEntityFormatter() {
   +    // The entity title is printed by default so we have to multiply the
   +    // limit by 2.
   +    $expected_occurences = EntityReferenceEntityFormatter::$recursiveRenderLimit * 2;
   +    $actual_occurences = substr_count($output, $referencing_entity->name->value);
   +    $this->assertTrue($actual_occurences <= $expected_occurences);
   

   We can make sure that we have at least a certain amount of matches? Or an exact match? This would happily pass if for some reason we wouldn't see the label at all, like when being on the wrong page or so.

Thanks for reviewing!

1. Sure, let's do a proper if.
2. I've no idea why I wrote it like that in the first place, switched to an equality check.

Patch looks ready to me. Just a minor suggestion.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
@@ -29,6 +29,13 @@
+  public static $recursiveRenderLimit = 20;

In views_field_view this is a configurable setting. Can we add this as a setting to this formatter?

I don't think that's useful nor related to this issue, we're just fixing a bug.

Yeah if you find a usecase for a level of 20, you can also replace the plugin implementation with your own.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
   @@ -29,6 +29,13 @@
   +  public static $recursiveRenderLimit = 20;
   

   There doesn't seem to be a reason to be public.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
   @@ -122,15 +136,27 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +        if (isset(static::$recursiveRenderDepth[$recursive_render_id])) {
   +          static::$recursiveRenderDepth[$recursive_render_id]++;
   +        }
   +        else {
   +          static::$recursiveRenderDepth[$recursive_render_id] = 1;
   +        }
   

   We could simplify this by using static::$recursiveRenderPath += [$recursive_render_id => 0]; and then skip the if/else

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
   @@ -122,15 +136,27 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +          $this->loggerFactory->get('entity')->error('Recursive rendering detected when rendering entity @entity_type @entity_id. Aborting rendering.', array('@entity_type' => $entity->getEntityTypeId(), '@entity_id' => $entity->id()));
   

   out of scope of this issue: this sounds like a trigger_error for me.

4. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php
   @@ -199,6 +200,34 @@ public function testEntityFormatter() {
   +    $expected_occurences = EntityReferenceEntityFormatter::$recursiveRenderLimit * 2;
   

   We can extract the value of this using one of the technique like anonymous function binding of some reflection instead.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
@@ -29,6 +29,13 @@
+  public static $recursiveRenderLimit = 20;

IMHO this should also be a constant, shouldn't it?

#20.1, 4, #21: No real preference, changed to a constant if you think it's better.

#20.2: Nope, I tried and that doesn't work :)

#20.3: No opinion here as well.

Yeah now that it is constant, we don't need it anymore.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
@@ -122,15 +136,27 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+        $recursive_render_id = $items->getFieldDefinition()->getTargetEntityTypeId()
+          . $items->getFieldDefinition()->getTargetBundle()
+          . $items->getName()
+          . $entity->id();
...
+          $this->loggerFactory->get('entity')->error('Recursive rendering detected when rendering entity @entity_type @entity_id. Aborting rendering.', array('@entity_type' => $entity->getEntityTypeId(), '@entity_id' => $entity->id()));

I might be mistaken but I can;t see where in the test that we're testing the static caching of recursive depth by a recursive_render_id. And if we're using all this information to count by is it not useful in the log message?

I might be mistaken but I can;t see where in the test that we're testing the static caching of recursive depth by a recursive_render_id.

The fact that the recursive render limit is taken into account is the one that proves the new static caching by a recursive_render_id is working, otherwise the test-only patch from #11 wouldn't have failed. But it doesn't hurt to expand the test coverage a bit and check with a second entity.

And if we're using all this information to count by is it not useful in the log message?

Good point, added the information about the field and bundle name to the log message.

The feedback from #25 has been addressed, so we should be good for RTBC again? :)

Sure.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
   @@ -36,6 +43,13 @@ class EntityReferenceEntityFormatter extends EntityReferenceFormatterBase implem
   +   * Counter for the recursive rendering protection.
   +   *
   +   * @var array
   +   */
   +  protected static $recursiveRenderDepth = [];
   

   It would be nice to explain why its an array.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
   @@ -122,15 +136,35 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +        if (isset(static::$recursiveRenderDepth[$recursive_render_id])) {
   +          static::$recursiveRenderDepth[$recursive_render_id]++;
   +        }
   +        else {
   +          static::$recursiveRenderDepth[$recursive_render_id] = 1;
   +        }
   

   This could be replaced by two lines of code: static::$recursiveREnderDepth += [$recursive_render_id => 0]; static::$recursiveREnderDepth++

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
   diff --git a/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php
   index cc41f7d..1a6f10f 100644
   
   index cc41f7d..1a6f10f 100644
   --- a/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php
   
   --- a/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php
   +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php
   

   This wont' apply to 8.2.x, so let's ensure to create a patch against that.

Looks good to me, but needs a re-roll.

#30.1: Done.
#30.2: You mentioned that before in #20, but I tried it and it didn't work.
#30.3: Done as well, this patch now applies cleanly to both 8.1.x and 8.2.x.

Committed 95dc914 and pushed to 8.1.x and 8.2.x. Thanks!