Regression: invalid HTML in menu blocks due to _block_get_renderable_block() clobbering #theme_wrappers of block contents

Yeah, I've seen this problem in the committed patch already.

We have a similar problem elsewhere in the page rendering process for a long time already: #1473548: CSS classes in $page['#attributes']['class'] are not applied to HTML BODY

Essentially, we need to detach 1) the inner content from 2) the outer/wrapping element.

I guess the solution is as simple as applying this fix wherever the block render array is merged:

 $build = array(
   '#type' => 'block',
 );
-$build += $plugin->blockRender();
+$build['content'] = $plugin->blockRender();

Alternatively, we could as well introduce the assumption that the top-level render array level always pertains to the wrapping block element, which in turn would allow callbacks to adjust the #title, #attributes, and so on of the wrapping block template. In turn, every blockRender() callback would be required to always return the actual render elements as sub-elements and never on the top-level; i.e.:

-$build = array(
+$build['foo'] = array(
   '#theme' => 'foo',
 );
 return $build;

That way, we'd open up the door for block plugins to easily declare or override variables for the wrapping block template; e.g.:

+$build['#title'] = t('@dynamic title', array('@dynamic' => 'Custom, dynamic'));
+$build['#attributes'] = array(
+  'role' => 'navigation',
+  'class' => array('foo-block-trigger'),
+);
 $build['foo'] = array(
   '#theme' => 'foo',
 );
 return $build;

Which in turn would resolve issues like #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks

I think I fully support sun's suggestion here. Currently there are a few wtfs around this in that if you were to say, return a form, you have to return array($form) instead of just return $form; and I think sun's suggestion would remove the need for that as well.

Eclipse

Thinking some more about it, I think we'd ideally do a mixture of both of #2:

Wherever the stuff is invoked/merged/output:

 $build = array(
   '#type' => 'block',
   '#title' => $plugin->definition->get('label'),
 );
+$build = $plugin->prepareBlock();
+$build = $plugin->buildBlockWrapper($build);

-$build += $plugin->blockRender();
+$build['content'] = $plugin->buildBlockContent();

i.e.: Instead of pursuing the disparity further, let's cleanly separate it into:

1) Prepare the block [for rendering]. Allow it to load data and prepare the data.

2) Allow the block plugin to perform adjustments for the wrapping block thingie; e.g., #title, #attributes, #whatever. Data that has been prepared in prepareBlock() is available in custom/private properties and can be used for conditional/dynamic values.

3) Make the block own the render array that it returns; i.e., nothing is merged into what buildBlockContent() returns. Thus, return $form; works exactly how it is supposed to work. Data that has been prepared in prepareBlock() is available in custom/private properties and can be used to generate the block content.

For example:

class NodeBlock {

  protected $blockNode;

  public function prepareBlock() {
    $this->blockNode = node_load($this->config->get('nid'));
  }

  public function buildBlockWrapper($build) {
    $build['#title'] = check_plain($this->blockNode->label());
    return $build;
  }

  public function buildBlockContent() {
    return $this->blockNode->render($this->config->get('view_mode')); // I wish... ;)
  }

}

#1871696: Convert block instances to configuration entities to resolve architectural issues introduces a render controller and rewrites all of this code. It will be pretty straightforward to write a test and fix with that in place.

Modified approach of #4, post BlockRenderController.

Whoops.

Ah, missed a spot.

+      $build[$entity_id]['content'] = $content;

Hmm... This essentially implements #2.

However, in light of our actual goal of #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks, I still think it would be better if we'd

1) properly detach the wrapper building from the inner content building

2) inherently allow blocks to load and prepare any data they need (for the wrapper and/or the inner build)

3) call the separate methods to populate the render array accordingly; e.g., like this:

  $entity->getPlugin()->prepare();
  // Defaults should come from BlockPluginBase::buildWrapper().
  $build[$entity_id] = $entity->getPlugin()->buildWrapper();
  // Defaults should come from BlockPluginBase::build().
  $build[$entity_id]['content'] = $entity->getPlugin()->build();

Which is essentially #4, and IMHO would move us really really close to the architecture we're aiming for in #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks.

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockHtmlIdTest.php
@@ -37,15 +32,14 @@ public static function getInfo() {
-    // Create an admin user, log in and enable test blocks.
-    $this->adminUser = $this->drupalCreateUser(array('administer blocks', 'access administration pages'));
-    $this->drupalLogin($this->adminUser);
+    $this->drupalLogin($this->drupalCreateUser(array('administer content types')));

Hat tip: If the user permissions are actually irrelevant for a test, you can skip the entire drupalCreateUser() procedure and just simply do this instead:

$this->drupalLogin($this->root_user);

That logs in uid 1. Only available in Web Tests.

The plugin doesn't care about its wrapper yet. If you want it to, that's definitely a follow-up.
I read #2 and #4, and while both solve our problem, #4 doesn't map to how Block plugins currently function. Seems like a nice to have.

So I went with #2, and it works great.

Thanks for the tip about root_user! That's the only change made here.

Ah, missed openid_block_view_user_login_block_alter().

assuming it passes tests, RTBC

Eclipse

Committed and pushed to 8.x. Thanks!