Layout builder does not protect against recursion

@samuel.mortenson didn't know you were working on this 😉

Including my fix and test and your test.

comedy of errors today 🎭

I also forgot group.

Just removing some debug code I left in.

Not sure why Drupal\Tests\layout_builder\Kernel\LayoutBuilderFieldLayoutCompatibilityTest::testCompatibility is failing

This mitigation presumably works for core plugins, but what about contrib? If it's not mitigated by a contrib plugin that has the issue, then I think this'd be a major bug.

Bumping to major per #2977309-8: Adding a view that will render the current entity to Layout Builder causes a WSOD and closing that as a duplicate.

Here's a reroll of #9.

The test failures for Drupal\Tests\layout_builder\Kernel\LayoutBuilderFieldLayoutCompatibilityTest::testCompatibility and Drupal\Tests\layout_builder\Kernel\LayoutBuilderInstallTest::testCompatibility are due to assertFieldAttributes() being called on the same entity multiple times in a the same Kernel test. It renders the entity as part of the test, and since those calls are part of the same request, the recursion counter increases and prevents rendering on any calls after the first.

I'm not yet sure how to best address this. It could be resolved by converting the tests to Functional, but I'd like to be sure there isn't a more straightforward option before doing that much refactoring and slowing down the tests.

If performance is that much of a concern, the test could use reflection to make the counter public and reset it.

I rerolled the patch and fixed the test failures, which is uploaded here.

I then checked to see if the tests added by this issue still failed without the fix. They do not fail locally - adding a test only patch to confirm that is the case overall. It's not yet clear if the tests need to be rewritten or if this issue was fixed as a result of another recent change. Sharing the results here to see if someone may be aware of a recent commit that may have addressed this.

In this patch:

