Plugin blocks cannot set their own attributes when put in the layout

Agreed with the concerns of #4

I've recreated/rerolled the patch against 8.9.x as it wasn't working for 8.8.x-rc1.

I tested the latest patch and it solves my problem. It would be great to have that merged.

We are facing missing attributes placed by entity_class_formatter module implementation (using hook_entity_view_alter() to set attributes), when custom blocks are inserted into Layout Builder. As a workaround, I created a patch to transfer missing attributes, but it's not really a general solution: https://www.drupal.org/project/entity_class_formatter/issues/3108054

Patch inside comment #7 failed only because the test block plugin was in the wrong folder.
Here is a fixed patch against 8.9.x.

Thanks for the patch!

1. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -124,9 +124,29 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      if ($content !== NULL && !Element::isEmpty($content)) {
   

   The result of BlockPluginInterface::build() is always an array, and isEmpty() is already called above, this whole line can be

   if (!$is_content_empty) {
   

2. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -124,9 +124,29 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +        // Place the $content returned by the block plugin into a 'content' child
   +        // element, as a way to allow the plugin to have complete control of its
   +        // properties and rendering (for instance, its own #theme) without
   +        // conflicting with the properties used above, or alternate ones used by
   +        // alternate block rendering approaches in contrib (for instance, Panels).
   +        // However, the use of a child element is an implementation detail of this
   +        // particular block rendering approach. Semantically, the content returned
   +        // by the plugin "is the" block, and in particular, #attributes
   +        // is information about the *entire* block. Therefore, we must move this
   +        // property from $content and merge them into the top-level element.
   

   This comment needs to be rewrapped. References to specific contrib modules should be removed. Mixing styles like "is the" and *entire* is odd, neither are found in core AFAIK.

3. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -124,9 +124,29 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +        foreach (['#attributes'] as $property) {
   

   Why the foreach loop for a single property? Is this expected to be expanded later?

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -580,6 +580,34 @@ public function testPluginDependencies() {
   +   * Tests if plugins can provide custom attributes, just like outside of the layout_builder.
   

   Shorten to 80 char. Remove everything after the , and clarify that it is testing a block plugin?

Thank you Tim for your guidance! Firstly it was only a quick fix.

I'm also thinking about merging the existing values there (if available), rather than destroying them. What do you think?

  if (isset($content['#attributes'])) {
-   $build['#attributes'] = $content['#attributes'];
+   $build['#attributes'] = array_merge_recursive($build['#attributes'], $content['#attributes']);
    unset($content['#attributes']);
  }

Please include an interdiff when making changes to an existing patch, to help others review your changes.

This issue seems similar to #3135899: Layout Builder does not seem to work with Entity Class Formatter and #3072231: Custom blocks break layout builder module - Quick Edit could not associate the rendered entity field markup, perhaps they can be combined?

Just a combo patch of this issue and this one: https://www.drupal.org/project/drupal/issues/3020876

Both change the same lines, so uploaded combo patch here for composer. Please ignore, has no bearing on the issue at hand. Just need it now.

Patch #13 is successfully applied for Drupal 9.1.

@kyberman, It would be helpful for reviewing if you will update interdiff.

Combining both the patches here in case someone needs it.
https://www.drupal.org/project/drupal/issues/3020876#comment-14051592

I think we can take forward the other issue and may be close to this one.

Adding NorthAmerica2021 and Easy Out of the Box tags for visibility.
DrupalCon NA is April 12-16 with a focus on EOOTB on Wednesday, April 14.
Thanks

Just a re-roll against the latest 9.2.x.

Quickly rerolled the patch for 9.3, tests probably needs adapting since a lot on this has changed in the other issue

The merge request applies cleanly on 9.3.0. Not sure what is needed to get this to RTBC.

Oh, this is confusing. The MR is missing some of the code from the patches. Please update the MR to contain everything needed, and stop posting new patches. Thanks!

As mentioned in #29 the MR seems to be missing some files. And there is an open thread on the 1 file added.

I've removed duplciate for this line below.

$build['content'] = $content;

Besides, the tests from #13 are already in drupal core 9.3.x, so that's why you don't see new files in the MR.

I've also created a static patch for those who does not like when patch from MR does not apply anymore due to new commits.

So if the tests were already added and they didn't catch this bug then I think the tests may not have been covering this bug. So we will need a failing test case to show this is still an issue I believe.

@smustgrave, you can check #3077734-2: Plugin blocks cannot set their own attributes when put in the layout comment, there are two patches, first patch contains only tests without the fix and it failed, second patch contains the fix + tests and it successed.

Would you expect to have one more test scenario that will fail, something like this?

$assert_session->elementNotExists('css', '.attribute-test-class');
$assert_session->elementNotExists('css', '[custom-attribute=test]');
$assert_session->elementNotExists('css', 'div[data-contextual-id*="layout_builder_test"]');

So if the fix is not working then test will be passed.

I'm not sure what else we can add here

I think #2 was just overlooked. Setting it back to NR for that matter.

The test-only patch from #2

One file is already in drupal core
The other is not but also contains deprecated function calls.

The MR and patch #34 do not contain any tests. Would expect a full patch to contain the tests also.

@smustgrave, you can find that the tests exist and already commited in 9.2.x, 9.3.x almost 2 years ago.

#3020876-68: Contextual links of reusable content blocks are not displayed when rendering entities built via Layout Builder,

that's why you can't see tests in the MR.

Then the tests weren't covering this bug or they would be failing on without this fix. So either this is a non issue now or we need new tests.

Indeed, it's very confusing issue due to mix from #3020876: Contextual links of reusable content blocks are not displayed when rendering entities built via Layout Builder.

See #3020876-29: Contextual links of reusable content blocks are not displayed when rendering entities built via Layout Builder, now I'm surprised that this issue even exists, seems like it should be fixed in #3020876.

Thoughts?

Can you see if the fix from that one solves the problem here? Could be the same issue and just 2 different approaches being taken.

Posted in the #bugsmash channel on slack too

It's indeed seems like a duplicate of #3020876: Contextual links of reusable content blocks are not displayed when rendering entities built via Layout Builder, the only difference is that here we have an extra condition

if (!$is_content_empty) { }

Honestly, that extra condition does not affect on functionality, at least the tests that cover block attributes change from #3020876: Contextual links of reusable content blocks are not displayed when rendering entities built via Layout Builder are passed.

I would closed this as duplicate, but I need one more confirmation of this theory

Looking at this again I do think it's a duplicate. Closing this out for the other and will move over credit.

Thanks!