Handle block rendering's #attributes correctly (fixes in-place editing of custom blocks)

Why is it important for the attributes to be on the outer element?

+++ b/core/modules/block/block.module
@@ -487,13 +487,13 @@ function template_preprocess_block(&$variables) {
+  // rendered entity. Setting #attributes on an entity means that it should be
+  // set on the entity's container, which is in this case the block's container.

I don't get that. The "custom block" entity is rendered inside block.html.twig's {{ content }}. Therefore, the entity's container can be either something internal to that (for example, if we chose to have a custom_block.html.twig file as we do for all our other content entities), or any wrapper around that provided by block.html.twig. Why is the nearest wrapper not sufficient, and what is it about the outer wrapper that makes things magically work?

a custom block is also a block... Unfortunately, the custom block entity is rendered as a child of the block entity.

Yeah. That's the heart of the problem. How about this then?

Since this is pretty brittle stuff, can you also add a test?

Or maybe even this?

Moved the test from custom_block to block, some other code around, and documented the reasoning in BlockViewBuilder, since someone will surely bang their head on this again.

I read through the patch twice and nothing jumps out to me as "the line that fixes the bug", but that's possibly because the test and test plugin are moved. Can someone upload a "test only" version so we can easily see what is failing?

+++ b/core/modules/block/block.module
@@ -487,16 +469,6 @@ function template_preprocess_block(&$variables) {
-  // The block template provides a wrapping element for the content. Render the
-  // #attributes of the content on this wrapping element rather than passing
-  // them through to the content's #theme function/template. This allows the
-  // content to not require a function/template at all, or if it does use one,
-  // to not require it to output an extra wrapping element.
-  if (isset($variables['content']['#attributes'])) {
-    $variables['content_attributes'] = NestedArray::mergeDeep($variables['content_attributes'], $variables['content']['#attributes']);
-    unset($variables['content']['#attributes']);
-  }
+++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
@@ -43,24 +45,53 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+        // 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 (e.g., its own #theme) without
+        // conflicting with the properties used above, or alternate ones used
+        // by alternate block rendering approaches in contrib (e.g., 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 and #contextual_links is information about the *entire*
+        // block. Therefore, we must move these properties from $content and
+        // merge them into the top-level element.
+        if (isset($content['#attributes'])) {
+          $build[$key]['#attributes'] = NestedArray::mergeDeep($build[$key]['#attributes'], $content['#attributes']);
+          unset($content['#attributes']);
+        }

This is the essential change in the patch. The rest is just some code reorganization to improve clarity and put related things together.

+++ /dev/null
@@ -1,56 +0,0 @@
-    $this->assertRaw('id="block-test-id-block"', 'HTML ID for test block is valid.');
+++ b/core/modules/block/lib/Drupal/block/Tests/BlockHtmlTest.php
@@ -0,0 +1,61 @@
+    $this->assertFieldByXPath('//div[@id="block-test-html-block" and @data-custom-attribute="foo"]', NULL, 'HTML ID and attributes for test block are valid and on the same DOM element.');

And this is the essential test for it, obscured by the rename of the test and supporting files from "HtmlId" to "Html", since it's no longer just about the ID (actually, even in HEAD, the test tests more than just ID).

And to sum up #9, essentially, we're changing the rendering of the #attributes that are attached to the render array returned by BlockPluginInterface::build() from being rendered on the DIV that wraps just the {{ content }} in block.html.twig to the DIV that wraps the entire block.

The code comment in #9 tries to explain this, but I'm wondering if this is sound reasoning both for core, and for modules like Panels.

The alternative was #5, but Wim disagrees with only doing it for Edit module attributes (see #6).

+++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
@@ -35,7 +36,8 @@ public function view(EntityInterface $entity, $view_mode = 'full', $langcode = N
-    foreach ($entities as $entity_id => $entity) {
+    foreach ($entities as $key => $entity) {
+      $entity_id = $entity->id();

This threw me off. When is $key not the entity ID?

Always. See HEAD's BlockViewBuilder::view() and HEAD's EntityViewBuilder::view().

Minor fixes, plus uploading just the tests and just the non-test code separately, in addition to combined.

The first is due to a change in the base class of the test block. The second is due to #contextual_links being merged to the top-level element earlier in the pipeline. This should fix both.

+++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
@@ -43,24 +44,51 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+            $build[$key][$property] += $content[$property];

+++ b/core/modules/block/block.module
@@ -487,16 +469,6 @@ function template_preprocess_block(&$variables) {
-    $variables['content_attributes'] = NestedArray::mergeDeep($variables['content_attributes'], $variables['content']['#attributes']);

My only worry is that this used to be NestedArray::mergeDeep, and now its a +=. Should we switch to that just to be safe?

@tim.plunkett

+++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
@@ -43,24 +44,51 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+          '#attributes' => array(),

It looks to be initialized to an empty array before the union so I don't think there is a case that needs the deepMerge. Maybe I'm wrong?

If $content['#contextual_links'] contains anything other than 'block', it won't be merged in.

So I think what's being said here is "Let the specific implementation handle attributes and contextual links as it pleases." If that's what you're proposing, then I am ++. Robustness of new implementations (and thus, overridability and/or implementation specific alterations) is definitely the goal. I'd argue that this probably gives panels et. al. more room to do what they want, not less.

Eclipse

Thanks for the feedback @EclipseGc, but not quite. There's no functional change in this patch with respect to contextual_links: only a movement of the code from _block_get_renderable_region() to inside BlockViewBuilder, so that does give Panels more room, since it can swap out BlockViewBuilder.

With respect to #attributes though, they key functional change is that in HEAD, the $build array returned by the plugin is presumed to describe the content of the block only, and therefore the #attributes is rendered on the nearest wrapper DIV of that content. It also means that the Block entity and the Block plugin have different elements they can each separately set #attributes on, and each gets rendered on a different DIV. But with this patch, while the plugin is still free to set #attributes on the $build that it returns, the meaning of that property is changed to now be assumed to be attributes of the entire block, not just its content. Which means the Block entity takes those attributes and moves them to its own element.

So in a way, it's taking some control *away* from the plugin (it's saying that while the plugin can have a say in what the attributes are, it can't have a say about what DIV element they get rendered on). In a way, that was also true in HEAD given the code in template_preprocess_block(), but that arguably was its own bug that could have been fixed with minimal effort, but with this patch, the lack of control over where is being instituted by design.

On the other hand, you could look at it as more control, in that with the patch, the plugin is able to return attributes for the top-level block DIV, whereas in HEAD, it can't.

Yeah, still think I'm ok with this.

Eclipse

It looks to be initialized to an empty array before the union so I don't think there is a case that needs the deepMerge. Maybe I'm wrong?

That's exactly right. By moving the merge from template_preprocess_block() to BlockViewBuilder, we go from not knowing the initial state to knowing it, so can use the simpler union operator.

If $content['#contextual_links'] contains anything other than 'block', it won't be merged in.

Huh? The new code is doing a + operation, same as the equivalent code removed from _block_get_renderable_region(). Possibly you meant the opposite: that if the plugin returns a 'block' key in #contextual_links, then that won't get merged in? That would be true, but that's the same as HEAD, and I think ok, since block.module owns that namespace.

Thanks for the detailed reviews, Tim. Can I assume that since you're focusing on that, that you're ok with the basic premise of #25?

Oh yeah, I'm +1 to the whole patch and what you outlined in #25. I'm just focusing on minutiae that might bite us later, like += vs mergeDeep.

But the mergeDeep was on #attributes, which as you said, is now initialized as empty. So I think we're okay.

Yep, I think we're good here.

git ac https://drupal.org/files/issues/block_attributes-2171269-19.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 17433  100 17433    0     0  10598      0  0:00:01  0:00:01 --:--:-- 17662
error: patch failed: core/modules/block/lib/Drupal/block/BlockViewBuilder.php:43
error: core/modules/block/lib/Drupal/block/BlockViewBuilder.php: patch does not apply
error: patch failed: core/modules/block/tests/modules/block_test/config/block.block.test_block.yml:4
error: core/modules/block/tests/modules/block_test/config/block.block.test_block.yml: patch does not apply
error: patch failed: core/modules/system/system.module:9
error: core/modules/system/system.module: patch does not apply

re-rolled

fixed previous re-roll

changed status

35: block_attributes-2171269-35.patch queued for re-testing.

Committed/pushed to 8.x, thanks!

+++ b/core/modules/block/block.module
@@ -501,16 +483,6 @@ function template_preprocess_block(&$variables) {
-  // The block template provides a wrapping element for the content. Render the
-  // #attributes of the content on this wrapping element rather than passing
-  // them through to the content's #theme function/template. This allows the
-  // content to not require a function/template at all, or if it does use one,
-  // to not require it to output an extra wrapping element.
-  if (isset($variables['content']['#attributes'])) {
-    $variables['content_attributes'] = NestedArray::mergeDeep($variables['content_attributes'], $variables['content']['#attributes']);
-    unset($variables['content']['#attributes']);
-  }
+++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
@@ -44,24 +45,51 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+        // 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 (e.g., its own #theme) without
+        // conflicting with the properties used above, or alternate ones used
+        // by alternate block rendering approaches in contrib (e.g., 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 and #contextual_links is information about the *entire*
+        // block. Therefore, we must move these properties from $content and
+        // merge them into the top-level element.
+        foreach (array('#attributes', '#contextual_links') as $property) {
+          if (isset($content[$property])) {
+            $build[$key][$property] += $content[$property];
+            unset($content[$property]);
+          }
+        }
+        $build[$key]['content'] = $content;

Both of the above hunks (the code removed and the code added by this patch) result in #2486267-10: Attributes of a block content are applied to block itself (well, prior to this patch, the attributes were moved to a different <div>, but still the same point that it got moved away from the <form>). If anyone here has input on how to resolve that, please comment on that issue. Thanks!

Re, the unintentional version change in #43: I filed a d.o. issue: #2772953: 8.0.x is missing from Drupal core's issue queue as a "version" option for past issues.