Convert block instances to configuration entities to resolve architectural issues

See also: #430886: Make all blocks fieldable entities

The new blocks architecture removes several hooks that allow modules to react to block instance CRUD, which is a regression from Drupal 7.

See my comment in #1535868-349: Convert all blocks into plugins - I'm not sure that's actually the case. If that's the only reason this is filed as a critical issue, then perhaps it should be downgraded?

I'm retitling, but we need agreement on that terminology.

module_invoke($form_state['values']['module'], 'block_save', $form_state['values']['delta'], $form_state['values']);

Nope, only the module that provided the hook_block_info() gets a say here. Otherwise it'd be module_invoke_all().

The reason for the delta is that it's called once for each block provided by that module.

hook_block_save() for instance, has a switch statement on the delta in core typically, but in contrib you could react to all blocks by just disregarding the delta, which is exactly what a handful of modules do and why it's being considered a regression.

So I don't really understand that, because if you look at the code that invokes this hook:

      module_invoke($form_state['values']['module'], 'block_save', $form_state['values']['delta'], $form_state['values']);

It only ever runs on the module which defines the block. So it's true that if your module defines 5 different blocks (with 5 different deltas) you can react to them all at once, but you can do that just as easily by defining a helper function and calling it from each of your blocks individually, so I don't see the regression. There's still no way to do it for other modules' blocks.

It's also the case that for this particular hook, it's invoked from a form submit handler, which makes it a "fun" example to begin with :) I'm sure you can always react to all blocks at once via hook_form_alter() anyway, regardless of what else changes... if reacting via the UI is good enough for your use case....

Oh, a lot of cross-posting there. But yeah. What Tim said.

I'll work on this.

To respond to 12

$config = config('plugin.core.block.bartik.user_login');
$config->set('key', 'value');
$config->save();

That programmatically should work. Same is true for deleting. If you load the config object and run delete, no need to go through the form handlers.

Responding to 14, yes that is happening currently. block_menu_delete() is responding directly to that. In fact there are many hooks that block module implements in order to keep this stuff straight, including deleting block instances when modules are uninstalled and other such behavior.

Hopefully that's helpful.

Eclipse