1. Identified and addressed what needed to be changed so both the Functional and Kernel tests would fail without the rest of the patch (largely consisted of explicity enabling layout builder for the display being tested.
2. Discovered the current patch did not have protection for the Views use case in the Functional tests, so I added recursion protection logic for views blocks in Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray
3. In manual testing I discovered the recursion testing can potentially trigger an error in the quickedit integration, and added a safeguard to prevent it

Patch in #20 no longer applies.

It would be good to re-upload a test-only patch as well since the one in #20 never ran.

Rerolled patch from 20

Next patch for fix it

I could apply the patch, so no reroll is needed

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -483,6 +507,48 @@ protected function getDefaultSection() {
   +    if ($entity instanceof RevisionableInterface) {
   +      $render_id_values['%revision_id'] = $entity->getRevisionId();
   +    }
   

   Why revision? I don't believe LB uses revisions this way. Also, why not use the getTempstoreKey() method on section storage? That takes into account language, which this doesn't.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -483,6 +507,48 @@ protected function getDefaultSection() {
   +      $error = 'Recursive rendering detected when rendering layout:';
   ...
   +      $this->getLogger()->error("$error. Aborting rendering.", $render_id_values);
   

   Which logger is this? Also, while exception messages aren't translated, I believe logged messages are supposed to be.

3. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -137,7 +157,53 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      if ($block instanceof ViewsBlock) {
   +        $this->checkForViewsBlockRecursion($block, $bypass_build);
   +      }
   +
   +      if (!$bypass_build) {
   +        $event->setBuild($build);
   +      }
   +
   

   Instead of embedding Views-specific logic here, what about another event subscriber dedicated to this? And if recursion is detected, it stops the event from propagating?

4. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -137,7 +157,53 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +    list($is_displaying,) = explode(':', $row_plugin->getPluginId(), 2);
   

   I don't believe the trailing comma is necessary. Also instead of using list(), could dereference the explode with [0]

5. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -137,7 +157,53 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +    if ($is_displaying == 'entity') {
   

   === please

6. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -137,7 +157,53 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +            \Drupal::logger('system')->error($error);
   

   Is this a system error? Why not use the LB logger (and inject it?)

7. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -129,7 +129,11 @@ public function entityViewAlter(array &$build, EntityInterface $entity, EntityVi
   +      // Recursion protection makes it possible for there to be a $section
   +      // without a '#layout' key. Skip when this is the case.
   +      if (!isset($section['#layout'])) {
   +        continue;
   +      }
          if (!Element::isEmpty($section)) {
   

   This is no longer needed after #3065474: An error occurs when viewing an entity after removing all layout sections and Quickedit module enabled.

8. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutRecursionTest.php
   @@ -0,0 +1,71 @@
   +class LayoutRecursionTest extends BrowserTestBase {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static $modules = [
   

   Nit, missing a blank line and this should be protected

9. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutRecursionTest.php
   @@ -0,0 +1,71 @@
   +  protected $defaultTheme = 'classy';
   

   Please rewrite the test to use stark

10. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutRecursionTest.php
    @@ -0,0 +1,71 @@
    +   * @throws \Behat\Mink\Exception\ResponseTextException
    

    Don't usually see this in test methods, why is it here?

11. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutRecursionTest.php
    @@ -0,0 +1,71 @@
    +    $assert_session->pageTextContains("List all nodes");
    

    Nit: switch to single quotes

12. +++ b/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderCompatibilityTestBase.php
    @@ -48,7 +49,7 @@ protected function setUp() {
    -    $this->config('system.theme')->set('default', 'classy')->save();
    +    $this->config('system.theme')->set('default', 'classy')->save();  $this->config('system.theme')->set('default', 'classy')->save();
    

    Remove this, please

13. +++ b/core/modules/layout_builder/tests/src/Kernel/SectionComponentRecursionTest.php
    @@ -0,0 +1,79 @@
    +  public static $modules = [
    

    Nit: protected

Added fix for 5-13 notices from #27
Still needs work on 1-4

Reapplying #28 patch. solving error of

error: cannot apply binary patch to 'modules/contrib/admin_toolbar/admin_toolbar_tools/src/Plugin/.DS_Store' without full index line
error: modules/contrib/admin_toolbar/admin_toolbar_tools/src/Plugin/.DS_Store: patch does not apply"

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -482,6 +506,48 @@ protected function getDefaultSection() {
   +      $this->getLogger()->error("$error. Aborting rendering.", $render_id_values);
   

   Another instance of $this->getLogger() that should be replaced with an injected logger, and the string translated.

2. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -33,14 +36,41 @@ class BlockComponentRenderArray implements EventSubscriberInterface {
   +  public function __construct(AccountInterface $current_user, LoggerInterface $logger) {
        $this->currentUser = $current_user;
   +    $this->logger = $logger;
   

   Tests are failing because a new argument was added to the constructor without a default value. An example of how to do this can be found at \Drupal\layout_builder\Controller\ChooseBlockController::__construct, look at the $current_user variable is implemented.

let me pick up this....

Please review this as this addresses above comment.

Covered points mentioned in #32, kindly review a new patch.

• +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
  @@ -482,6 +506,49 @@ protected function getDefaultSection() {
  +      $error = $this->t("$error. Aborting rendering.");
  

  t() is not automatically available in this class. It can be made available with Drupal\Core\StringTranslation\StringTranslationTrait;

  Also, this function should not be concatenating a string with variables, use placeholders instead.
  https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Rend...

• +++ b/core/modules/layout_builder/layout_builder.services.yml
  @@ -42,7 +42,7 @@ services:
  +    arguments: ['@current_user', '@logger.channel.layout_builder']
  
  +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
  @@ -33,14 +36,45 @@ class BlockComponentRenderArray implements EventSubscriberInterface {
  +  public function __construct(AccountInterface $current_user, LoggerInterface $logger = NULL) {
  

  This change looks good. Some of the test failures in the patch are because the unit tests need to be updated so the new LoggerInterface arg is taken into account. Instances of new BlockComponentRenderArray(), need the logger argument added.

I have covered point first of #36, and also fixed coding issues of patch.

Needs to include logger argument in instances of new BlockComponentRenderArray() in failed test files.

Added logger variable in test files, lets see if test got pass.

Thanks @swatichouhan012

+++ b/core/modules/layout_builder/tests/src/Unit/BlockComponentRenderArrayTest.php
@@ -60,6 +60,7 @@ protected function setUp() {
     $this->account = $this->prophesize(AccountInterface::class);
+    $logger = $this->container->get('logger.channel.layout_builder');

The logger should be added similar to how $this->account is added. It will look something like $this->prophesize(LoggerInterface::class)

When working with tests, you may find it easier to set up tests locally instead of waiting over an hour for DrupalCI to run the full test suite. A unit test like this will run in seconds. Here's addiitonal info on running tests from the command line. If you have PHPStorm, integrating PHPUnit tests makes it even easier to test locally.

Solving failed test cases.

+++ b/core/modules/layout_builder/tests/modules/layout_builder_recursive_test/config/install/views.view.list_all_nodes.yml
@@ -0,0 +1,165 @@
+#core: 8.x

+++ b/core/modules/layout_builder/tests/modules/layout_builder_recursive_test/layout_builder_recursive_test.info.yml
@@ -0,0 +1,8 @@
+core: 8.x

Must be removed

Removed core: 8.x from #49 as mentioned in #50, please review.

Working on this.

Here I have tried to fix failed tests.

Currently, I have one menu that's placed twice on the page (a problem on its own, but that's what I have at the moment). The menu also uses Menu Item Extras so that we can build a mega menu with one of the instances. Left alone, the setup would generate the warnings because:

1. Menu Item Content instances (the menu links) generate the same $recursive_render_id towards the same thing, instead of two independent renders of the link from two separate instances of the menu.
2. The Menu Item Extras's use of the render twig filter to check for an empty render array. This also generates the same $recursive_render_id and counts towards the same thing, should the render filter encounter that same link.

In total, I would get 4 warnings for just one menu item (2 links x 2 twig render). Do that for each menu link in the menu, and my db log is flooded with warnings. The first case (separate renders counted the same) is something that could easily be done with other entities via other methods (e.g. two views rendering the same entity with the same display). And the use of render in twig is a known workaround in themes.

I have not found a solution on my end. This may be a non-issue even (I could be doing it wrong?). Just posting my findings.

Closed #3069367: Unable to use layout builder - consumes all memory as a duplicate

Re-roll against 9.3.6 ...

Re-roll against #62. please review it.

Patch did not apply. Here is the re-roll. Please review.

I had patch #65 already applied to my project and I applied patch #8 from this ticket https://www.drupal.org/project/drupal/issues/2940605#comment-14687218 and they seemed to conflict.
After applying the new patch, the LB protection mechanism provided by patch #65 was blocking all the rendering.
After removing patch #65 I was able to do batch rendering without any error log or message.

I apply the patch $65 but I have this issue on browser:

Fatal error: Allowed memory size of 536870912 bytes exhausted (tried to allocate 32768 bytes) in /var/www/html/web/core/lib/Drupal/Core/Utility/Error.php on line 1

I rerolled and fixed these items:
- Core module quickedit has been removed https://www.drupal.org/node/3252839

In a site I am managing, patches 65 and 69 causes Paragraphs using Layout Builder in their view mode to not render their content.

I fixed some test issues here

Als @dk-massive, following the task steps and last patch I got fixed this issue.

I fixed another test issues

Fixing some another testing issues.

Fixing Cs Issue #75

Test failures.

Fixed test issue.

fix testing issue

fix test. adjust test to use prophesize for logger service.

trigger_error messages will have to be updated. Currently says D9

Also a change record will be needed for the new parameter in the service.

About to trigger_error message there is one a core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php that is:

      @trigger_error('The logger.channel.layout_builder service must be passed to BlockComponentRenderArray::__construct(), it is required before Drupal 9.0.0.', E_USER_DEPRECATED);

As I see says "It is required before Drupal 9.0.0" I don't think is needed to change the core version to 10 in this message. D9 is still in production, also the message says "before...".

9.0.0 is no longer supported. Hasn’t been for some time.

Also can’t be made required in a minor release which this would be. So would have to be done in a major release. So if this trying to get in for 10.1 or would have to be triggered in 10.1 and required in 11

Ok, @smustgrave sounds logic.

I modified the @trigger_error message.

About "change record" for the service. I think this should be added to this list manually https://www.drupal.org/list-changes/drupal

after or at the time that this issue merged.

Thanks but still needs a change record that will be added to the trigger_error.

Thought there was a check for it but the trigger_error follows a pattern

Example

   if ($extension_list_theme === NULL) {
      @trigger_error('Calling ' . __METHOD__ . ' without the $extension_list_theme argument is deprecated in drupal:10.1.0 and will be required in drupal:11.0.0. See https://www.drupal.org/node/3284397', E_USER_DEPRECATED);
      $extension_list_theme = \Drupal::service('extension.list.theme');
    }

Ahh, thanks @smustgrave for the guides.

I think was fixed. Pleas review.

Unfortunately missed the 10.1 for deprecation. If those can be updated fir 10.2 please.

The patches #90 fixed the problem for me.

Patch no longer applies to 10.3.x.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

Fixed some of the issues related to pipeline and added a commit for the same MR. Still some of the tests are getting failed, keeping the issue status as unchanged.
Thanks!!.

Last patch does not apply to 10.3. Attaching one for 10.3 (I need it).

Re-upping patch from #104 with new files included.

MR patch for 11.2.x support