Remove the tight coupling between Block Plugins and Block Entities

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -53,7 +54,7 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
+        'subject' => check_plain($configuration['label']),

So the $entity->label() is no longer used as the label anywhere? That differs from every other entity, config or otherwise.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -123,8 +123,7 @@ public function getPlugin() {
+        $this->instance = drupal_container()->get('plugin.manager.block')->createInstance($this->plugin, $this->getExportProperties());

getExportProperties is only used by CMI to find the properties to write to file, I don't know that it is appropriate to use here.

+++ b/core/modules/node/node.moduleundefined
@@ -2030,7 +2030,8 @@ function node_form_block_form_alter(&$form, &$form_state) {
+  $configuration = $block->getConfig();

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -486,7 +486,8 @@ function overlay_block_access($block) {
+    $configuration = $block->getConfig;

I *really* wanted to remove all calls to getConfig(). I don't know why we should ever need it.

+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -77,7 +77,7 @@ public function form($form, &$form_state) {
-    $this->entity->set('label', filter_xss_admin($this->view->getTitle()));
+    $this->configuration['label'] = filter_xss_admin($this->view->getTitle());

If this works, I'll be happy.

From IRC:

and while the argument comes off like architectural purity (I admit, that is totally what it sounds like when I listen to my own argument) it's also not difficult to achieve and would shut me the hell up.

:D

So, to that end, I'm trying to grok this patch. Here's a few spots where some docs would clarify things:

+++ b/core/modules/block/block.moduleundefined
@@ -327,7 +327,8 @@ function _block_get_renderable_region($list = array()) {
+        '#entity' => $block,
+        '#block' => (object) $block->getPlugin()->getConfig(),

+++ b/core/modules/node/node.moduleundefined
@@ -2027,7 +2027,8 @@ function node_form_block_form_alter(&$form, &$form_state) {
+  $configuration = $block->getConfig();
+  $visibility = $configuration['visibility'];

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -486,7 +486,8 @@ function overlay_block_access($block) {
+    $configuration = $block->getConfig();
+    if (!in_array($configuration['region'], $regions_to_render)) {
       return FALSE;
     }

I've found the getConfig() method to seem a bit redundant and confusing, and we've discussed removing it in the past. This patch seems to add back a lot of uses of it. Maybe that's the point (because it's how you're decoupling the block from its storage, through use of that method), but what's the status of those other related issues about that method?

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,28 @@ public function submit($form, &$form_state) {
+      // Label needs special handling.
+      $build['#block']->label = check_plain($this->configuration['label']);

Why does it need special handling? Is that referring to the check_plain()?

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -73,15 +37,12 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+      list(, $name) = explode('.', $entity->id());

Can we slap a comment on here that says "Extract the entity ID machine name from the configuration object ID" or whatever? The comment wasn't there beforehand, but we're rewriting the whole hunk here already.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -123,8 +123,16 @@ public function getPlugin() {
+        $this->instance = drupal_container()->get('plugin.manager.block')->createInstance($this->plugin);

Let's use the new Drupal::service() pattern here since we can now. :)

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -138,6 +146,14 @@ public function getPlugin() {
+   * Overrides \Drupal\Core\Entity\EntityInterface::label().
+   */
+  public function label($langcode = NULL) {
+    $configuration = $this->getPlugin()->getConfig();
+    return $configuration['label'];
+  }

So it seems like, in general, we're switching things up so that the entity wraps the block API instead of vice versa. Conceptually, I guess that makes sense.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -159,14 +175,42 @@ public function get($property_name, $langcode = NULL) {
+      // Support geting plugin specific settings with the get() method.

Nitpick: "plugin-specific settings" (with a hyphen).

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -159,14 +175,42 @@ public function get($property_name, $langcode = NULL) {
-   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::getExportProperties();
+   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::set();
...
+  public function set($property_name, $value) {
+    if (!in_array($property_name, $this->exportProperties()) && $property_name != 'settings') {
+      $settings = $this->get('settings');
+      $settings[$property_name] = $value;
+      $this->getPlugin()->setConfig($property_name, $value);
+      parent::set('settings', $settings);
+    }
+    elseif ($property_name == 'settings' && is_array($value)) {
+      $settings = $this->get('settings');
+      foreach ($value as $key => $key_value) {
+        $this->getPlugin()->setConfig($key, $key_value);
+        $settings[$key] = $key_value;
+      }
+      parent::set('settings', $settings);
+    }
+    else {
+      $this->getPlugin()->setConfig($property_name, $value);
+      parent::set($property_name, $value);
+    }

This looks like a really interesting/important hunk of code. We're overriding the setter? That seems unusual, so let's describe why.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -159,14 +175,42 @@ public function get($property_name, $langcode = NULL) {
+  protected function exportProperties() {
+    return array(

Docs, my brother.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,28 @@ public function submit($form, &$form_state) {
+  public function buildBlock() {
+    $build = array();

Docs please. Also, shouldn't it be blockBuild() instead of buildBlock(), or otherwise something tat is unambiguously different from blockBuild()?

@@ -44,12 +44,7 @@ function testBlockThemeHookSuggestions() {
     $variables = array();
-    $variables['elements']['#block'] = $block;
-    $variables['elements']['#block_config'] = $block->getPlugin()->getConfig() + array(
-      'id' => $block->get('plugin'),
-      'region' => $block->get('region'),
-      'module' => $block->get('module'),
-    );
+    $variables['elements'] = $block->getPlugin()->buildBlock();
     $variables['elements']['#children'] = '';
     // Test adding a class to the block content.

So, this change is causing the test to fail with Drupal style.

After this change, the block isn't built properly because the menu API is returning an empty menu to SystemMenuBlock::buildBlock(). Specifically, in menu_tree_page_data() on line 1204, menu_tree_get_path() is called with the path never having been set, so it overwrites the menu name with NULL in the following hunk:

// Check if the active trail has been overridden for this menu tree.
1204   $active_path = menu_tree_get_path($menu_name);
1205   // Load the menu item corresponding to the current page.
1206   if ($item = menu_get_item($active_path)) {

That seems unstable to me, and the test passes when I make the following change:

diff --git a/core/includes/menu.inc b/core/includes/menu.inc
index 3b68bf0..7568c89 100644
--- a/core/includes/menu.inc
+++ b/core/includes/menu.inc
@@ -1201,7 +1201,8 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
   $language_interface = language(LANGUAGE_TYPE_INTERFACE);
 
   // Check if the active trail has been overridden for this menu tree.
-  $active_path = menu_tree_get_path($menu_name);
+  $active = menu_tree_get_path($menu_name);
+  $active_path = !empty($active) ? $active : $menu_name;
   // Load the menu item corresponding to the current page.
   if ($item = menu_get_item($active_path)) {
     if (isset($max_depth)) {

Well, I'm specifically getting the block system ready to move to FormInterface which does formBuild() and so I was following that paradigm, which I still think is a good idea. I'm open to suggestions here, but that's what I was going for, and I'll admit to having confused myself a few times already wondering whether I'd named it blockBuild() or buildBlock(). I think one of the earlier patches even has it wrong.

Right, but we had the pattern of access() -> blockAccess(), etc. So that means we now have one method that doesn't follow that pattern while the rest do.

Yes, and I think this could expose some other issues here, with metadata and xss and translation, but this seemed the most logical way to handle it w/o it exploding into a bigger issue for the time being.

We definitely need a more thorough explanation in the comment, then, and maybe an @todo and followup issue?

<out_of_scope>
Is there any chance we could at least give getConfig()/setConfig() different names in a followup, or something? There's been an @todo in the patch to that end since December, and even after three months, I still don't understand why there are those two methods AND the settings() method on top of the annotation. I also still think it's weird for the admin label to be set in getConfig().

The issue around all that is #1764380: Merge PluginSettingsInterface into ConfigurableInterface; all the comments there are relevant as well.
</out_of_scope>

1. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
   @@ -486,7 +489,8 @@ public function buildBlock() {
   +      // The label is the only string we actually render, so we need to handle
   +      // it specially.
          $build['#block']->label = check_plain($this->configuration['label']);
   

   Ah, this makes more sense. Let's add an @todo and followup issue here for translation and sanitization for this?

2. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
   @@ -497,4 +501,14 @@ public function buildBlock() {
   +   * Returns a plugin specific render array.
   

   plugin-specific :) Though I'd actually suggest:
   Builds the renderable array of the block's content.

3. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
   @@ -187,6 +187,19 @@ public function get($property_name, $langcode = NULL) {
   +   * The plugin configuration is the canonical source for any configuration
   +   * within the plugin. The plugin does not care about the storage mechanism,
   +   * so the entity must maintain both its own properties as well as the
   +   * configuration of the plugin.
   +   *
   +   * Any property that exists in Block::exportProperties() can be handled by
   +   * the parent method, with the exception of the settings method. Any property
   +   * that is not documented in Block::exportProperties() is to be considered an
   +   * internal configuration property of the plugin and should be maintained on
   +   * the entity within the settings property. Setting of the settings property
   +   * directly is supported, but all set calls to the settings property are
   +   * addative and will not remove settings values.
   

   This helps a lot, thank you.

   Nitpicks: "All set() calls" should have parens, and "additive" is misspelled.

4. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
   @@ -209,6 +222,9 @@ public function set($property_name, $value) {
   +   * An array of keys of propeties, of this entity, that should be exportable.
   

   How about "Returns a list of properties to export for this entity." Then, the docblock also needs an @return, which is where you document that it's an array of property keys.

5. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
   @@ -159,14 +175,58 @@ public function get($property_name, $langcode = NULL) {
   +    if (!in_array($property_name, $this->exportProperties()) && $property_name != 'settings') {
   +      $settings = $this->get('settings');
   +      $settings[$property_name] = $value;
   +      $this->getPlugin()->setConfig($property_name, $value);
   +      parent::set('settings', $settings);
   +    }
   +    elseif ($property_name == 'settings' && is_array($value)) {
   +      $settings = $this->get('settings');
   +      foreach ($value as $key => $key_value) {
   +        $this->getPlugin()->setConfig($key, $key_value);
   +        $settings[$key] = $key_value;
   +      }
   +      parent::set('settings', $settings);
   +    }
   +    else {
   +      $this->getPlugin()->setConfig($property_name, $value);
   +      parent::set($property_name, $value);
   +    }
   +  }
   

   Let's get a couple inline comments here too.

Thanks @EclipseGc!

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -198,17 +198,23 @@ public function get($property_name, $langcode = NULL) {
+      // If the property is not a property of the entity, assume it belongs in
+      // the settings property of the entity, and set the plugin configuration
+      // value appropriately.
       $settings = $this->get('settings');
       $settings[$property_name] = $value;
       $this->getPlugin()->setConfig($property_name, $value);
       parent::set('settings', $settings);
     }
     elseif ($property_name == 'settings' && is_array($value)) {
+      // If we are updating the settings property and the value passed was an
+      // array, add each of the key value pairs to the plugin configuration and
+      // set the entity's 'settings' property.
       $settings = $this->get('settings');
       foreach ($value as $key => $key_value) {
         $this->getPlugin()->setConfig($key, $key_value);
@@ -217,13 +223,17 @@ public function set($property_name, $value) {

@@ -217,13 +223,17 @@ public function set($property_name, $value) {
       parent::set('settings', $settings);
     }
     else {
+      // If we're updating a property of the entity, just add the value to the
+      // plugin configuration as well.

Reading this, I still don't understand why. For now, maybe leave it and someone who understands the problem space better than I can help clarify the comments.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -217,13 +223,17 @@ public function set($property_name, $value) {
+   * @return array

You still need to describe the return structure here. :) http://drupal.org/node/1354#return

ok, bit of a review.

in some respects, i think this patch makes the logic between blocks and their config even a bit more circuitous...but that's because it's a step on the way towards decoupling things, and that means a little more abstraction. so yeah, i think we're probably headed in a good direction.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -123,8 +123,16 @@ public function getPlugin() {
-        $this->instance = drupal_container()->get('plugin.manager.block')->createInstance($this->plugin, $this->settings, $this);
-        $this->settings += $this->instance->getConfig();
+        $properties = $this->getExportProperties();
+        unset($properties['settings']);
+        $this->instance = \Drupal::service('plugin.manager.block')->createInstance($this->plugin);
+        $this->instance->getConfig();
+        foreach ($properties as $key => $value) {
+          $this->instance->setConfig($key, $value);
+        }
+        foreach ($this->settings as $key => $value) {
+          $this->instance->setConfig($key, $value);

Ugh.

Though still probably an improvement over the circular situation where the entity contains info about getting the plugin, and the plugin then also contains the entity.

also, are the assorted calls to getExportProperties() enough to cause any kind of appreciable slowdown? it's not the cheapest way to access this config information.

in any case, it seems like this is a step in the right direction.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -159,14 +175,68 @@ public function get($property_name, $langcode = NULL) {
+   * The plugin configuration is the canonical source for any configuration
+   * within the plugin. The plugin does not care about the storage mechanism,
+   * so the entity must maintain both its own properties as well as the
+   * configuration of the plugin.
+   *
+   * Any property that exists in Block::exportProperties() can be handled by
+   * the parent method, with the exception of the settings method. Any property
+   * that is not documented in Block::exportProperties() is to be considered an
+   * internal configuration property of the plugin and should be maintained on
+   * the entity within the settings property. Setting of the settings property
+   * directly is supported, but all set() calls to the settings property are

i get the general reasoning here, but am having difficulty understanding specifically why the 'settings' property is special. it doesn't help that the initial line, "The plugin configuration is the canonical source for any configuration within the plugin" is almost a self-referential definition.

also - s/settings method/settings property/ in the second paragraph.

+++ b/core/modules/block/block.moduleundefined
@@ -327,7 +327,8 @@ function _block_get_renderable_region($list = array()) {
+        '#block' => (object) $block->getPlugin()->getConfig(),

@@ -534,7 +536,7 @@ function block_rebuild() {
-  $variables['block'] = (object) $variables['elements']['#block_config'];
+  $variables['block'] = (object) $variables['elements']['#block'];

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,44 @@ public function submit($form, &$form_state) {
+        '#block' => (object) $this->getConfig(),

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.phpundefined
@@ -44,12 +44,7 @@ function testBlockThemeHookSuggestions() {
+    $variables['elements']['#block'] = (object) $block->getPlugin()->getConfig();

These (object) casts are obviously hacks, and we should keep track of them in some way, like an @todo or something.

+++ b/core/modules/block/block.moduleundefined
@@ -534,7 +536,7 @@ function block_rebuild() {
-  $variables['block'] = (object) $variables['elements']['#block_config'];
+  $variables['block'] = (object) $variables['elements']['#block'];

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,44 @@ public function submit($form, &$form_state) {
+        '#block_config' => array(
+          'id' => $this->configuration['plugin'],
+          'region' => $this->configuration['region'],
+          'module' => $this->configuration['module'],
+          'label' => check_plain($this->configuration['label']),

Now we can ditch #block_config completely! Yay.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,44 @@ public function submit($form, &$form_state) {
+      // @Todo: Give the implications of this line further thought in #1941244.

@todo, lowercase with no colon.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,44 @@ public function submit($form, &$form_state) {
+   * Builds the renderable array of the block's content.
+   *
+   * @return array
+   *   A renderable array representing the content of the block.
+   *
+   * @see \Drupal\block\BlockRenderController
+   */
+  abstract public function blockBuild();

Is this in the interface? If not, why not? If so, it doesn't need to be here.

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -23,42 +23,6 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
-    // @todo \Drupal\block\Tests\BlockTest::testCustomBlock() assuemes that a
-    //   block can be rendered without any of its wrappers. To do so, it uses a
-    //   custom view mode, and we choose to only add the wrappers on the default
-    //   view mode, 'block'.
-    if ($view_mode != 'block') {
-      return array();

Where is this logic being handled now?

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -23,42 +23,6 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
-      '#block' => $entity,
-      '#weight' => $entity->get('weight'),
-      '#theme_wrappers' => array('block'),
-      '#block_config' => array(
-        'id' => $entity->get('plugin'),
-        'region' => $entity->get('region'),
-        'module' => $entity->get('module'),
-        'label' => check_plain($entity->label()),

I'm not sure how I feel about this being moved out of here. Ideally we'd be moving toward EntityRenderController, not away from it (we currently only use the interface, not the base class)

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -159,14 +175,69 @@ public function get($property_name, $langcode = NULL) {
+      // If the property is not a property of the entity, assume it belongs in
+      // the settings property of the entity, and set the plugin configuration
+      // value appropriately.
+      $settings = $this->get('settings');
+      $settings[$property_name] = $value;
+      $this->getPlugin()->setConfig($property_name, $value);
+      parent::set('settings', $settings);
+    }
+    elseif ($property_name == 'settings' && is_array($value)) {
+      // If we are updating the settings property and the value passed was an
+      // array, add each of the key value pairs to the plugin configuration and
+      // set the entity's 'settings' property.
+      $settings = $this->get('settings');
+      foreach ($value as $key => $key_value) {
+        $this->getPlugin()->setConfig($key, $key_value);
+        $settings[$key] = $key_value;
+      }
+      parent::set('settings', $settings);
+    }
+    else {
+      // If we're updating a property of the entity, just add the value to the
+      // plugin configuration as well.
+      $this->getPlugin()->setConfig($property_name, $value);
+      parent::set($property_name, $value);

This whole thing is still a huge mess of crazy. I'm not sure how okay I am with this going in. It doesn't have any @todo's, and I can't imagine anyone volunteering to clean it up otherwise.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -179,11 +250,25 @@ public function getExportProperties() {
+    $this->settings = array_diff_key($this->getPlugin()->getConfig(), $this->getExportProperties());

Inline comment, please. I always have to think about what these array functions do, so some simple language explaining the intent more than the action would be good.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.phpundefined
@@ -35,14 +35,7 @@ public function __construct(array $namespaces) {
     $this->discovery = new CacheDecorator($this->discovery, 'block_plugins:' . language(LANGUAGE_TYPE_INTERFACE)->langcode, 'cache_block', CacheBackendInterface::CACHE_PERMANENT, array('block'));
-  }
-
-  /**
-   * Overrides \Drupal\Component\Plugin\PluginManagerBase::createInstance().
-   */
-  public function createInstance($plugin_id, array $configuration = array(), Block $entity = NULL) {
-    $plugin_class = DefaultFactory::getPluginClass($plugin_id, $this->discovery);
-    return new $plugin_class($configuration, $plugin_id, $this->discovery, $entity);
+    $this->factory = new DefaultFactory($this);

This is the only part of the patch I truly love :) Yay for this.

Is this in the interface? If not, why not? If so, it doesn't need to be here.

Oooh, ooh! I know the answer!

The idea here is that this method is not part of the public interface for interacting with blocks--it's an internal method for block plugins to add their own customized build content without having to re-add the wrappers each time. (We are using the same pattern for other methods on this class as well.) So making it abstract here forces child blocks to declare it, without cluttering the public interface.

This whole thing is still a huge mess of crazy. I'm not sure how okay I am with this going in. It doesn't have any @todo's, and I can't imagine anyone volunteering to clean it up otherwise.

Yeah, this is really my feeling about that hunk as well.

Ah, it's only called internally, it should be protected. That would make sense, since interfaces cannot house protected methods.

Ah, it's only called internally, it should be protected. That would make sense, since interfaces cannot house protected methods.

I bet this is true of the other blockWhatsit() methods as well. Might be interesting to change them to protected and make sure nothing blows up. Edit: Out of scope here; let's file a novice issue.

Ok, I found this patch pretty difficult to comprehend, and I haven't read any of the comments here yet, so feel free to let me know if I'm misunderstanding what the code is doing.

Calling code:

>
-function custom_block_block_view_alter(array &$build, Block $block) {
+function custom_block_block_view_alter(array &$build, BlockInterface $block) {
   // Add contextual links for custom blocks.
-  if ($block->getPlugin() instanceof CustomBlockBlock) {
+  if ($block instanceof CustomBlockBlock) {

This is an improvement.

>
   if ($regions_to_render = overlay_get_regions_to_render()) {
-    if (!in_array($block->get('region'), $regions_to_render)) {
+    $configuration = $block->getConfig();
+    if (!in_array($configuration['region'], $regions_to_render)) {
       return FALSE;
     }
   }

This is a regression. What's config mean on a block? I just want to know the region.

>
-        '#block' => $block,
+        '#block' => (object) $block->getPlugin()->getConfig(),

Another regression. This is putting quite a burden on the calling code, having to know that much about your object.

Implementations:

>
   /**
-   * Implements \Drupal\block\BlockBase::build().
+   * Implements \Drupal\block\BlockBase::blockBuild().
    */
-  public function build() {
+  public function blockBuild() {
     return array(drupal_get_form('search_block_form'));
   }

It may just be naming, but as it is, the distinction/relationship between build and blockBuild makes no sense.

Internals:

This patch removes the Entity dependency from BlockBase, so instead, the plugin manager's factory returns an unconfigured block. It's now up to the caller to make it usable by calling setConfig, with whatever input it has. In the case of Entity\Block, it's coming from the config entity's properties.

I'm not crazy about the factory being so generic that all it can do is return an unusable object. That may be too much of an abstraction. In the case where a block would need to be setup with data from somewhere other than a config entity, and thus not discoverable by block.module, as long as it implements BlockInterface, it could do whatever it wanted with its own factory. I don't see that as a huge limitation.

Whatever this abstraction buys us though, it's really coming at the cost of complexity within Entity\Block.

>
  /**
   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::get();
   */
  public function get($property_name, $langcode = NULL) {
    // The theme is stored in the entity ID.
    $value = parent::get($property_name, $langcode);
    if ($property_name == 'theme' && !$value) {
      list($value) = explode('.', $this->id());
    }
    elseif (!in_array($property_name, $this->exportProperties())) {
      // Support geting plugin-specific settings with the get() method.
      $settings = parent::get('settings');
      if (isset($settings[$property_name])) {
        $value = $settings[$property_name];
      }
    }
    return $value;
  }

This is super ugly.

>
   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::set();
   *
   * The plugin configuration is the canonical source for any configuration
   * within the plugin. The plugin does not care about the storage mechanism,
   * so the entity must maintain both its own properties as well as the
   * configuration of the plugin.
   *

This is just adding complexity and making the relationship even harder to reason about.

>

  /**
   * Implements \Drupal\Core\Entity\EntityInterface::save().
   */
  public function save() {
    $this->settings = array_diff_key($this->getPlugin()->getConfig(), $this->getExportProperties());
    return parent::save();
  }

Ouch.

I can't say I'm terribly happy with the current state, as the relationship of Entity and Plugin to Blocks is very confusing, but I'm not sure the complexity introduced in this patch is worth what it buys us in flexibility.

Right now, we typehint with Drupal\block\Plugin\Core\Entity\Block in certain places, and that is not good, since it precludes other entities from using block plugins (think Display Suite or Panels). But when I look again, there is only one place that it is a problem: the BlockBase::__construct()/BlockManager::createInstance() pairing. And AFAIK, that's the part that irks @EclipseGc most.

The rest is hook_block_view_alter(), block.module or custom_block.module menu callbacks, or the Block entity controllers, which is all actually fine.

So to enable other modules or even core to properly access a block plugin, we just need a common interface and a PluginBag.

This is a WIP and I need to add tests to help illustrate my approach. Please don't tear it apart just yet.

Is #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin a dupe of this?

Also, thanks to a couple helpful souls and Tim's proof-of-concept there, I'm finally understanding what the real concerns are here. This issue needs to have a higher priority.

#66 looks pretty good

#66: block-1927608-66.patch queued for re-testing.

Needed reroll because of #1875182: Pass plugin definition to plugin instances instead of discovery object.

This may not solve every concern of architectural purity, but it unblocks contrib or core displays from being able to use block plugins at all. Without this, Display Suite or Panels or any other module cannot use block plugins.

This would enable us to have stuff like

class KeyValueBlockContainer implements BlockContainerInterface
class PanelsPane implements BlockContainerInterface
class DisplaySuiteThingie implements BlockContainerInterface

and we can flesh out the interface once we have more implementations. This removes the coupling to Entity, and allows SCOTCH to make progress with displays.

#75 makes the code more clear and adds flexibility, so it seems like an obvious win over what we have currently.

Yes, I spent about 4 hours today discussing this. @msonnabaum and @merlinofchaos also tried reasoning with you.

You say that the patch doesn't suit your needs, fine, write some tests that fail with it.

This introduces three main @todos.

- Block visibility is controlled by the plugin, but vertical tabs seem to be bound to being top-level elements of the form.
There are some workarounds to make this all work.
@EclipseGc says this isn't a big problem, since block visibility is changing completely anyway.

- Bartik and Overlay attempt to manipulate the block plugin based on the region it is placed in.
This introspection should not be allowed, according to sdboyer.

Thankfully/sadly, neither of the bartik/overlay hacks have test coverage, so this doesn't "break" anything.

EDIT: In case anyone thinks I can't count, Overlay and Bartik were #2 and #3 :)

Not a full review, but a quickie... (Hooray for you two being on the same page btw!)

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -554,10 +554,10 @@ function aggregator_filter_xss($value) {
- * Implements hook_preprocess_HOOK() for block.tpl.php.
+ * Implements hook_preprocess_HOOK() for block-plugin.tpl.php.

+++ b/core/modules/block/block.moduleundefined
@@ -88,6 +89,14 @@ function block_help($path, $arg) {
+    'block_plugin' => array(
+      'variables' => array(
+        'content' => NULL,
+        'configuration' => NULL,
+        'plugin_id' => NULL,
+      ),
+      'template' => 'block-plugin',

At a glance, I don't like this at all. It's pushing implementation details way too far down the stack. Why should a themer have to care that blocks are plugins? They just need to know they're blocks. Is this absolutely required for this change? If so, why?

Also, I don't quite follow all the back and forth above, but bear in mind the phase of the release cycle we're in right now. It's fine to make incremental steps towards a vision, but each of those steps need to move core forward on its own, independently. We can't be introducing big changes that require dozens of follow-up issues to be complete anymore. From glancing at the patch, I don't believe this to be the case here, but the end of #80 scared the crap out of me, so figured I'd reiterate that. ;)

The naming of that is admittedly crap. Also, the new hooks hook_block_plugin_view_alter/hook_block_plugin_access are due to the same naming. (which have some @todos in the api.php docs, if you could fill them in @EclipseGc)

That is a bikeshed for #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin. But the idea is that "block plugin" needs a new name, since it will be used by both block and display entities.

And yes, part of the reason I abandoned my idealistic and high-level approach was because it would be very follow-up heavy. This should theoretically allow SCOTCH to stop caring about blocks altogether...

Attached is also a patch excluding whitespace changes, for easier reviewing. The file is almost the same size, but the diffstat is in the 400s, not 600s.

EntityAccessController is bizarre, NodeAccessController has to work around them in the same exact way. I'll open an issue for that and link it here.
Also, I added docs for those hooks anyway.

Any other changes needed better make this patch smaller, I'm not going over 100K!

Whoops, didn't rebase properly, that included the last two MAINTAINERS.txt commits... :\

#92: 1927608-92.patch queued for re-testing.

EDIT: Random test failures.

i've talked this through with Kris - while this is kinda orthogonal o what's happening in princess, this shouldn't hamper it overmuch. the most salient issue continues to be *what* we inject into plugins - an array, or an entity.

this code goes the route of hiding the entity details from the plugin by making the entity simply report its relevant settings as an array, but still having external code interact with the entity. to a large extent, this is unavoidable in our current context as we still need to interact with the entities from the outside in order to keep blocks, as they exist right now, working.

as we've discussed all this at length, i've found it convincing that we could, and should, treat ConfigEntities like a dumb CRUD bag we are, of course, discovering elsewhere that some of the contracts inherent to entities are incompatible with this (ComplexDataInterface, TypedDataInterface, as well as D8MI wanting to decorate entities... etc.) - but i'm hopeful that we'll disentangle the simpler CRUD required for ConfigEntities from these more traditional entity constructs.

so, on princess, i've gone the route of ALWAYS injecting entities, but inverting the block plugin manager's factory-ish patterns so that calling code never/barely touches the entity - only the plugin. this addresses what has always been my primary concern, which is that i felt it confusing to force calling code to interact with configuration for a thing like it is the thing itself. i do feel that this is a preferable approach in general, and have included a set of interfaces (ConfigEntityAwarePluginInterface, EmbeddedPluginConfigInterface, ConfigDrivenMapperInterface). the biggest reason for this is that it gives us the *option* (as opposed to the requirement, like now) of having standalone blocks (i.e, not embedded in a single display) for free.

some of the separations in here are a bit uncomfortable (for example, the idea of two separate access hooks, one on the block entity and one on the block plugin, makes no sense in the context of SCOTCH), but i don't see anything that isn't pretty easily changed as needed. so, we can keep on rolling.

Okay, here it is with the method names swapped to match all of the other foo()/blockFoo() pairs. Consistency++

The key thing though is we need to leave core in a shippable state with every patch we commit from here out, and

 /**
- * Implements hook_preprocess_HOOK() for block.tpl.php.
+ * Implements hook_preprocess_HOOK() for block-plugin.tpl.php.
  */
-function aggregator_preprocess_block(&$variables) {
-  if ($variables['block']->module == 'aggregator') {
+function aggregator_preprocess_block_plugin(&$variables) {
+  if ($variables['configuration']['module'] == 'aggregator') {

That's not a shippable state.

If that means postponing this on #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin then so be it.

To clarify, what's not shippable is:

s/block.tpl.php/block-plugin.tpl.php/g

I don't care about the internals of those preprocess functions as much, as they should affect relatively few people.

In IRC we discussed:

s/block-plugin.tpl.php/block-wrapper.tpl.php/g

This works for me. It's analogous to comment-wrapper.tpl.php and keeps block.tpl.php matching with what node.tpl.php and comment.tpl.php and others do.

I think you meant

s/block.tpl.php/block-wrapper.tpl.php
s/block-plugin.tpl.php/block.tpl.php

Will work on that, and confirm in IRC

Yes, sorry. I want block.tpl.php and node.tpl.php and comment.tpl.php to all look roughly the same. Sounds like #103 gets us there. Woohoo! :)

I fully expect this to fail somewhere, but I jsut wanted to post something.

Okay, went through the test coverage, fixed the obvious stuff. Will need to see the results.

One more pass...

                  __________
              _.-'|         \,._
           .-'    |         |   ~'=-.
        .-'       |         |       |
        | ___ []  | _______ |  []   |___.-----.
        ||___|    ||       ||       |___|__(*=/
        |_..._____||_______||.._____|   \##*----=
        [__        \__|||__,"     _/
           ((-))______--_____((-))
           | I |             | I |
           |   |             |   |
           || ||             || ||
           || ||             || ||    ____
           ((-))             ((-))   | ___\_
          -|===|=============|===|---|O___|_
           |   |             |   |   |____/
           /O.O\             /O.O\
          `====='           `====='
          -"^-^"-           -"^-^"-

big +1 to webchick's request, as defined in #103, and glad that we went for that. should've raised it in my response, as well. i've got a 'block-ng' template in princess, but only as a way of letting things temporarily coexist without breakages while we convert things over. it's functionally more equivalent to what had originally been the "block-plugin.tpl.php"; i see no reason to think that what is now "block-wrapper.tpl.php" will continue to exist in over there.

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.phpundefined
@@ -0,0 +1,220 @@
+  /**
+   * Test configuration and subsequent form() and build() method calls.
+   *
+   * This test is attempting to ensure the existing block plugin api and all
+   * functionality that is expected to remain consistent. The arrays that are
+   * used for comparison can change, but only to include elements that are
+   * contained within BlockBase or the plugin being tested. Likely these
+   * comparison arrays should get smaller, not larger, as more form/build
+   * elements are moved into a more suitably responsible class.
+   */
+  public function testBlockInterface() {

I think it would be better to use a test plugin here, rather than the user block, to decouple the test from a specific implementation. Otherwise we have to maintain this relative to that plugin.

What are we testing for the absence of?

(Still reviewing.)

No, it'd be a lot better if the test plugin were explicit about what it's testing. For example, a block that exposed a simple "Hello $foo" string, and had a config form to change the value of the string, or whatever. Because who's online does a lot, I can't tell what of those form properties are actually integral to making sure the architecture works, and what of those are just crap form api barfs in.

I think both xjm and effulgentsia wanted to give this a spin before marking it RTBC.

i see no reason to think that what is now "block-wrapper.tpl.php" will continue to exist in over there.

I see no reason for it to exist here either then. Why do we need a template and view_alter() hook for the entity at all? How about just hook_block_view_alter() and theme('block'), both invoked by the plugin, and that's it?

Two reasons:

1. We need it for zebra striping.
2. I was under the impression that hook_entity_view() and hook_entity_view_alter() were part of the contract of EntityRenderControllerInterface.

Sam wants to kill zebra striping in displays later, so I'm not sure about that.
But if you can hook_entity_view_alter all other entity types, why not this one?

We need it for zebra striping.

We'll probably end up killing zebra striping either in the princess branch (cause princesses like unicorns, not zebras) or in #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors, but why is zebra striping relevant to this discussion? If the plugin sets up #theme => block, then block.tpl.php can still print out $zebra like any other template.

I was under the impression that hook_entity_view() and hook_entity_view_alter() were part of the contract of EntityRenderControllerInterface.

I don't think so. I think we expect all content entities to invoke them, but I wouldn't expect all (or frankly, even any) config entities to invoke them.

Block plugins cannot know what region they're in. That was one of the main things @sdboyer stressed to me.
And to continue doing the zebra striping and allowing themers to inspect regions, we need this wrapper.

Think of the wrapper as a remnant of the region-based page display. That's why it will hopefully be going away.

I discussed this more with tim.plunkett and he explained the motivation of wanting the block.tpl.php and block-wrapper.tpl.php split now, so as to allow sandboxes/contrib to begin working properly with fully decoupled block plugins not wrapped in a block entity. I can appreciate that, so given that...

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.php
@@ -73,15 +37,16 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
-      drupal_alter(array('block_view', "block_view_$id", "block_view_$name"), $build[$entity_id], $entity);
+      drupal_alter(array('block_view', "block_view_$name"), $build[$entity_id], $plugin);

Can we rename these alters to hook_block_wrapper_view_alter(), so as to allow the plugin to use hook_block_view_alter() instead of hook_block_plugin_view_alter()? I think it's important for these to be consistent with their corresponding theme functions.

Tim even suggested (only half seriously) that we could go all the way and rename the 'block' entity type to 'block_wrapper', and thus resolve #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin. But I suggest deferring that discussion and work to that issue.

Rerolling for #1947814: New config entities often save as langcode: und incorrectly anyway, it just did s/LANGUAGE_NOT_SPECIFIED/language_default()->langcode

I swapped the view_alters, and started fixing the hook_block_access hook_block_plugin_access calls as well, but hook_block_access isn't invoked directly by us, and is dependent on the entity type name, so that will be part of #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin.

Thanks. An unrelated question:

+++ b/core/modules/block/block.module
-      'render element' => 'elements',
+      'variables' => array(
+        'content' => NULL,
+        'configuration' => NULL,
+        'plugin_id' => NULL,
+      ),
...
+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
+    if ($content = $this->blockBuild()) {
+      $build = array(
+        '#theme' => 'block',
+        '#content' => $content,
+        '#configuration' => $this->configuration,
+        '#plugin_id' => $plugin_id,
+      );
+    }

Is there a reason we're changing theme('block') from acting on a render element to acting on a bunch of variables? Doing so creates the wtf of having a '#PROPERTY' (in the case above, #content) set to a render array, which is weird, and I can't tell what the benefit is to justify the wtf.

I did that out of personal preference. render element didn't seem to make sense here when I first started. Also, IIRC, it added yet another layer of nesting, $variables['elements']['content']['#content'] instead of $variables['content']['#content'].

Tagging... after a conversation with @timplunkett in IRC about #1076132: hook_block_view_MODULE_DELTA_alter fails with menu blocks

Discussed the block.tpl.php vs. block-wrapper.tpl.php with sdboyer and Snugug, and that's not gonna fly. I'll open a spin-off issue that details why and moves us forward, but in the meantime, setting this to "needs work".

at @tim.plunkett's suggestion, we're gonna postpone this and mark it with 'if SCOTCH fails'. as @effulgentsia said, splitting off to the two templates is not the direction princess is going - we're keeping it all in a single template.

there are other relevant aspects here - namely, whether block plugins expect to interact with an array or an entity for config - but this patch went the all-array route, and i've gone the all-config-entity route in princess. i could still change things, and am open to doing so, but a) it seemed more elegant than continued reliance on anonymous arrays (assuming that we can escape some of the contracts associated with entities for ConfigEntities), and b) i think it gives us standalone blocks for free with fewer contortions. the proof will come when we create the forms for managing displays and are actually doing this CRUD, and at that point, we could still flop over to using arrays pretty easily, due to the groundwork done in this patch.

right now, though, it's best to look at this as a step forward if we DON'T use displays.

I don't think this needs to be postponed until princess is committed or rejected, but I do think it needs to be rerolled to not have theme('block-wrapper'). That's not just a minor wtf: it's knowingly introducing something that will confuse themers working on Twig or updating their contrib themes, and we're 99% sure we'll end up reverting it, so it's a lot of confusion that is not needed by this issue. It was added to the scope of this issue with the belief that it's in the service of where SCOTCH will end up, but we now think that was an incorrect assumption. I'll reroll this when I have a chance, but leaving this postponed for now, because I think there are a couple dependent issues we'll want to work through first; I'll detail what I think they are when I have a chance.

yeah, i'll relent. having looked through the patch again, there's certainly benefit beyond that which is introduced by the template bifurcation. we pull that out, and it's worth having it go ahead.

some things will need refactoring, to be sure - the block plugin bag, for example, will need to be retooled on displays. but that's fine. the only big thing this really raises has to do with the interaction pattern around injecting entities vs. arrays into plugins, but again, i can still work either way in princess.

sorry to have jumped so quickly to postpone.

To reunify block.tpl.php and block-wrapper.tpl.php, but retain the excellent back-end decoupling work that's been done here, we need to remove some variables from the theme('block') pipeline. Here's one: #1968322: Remove unused $id and $zebra variables from templates. I'll post others shortly. If any of those issues get stuck, I'll roll a patch here with some temporary shims, but I'm also hoping that most of those issues can land quickly and not need that.

Here's the next one: #1968360: Remove per-region block markup. Please help spread the word about that issue to themers, so that if anyone has a concern with it, they can speak up.

Alex, could you post about this to http://groups.drupal.org/core maybe? That'd hit both planet and twitter.

and probably cross-post to http://groups.drupal.org/design-drupal and http://groups.drupal.org/theme-development for that matter.

I'd do it myself but I'm kinda mystified on why we're doing this, and figure you could explain it better.

For clarity, "this" == removing these template variables that've been around since at least Drupal 6, iirc.

Re-unifying these into a single template file gets a big "hell yeah!" from me. :D

http://groups.drupal.org/node/293623

Just rerolled for HEAD changes. Not ready for review yet.

While we wait for themer input on the other two issues, here's a no-brainer spin-off: #1969276: Refactor block plugins to make room for a BlockBase::build() implementation. Some might think it's silly to spin that off from this issue, but I work by applying patches to the codebase, and then using a git GUI to see what files are changed, and cutting that list down by a couple dozen is helpful for me.

Another spin-off from this patch: #1970208: Views blocks are missing contextual links

In this issue I cleaned up how contextual links were added to blocks, to remove one-off code from custom_block. It turns out that Views needs this.

Still not ready for review, but here's a reroll that merges HEAD, is rebased on top of #1968360: Remove per-region block markup, and removes everything related to block-wrapper.

That didn't apply because #1968360-44: Remove per-region block markup wasn't complete

#143: block-1927608-143.patch queued for re-testing.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -23,12 +23,10 @@ public function __construct(array $configuration, $plugin_id, array $plugin_defi
-      'label' => '',
-      'admin_label' => $plugin_definition['admin_label'],

@@ -218,7 +159,7 @@ public function form($form, &$form_state) {
-      '#default_value' => !empty($this->configuration['label']) ? $this->configuration['label'] : $this->configuration['admin_label'],
+      '#default_value' => $this->configuration['label'],

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockStorageUnitTest.phpundefined
@@ -139,9 +138,11 @@ protected function renderTests() {
     $expected[] = '  <div id="block-test-block"  class="block block-block-test">';
-    $expected[] = '      ';
-    $expected[] = '<div class="content">';
-    $expected[] = '  </div>';
+    $expected[] = '';
+    $expected[] = '    <h2 class="">Test block html id</h2>';
+    $expected[] = '  ';
+    $expected[] = '  <div class="content">';
+    $expected[] = '      </div>';

When I read these code changes, I thought "hm, I wonder how that still passes tests?"

Well it doesn't, you just changed the tests.

That doesn't have anything to do with visibility or fixing test failures, this has to be reverted.

The two renders in that test are as follows:

    $entity = $this->controller->create(array(
      'id' => 'stark.test_block',
      'plugin' => 'test_html_id',
    )); 

    // Test the rendering of a block.
    $output = entity_view($entity, 'block');

That does not set a title, the test_html_id block plugin has no custom default title or anything, so the block should not output the title.

Then we take that same entity:

    $entity->set('label', 'Powered by Bananas');
    $output = entity_view($entity, 'block');

We provide a label, and when rendering, the block has a label.

Combining the concept of admin_label and label is out of scope for this issue. That used to be called "subject", and had/has more than one issue about it.

+++ b/core/modules/menu/menu.moduleundefined
@@ -399,13 +399,13 @@ function _menu_parents_recurse($tree, $menu_name, $indent, &$options, $exclude,
-function menu_block_view_alter(array &$build, Block $block) {
+function menu_block_view_system_menu_block_alter(array &$build, BlockInterface $block) {
   // Add contextual links for system menu blocks.
-  if ($block->getPlugin() instanceof SystemMenuBlock) {
-    foreach (element_children($build['content']) as $key) {
-      $build['#contextual_links']['menu'] = array('admin/structure/menu/manage', array($build['content'][$key]['#original_link']['menu_name']));
+  if (isset($build['#content'])) {
+    foreach (element_children($build['#content']) as $key) {
+      $build['#content']['#contextual_links']['menu'] = array('admin/structure/menu/manage', array($build['#content'][$key]['#original_link']['menu_name']));

This would need the same changes as openid just had, but its masked by the isset($build['#content']). I really hope tests fail for this, or we need new ones.

+++ b/core/modules/block/lib/Drupal/block/BlockAccessController.phpundefined
@@ -20,7 +20,78 @@ class BlockAccessController extends EntityAccessController {
+    // @todo \Drupal\Core\Entity\EntityAccessController::viewAccess() casts to a
+    //   Boolean, which incorrectly implies that access was always explicitly
+    //   granted or denied. To work around this call
+    //   \Drupal\Core\Entity\EntityAccessController::access() directly and check
+    //   the return type.

If #1947892: Improve DX with EntityAccessControllerInterface goes in, we need to revisit this before commit.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -20,20 +19,17 @@
-  /**
-   * Overrides \Drupal\Component\Plugin\PluginBase::__construct().
-   */
-  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Block $entity) {
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition) {

I guess this needs {@inheritdoc} now

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -38,7 +193,17 @@ public function validate(array $form, array &$form_state) {
+    $settings =  array();

double space here

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -38,7 +193,17 @@ public function validate(array $form, array &$form_state) {
+    $settings =  array();

@@ -49,11 +214,16 @@ public function submit(array $form, array &$form_state) {
+    $settings =  array();

These have an extra space. Also they are completely unnecessary

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -38,7 +193,17 @@ public function validate(array $form, array &$form_state) {
+    $settings['values'] = &$form_state['values']['settings'];
+    $entity->getPlugin()->validate($form, $settings);

@@ -49,11 +214,16 @@ public function submit(array $form, array &$form_state) {
+    $settings['values'] = &$form_state['values']['settings'];
+    $entity->getPlugin()->submit($form, $settings);

This could use an inline comment

+++ b/core/modules/block/lib/Drupal/block/BlockPluginBag.phpundefined
@@ -0,0 +1,69 @@
+   * @param Plugin\Core\Entity\Block $entity

missing \Drupal\block\

+++ b/core/modules/block/lib/Drupal/block/BlockPluginBag.phpundefined
@@ -0,0 +1,69 @@
+   * Overrides \Drupal\Component\Plugin\PluginBag::initializePlugin().

+++ b/core/modules/block/lib/Drupal/block/BlockStorageController.phpundefined
@@ -53,4 +39,13 @@ public function loadByProperties(array $values = array()) {
+   * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::preSave().

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -92,32 +74,34 @@ class Block extends ConfigEntityBase {
+   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::__construct();

@@ -162,6 +127,14 @@ public function uri() {
+   * Overrides \Drupal\Core\Entity\Entity::label();

+++ b/core/modules/block/tests/lib/Drupal/block_test/Plugin/block/block/TestBlockInstantiation.phpundefined
@@ -0,0 +1,69 @@
+   * Overrides \Drupal\block\BlockBase::settings().
...
+   * Overrides \Drupal\block\BlockBase::blockAccess().
...
+   * Overrides \Drupal\block\BlockBase::blockForm().
...
+   * Overrides \Drupal\block\BlockBase::blockSubmit().
...
+   * Implements \Drupal\block\BlockBase::blockBuild().

{@inheritdoc}

+++ b/core/modules/block/tests/config/block.block.stark.test_block.ymlundefined
@@ -1,11 +1,11 @@
-label: ''
 region: '-1'
-weight: ''
-module: block_test
-status: '1'
-visibility: {  }
 plugin: test_html_id
 settings:
+  label: ''
+  weight: ''
+  module: block_test
+  status: '1'
+  visibility: {  }
   cache: '1'
   admin_label: 'Test block html id'

I moved these all around, but I'm not sure that they were re-exported since all of the recent changes were made. All of these should be checked again.

One of the easiest ways is to copy them into into the sites/default/files/config_1234/staging and looking at the diffs

+++ b/core/profiles/standard/config/block.block.bartik.content.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'Main page content'

@@ -14,9 +22,3 @@ visibility:
-label: ''

+++ b/core/profiles/standard/config/block.block.bartik.footer.ymlundefined
@@ -1,9 +1,16 @@
+  label: Footer
...
-label: 'Footer menu'

+++ b/core/profiles/standard/config/block.block.bartik.help.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'System Help'

@@ -14,9 +22,3 @@ visibility:
-label: ''

+++ b/core/profiles/standard/config/block.block.bartik.powered.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'Powered by Drupal'

@@ -14,9 +22,3 @@ visibility:
-label: ''

+++ b/core/profiles/standard/config/block.block.seven.content.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'Main page content'

@@ -14,9 +22,3 @@ visibility:
-label: ''

+++ b/core/profiles/standard/config/block.block.seven.help.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'System Help'

@@ -14,9 +22,3 @@ visibility:
-label: ''

The new settings:label needs to match the original top level label.
Otherwise we end up with something like this:

The minimal and core/modules/block/tests/config blocks weren't converted.

Curious, how did you convert these? Some things are quoted, others are not. Was it by hand, or copying out of your active config?

I'm pretty damn happy with this thing considering what we've gone through. Thanks especially to @EclipseGc, @webchick, and @effulgentsia.

I'd really like to have @effulgentsia be the one to RTBC this, as a good deal of this code is still mine.

I just have one question remaining about the patch:

+++ b/core/modules/block/block.moduleundefined
@@ -499,7 +499,7 @@ function _block_get_renderable_block($element) {
-    $element += entity_view($block, 'block');
+    $element = NestedArray::mergeDeep($element, entity_view($block, 'block'));

I don't remember this change, why do we need it exactly? I can see why the original code might not work in some cases, but $element = entity_view($block, 'block') + $element; should be fine here, right?

I think this patch is getting very close. Thanks for all the patience and persistence.

+++ b/core/modules/block/block.api.php
+function hook_block_plugin_access(\Drupal\block\BlockInterface $block) {
+  if ($block->getPluginID() == 'my_plugin_id') {
+    return FALSE;
+  }
+}

Hm. I was happy that in the last 50 comments or so we were able to remove the distinction of 'block' vs. 'block_plugin' from hook_block_view_alter(). So, I'm sad to see the distinction re-emerge for access. Also, in hook_block_view_alter(), 'block' in the hook name refers to the block plugin. But the above makes it different for access. Would it be possible to remove hook_block_plugin_access() from this patch (since it's a new hook that doesn't exist in HEAD), and have a follow up for figuring out how to add it with sane terminology?

+++ b/core/modules/block/block.module
@@ -499,7 +499,7 @@ function _block_get_renderable_block($element) {
-    $element += entity_view($block, 'block');
+    $element = NestedArray::mergeDeep($element, entity_view($block, 'block'));

I agree with #167. Let's remove this hunk and see if anything breaks.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
@@ -500,9 +270,20 @@ public function blockSubmit($form, &$form_state) {}
+    drupal_alter(array('block_view', "block_view_$name"), $build, $this);

block.api.php still lists hook_block_view_ID_alter(), but the above fails to call it. I see 3 options:

1) Inject $instance_id into the plugin during construction.
2) Add it as a parameter to the build() method.
3) Add a more generic parameter to build(), like $extra_alters that could be an array.

Any other ideas or thoughts on these?

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.php
@@ -45,12 +45,9 @@ function testBlockThemeHookSuggestions() {
+    $variables['elements']['content']['#children'] = '';

I think $variables['elements']['content'] = array() would be cleaner.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -364,15 +362,20 @@ protected function drupalCreateContentType($settings = array()) {
-  protected function drupalPlaceBlock($plugin_id, array $values = array(), array $settings = array()) {
-    $values += array(
+  protected function drupalPlaceBlock($plugin_id, array $settings = array()) {

Why couple $values and $settings here, when the rest of this issue is all about decoupling?

+++ b/core/profiles/minimal/config/block.block.stark.admin.yml
@@ -1,9 +1,16 @@
-label: Administration
-weight: '1'
+label: ''
 status: '1'
+settings:
+  label: Administration
+  weight: '1'
+  status: '1'

Why move 'weight' inside settings? Why leave a (empty) 'label' outside settings? Any why add a copy of 'status' inside settings?

Just a merge of HEAD. Same errors as #168. Will follow up with additional patches and interdiffs.

This is 90% just indentation changes.

I discussed #172 with EclipseGc and tim.plunkett. Here's a fix for all but the last two parts. I also cleaned up the documentation of block.api.php to match similar wording and naming used by hook_node_view_alter(), hook_form_alter(), and hook_node_access().

I still want to do the rest of #172, but want to get this green first.

silly typo

And another one.

Yay, green! Back to needs work for the last two items in #172.

Kartagis just opened #1987952: Blocks are not rendered in order by weight today

This just moves TestBlockInstantiation to the new and improved block plugin location per #1987298: Shorten directory structure and PSR-0 namespacing for plugins.

I'm happy with #188. #167 asked me to RTBC it, and I might later tonight, but am holding off for now to give @tim.plunkett a chance for a final review of the latest changes.

Since I think we're within hours of an RTBC here, I'm restoring the "Avoid commit conflicts" tag that I removed in #131. Fortunately, this patch does not conflict with #1875992-114: Add EntityFormDisplay objects for entity forms.

I'm just going to go ahead and RTBC at this point. It seems we're finally all happy!