Just like CachedDiscoveryInterface provides a cache clearing facility on top of DiscoveryInterface we could do a DerivativeDiscoveryInterface which provides a facility to rescan for derivatives, or which allows the plugin manager to react to changed derivatives or whatever. That way menu.module could say, "Hey BlockManager menu_foo block is gone!" and BlockManager would delete it. (I'm not into the new blocks system enough to provide concrete code examples...)

#19

These are actually different things. Derivatives are showing plugin definitions, and we're generally discussing plugin instances here. So the primary discussion here is around the config object itself, not plugin definitions.

#18

xjm, I totally agree for all the same reasons that we discussed with regard to cache invalidation. I think the arguments you made on that topic apply here too, so I'm 100%++ to what you just said.

I worked on this some this weekend, I'll be posting a patch tonight

Still some failures, and plenty left to do, but I wanted show some progress.
Also pushed up a sandbox: http://drupalcode.org/sandbox/eclipsegc/1441840.git/tree/refs/heads/1871...
Sorry my commits were sloppy.

This allows for something like:

$block = entity_create('block', array(
  'id' => 'bartik.search',
  'plugin' => 'search_block_form',
  'region' => 'sidebar_first',
));
$block->save();

and

$block = entity_load('block', 'bartik.search');

de46752 Move Block::access() to an access controller.
5d95b3b Restore module_exists checks to aggregator calls.
e7ee84c node_modules_uninstalled is bogus, remove it

3cea82e Add a render controller
ee419f6 Fix contextual links for menus
4a16bdb Convert block_theme_initialize()
b2c8f5b Fix label overrides from blockBuild
30da84b Fix custom blocks

9ecaa85 Remove Block::getDefinition()
f637b09 Fix UserBlocksTests
0c139c6 Fix custom block rendering.

Rerolled for #1875970: Pass EntityDisplay objects to the whole entity_view() callstack

There were a BUNCH of blocks-related commits tonight. I tried to reroll, we'll see what happens.

Well the merge went okay, but now I'll have to rewrite everything added in #1881096: Regression: Site broken when disabling module without removing block instances. and #1880378: Autocomplete broken to search blocks - also triggers notice after clicking 'Next'.

Should fix those bugs, and I added some more @todo's for myself after I finally get a green patch.

Don't forget these tests are disabled:

-  function testWizardMixedDefaultOverriddenDisplays() {
+  function _testWizardMixedDefaultOverriddenDisplays() {
     // Create a basic view with a page, block, and feed. Give the page and feed
     // identical titles, but give the block a different one, so we expect the
     // page and feed to inherit their titles from the default display, but the
@@ -163,7 +163,7 @@ function testWizardMixedDefaultOverriddenDisplays() {
   /**
    * Tests that the revert to all displays select-option works as expected.
    */
-  function testRevertAllDisplays() {
+  function _testRevertAllDisplays() {

LOL whoops.
Making some other cleanups, I'll fix that as well in the next patch.

Okay, fixed that, and broke up Block::__construct()

Took some time to review this patch.

Overall, I think we're heading in the right direction, but I have a couple of concerns - mostly revolving around the compound entity IDs of block instances. Those concern me from a general architectural standpoint, coming from the configuration system perspective. The dynamically constructed IDs and config object names are hardly maintainable and reliable.

Also, the theme name as prefix was very unexpected — given that these config objects are the canonical representations of plugin instances, and given that every plugin instance is of a certain type, which in turn is provided by a certain module/extension, I would have expected config object names of:

block.block.$extension.$plugin_base_id.$machine_name

That would inherently solve a couple of problems that are apparent in the current code:

- A module gets uninstalled? No problem: config()->listAll("block.block.$module."); Process the limited subset. Done.

- A plugin ID ceases to exist or a derivative is deleted? No problem: config()->listAll("block.block.$module.$plugin_base_id"); Process the limited subset. Done.

The existing hook implementation for deleted menu blocks would be vastly simplified. And perhaps I'm wrong, but I think it is safe to expect that we'll actually have a lot of derivative plugin implementations + instances in D8, so architecturally + storage-wise, we should be prepared for that.

If I get it right, then the theme name was chosen as prefix, since we need to load all block instances for a theme. But frankly, I'd much rather solve that use-case through a cache (or even state?) of block entity IDs per theme. The storage/identifiers should be optimized for handling CRUD operations.

Alternatively/additionally, I could also offer to extend listAll(), so that it is possible to use wildcards within config names to find; e.g., listAll("block.block.*.*.$theme."), and we'd use config object names of

block.block.$extension.$plugin_base_id.$theme.$machine_name

(which would limit the machine_name length to 250 - 65 - 65 - 65 - 12 == 43 characters, but that should be sufficient).

Anyway, point taken. I think the config object names in this patch require some more architectural design considerations.

On to the other topics:

+++ b/core/modules/block/block.admin.inc
@@ -195,10 +190,8 @@ function block_admin_display_form_submit($form, &$form_state) {
+function _block_compare($a, $b) {

Can we add type-hints here?

+++ b/core/modules/block/block.module
@@ -121,19 +121,26 @@ function block_menu() {
+  $items['admin/structure/block/add/%/%'] = array(
     'title' => 'Configure block',

Let's change the title to "Add block"?

+++ b/core/modules/block/block.module
@@ -121,19 +121,26 @@ function block_menu() {
+  $items['admin/structure/block/manage/%'] = array(
...
+    'page arguments' => array(4, 5),
...
+  $items['admin/structure/block/manage/%/delete'] = array(
...
     'page arguments' => array('block_admin_block_delete', 4, 5),

arg(5) does not exist.

+++ b/core/modules/block/block.module
@@ -306,27 +313,24 @@ function _block_get_renderable_region($list = array()) {
+        $build[$key] = entity_view($block, 'full');

@@ -538,17 +476,7 @@ function _block_get_renderable_block($element) {
+    $element += entity_view($block, 'full');

Shouldn't we reserve the 'full' view mode for a block being displayed as main page content?

I'd suggest to simply change the view mode here to 'block'. Maps to the #theme_wrapper and also it's conceptual view mode.

+++ b/core/modules/block/block.module
@@ -338,9 +342,8 @@ function _block_get_renderable_region($list = array()) {
+    if (!in_array($block->get('plugin'), array('system_help_block', 'system_main_block'))) {

I don't understand why the plugin IDs of blocks are suffixed with "_block"...?

We don't do that for any other plugin type.

It's ridiculous how many times the term "block" is repeated in the block plugin architecture (namespaces, class names, plugin IDs, etc.pp.). We definitely need to scale that down.

+++ b/core/modules/block/block.module
@@ -414,28 +409,23 @@ function block_themes_enabled($theme_list) {
 function block_theme_initialize($theme) {
   // Initialize theme's blocks if none already registered.
-  $has_blocks = config_get_storage_names_with_prefix('plugin.core.block.' . $theme);
+  $has_blocks = entity_load_multiple_by_properties('block', array('theme' => $theme));
   if (!$has_blocks) {

This (pre-existing/legacy) logic is a bit strange in the new design. Essentially:

If I intentionally delete all block instances for a theme, then all block instances of the default theme are copied and populated for the theme again... Weird? ;)

Combined with #1067408: Themes do not have an installation status, I guess that this logic actually belongs into a hook_themes_installed() implementation, so that it only runs once. :)

+++ b/core/modules/block/block.module
@@ -643,14 +570,12 @@ function template_preprocess_block(&$variables) {
+  foreach (entity_load_multiple('block') as $block_id => $block) {
+    $visibility = $block->get('visibility');
+    if (isset($visibility['roles']['roles'][$role->rid])) {
+      unset($visibility['roles']['roles'][$role->rid]);
+      $block->set('visibility', $visibility);

Hm. I'm a bit scared about performance here... this will potentially load hundreds of block entities (all block instances for all themes)...

Once roles are converted to config entities, too, the overall problem space appears clear: We need an efficient way for recording and tracking entity references between config entities. Which will probably involve a giant mapping/matrix table, or possibly even something like Relation in core.

Regardless of that:

Any chance we could use

entity_load_multiple_by_properties('block', array('visibility.roles.roles' => $role->rid));

here?

+++ b/core/modules/block/block.module
@@ -659,11 +584,9 @@ function block_user_role_delete($role) {
 function block_menu_delete($menu) {

I think we need and want to move and reverse this hook implementation into the Menu module now, so that it deletes any block instances when it deletes a menu?

+++ b/core/modules/block/block.module
@@ -681,35 +604,18 @@ function block_admin_paths() {
 /**
- * Implements hook_modules_uninstalled().
- *
- * Cleans up any block configuration for uninstalled modules.
- */
-function block_modules_uninstalled($modules) {
-  $block_configs = config_get_storage_names_with_prefix('plugin.core.block');
-  foreach ($block_configs as $config_id) {
-    $config = config($config_id);
-    if (in_array($config->get('module'), $modules)) {
-      $config->delete();
-    }
-  }
-}

Why is this removed?

Since the config objects are still using block.* prefixes, Block module is responsible for maintaining them.

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -0,0 +1,270 @@
+    // Get the block subject for the page title.
+    if ($label = $entity->label()) {
+      drupal_set_title(t("%label block in %theme", array('%label' => $label, '%theme' => $theme_title)), PASS_THROUGH);
+    }

This should not be contained in the form controller — the page title should be set in the page callback, before the form controller is invoked.

If necessary, we can add a custom getPageTitle() helper method to the Block entity class, in order to retain the custom $theme handling here.

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -0,0 +1,270 @@
+    $form['settings']['machine_name'] = array(
+      '#type' => 'textfield',
+      '#title' => t('Block machine name'),
+      '#maxlength' => 64,
...
+    if (empty($form['settings']['machine_name']['#disabled'])) {
+      if (preg_match('/[^a-zA-Z0-9_]/', $form_state['values']['machine_name'])) {
+        form_set_error('machine_name', t('Block name must be alphanumeric or underscores only.'));
+      }
+    }

Why do we duplicate #type machine_name here?

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -0,0 +1,270 @@
+  protected function actions(array $form, array &$form_state) {
+    $actions = parent::actions($form, $form_state);
+    $actions['submit']['#value'] = t('Save block');
+    return $actions;

Why "Save block" instead of just the default "Save" ?

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -0,0 +1,270 @@
+  public function validate(array $form, array &$form_state) {
...
+    if ($form_state['values']['module'] == 'block') {
+      $custom_block_exists = (bool) db_query_range('SELECT 1 FROM {block_custom} WHERE bid <> :bid AND info = :info', 0, 1, array(
...
+    $entity->getPlugin()->blockValidate($form, $form_state);

This custom block validation should be moved into custom_block's block plugin.

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.php
@@ -0,0 +1,99 @@
+    // @todo Added to facilitate the possibly bogus assertion in
+    //   \Drupal\block\Tests\BlockTest::testCustomBlock().
+    if ($view_mode != 'full') {

Can we clarify what this @todo is actually about?

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.php
@@ -0,0 +1,99 @@
+    $defaults['#block_config'] += $entity->getPlugin()->getConfig();
+
+    // @todo This is a hold-over from the old behavior, it is crazy.
+    if (!empty($defaults['#block_config']['label'])) {
+      $defaults['#block_config']['subject'] = $defaults['#block_config']['label'];
+    }
+    elseif ($label = $entity->label()) {
+      $defaults['#block_config']['subject'] = $label;
+    }

It's not clear why this is necessary and what the actual @todo is.

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.php
@@ -0,0 +1,99 @@
+      $block = $entity->getPlugin()->blockBuild();
+      // Allow blocks to be empty, do not add in the defaults.
+      if (!empty($block)) {
+        $block = $this->getBuildDefaults($entity, $view_mode, $langcode) + $block;
+      }

I don't really understand why we want to allow blocks to return an empty build/render array.

Also, with regard to render arrays, we normally populate a basic $build with default info + properties, before passing it to the callback/plugin — e.g., see drupal_retrieve_form() and Field API field widgets / formatters, etc. This allows the actual build methods to base their data on prepopulated/supplied info, but also to customize or override the default build/render info.

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.php
@@ -0,0 +1,99 @@
+      drupal_alter(array('block_view', "block_view_$id", "block_view_$name"), $block, $entity);
...
+      $build[$entity_id] = $block;

The $block variable is really confusing here... I almost commented that we should pass $build instead of the $block entity object as first parameter to hook_view_alter, but alas, $block == $build here, and $entity == $block... ZOMG. :P

How about renaming the $build variable to $all or $render or $array or $return or whatever, so we can use $build instead of $block? :)

+++ b/core/modules/block/lib/Drupal/block/BlockStorageController.php
@@ -0,0 +1,80 @@
+  public function loadByProperties(array $values = array()) {
...
+        return $value == $block->get($key);

We might need a strict comparison (===) here to prevent false-positive matches.

+++ b/core/modules/block/lib/Drupal/block/BlockStorageController.php
@@ -0,0 +1,80 @@
+  protected function preSave(EntityInterface $entity) {
...
+    // Ensure that the theme is stored directly on the entity, to simplify
+    // \Drupal\Core\Config\Entity\ConfigStorageController::loadByProperties().
+    if (!$entity->get('theme')) {
+      list($theme) = explode('.', $entity->id());
+      $entity->set('theme', $theme);
+    }

This concerns me.

Having the entity ID in the config object name in addition to a config property is an architectural bug and problem already, since it inherently means double-housekeeping and duplicate validation layers, and even with those in place, there's still no guarantee that they won't get out of sync.

Adding another property + duplicated value there is guaranteed to introduce bugs.

This code does not account for the possibility of $theme getting changed, which inherently means a rename. Essentially, this controller would have to extend and perform the identical processing that happens for the ID in ConfigStorageController already.

I understand the idea and intentions, but I'm really not a fan of this proposal... :-/

+++ b/core/modules/block/lib/Drupal/block/BlockStorageController.php
@@ -0,0 +1,80 @@
+    // Cache settings are stored directly on the block, remove them from
+    // configuration.
+    $configuration = $entity->get('configuration');
+    unset($configuration['cache']);
+    $entity->set('configuration', $configuration);

Looks like something should be using #parents?

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -0,0 +1,178 @@
+ *   config_prefix = "plugin.core.block",

I thought we wanted to rename to 'block.block' here?

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -0,0 +1,178 @@
+  /**
+   * The plugin instance configuration.
+   *
+   * @var array
+   */
+  protected $configuration = array();

Is there any chance to rename this to $settings, pretty please? :)

A configuration object containing a key 'configuration' really is beyond sanity ;)

Also, why is this protected and not public?

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -0,0 +1,178 @@
+   * The module owning this block.
...
+  protected $module;

Isn't the module owning the plugin?

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -0,0 +1,178 @@
+   * The plugin instance ID.
...
+  protected $plugin;

Shouldn't this be "The plugin ID of this instance."...?

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -0,0 +1,178 @@
+  public function getExportProperties() {
...
+      'id',
+      'label',
+      'uuid',
...
+      'module',
+      'theme',
...
+      'plugin',

Can we sort these, so that the important properties are contained first? I.e.:

id
uuid
plugin
module
theme
status
region
weight
label
...
settings

Ideally, we should also re-order the actual class properties in the code in the same way (DX).

+++ b/core/modules/menu/menu.module
@@ -339,11 +339,12 @@ function menu_delete($menu) {
   // Invalidate the block cache to update menu-based derivatives.
   if (module_exists('block')) {
     drupal_container()->get('plugin.manager.block')->clearCachedDefinitions();
   }

Hm. This is I think where we need to tell the plugin manager that a plugin ID has been deleted.

Merely clearing the cache of definitions isn't really helpful, since we actually need to trigger CRUD operations, so all code can properly react to a plugin ID that is removed from the system.

I almost think that we'd ideally make that part of a PluginConfigEntityBaseStorageController (oh boy, what a class name ;)), so all config entities that are based on (or wrapping?) a plugin instance automatically get the required preSave() + preDelete() behavior.

+++ b/core/modules/menu/menu.module
@@ -481,7 +482,7 @@ function menu_reset_item($link) {
 function menu_block_view_alter(&$build, $block) {
   // Add contextual links for system menu blocks.
-  if ($block instanceof SystemMenuBlock) {
+  if ($block->getPlugin() instanceof SystemMenuBlock) {

I've to admit I don't really understand the overall architecture of the plugin system anymore — isn't it architecturally wrong to test for the class name of a plugin instance? I thought one of the main architectural aspects of the plugin system was that plugins can be swapped out with different implementations? (which in turn would make the class name condition invalid and we need to check for the plugin ID string instead)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -369,47 +369,20 @@ protected function drupalCreateContentType($settings = array()) {
   protected function drupalPlaceBlock($plugin_id, array $settings = array(), $theme = NULL) {

Hm. The signature doesn't really make sense to me anymore... Can we change it into:

drupalPlaceBlock($plugin_id, $settings = array(), $properties = array())

Whereas:

- $plugin_id == like now.

- $settings == The block plugin settings, as stored in the 'settings' property (i.e., current 'configuration').

- $properties == Entity properties.

The most common use-case will be to supply $plugin_id + $settings, done.

More advanced use-cases will want to additionally supply custom $properties. (You'd supply 'theme' in there instead of the current $theme, but as mentioned, only more advanced tests will need that.)

In total, you can essentially customize the entire thing like you want, but we make the most common operation as simple as possible.

Btw, since the block instance $label most likely gets rendered by the block plugin in many cases, we should use randomString() instead of randomName() as default value there.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BlockUpgradePathTest.php
@@ -35,23 +35,27 @@ public function setUp() {
   public function testBlockUpgradeTitleLength() {

Huh? Why is this title length testing buried into an upgrade test class?

+++ b/core/modules/user/lib/Drupal/user/Tests/UserBlocksTests.php
@@ -80,9 +80,8 @@ function testUserLoginBlock() {
+    $block = entity_load('block', 'stark.online');

So here's the thing:

As a module developer, I should be able to enable a block of my module by default (if it's the >80% use-case).

But what kind of ID/machine name do I use to do that, which doesn't potentially clash with a custom/user-created ID that exists already?

E.g., let's consider this module developer need, and let's consider I'm the http://drupal.org/project/online module (oh how awesome, that module actually existed in 2003, to provide the "Who's online" block :-D), which ID would I choose to create my block on install?

+++ b/core/profiles/testing/config/plugin.core.block.stark.admin.yml
@@ -1,4 +1,6 @@
+plugin: 'system_menu_block:menu-admin'

+++ b/core/profiles/testing/config/plugin.core.block.stark.online.yml
@@ -1,11 +1,15 @@
+plugin: user_online_block

+++ b/core/profiles/testing/config/plugin.core.block.stark.tools.yml
@@ -1,4 +1,6 @@
+plugin: 'system_menu_block:menu-tools'

Ugh. Why have these been added to the Testing profile? That's really wrong. :-/

The Testing profile is supposed to be completely empty. Whatever was and still is in there is too much and has to be eliminated.

The Testing profile must not contain any configuration or any other "defaults." This is really important.

I'll respond to the feedback with comments and an interdiff in a bit, but I will say now that I am only solving/addressing issues introduced in this patch. There are follow-ups already for many of the things you mentioned, and I will likely create others.

Leaving assigned to myself.

If you notice, this issue doesn't change the config object names, because we have #1839904: Rename plugin.core.block CMI prefix to block.block and #1879496: Do we need to worry about plugin ID collisions?.

Added type hinting to _block_compare()

Not changing 'Configure block' in this patch: #1875780: "Configure block" button text in the Block Library is confusing

Changed from full to block

Not changing any of the individual plugin IDs, that's reasonable to file a follow-up for if you'd like.

I don't know what you mean, that only happens if you enable a theme, delete all of its block placements, disable the theme, and re-enable it. Yes, it might make more sense to move that to hook_themes_installed, but if you'd like that behavior to change, open a follow-up.

We don't have any benchmarks on how much more expensive it is to load a ConfigEntity than a vanilla CMI file, and otherwise your concerns here are equally important in HEAD.

I don't see a reason to move block_menu_delete(), and definitely not as part of this conversion.

The removal of block_modules_uninstalled() is a one-off attempt to solve #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API and has no test coverage. It does not belong in here until that issue is resolved for all ConfigEntities.

I moved the drupal_set_title() call out of the form controller, it actually only was needed for one of the two anyway. Thanks for that one!

Yes, the #machine_name code is wrong, but it's just moved here, not changed. I will personally open a follow-up for that.

That "Save block" is moved code, but also, why not? Views does it :)

I moved the custom_block validation, not sure why that wasn't done already.

I've clarified my comment about the view modes, I have to check with @EclipseGc about his expectations.

I'm also not sure about using the plugin class names directly, but that's not a part of this conversion.

I'll think more about the drupalPlaceBlock() signature and a possible change, but not including it in this reroll.

testBlockUpgradeTitleLength isn't part of this, but yeah that's a little strange :)

Yes, the $theme.$foo is a bit awkward, but see the first line of this comment for the follow-ups.

AFAIK I'm not adding anything to Testing, just changing existing file. Not 100% sure why they're in there.

Updated issue summary.

Added #1884758: Remove testing profile default blocks and #1884762: Block forms should use #type => machine_name to the issue summary as follow-ups.

Urgh, when I moved the drupal_set_title() call out of the form controller, I removed a variable that was being used elsewhere.

Added #1884860: Remove 'block' prefix from BlockInterface method names to the issue summary as a follow-up

Fixed the fails.

d5ac348 Adjust to removed $theme parameter in block_admin_block_delete().
87f80ad Add missing delta in CustomBlock::blockValidate().

It seems I missed responding to the middle of sun's review. More to do!

Starting with "I don't really understand why we want to allow blocks to return an empty build/render array."

If a block has no content, the title and wrapper shouldn't be shown. Think of a view with no results.

Yes, that $block in BlockRenderController::viewMultiple is weird. Fixed.

Fixed the strict comparison.

I'm not dealing with the duplicated theme key in the export, especially since we're talking about removing it from the ID, which would make it the only place it's stored.

"Looks like something should be using #parents?" Not sure what you mean here, there's no form involved.

Not doing s/plugin.core.block/block.block/ here, see the issue linked in my last comment.

Meh, I can rename $configuration to $settings if anyone else thinks it matters.

Fixed the Block::$module doc

No, this is the ID of the plugin instance. We could do $entity->getPlugin()->getPluginId() each time, but ouch.

I'll reorder all the properties later, but good idea.

---
Stopping here, I'll pick up with "Hm. This is I think where we need to tell the plugin manager that a plugin ID has been deleted." tomorrow.

I'll post the patch tomorrow, it looks like #1814916: Convert menus into entities is about to be committed so I'll have to reroll anyway. Here's the interdiff for this review response.

Starting up again:

We don't have CRUD operations for plugin IDs yet, and this would also apply only to derivatives. Out of scope, since I'm not actually changing any of those lines.

-  if ($block instanceof SystemMenuBlock) {
+  if ($block->getPlugin() instanceof SystemMenuBlock) {

Yes, this is wrong, but I'm not changing it here. I'll open a follow-up. EDIT: #1885354: When identifying plugin instances, use either the plugin ID string or the interface, never a class

I've also adjusted how drupalPlaceBlock works. I did, similar to your suggestion, split up the entity values and the plugin configuration. However, it reads protected function drupalPlaceBlock($plugin_id, array $values = array(), array $configuration = array()), because there were only 4 instances of plugin configuration being passed.

---
That should cover all of sun's feedback.
The menu conversion didn't get committed, but this will need a reroll for that soon, so leaving assigned.

I didn't repush the branch yet, but here are the fixes. My grep was off so I didn't catch these.
I'll do a full reroll after menu config entities go in.

Menus are now ConfigEntities!
Rerolled, I'll push up the branch when the tests pass.

Added an implementation of PluginBag, that's the new way for a ConfigEntity (or anything) to manage its plugins.
Reminder, code is in the sandbox, see the issue summary.

Assigning to @EclipseGc for a review, I still intend to work on the tests and stuff from the OP.

It wasn't any lines changed in this patch, just in the patch context from #1884828: Replace md5 calls with sha256 in Views in core

For each block, one pluginBag holding one single plugin ? Surprising - seems like unneeded complexity and overhead ?

@yched, I thought the same thing at first. It can be easily removed again later, but for now I wanted to follow suit with the other pattern of "ConfigEntity containing plugins".

Okay, reverting the code in the branch for now, #55 is the code to review. I'll revisit the PluginBag later.

Still needs review.

6fefa95 Add default value and comment for Block::$region.
6749817 Harden the exception for disabled module plugins.
38233b9 Throw an exception if a block is without a plugin.
329dfd1 Add CRUD unit tests.
511250d Add a test for default blocks.

Still needs tests for hook invocation, and cleanup of instances when a plugin goes away.

Okay, the instance cleanup is out of scope for this issue. Here's the CRUD hook tests, as well as a cleaner unit test.

8ec99cf Remove unnecessary coupling to System module.
e9a9586 Add entity CRUD hook tests for blocks.

I think we're good now?

Removing tags.

Adding followup

Add follow-up, clarified scope

8ec99cf Remove unnecessary coupling to System module.
e9a9586 Add entity CRUD hook tests for blocks.
d821012 Resolve issue of duplicate help blocks.
8d13cd0 Inject the block entity into its plugin during building.
7d2e7ee Use the injected entity to alter the View title.
9404e71 Remove overly complex subject handling.
40bbc50 Fix default title during block creation.

I think I'm completely happy with this as-is now. Everything else should be a follow-up.
We need a review from a blocks person, but barring any typos or odd fails, I think it's done.

A couple small test fixes.

isNew() === TRUE, $status === SAVED_NEW. That makes sense.

1) Should be moved out separately, and is why 15 was changed as well.
3) It's still possible of course, but we should be trying to remove all occurrences of hardcoded config prefixes, everywhere.
5) Well, they're not really helpers per se, it's a pattern we've used in views to help break up monolithic test methods without a reinstall. Still want them moved?
6) That's the way we've started testing theme output with assertThemeOutput(), but that is not available to DUTB. The major bonus of doing it this way, a lot of the whitespace is total crap, and things like class="" are wrong, so this will ensure that tests are actually updated when we change/fix these templates
8) There is no open issue that I know of
9) Hmm, okay, I'll remove those ;)
11) Removed in patch
12) That's an existing string, not one I made up.
13) Nope! :)
14) That was a specific request from sun. I don't care
15) See point 1
17) Fixed in patch
18) Yes, one is for the block library, one is the configuring of a placed instance
19) Fixed in patch
24) Nope, it doesn't touch block_node_type, but neither does that hook. It doesn't actually do anything. If it did, it needs test coverage.
25) Well, it's needed only internally. Not internal to the class, so it can't be protected though :(
26) See 14
28) Same as 11
29) No follow-up filed yet
32) The argument is passed because the block plugin doesn't need to know about the block entity at any other time
33) Fixed in patch, that was just debugging stuff
34) The theme name is already in the config object name, and this doesn't actually change that.
35) That's just from the entity system. Views has the same thing. Who knows if its translatable?
36) It's the beginning of an API change. In reality, the 'subject' part is only used on the theme layer, and that is still called subject there (but will be much easier to change now)

Reverting the change to isNew(), I'll cross-link the issue when I open it next.
Also, renamed configuration to settings, per sun's request.

In doing so, I realized we were jumping through a bunch of hoops in about 4 places to have the cache setting on the entity, when it belongs to the plugin. So I moved that back.

In addition, I removed the duplicate theme property from the export, and handle it in Block::get() now.

Updated issue summary.

156ec81 Move the default cache check code to somewhere it will run reliably.
38b9149 Fix Block::get() to handle creating new blocks.

1) They were always keyed by something useful, this just blew that away for no reason. But yes, now they are keyed by entity_id
2) Fixed
3) Fixed
4) Fixed
5) No, the add form doesn't use this in HEAD
6) Fixed
7) Fixed
8) It's now only used as a #pre_render callback.
12) Fixed
14) I guess that's a change, #title is gone.
16) DerivativeDiscoveryDecorator::decodePluginId() but as you said, doesn't work always
17) I doubt it does.
20) I possibly did break this, but the content entity issue should take care of this. Otherwise, we'll need test coverage
21) I'm just moving this code out of BlockBase::validate() :(

1) Fixed
3) Fixed
4) Because we don't use $block['content'] attribute in block.tpl.php, mostly because it's pretty field-centric
5) That's copied verbatim from EntityRenderController and EntityRenderControllerInterface
6) This is subject so that block.tpl.php doesn't change
11) Fixed
12) Whoops, there was a region constant, fixed. Also, not actually sure why we have block->status
13) Not new, that contains things like language, path, node type...
14) It is, between settings and visibility
16) It's not in any assertion, I think BAP moved it

Interdiff is this and #84.

Per @EclipseGc's request, I repopulated the BlockInterface with the original docs, the only change being adding in the 'block' prefix, so that we don't have to move #1884860: Remove 'block' prefix from BlockInterface method names into here and change every single block plugin.
The interdiff is against the last patch, the other txt file is the full diff of the interface from 8.x through this patch.

Updated issue summary.

Added #1888594: Refactor BlockListController to use generic tabledrag as another follow-up. It's postponed on #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController) as well.

I realized that we can use a list controller to wrap the existing form, and try to refactor once there is generic tabledrag support.

Removed:
block_admin_display_prepare_blocks()
_block_compare()
Moved:
block_admin_display_form() => BlockListController::form()
block_admin_display_form_submit() => BlockListController::submit()

On the chopping block: _block_rehash() (still there for now)

I also added Block::uri().

Ok, I'm not done with the review, but I'm about half way through and after taking some time to talk to timplunkett, xjm, msonnabaum, and effulgentsia, my thoughts are becoming a bit more concrete and worth sharing. I'll try to keep this short.

1.) Going this route seems to lock us in with THIS access controller, THIS form controller, THIS render controller. You can't swap it per block, you can't do a lot of things without repercussions. How do we address that? I personally consider this a blocker, do people agree, or is there reason this could be a follow up? As I understand it this is a feature of the entity system in general.

2.) We have to instantiate a lot more classes and pass through a lot more methods for anything now. Just to render a block, we need the Block Entity class, the Access Controller, the Render controller and the Plugin itself. This isn't a concern I can speak to all that much except to say that the plugin system is complex, the entity system is complex, and tightly coupling them in this way is a huge WTF for me.

3.) My expectations were quite contrary to this. What I expected was a config entity to be created, and all instances of config() and config_get_name_by_prefix() to be replaced. Then any resulting entity's values to be injected into plugin instances in the same way as happens currently (probably requiring a small update to ConfigMapper.php) What this is instead is a complete rewrite, again, to utilize the entity API where a custom block API was written already. This exposes a whole new set of API methods that are, by and large, unnecessary for day to day plugin usage. Furthermore, it's worth noting that my expectations would have maintained the manifest properly and given us the CRUD hooks we were hoping to get, all w/o this scale of rewrite.

4.) Finally, I think it's totally worth noting that every other instances of config entities and plugins working together in core today are what I'd term "Plugin Composition" i.e. it's generally many plugins working together with one configuration entity backing them up. This is not what blocks are, blocks have one configuration to one block. I understand that the use cases here have some pretty serious overlaps, but I also think this single point alone gives a lot of extra weight to the argument that we be very careful with what we do here.

Eclipse

PS: I really like the notion of a ListController for all of this. Being able to swap out the listing for another from contrib gives me great peace of mind.

Rerolled since #1887692: Clean up unused {block_node_type} table and references got in.

The patch supports all of the use cases of core functionality, and what is covered by test coverage. If there are hidden or undocumented use cases with no test coverage, then they need to be fleshed out in other issues. And I'm reasonably sure that this architectural shift won't prevent us from supporting something the old architecture could have.

As I understand it, here is how you programmatically place the user login block into the first sidebar, with default values:

HEAD:

$config = config('plugin.core.block.bartik.user_login');
$config->setData(array(
  'id' => 'user_login_block',
  'subject' => 'User login',
  'whois_new_count' => '5',
  'status' => TRUE,
  'cache' => DRUPAL_NO_CACHE,
  'visibility' => array(),
  'module' => 'user',
  'region' => 'sidebar_first',
  'weight' => 0,
));
$config->save();

No hooks are fired.

With the patch:

$entity = entity_create('block', array(
  'plugin' => 'user_login_block',
  'id' => 'bartik.user_login',
  'region' => 'sidebar_first',
));
$entity->save();

Hooks fired:

• hook_block_presave()
• hook_entity_presave()
• hook_block_insert()
• hook_entity_insert()

Going this route seems to lock us in with THIS access controller, THIS form controller, THIS render controller. You can't swap it per block, you can't do a lot of things without repercussions. How do we address that? I personally consider this a blocker, do people agree, or is there reason this could be a follow up? As I understand it this is a feature of the entity system in general.

You can always have a controller that's a proxy to other controllers.

It seems that EclipseGC's issues with this conversion stem mostly around the use of the list and form controllers. Can we do a straight conversion here to move forward, and move that argument to followup issues?

I'm actually ok with list, my issue is form, access, and render. Likewise I really dislike the plugin being in the entity. Those are my main issues. I just told tim I'd review the render controller again, but I wanted to comment here to make sure my views are completely represented.

Eclipse

The list controller was added at @EclipseGc's request, and doesn't change any code in any way, other than moving functions to methods.
The render controller just moves complex build logic out of template_preprocess_block(), and the only change is that it prevents you from accidentally removing calls to hook_block_view_alter().

I maintain that the form and access controller changes are "correct", in that they are how the rest of Core does things. But since those do indeed remove an undocumented use case, we can change them to be a pure proxy call to BlockBase::form() and BlockBase::access(), as before. Then we can open up an issue for each to discuss their merits.

+1 to #99. I agree that BlockRenderController looks to have exactly the stuff we don't want varying on a block by block basis, but I'll let EclipseGc review that in more detail and comment if he agrees or not.

However, IMO, form and access should be fully in the control of the plugin. Or at any rate, this issue shouldn't be the one to change that.

#92.2 also mentions performance concerns about a render and access controller existing as extraneous intermediaries. I don't think we should worry about that in this issue, but have a follow up for evaluating that, and potentially removing them if the optimization of doing so is significant.

I'm pretty convinced that Render is fine at this point.

Okay, that's done.

I've attached an interdiff, but also the full diff of BlockBase and BlockInterface, to make it clearer where we're at.

Added some typehinting/docblocks

adding followup

Now that three methods on BlockInterface need instances of entity blocks, it looks kinda awkward for them to be arguments. Can we not just add entity blocks to the constructor and have it be an internal property?

Also, it's kind of weird to see the type hint as "Block". If they do stay as arguments, maybe we could do something like this instead to be clearer:

use Drupal\block\Plugin\Core\Entity\Block as EntityBlock;

d577e82 Fix bug with the block theme during validate.
421980c Remove Block $entity from BlockBase methods, add BlockBase::$entity.
c4eea01 Issue #1889826 by tim.plunkett: DefaultFactory::getPluginClass() is very useful and should be public and static.
9fc0336 Move from a setter to a param in the constructor.

As you can see from that commit log, this is now blocked on #1889826: DefaultFactory::getPluginClass() is very useful and should be public and static. This patch includes it.

Because the plugin now has a reference to the entity, I was able to revert all the additions to blockBuild(). I might have missed one, we'll see.

Yay for tests.

582b827 Fix ViewsBlock::__construct().

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.phpundefined
@@ -30,7 +31,14 @@ public function __construct() {
+  /**
+   * 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);

I'm surprised this works. Isn't the overriden method supposed to have the same signature than in the parent & interface ? (no Block $entity param on those) ?

Also, we have class Block as an entity, and BlockBase as a base class for plugins. So Block *not* extends BlockBase and *not* implements BlockInterface, and the two are in fact in completely disconnected class hierarchies.
Sounds like a code smell that some naming is off wrt the underlying model.
Something like : 'blocks' are the entities; plugins are not blocks, but block handlers ? (thus, BlockManager -> BlockHandlerManager, BlockBase -> BlockHandlerBase, BlockInterface -> BlockhandlerInterface...)

I think that goes back to my previous comment that using "Block" for "Entity\Block" is too ambiguous. We can just use it as EntityBlock and it's no longer confusing.

That works because I gave it a default value. You can do that in PHP.

If we want to open an issue to discuss the naming of those three, let's do that, but this patch is already big enough. Especially since we'd bikeshed on the name (handlers are a D7 Views thing, or possibly the new name of D8 entity controllers).

@EclipseGc pointed out that we don't need to pass the entity now that its in the constructor.

I think we're done here.

"handlers are a D7 Views thing, or possibly the new name of D8 entity controllers"
Well, those are sufficiently generic enough terms, no given subsystem should be entitled to "own" them without some context or namespacing of some sort IMO.
But sure, followup makes sense - created #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin

I have a lot of outstanding objections to this, but it's very apparent that I'm not going to get this to go where I think it should be, and having spoken with tim he's happy with the current patch. That being said, I'll get out of the way and rtbc this, but I am not happy about passing entities to plugins. That's a huge wtf in my opinion.

Eclipse

I am not happy about passing entities to plugins

I have no objection to passing entities to plugins in general, but I also have some concerns about passing block entities to block plugins, since I think the block entity should encapsulate the plugin, not introduce a circular reference. We can take that up in #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin or another spin off, however.

The current design seems pretty sensible to me. BlockBase doesn't get infected with another object's interface, and it gets all the data it needs from an injected dependency. This is an ideal use of ConfigEntity IMO.

Very happy with the latest patch.

I wanted to take a minute and document my actual remaining issue with this patch. I've RTBC'd it, and I think that should probably stand, but I've also not outlined my problems with the patch (at this phase) publicly, so what follows is exactly that.

My main objection at this point is the injection of the entity in the plugin at all. I don't see the need. The entity is a container for values, that is its primary role as configuration. The block plugins were always expecting raw values from a config object. Just because that object became an entity didn't really change anything in my opinion.

Objections about passing entities:

pseudo-code example of what we used to do:

$config = config('plugin.core.block.' . $config_id);
$plugin = $manager->createInstance($plugin_id, $config->get());

Passing the whole entity, instead of just the values of that entity, blurs the division of responsibility. The block plugins (back to xjm's point earlier) don't know anything about CRUD because block plugins don't, and more importantly SHOULDN'T care about CRUD. That is the administrative wrapper around blocks' problem. Also, passing the whole entity isn't actually beneficial to us in any way I can see. It gives the block the ability to perform unexpected tasks, the old blocks can do everything these can do (at least with regard to updating their internal configuration). None of this requires a config object/entity. It is then the responsibility of the wrapper to take anything in BlockBase::configuration and parse and save that appropriately for the entity. This is actually something I would totally support the Form Controller doing. It's a wrapper around returned plugin configuration, it makes perfect sense for this to exist there.

Stuff in this patch:

Block::getPlugin() does the following at one point:

$this->instance = drupal_container()->get('plugin.manager.block')->createInstance($this->plugin, $this->settings, $this);
$this->settings += $this->instance->getConfig();

From speaking with timplunkett, $this->settings is always an empty array on create, and is stuffed with values from the CMI object on load. So, in the case of create, we literally pass an empty array for values, and the full Block entity, nesting a block entity within a block plugin within the same block entity. CreateInstance() isn't even intended to take that 3rd parameter, and with good reason, it expects the second parameter to contain all the data it needs in order to configure itself appropriately. All of this is worrisome in general when you consider the fact that before this patch, I didn't need an entity to get a block one off block instance, and now I do. In fact, not only do I need that instance, but using the actual plugin manager is basically out of the question now because "Hey, blocks should be entities."

All of this is exceptionally frustrating when you compare with CMI's raw calls itself. ConfigEntities don't even wrap a CMI object, they utilize a storage controller that wraps a CMI object, and you can't really even get at that object even knowing all of that. Standard get() method calls for CMI objects don't work for entities because entities implement a method of the same name and it requires a parameter in entity's case. Plus ConfigEntities extend Entity, not Config, and perhaps that's as it should be, but I like the config system a lot better for what I was doing here than I am ever likely to like ConfigEntities in this instance.

In closing, everything is a freaking entity. Entities are entities, and apparently config is an entity, but it doesn't expose the config object ($entity->getConfig()?? it's apparently what we did for block plugins) and blocks are entities and who knows what else has been converted in this way. There's plenty of code that's trying to abstract this whole configurable plugin thing right now, and basically we're at a point where "configurable" is a pure 100% drupalism. You can't get anything seriously considered that's not leveraging entities, and somehow we replaced apis that worked in their individual areas with... you guessed it, the entity API. I'm using ConfigEntities in my work with ConditionGroups right now, and I really like it there. But I have serious reservations about the implementation in this patch. That being said, I've resigned myself to seeing how this all pans out and filing follow up issues if needed, but I worry very seriously about the message this sends and how far reaching this is. I've let a few things slide in the Plugin system already and am paying in spades for it. I'd have preferred to prevent any chance of that here. Oh well.

Eclipse

I spent a good 20 minutes looking this over, and it looks great, and very nice, consistent clean-up. I still am a little wavery on EclipseGc's specific concerns since it gets into details of the plugin system architecture that I don't totally understand, but have been informed there's a follow-up issue to discuss it more in-depth so that sounds good to me.

Onward with making SCOTCH rock!

I'm going to go out on a limb and imagine that this needs a change notice. :)

This issue's commit is causing installation to fail:

Fatal error: Call to a member function getPlugin() on a non-object in /Users/erik/www/drupal8/core/modules/block/lib/Drupal/block/BlockStorageController.php

I've created an issue at #1891188: Installation fails with Call to getPlugin() on a non-object..

The problem was caused by old files (created by the code pre-dating this current issue) in the sites/default/files directory. Clearing the files directory fixed the problem.

Since #1535868: Convert all blocks into plugins is just about at 300 comments, and this one needs a change notice and might as well add it into the same one, I'll put some bugs/suggestions I found at http://drupal.org/node/1880620 here. Overall, btw, that change notice is actually super helpful, w/ the way it's laid out, cross-linking to all the "WTF IS THIS?!?!" stuff (PSR-0, annotations, etc.) Well done.

1) Under "Defining a block type" example, the hook that's put there is blockSettings(). However, very few blocks actually expose blockSettings; probably blockBuild() would be better. (Maybe just copy/paste SystemPoweredByBlock as a very rudimentary example?)

2) However! There is no more blockBuild(). :) Now it's just build(). (I think). So the example under "Defining a block type's output" is invalid and needs to be updated.

3) Presumably so do all of the other examples on this page.

It'd be great if we could try and prioritize this, assuming no other major API changes are coming down the pipe soon, because that change notice is really something you end up clinging to as you wade your way into D8.

Yeah, in fact, since if you don't implement the build function you get an error like this:

Fatal error: Class Drupal\pants\Plugin\block\block\PantsChangeBlock contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\block\BlockInterface::build) in /Users/webchick/Sites/8.x/modules/pants/lib/Drupal/pants/Plugin/block/block/PantsChangeBlock.php on line 23

I would upgrade that recommendation in #121.1 to a requirement, probably.

I'm a little bit confused and spent a good chunk of time with hopping through entity + plugin classes, but it's close to impossible to figure out what's going on under the hood now.

In a test, I have this:

    $this->block = $this->drupalPlaceBlock('menu_navigation', array('region' => 'account'), array(
      'menu' => 'account',
      'level' => 0,
      'id' => 'secondary-menu',
    ));
    $this->blockInstance = $this->container->get('plugin.manager.block')->???

1. drupalPlaceBlock() returns the block entity, not the block plugin instance.
2. Now, plugin.manager.block points at Drupal\block\Plugin\Type\BlockManager.
3. No idea why BlockManager is in \Type, that's not a plugin type, but a plugin manager.
4. BlockManager extends Drupal\Component\Plugin\PluginManagerBase, which has a getInstance() method, but hey, it takes $options as the only parameter, and its docblock refers to PluginManagerInterface::getInstance()... but that method is not documented *at all* on the interface.
5. Furthermore, PluginManagerBase::getInstance() actually calls into $this->mapper->getInstance(), but where does $this->mapper come from?!? It's neither assigned by PluginManagerBase, nor by BlockManager. And there is no other class in between. What kind of utterly ugly magic is going on there? :)

In short: I'm completely lost there right now. :-/

To make up for this rant and combat another/different weirdness, I filed #1899206: Enhance ConfigEntityBase::get() to support dot-delimited property names

Aside from that, I also don't know why the signature of drupalPlaceBlock() was changed once more, but as I've mentioned before already, a plain and simple $plugin_id, $values = array() signature is what test authors need and expect. Two options-style arrays are bad DX. Just += merge $values with defaults and auto-generated values as needed. A 'settings' key in $values is much more expected than two separate $values and $settings, which is absolutely weird.

drupalPlaceBlock() should and does return the entity, the plugin instance is never used outside the entity.
PluginManagerBase implements MapperInterface. And yes, if you call getInstance right now on half the managers, it will blow up. Nothing to do with blocks.
Yes, the \Type directory should just go away. I'll open an issue.
And, wtf, you are the one who asked for drupalPlaceBlock() to take three parameters. xjm was against it, but you asked for it and I did it...

@sun - re 5: http://drupal.org/node/1894130

Thanks for the clarifications.

Is anyone able to fill me up and thus unblock the other core issue I'm working on? All I'm seeing in this patch are calls in the form of

  $instance = $block->getPlugin();

...and while I'm happy to admit that I could simply do the same in my innocent test, I'd still like to understand "WTF I'm doing" ;)

In other words, what's the equivalent of that for this:

  $instance = $this->container->get('plugin.manager.block')->???

Any clues, anyone? :)

AFAICS, getPlugin() performs some higher-level logic (which most probably shouldn't be in that helper method), and then calls into createInstance() instead of getInstance(), which generally speaking means that we're creating a new instance from scratch even if there could be a (loaded) plugin instance already.

It looks like I'd have to resemble this entire code snippet of getPlugin(), in order to retrieve the actual plugin instance of a block:

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -0,0 +1,194 @@
+  public function getPlugin() {
+    if (!$this->instance) {
...
+      // Create a plugin instance and store its configuration as settings.
+      try {
+        $this->instance = drupal_container()->get('plugin.manager.block')->createInstance($this->plugin, $this->settings, $this);
+        $this->settings += $this->instance->getConfig();
+      }
...
+    }
+    return $this->instance;
+  }

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php
@@ -31,8 +31,14 @@ public function __construct() {
+  /**
+   * 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);
   }

BlockManager::createInstance() takes an additional, non-interface-compliant argument that contains the block entity object, which in turn duplicates the plugin settings.

However. The $entity is passed forward as-is, and the $configuration is also passed forward as-is. Thus, both $entity->settings AND $configuration contain the effective configuration for the block plugin instance. One of both naturally must win in case of overlaps, but unless I'm missing something, the code that handles this is not part of the committed patch, so must have existed before already.

Lastly, thinking some more through this, I've the impression that the design is reversed:

1. A plugin instance contains a block entity.
2. But yet, a block entity loads the plugin instance. The plugin instance does not appear to load the block entity.

In other words, inside-out: Right now, it looks like the inner thing loads the outer thing. Normally, the outer thing would load the inner thing, and the inner thing has no idea of the outer thing.

With regard to plugins that are based on configuration, we have a slight but important double-notion of what exactly "instance" actually means though: The "plugin instance" has a pure meaning in runtime code only. But alas, only the configuration for a plugin instance actually "declares" the plugin instance for realz. In turn, the "inner" and "outer" things are extremely abstract in that constellation, which, in turn, leads to a "funky DX." ;)

There is no reason to be accessing the block manager (plugin.manager.block) directly. Can you link to the issue you are working on?

The entity is the "instance", in that it is a configured block plugin that has been placed into a region of a theme.

Yes, Entity\Block::getPlugin() calls BlockManager::createInstance() each time. Almost all of the logic of getPlugin() could be moved into BlockManager::getInstance() if desired, but that will still end up calling createInstance(). See all of the implementations of getInstance() in core, they all call createInstance().

The block entity is passed to the block plugin at the request of several people above, I can't remember at this point. Before, I was only passing it to the individual block plugin methods that needed it. This is exactly what #1868772: Convert filters to plugins is doing, since as you stated there, the filters need information from the format.

So, apparently:

$block->get('settings') === $block->getPlugin()->getConfig()

These are identical, the same thing. Even though those settings contain a 'subject' key, I've yet to figure out what is actually used as the "title" of a block when being rendered.

The 'subject' for a block (plugin instance?) entity does not seem to get populated by default, which leads me to believe that the fallback title is the 'label' property of the block entity...?

I'm unable to test/verify that through regular debugging, since var_dump($block) blows up with:

Nesting level too deep - recursive dependency?

:-/

Well, in Block::getPlugin() $this->settings += $this->instance->getConfig();
So everything in the plugin config is in the block settings, and I'm not coming up with an example off hand where it contains something else.

The block plugin subject is *only* used to prepopulate the label form field on BlockFormController. That is when you have the opportunity to use that subject, set your own, or delete it. Block::label() is used in BlockRenderController::getBuildDefaults(). Also, the BlockInterface::blockBuild() method can override it, as ViewsBlock chooses to do.

Of course you can't var_dump() an entity, that's not block specific.

The problem with having a plugin manager that's supposed to manage something, but factually does not, because you intentionally circumvent it by directly calling into createInstance()...

...is this: #1901380: [Block]PluginManager repetitively triggers full-stack Annotation parsing on a plugin instance ID miss

This has been waiting for a change notice for over 4 weeks now. Let's please get this knocked out.

over 6 weeks now (

And I'd like for it to continue waiting until at least #1927608: Remove the tight coupling between Block Plugins and Block Entities and #1941948: Remove the tight coupling between Block Plugin Forms and the Block Entity Form Controller are both in. This will fundamentally change the nature of the interaction, and that will be very important to the Change Notice.

Eclipse

looks like this one postponed due to #133
#1927608: Remove the tight coupling between Block Plugins and Block Entities
#1941948: Remove the tight coupling between Block Plugin Forms and the Block Entity Form Controller

https://drupal.org/contributor-tasks/draft-change-notification
is instructions for drafting a change notice.
Anyone can try the first draft. :)
Just make a comment saying you are going to try, and ask questions if you have any.

We might be able to use https://drupal.org/node/1880620 as the change notice for this, and update the info there. (See #121)

UX change #1995046-2: BlockListController doesn't call parent::getOperations() and so "delete" is the default dropbutton operation

#2083415: [META] Write up all outstanding change notices before release.

Removing myself from the author field so I can unfollow. --xjm

Added a change notice for the main change itself (that blocks are config entities) with a short example on changing something using the API on the block. https://drupal.org/node/2187853 I don't think we need to document all the block API in this change notice, that will be / is the role of the docs. Ie. defining block forms, altering block forms, etc. (Which indeed was changed several times *after* this patch landed).

I think the main reason the block change notices are not being written is the set of changes are a maze and hard to figure out which issue should document what. Much better focus of our time would be to document defining blocks and working with them properly (which "accidentally" is the same as documenting how to define config entity forms, altering them, etc. obviously :D)