Problem/Motivation

This is a followup issue for #1535868: Convert all blocks into plugins.

Proposed resolution

  • Convert block instances to configuration entities.
  • Add test coverage for the integration of blocks with the configuration system, including:
    • Block instance CRUD and storage.
    • Config and entity hook implementations.
    • The installation of default blocks.
    • Decoupled test coverage for the ConfigMapper in whatever form it exists following the conversion.

    Use the test implementation added in #1874830: Add a test block plugin implementation with test coverage

Remaining tasks

Follow-ups

Related issues

API changes

  • The machine name for the block title is now label instead of subject.
  • IDs of some default core blocks changed.
  • block_admin_configure() removed.
  • hook_block_view_alter() parameter change.
Files: 
CommentFileSizeAuthor
#112 blocks-1871696-112.patch160.64 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,693 pass(es). View
#112 interdiff.txt2.22 KBtim.plunkett
#108 blocks-1871696-108.patch164.66 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,565 pass(es). View
#108 interdiff.txt1.2 KBtim.plunkett
#106 blocks-1871696-106.patch163.79 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,470 pass(es), 44 fail(s), and 76 exception(s). View
#106 interdiff.txt34.97 KBtim.plunkett
#103 blocks-1871696-103.patch181.54 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,979 pass(es), 4 fail(s), and 5 exception(s). View
#103 interdiff.txt2.5 KBtim.plunkett
#102 blocks-1871696-102.patch181.04 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#102 block-base.txt18.07 KBtim.plunkett
#102 block-interface.txt2.82 KBtim.plunkett
#102 interdiff.txt29.52 KBtim.plunkett
#94 blocks-1871696-94.patch194.6 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,516 pass(es). View
#91 blocks-1871696-91.patch195.18 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,440 pass(es). View
#91 interdiff.txt15.82 KBtim.plunkett
#89 blocks-1871696-89.patch185.01 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,434 pass(es). View
#89 interdiff.txt2.87 KBtim.plunkett
#89 block-interface.txt3.36 KBtim.plunkett
#88 delete_works.png7 KBxjm
#88 blank_title.png6.61 KBxjm
#88 custom_block_title.png2.95 KBxjm
#87 blocks-1871696-87.patch184.57 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,262 pass(es). View
#87 interdiff.txt3.45 KBtim.plunkett
#83 blocks-1871696-83.patch184.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,256 pass(es). View
#83 interdiff.txt6.1 KBtim.plunkett
#79 blocks-1871696-79.patch184.02 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,388 pass(es). View
#79 interdiff.txt1.83 KBtim.plunkett
#77 blocks-1871696-77.patch183.84 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#77 interdiff.txt25.27 KBtim.plunkett
#75 blocks-1871696-75.patch185.27 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,278 pass(es). View
#75 interdiff.txt6.99 KBtim.plunkett
#73 blocks-1871696-73.patch185.9 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,209 pass(es). View
#73 interdiff.txt671 bytestim.plunkett
#71 blocks-1871696-71.patch185.24 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,188 pass(es), 1 fail(s), and 0 exception(s). View
#71 interdiff.txt1.97 KBtim.plunkett
#69 blocks-1871696-69.patch184.05 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,324 pass(es), 3 fail(s), and 0 exception(s). View
#69 interdiff.txt40.38 KBtim.plunkett
#66 blocks-1871696-66.patch162.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,293 pass(es). View
#66 interdiff.txt11.31 KBtim.plunkett
#65 blocks-1871696-65.patch157.88 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,181 pass(es). View
#65 interdiff.txt8.55 KBtim.plunkett
#60 blocks-1871696-60.patch150.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,083 pass(es), 0 fail(s), and 36 exception(s). View
#56 blocks-1871696-56.patch156.17 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-1871696-56.patch. Unable to apply patch. See the log in the details link for more information. View
#56 interdiff.txt4.25 KBtim.plunkett
#55 blocks-1871696-55.patch147.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,072 pass(es). View
#54 interdiff.txt1.64 KBtim.plunkett
#52 blocks-1871696-52.patch147.84 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,080 pass(es), 5 fail(s), and 2 exception(s). View
#52 interdiff.txt8.71 KBtim.plunkett
#51 interdiff.txt3.54 KBtim.plunkett
#49 blocks-1871696-49.patch144.01 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,097 pass(es). View
#49 interdiff.txt1.94 KBtim.plunkett
#47 blocks-1871696-47.patch143.67 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,103 pass(es), 0 fail(s), and 10 exception(s). View
#47 interdiff.txt659 bytestim.plunkett
#45 blocks-1871696-45.patch143.67 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,117 pass(es), 1 fail(s), and 38 exception(s). View
#45 interdiff.txt10.11 KBtim.plunkett
#42 blocks-1871696-42.patch142.78 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,081 pass(es). View
#42 interdiff.txt10.66 KBtim.plunkett
#39 blocks-1871696-39.patch143.29 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,939 pass(es). View
#39 interdiff.txt8.78 KBtim.plunkett
#36 blocks-1871696-36.patch137.65 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,911 pass(es), 24 fail(s), and 8 exception(s). View
#32 blocks-1871696-32.patch135.62 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,201 pass(es), 14 fail(s), and 5 exception(s). View
#32 interdiff.txt722 bytestim.plunkett
#29 blocks-1871696-29.patch135.62 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
#29 interdiff.txt8.79 KBtim.plunkett
#27 interdiff.txt12.38 KBtim.plunkett
#27 blocks-1871696-27.patch134.98 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,929 pass(es), 12 fail(s), and 5 exception(s). View
#25 interdiff.txt11.01 KBtim.plunkett
#25 blocks-1871696-25.patch128.56 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,977 pass(es), 17 fail(s), and 1 exception(s). View
#22 blocks-1871696-22.patch130.87 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,769 pass(es), 81 fail(s), and 1 exception(s). View

Comments

David_Rothstein’s picture

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?

xjm’s picture

@EclipseGc's reply in the other issue:

This is true, but only for core, not for contrib. 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. I was in exactly the same frame of mind as you when I wrote these portions of the patch, I've since been convinced that I was wrong.

tim.plunkett’s picture

Title: Convert blocks to configuration entities to resolve regressions and architectural issues » Convert block instances to configuration entities to resolve regressions and architectural issues

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

xjm’s picture

Title: Convert block instances to configuration entities to resolve regressions and architectural issues » Convert blocks to configuration entities to resolve regressions and architectural issues

To clarify, here is the hook signature from D7:

function hook_block_save($delta = '', $edit = array())

Most modules check $delta and respond only to their deltas, but you can also take whatever action you want regardless of what the delta is. It gets invoked here: http://api.drupal.org/api/drupal/modules%21block%21block.admin.inc/funct...

xjm’s picture

Title: Convert blocks to configuration entities to resolve regressions and architectural issues » Convert block instances to configuration entities to resolve regressions and architectural issues

Yes, that's what I meant.

tim.plunkett’s picture

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.

David_Rothstein’s picture

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....

David_Rothstein’s picture

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

xjm’s picture

Oh bah, @tim.plunkett and @David_Rothstein are correct -- I'd completely forgotten, but yes, in the past I've replaced the whole dang submit handler. So maybe it's not a regression.

I still think it's critical for the other reasons in the summary, though. Just a task rather than a bug.

xjm’s picture

Title: Convert block instances to configuration entities to resolve regressions and architectural issues » Convert block instances to configuration entities to resolve architectural issues
Category: bug » task

À propos?

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: +Needs tests
xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Block plugins are also currently coupled to the procedural FAPI code in block.admin.inc:

  • block_admin_configure() adds the $form['theme'] element that is referenced inside BlockBase::form(), which creates a circular dependency.
  • block_admin_configure_submit() is the only way to save a block instance. In addition to calling the block's submit() method, it namespaces the block instance configuration object (hardcoded) and writes the object to the configuration system.
  • block_admin_block_delete_submit() is similarly the only way to delete a block instance.
xjm’s picture

xjm’s picture

Something we'll need to take into account is block derivatives, and how we respond to changes in the derivative definitions. E.g., if you delete a menu, you're presumably also deleting all menu block instances that use that menu. I'm not sure if this happens currently, but we should make sure to cover that case and add test coverage for it.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Postponed » Active
tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

I'll work on this.

EclipseGc’s picture

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

xjm’s picture

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.

I completely forgot about this; I noted it in a review back in early December but forgot to follow up. IIRC, currently the Block module is implementing a menu hook, which is backwards--block shouldn't care about specific block definitions from other modules. Menu should be responsible for that action, or the subsystem that handles the plugin/config interaction should.

Edit: Here's the snippet:

+++ b/core/modules/block/block.moduleundefined
@@ -1028,23 +645,29 @@ function template_preprocess_block(&$variables) {
 /**
  * Implements hook_menu_delete().
  */
 function block_menu_delete($menu) {
-  db_delete('block')
-    ->condition('module', 'menu')
-    ->condition('delta', $menu['menu_name'])
-    ->execute();
-  db_delete('block_role')
-    ->condition('module', 'menu')
-    ->condition('delta', $menu['menu_name'])
-    ->execute();
+  $block_configs = config_get_storage_names_with_prefix('plugin.core.block');
+  foreach ($block_configs as $config_id) {
+    $config = config($config_id);
+    if ($config->get('id') == 'menu_menu_block:' . $menu['menu_name']) {
+      $config->delete();
+    }
+  }
tstoeckler’s picture

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...)

EclipseGc’s picture

#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.

tim.plunkett’s picture

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

tim.plunkett’s picture

Status: Active » Needs review
FileSize
130.87 KB
FAILED: [[SimpleTest]]: [MySQL] 49,769 pass(es), 81 fail(s), and 1 exception(s). View

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.

tim.plunkett’s picture

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');
tim.plunkett’s picture

FileSize
128.56 KB
FAILED: [[SimpleTest]]: [MySQL] 49,977 pass(es), 17 fail(s), and 1 exception(s). View
11.01 KB

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

tim.plunkett’s picture

FileSize
134.98 KB
FAILED: [[SimpleTest]]: [MySQL] 49,929 pass(es), 12 fail(s), and 5 exception(s). View
12.38 KB

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

tim.plunkett’s picture

Status: Needs review » Needs work
FileSize
8.79 KB
135.62 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
722 bytes
135.62 KB
FAILED: [[SimpleTest]]: [MySQL] 50,201 pass(es), 14 fail(s), and 5 exception(s). View
tim.plunkett’s picture

FileSize
137.65 KB
FAILED: [[SimpleTest]]: [MySQL] 49,911 pass(es), 24 fail(s), and 8 exception(s). View

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

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.78 KB
143.29 KB
PASSED: [[SimpleTest]]: [MySQL] 49,939 pass(es). View

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

aspilicious’s picture

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() {
tim.plunkett’s picture

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

tim.plunkett’s picture

FileSize
10.66 KB
142.78 KB
PASSED: [[SimpleTest]]: [MySQL] 49,081 pass(es). View

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

sun’s picture

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.

tim.plunkett’s picture

Status: Needs review » Needs work

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
10.11 KB
143.67 KB
FAILED: [[SimpleTest]]: [MySQL] 49,117 pass(es), 1 fail(s), and 38 exception(s). View

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.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

tim.plunkett’s picture

FileSize
659 bytes
143.67 KB
FAILED: [[SimpleTest]]: [MySQL] 49,103 pass(es), 0 fail(s), and 10 exception(s). View

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue tags: -VDC
FileSize
1.94 KB
144.01 KB
PASSED: [[SimpleTest]]: [MySQL] 49,097 pass(es). View

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().

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

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

tim.plunkett’s picture

FileSize
3.54 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.71 KB
147.84 KB
FAILED: [[SimpleTest]]: [MySQL] 49,080 pass(es), 5 fail(s), and 2 exception(s). View

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.

tim.plunkett’s picture

FileSize
1.64 KB

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.

tim.plunkett’s picture

FileSize
147.95 KB
PASSED: [[SimpleTest]]: [MySQL] 49,072 pass(es). View

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

tim.plunkett’s picture

Assigned: tim.plunkett » EclipseGc
FileSize
4.25 KB
156.17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-1871696-56.patch. Unable to apply patch. See the log in the details link for more information. View

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.

tim.plunkett’s picture

FileSize
150.22 KB
FAILED: [[SimpleTest]]: [MySQL] 49,083 pass(es), 0 fail(s), and 36 exception(s). View

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

yched’s picture

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

tim.plunkett’s picture

@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".

tim.plunkett’s picture

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

Still needs review.

tim.plunkett’s picture

FileSize
8.55 KB
157.88 KB
PASSED: [[SimpleTest]]: [MySQL] 49,181 pass(es). View

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.

tim.plunkett’s picture

Assigned: EclipseGc » Unassigned
FileSize
11.31 KB
162.85 KB
PASSED: [[SimpleTest]]: [MySQL] 49,293 pass(es). View

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?

tim.plunkett’s picture

Issue tags: -Needs tests

Removing tags.

tim.plunkett’s picture

Issue summary: View changes

Adding followup

tim.plunkett’s picture

Issue summary: View changes

Add follow-up, clarified scope

xjm’s picture

tim.plunkett’s picture

FileSize
40.38 KB
184.05 KB
FAILED: [[SimpleTest]]: [MySQL] 49,324 pass(es), 3 fail(s), and 0 exception(s). View

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.

tim.plunkett’s picture

FileSize
1.97 KB
185.24 KB
FAILED: [[SimpleTest]]: [MySQL] 49,188 pass(es), 1 fail(s), and 0 exception(s). View

A couple small test fixes.

tim.plunkett’s picture

FileSize
671 bytes
185.9 KB
PASSED: [[SimpleTest]]: [MySQL] 49,209 pass(es). View

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

xjm’s picture

Alright, I've reviewed everything but the changes to block.module itself, about 60% of the patch.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
    @@ -298,7 +298,7 @@ public function save(EntityInterface $entity) {
    -    $is_new = $config->isNew();
    +    $is_new = $entity->isNew();
    

    Is this the reason for the test change I saw earlier? This doesn't seem like it should be happening in this patch. If there's a bug (which would not surprise me given how much fun we've had with isNew()), shouldn't it be filed separately?

  2. +++ b/core/modules/aggregator/aggregator.moduleundefined
    @@ -371,11 +371,9 @@ function aggregator_save_category($edit) {
    -      $block_configs = config_get_storage_names_with_prefix('plugin.core.block');
    -      foreach ($block_configs as $config_id) {
    -        $config = config($config_id);
    -        if ($config->get('id') == 'aggregator_category_block:' . $edit['cid']) {
    -          $config->delete();
    +      if (module_exists('block')) {
    +        foreach (entity_load_multiple_by_properties('block', array('plugin' => 'aggregator_category_block:' . $edit['cid'])) as $block) {
    +          $block->delete();
    

    Nice. Also, note to self, Aggregator is actually properly cleaning up its derivative instances.

  3. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockLanguageTest.phpundefined
    @@ -89,19 +89,23 @@ public function testLanguageBlockVisibility() {
    -    $config = config('plugin.core.block.' . $default_theme . '.language_block_test');
    -    $setting = $config->get('visibility.language.langcodes.fr');
    +    $visibility = $block->get('visibility');
    +    $setting = $visibility['language']['langcodes']['fr'];
    

    Interesting. Is it not possible to select it directly from config anymore, or just bad practice?

  4. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockStorageUnitTest.phpundefined
    @@ -0,0 +1,207 @@
    +class BlockStorageUnitTest extends DrupalUnitTestBase {
    

    Excellent.

  5. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockStorageUnitTest.phpundefined
    @@ -0,0 +1,207 @@
    +  public function testBlockCRUD() {
    ...
    +  protected function createTests() {
    ...
    +  protected function deleteTests() {
    ...
    +  public function testDefaultBlocks() {
    

    I suppose it doesn't really matter that much, but could we move the helpers before all the test methods? I accidentally deleted a couple test methods in #1874598: Add BlockTestBase because they were scattered about among the test methods.

  6. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockStorageUnitTest.phpundefined
    @@ -0,0 +1,207 @@
    +    $expected = array();
    +    $expected[] = '  <div id="block-test-block"  class="block block-block-test">';
    +    $expected[] = '';
    +    $expected[] = '    ';
    +    $expected[] = '  <div class="content">';
    +    $expected[] = '      </div>';
    +    $expected[] = '</div>';
    +    $expected[] = '';
    +    $expected_output = implode("\n", $expected);
    

    Hm, interesting... Do other tests test rendering like this? I'm not sure about testing the specific markup or whitespace, either. Finally, I think elsewhere in core we use the heredoc syntax in cases like this? (Not that we do this frequently or consistently.)

  7. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockStorageUnitTest.phpundefined
    @@ -0,0 +1,207 @@
    +    $entity->set('label', 'Powered by Bananas');
    

    :)

  8. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -110,21 +108,23 @@ public function testCustomBlock() {
    +    // @todo This assumes that a block's content can be rendered without its
    +    //   wrappers. If this is a reasonable expectation, it should be documented
    +    //   elsewhere.
    

    I think there's an open issue related to this? And/or we should file a followup. Unless I'm thinking of a followup you already filed?

  9. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -399,12 +392,13 @@ function testBlockModuleDisable() {
         // Disable the block test module and refresh the definitions cache.
         module_disable(array('block_test'), FALSE);
         $this->assertFalse(module_exists('block_test'), 'Test block module disabled.');
    +    // @todo Remove this.
         $manager->clearCachedDefinitions();
    

    See, this (and similar) I don't think we can or should remove, unlike the ones at the beginnings of test methods. module_enable() does not refresh any caches; it happens in the submission handling, by design. within a test method. So if test methods need uncached data, they have to clear the caches themselves, not just for blocks.

  10. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    --- /dev/null
    +++ b/core/modules/block/tests/config/plugin.core.block.stark.test_block.ymlundefined
    

    Excellent.

  11. +++ b/core/modules/block/tests/config/plugin.core.block.stark.test_block.ymlundefined
    @@ -0,0 +1,13 @@
    +uuid: 6dcd7278-40c5-426a-9804-4fd859748a30
    

    It has a UUID? Our other exported block instances don't. Is that just because they predate the entity conversion, and should they have one, or should this one not?

  12. +++ b/core/modules/block/tests/config/plugin.core.block.stark.test_block.ymlundefined
    @@ -0,0 +1,13 @@
    +  subject: 'Test block html id'
    

    HTML ID, not html id :(

  13. +++ b/core/modules/book/lib/Drupal/book/Tests/BookTest.phpundefined
    @@ -345,12 +345,16 @@ function testNavigationBlockOnAccessModuleEnabled() {
    +    // Test the 'book pages' block_mode setting.
    +    $this->drupalGet('<front>');
    +    $this->assertNoText($block->label(), 'Book navigation block is not shown on non-book pages.');
    

    Huh, so this didn't have coverage previously?

  14. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentBlockTest.phpundefined
    @@ -46,7 +46,7 @@ public static function getInfo() {
    -    $block = $this->drupalPlaceBlock('recent_comments', array('block_count' => 2));
    +    $block = $this->drupalPlaceBlock('recent_comments', array(), array('block_count' => 2));
    

    So with two arguments instead of three, this would be:

    $this->drupalPlaceBlock('recent_comments', array(
      'configuration' => array(
        'block_count' => 2,
      ),
    ));
  15. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.phpundefined
    @@ -143,7 +143,7 @@ function testCRUD() {
    -    $this->assertIdentical($status, SAVED_UPDATED);
    +    $this->assertIdentical($status, SAVED_NEW);
    

    Huh? This seems totally out of the scope of the patch. Why is this necessary? Is there some change to ConfigEntity entangled with this patch?

  16. +++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.phpundefined
    @@ -59,7 +59,7 @@ public function testNewForumTopicsBlock() {
    -    $this->assertText(t('The block configuration has been saved.'), '"New forum topics" block was enabled');
    

    Oops, that assertion should have been deleted in #1872540: Provide a test helper method for creating block instances. My bad.

  17. +++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.phpundefined
    @@ -90,7 +89,7 @@ public function testNewForumTopicsBlock() {
    -  public function testActiveForumTopicsBlock() {
    +  public function _testActiveForumTopicsBlock() {
    

    Um... I don't think we should be disabling this test.

  18. +++ b/core/modules/language/language.moduleundefined
    @@ -47,7 +47,8 @@ function language_help($path, $arg) {
    +    case 'admin/structure/block/manage/%':
    +    case 'admin/structure/block/add/%/%':
    

    Presumably due to the form controller conversion again?

  19. +++ b/core/modules/menu/menu.moduleundefined
    @@ -433,7 +434,7 @@ function menu_reset_item($link) {
     function menu_block_view_alter(&$build, $block) {
    ...
    -  if ($block instanceof SystemMenuBlock) {
    +  if ($block->getPlugin() instanceof SystemMenuBlock) {
    

    Huh. So this alter hook now passes the entity, rather than the instantiated plugin. That makes a lot more sense to me. Also, we should add typehints to this function.

  20. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeSyndicateBlockTest.phpundefined
    @@ -41,6 +41,7 @@ function setUp() {
       public function testSyndicateBlock() {
         // Place the "Syndicate" block and confirm that it is rendered.
         $this->drupalPlaceBlock('node_syndicate_block', array('machine_name' => 'test_syndicate_block'));
    +    $this->drupalGet('');
         $this->assertFieldByXPath('//div[@id="block-test-syndicate-block"]/*', NULL, 'Syndicate block found.');
    

    Huh. I'm not sure whether to ask why this was necessary, or how it ever worked in the first place. Because the drupalPost() followed the redirect after posting, of course.

  21. +++ b/core/modules/node/node.moduleundefined
    @@ -1986,14 +1986,13 @@ function theme_node_recent_content($variables) {
    - * Implements hook_form_FORM_ID_alter() for block_admin_configure().
    + * Implements hook_form_FORM_ID_alter() for block_form().
    ...
    - * @see node_form_block_admin_configure_submit()
    

    Presumably the result of the form controller conversion. API change.

  22. +++ b/core/modules/node/node.moduleundefined
    @@ -1986,14 +1986,13 @@ function theme_node_recent_content($variables) {
    +function node_form_block_form_alter(&$form, &$form_state) {
    

    I meant to suggest this on the original issue, but we probably want to file an "If SCOTCH fails" followup to move this out of a form alter. Presumably Layouts will have a better solution.

  23. +++ b/core/modules/node/node.moduleundefined
    @@ -1986,14 +1986,13 @@ function theme_node_recent_content($variables) {
    +  $block = $form_state['entity'];
    

    The entity is finally in entity in the form state now? Blessed be. There's an issue out there somewhere I can close if this is fixed everywhere because of the entity form controller.

  24. +++ b/core/modules/node/node.moduleundefined
    @@ -2005,39 +2004,21 @@ function node_form_block_admin_configure_alter(&$form, &$form_state) {
    - * Implements hook_modules_uninstalled().
    - *
    - * Cleans up the {block_node_type} table from modules' blocks.
    

    So this patch also removes that table from the schema? Another API change, and we should add the table to that list of tables to remove in 9.x. Edit: Also remove it from node_schema().

  25. +++ b/core/modules/overlay/overlay.moduleundefined
    @@ -477,8 +477,7 @@ function overlay_block_access($block) {
    -    $config = $block->getConfig();
    -    if (!in_array($config['region'], $regions_to_render)) {
    +    if (!in_array($block->get('region'), $regions_to_render)) {
    

    Yay. Is getConfig() no longer necessary with this conversion?

  26. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
    @@ -347,69 +347,43 @@ protected function drupalCreateContentType($settings = array()) {
    +   * @param array $values
    +   *   (optional) An associative array of values for the block entity.
    +   *   Override the defaults by specifying the key and value in the array, for
    ...
    +   *       'label' => t('Hello, world!'),
    ...
    +   *   - label: Random string.
    ...
    +   *   - theme: The default theme.
    +   * @param array $configuration
    +   *   (optional) An associative array of plugin-specific configuration.
    ...
    +  protected function drupalPlaceBlock($plugin_id, array $values = array(), array $configuration = array()) {
    ...
    +    $values['configuration'] = $configuration;
    

    Do we need two arguments? Can we just add the configuration as one of the keys for $values? A little bit confusing.

  27. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
    @@ -347,69 +347,43 @@ protected function drupalCreateContentType($settings = array()) {
    +    $block = entity_create('block', $values);
    

    Yay, faster tests!

  28. +++ b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsReportsTest.phpundefined
    @@ -72,8 +72,8 @@ function testAccessLogging() {
         // Clear the block cache to load the Statistics module's block definitions.
    ...
    +    // @todo Remove this.
    +    $this->container->get('plugin.manager.block')->clearCachedDefinitions();
    

    There's something different between SimpleTest installation and normal installation that causes the definitions cache to not contain derivative definitions at the beginning of tests; do you think that this needs to be fixed for simpletest somehow?

  29. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityCrudHookTest.phpundefined
    @@ -67,6 +67,54 @@ protected function assertHookMessageOrder($messages) {
    +   * Tests hook invocations for CRUD operations on blocks.
    

    Yay. Did we file the followup yet to add coverage for the other config entities?

  30. +++ b/core/modules/user/lib/Drupal/user/Tests/UserBlocksTests.phpundefined
    @@ -80,9 +80,8 @@ function testUserLoginBlock() {
    -    $plugin_id = 'plugin.core.block.' . variable_get('theme_default', 'stark') . '.online';
    -    $block = $this->container->get('plugin.manager.block')->getInstance(array('config' => $plugin_id));
    -    $config = $block->getConfig();
    +    $block = entity_load('block', 'stark.online');
    +    $config = $block->get('configuration');
    

    Hooray.

  31. +++ b/core/modules/user/lib/Drupal/user/Tests/UserBlocksTests.phpundefined
    @@ -100,7 +99,7 @@ function testWhosOnlineBlock() {
    -    $content = $block->build();
    +    $content = entity_view($block, 'block');
    

    Yay yay yay

  32. +++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
    @@ -71,10 +72,10 @@ public function blockForm($form, &$form_state) {
    +  public function blockBuild(Block $block) {
    ...
    -    // Set the subject to the title configured in the view.
    -    $this->configuration['subject'] = filter_xss_admin($this->view->getTitle());
    +    // Set the label to the title configured in the view.
    +    $block->set('label', filter_xss_admin($this->view->getTitle()));
    

    YAY. This is much more consistent and intuitive. This also helps clarify why it's taking the Block entity as an argument, though it still seems like $this->set() or $this->something->set() would make sense. I'm still a little confused as to why passing the argument is necessary.

  33. +++ b/core/modules/views/lib/Drupal/views/Tests/UI/OverrideDisplaysTest.phpundefined
    @@ -58,7 +58,7 @@ function testOverrideDisplays() {
    -    $this->drupalPlaceBlock("views_block:{$view['name']}-block_1");
    +    $this->drupalPlaceBlock("views_block:{$view['name']}-block_1", array('label' => ''));
    

    Hm, why is this change required? And how will that work with #1875260: Make the block title required and allow it to be hidden? Do I need to postpone that patch too?

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

    Hmmm. A few things:

    1. If we're changing all the block IDs here, we'll need to add a table of them to the change notice. (None changed to in the original conversion.)
    2. Won't this fail the machine name validation? Or is only the last segment exposed in the UI?
    3. This doesn't seem like a very good unique machine name. See also: #1879496: Do we need to worry about plugin ID collisions?
    4. Is including the theme in all of the config object name, an instance property, and the ID an accident? Why do we need it in three places? Is this only temporary until the prefix change and/or layouts?
    5. Edit: Oh. Here's the plugin ID. Did the previous storage mechanism only care about the full config object name?
  35. +++ b/core/profiles/testing/config/plugin.core.block.stark.online.ymlundefined
    @@ -21,3 +25,4 @@ visibility:
    +langcode: und
    

    Interesting. Are they actually translatable, or is this just an accident of using the entity system?

  36. +++ b/core/profiles/testing/config/plugin.core.block.stark.tools.ymlundefined
    @@ -12,7 +14,8 @@ visibility:
    -subject: ''
    +label: ''
    

    Cool. Also an API change.

Edit: Note that these have been renumbered as I removed a few superfluous points.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

FileSize
6.99 KB
185.27 KB
PASSED: [[SimpleTest]]: [MySQL] 49,278 pass(es). View

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)

xjm’s picture

So, followups to file:

  1. One for the created/updated status of isNew(). #1887654: Creating a config entity with an existing machine name shouldn't work
  2. Whether blocks can/should be renderable without wrappers. #1880588: Regression: invalid HTML in menu blocks due to _block_get_renderable_block() clobbering #theme_wrappers of block contents is the issue I was thinking of.
  3. Why is clearing the definitions cache at the beginning of test methods necessary in some tests?
  4. node_schema(): #1887692: Clean up unused {block_node_type} table and references
  5. "html id"
  6. Add node type block settings in a better way than a form alter.
  7. Theme key in the machine name.
  8. Should we recommend that, in general, default config provided by modules has the owner name as a prefix on the ID, to avoid collisions? (This has come up twice now, bit different from #1879496: Do we need to worry about plugin ID collisions? because the instance ID is not the same as the plugin ID.) #1888218: Default configuration entities provided by a module should include the module name in the machine name

I don't care enough about drupalPlaceBlock() to file an issue for that or change it again, but I think it was better before.

tim.plunkett’s picture

FileSize
25.27 KB
183.84 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

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.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

FileSize
1.83 KB
184.02 KB
PASSED: [[SimpleTest]]: [MySQL] 49,388 pass(es). View

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

xjm’s picture

Issue tags: +Block plugins
xjm’s picture

xjm’s picture

Issue summary: View changes

Added followup issues.

xjm’s picture

Issue tags: +VDC

Alright I only got from the top through the access plugin, but pasting my notes because I have to come back to it later. Probably 80% of my observations are not actually related to the patch, but "we need a followup for this".

  1. +++ b/core/modules/block/block.admin.incundefined
    @@ -56,7 +58,7 @@ function block_admin_display_prepare_blocks($theme) {
    -  usort($blocks, '_block_compare');
    +  uasort($blocks, '_block_compare');
    

    Presumably because the blocks are now keyed by machine name?

  2. +++ b/core/modules/block/block.admin.incundefined
    @@ -115,12 +117,7 @@ function block_admin_display_form($form, &$form_state, $blocks, $theme, $block_r
       foreach ($blocks as $key => $instance) {
    -    $block = $instance->getConfig();
    -    $form['blocks'][$key]['config_id'] = array(
    -      '#type' => 'value',
    -      '#value' => $block['config_id'],
    -    );
    -    $info = $instance->getDefinition();
    +    $info = $instance->getPlugin()->getDefinition();
    

    Minor thing, but should we replace $instance with $block here (and following) for clarity/consistency?

  3. +++ b/core/modules/block/block.admin.incundefined
    @@ -215,29 +210,31 @@ function _block_compare($ainstance, $binstance) {
       // Sort by title.
    -  $ainfo = $ainstance->getDefinition();
    -  $binfo = $binstance->getDefinition();
    +  $ainfo = $a->getPlugin()->getDefinition();
    +  $binfo = $b->getPlugin()->getDefinition();
       return strcmp($ainfo['subject'], $binfo['subject']);
    

    So this is (both in HEAD and in the patch) using the default title instead of the instance's configured title. Now that we can place multiple copies of a block, and once #1875260: Make the block title required and allow it to be hidden is fixed, we should use the configured title instead (not just here but everywhere in the admin UI). => followup

  4. +++ b/core/modules/block/block.admin.incundefined
    @@ -245,60 +242,47 @@ function _block_compare($ainstance, $binstance) {
    +function block_admin_edit($entity_id) {
    +  $entity = entity_load('block', $entity_id);
    

    The ID and not the object? Could we use an autoloader here?

  5. +++ b/core/modules/block/block.admin.incundefined
    @@ -245,60 +242,47 @@ function _block_compare($ainstance, $binstance) {
    +  // Get the block subject for the page title.
    +  drupal_set_title(t("%label block in %theme", array('%label' => $entity->label(), '%theme' => $theme_title)), PASS_THROUGH);
    

    Do we also want to do this for the add form?

  6. +++ b/core/modules/block/block.admin.incundefined
    @@ -306,19 +290,16 @@ function block_admin_configure_submit($form, &$form_state) {
      * @param string $plugin_id
      *   The plugin ID for the block instance.
    ...
    +function block_admin_block_delete($form, &$form_state, $plugin_id) {
    +  $block = entity_load('block', $plugin_id);
    +  $subject = $block->label();
    +  list($theme) = explode('.', $plugin_id);
    

    Shouldn't this be $entity_id now rather than $plugin_id?

  7. +++ b/core/modules/block/block.moduleundefined
    @@ -121,22 +121,29 @@ function block_menu() {
    +  $items['admin/structure/block/add/%/%'] = array(
    ...
    +    'page arguments' => array(4, 5),
    ...
    +  $items['admin/structure/block/manage/%'] = array(
    ...
    +    'page arguments' => array(4),
    ...
    +  $items['admin/structure/block/manage/%/configure'] = array(
    ...
    +  $items['admin/structure/block/manage/%/delete'] = array(
    ...
    +    'page arguments' => array('block_admin_block_delete', 4),
    

    Yeah, autoloading the block would be nice where appropriate.

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

    Thank goodness. Is _block_get_renderable_block() removed now?

  9. +++ b/core/modules/block/block.moduleundefined
    @@ -357,33 +360,25 @@ function _block_get_renderable_region($list = array()) {
    +  $blocks = entity_load_multiple_by_properties('block', array('theme' => $theme));
    
    +++ b/core/modules/block/lib/Drupal/block/BlockStorageController.phpundefined
    @@ -0,0 +1,56 @@
    +  /**
    +   * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::loadByProperties().
    +   */
    +  public function loadByProperties(array $values = array()) {
    +    $blocks = $this->load();
    +    foreach ($values as $key => $value) {
    +      $blocks = array_filter($blocks, function($block) use ($key, $value) {
    +        return $value === $block->get($key);
    +      });
    +    }
    +    return $blocks;
    

    So in HEAD, ConfigStorageController::loadByProperties() returns an empty array and so entity_load_multiple_by_properties() does not work for config entities. @tim.plunkett pointed me to the fact that the BlockStorageController overrides it currently with this, though really this code is generic enough to work for any config entity. That merits a followup.

    Another thing I thought of is that we could potentially make this more performant for selecting blocks by theme (or, if and when stuff is converted, by display) by using listAll(plugin.core.block.themename). That also could be a followup issue.

    Related: #1846454: Add Entity query to Config entities

    Also, inline comments, please.

  10. +++ b/core/modules/block/block.moduleundefined
    @@ -357,33 +360,25 @@ function _block_get_renderable_region($list = array()) {
    -      drupal_set_message(t('The block %info was assigned to the invalid region %region and has been disabled.', array('%info' => $config->get('id'), '%region' => $region)), 'warning');
    +      drupal_set_message(t('The block %info was assigned to the invalid region %region and has been disabled.', array('%info' => $block_id, '%region' => $region)), 'warning');
    

    Also out of scope, but this behavior (and "invalid regions" generally) merits a followup issue.

  11. +++ b/core/modules/block/block.moduleundefined
    @@ -414,28 +409,22 @@ function block_themes_enabled($theme_list) {
    +      list(, $machine_name) = explode('.', $default_theme_block_id);
    ...
    +      $block->set('id', $theme . '.' . $machine_name);
    

    D'we want to add methods for this business of constructing and desconstructing the theme.machine_name IDs? Because this explode is present more than once, and the concatenation happens repeatedly. (The theme.machine_name IDs seem a little weird anyway, but @tim.plunkett has pointed out that it's a pre-existing pattern and out of scope here.)

  12. +++ b/core/modules/block/block.moduleundefined
    @@ -468,62 +461,6 @@ function block_list($region) {
    -function block_load($plugin_id, array $conf = array()) {
    

    Do we want to keep block_load() as a BC wrapper? E.g. we still have node_load().

  13. +++ b/core/modules/block/block.moduleundefined
    @@ -468,62 +461,6 @@ function block_list($region) {
    -function _block_load_blocks() {
    

    Yes. Kill it. Kill it dead.

  14. +++ b/core/modules/block/block.moduleundefined
    @@ -538,17 +475,7 @@ function _block_get_renderable_block($element) {
    -      if (isset($build['#title'])) {
    -        $element['#title'] = $build['#title'];
    -      }
    ...
    +    $element += entity_view($block, 'block');
    

    Is this #title element set when this gets built by the entity API, or is there an API change here with the block render array structure?

    Also, we can probably file a followup for removing _block_get_renderable_block() entirely.

  15. +++ b/core/modules/block/block.moduleundefined
    @@ -590,10 +517,9 @@ function block_rebuild() {
    -  $variables['block'] = (object) array_merge($variables['elements']['#block']->getDefinition(), $variables['elements']['#block']->getConfig());
    -  if (!empty($variables['elements']['#title']) && empty($variables['block']->subject)) {
    -    $variables['block']->subject = $variables['elements']['#title'];
    -  }
    +
    +  $variables['block'] = (object) $variables['elements']['#block_config'];
    

    Ahh, much nicer. :)

  16. +++ b/core/modules/block/block.moduleundefined
    @@ -623,14 +549,14 @@ function template_preprocess_block(&$variables) {
    -  $parts = explode(':', $variables['elements']['#block']->getPluginId());
    +  $parts = explode(':', $variables['elements']['#block']->get('plugin'));
    

    I think there is actually a method in the plugin system for this, but it's broken and assumes only one derivative. Another followup I still haven't filed, I think.

  17. +++ b/core/modules/block/block.moduleundefined
    @@ -643,14 +569,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);
    +      $block->save();
    
    @@ -681,35 +603,18 @@ function block_admin_paths() {
    +  foreach (entity_load_multiple('block') as $block_id => $block) {
    +    $visibility = $block->get('visibility');
    +    if (isset($visibility['language']['langcodes'][$language->langcode])) {
    +      unset($visibility['language']['langcodes'][$language->langcode]);
    +      $block->set('visibility', $visibility);
    +      $block->save();
    

    Data cleanup in config is much on my mind; I wonder if this has test coverage?

  18. +++ b/core/modules/block/block.moduleundefined
    @@ -659,11 +583,9 @@ function block_user_role_delete($role) {
    +  foreach (entity_load_multiple('block') as $block_id => $block) {
    +    if ($block->get('plugin') == 'menu_menu_block:' . $menu->id()) {
    +      $block->delete();
    

    I also still need the followup for moving this to menu.module.

  19. +++ b/core/modules/block/block.moduleundefined
    @@ -681,35 +603,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();
    -    }
    -  }
    

    Removing this can happen ahead of time in #1887692: Clean up unused {block_node_type} table and references (since it's actually a separate bug).

  20. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -29,17 +30,6 @@
       /**
    -   * Overrides \Drupal\block\BlockBase::blockSettings().
    -   */
    -  public function blockSettings() {
    -    // By default, use a blank block title rather than the block description
    -    // (which is "Custom Block").
    -    return array(
    -      'subject' => '',
    -    );
    -  }
    

    Have we tested that the default title for custom blocks is not "Custom Block" but blank? I knew I should have added a test for that. Actually it's probably not worth worrying about before #1871772: Convert custom blocks to content entities goes in; that will probably fix this by default.

  21. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -78,11 +68,25 @@ public function blockForm($form, &$form_state) {
    +  public function blockValidate($form, &$form_state) {
    +    list(, $bid) = explode(':', $form_state['entity']->get('plugin'));
    +    $custom_block_exists = (bool) db_query_range('SELECT 1 FROM {block_custom} WHERE bid <> :bid AND info = :info', 0, 1, array(
    +      ':bid' => $bid,
    +      ':info' => $form_state['values']['info'],
    +    ))->fetchField();
    +    if (empty($form_state['values']['info']) || $custom_block_exists) {
    +      form_set_error('info', t('Ensure that each block description is unique.'));
    +    }
    

    Oy, comments?

  22. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -94,7 +98,7 @@ public function blockSubmit($form, &$form_state) {
    -    $this->configuration['id'] = 'custom_block:' . $block['bid'];
    +    $form_state['entity']->set('plugin', 'custom_block:' . $block['bid']);
    

    This is also in HEAD, but bid? That's not confusing or anything. Presumably fixed by or in a followup to #1871772: Convert custom blocks to content entities.

  23. +++ b/core/modules/block/lib/Drupal/block/BlockAccessController.phpundefined
    @@ -0,0 +1,105 @@
    +class BlockAccessController extends EntityAccessController {
    

    So, way cool. We should get @sdboyer to take a peek at this; he has referred several times to how page/language visibility settings (at least) are not actually access restrictions but something else (condition plugin-y stuff I think). Also #1880274: Reimplement block visibility settings.

    I'm assuming this is pretty much exactly what used to be in blockAccess(), which I refactored in the original patch, so I skimmed it.

tim.plunkett’s picture

FileSize
6.1 KB
184.58 KB
PASSED: [[SimpleTest]]: [MySQL] 49,256 pass(es). View

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() :(

xjm’s picture

+++ b/core/modules/block/block.admin.incundefined
@@ -256,15 +254,13 @@ function block_admin_add($plugin_id, $theme = NULL) {
+ * @param \Drupal\block\Plugin\Core\Entity\Block $entity
+ *   The block instance.
...
+function block_admin_edit($entity) {

@@ -288,21 +284,16 @@ function block_admin_edit($entity_id) {
+ * @param \Drupal\block\Plugin\Core\Entity\Block $entity
+ *   The block instance.
...
+function block_admin_block_delete($form, &$form_state, $entity) {
+  $form['id'] = array('#type' => 'value', '#value' => $entity->id());

Aha. Can we add typehints in the signatures as well? array and Block EntityInterface I guess?

+++ b/core/modules/block/block.moduleundefined
@@ -128,19 +128,19 @@ function block_menu() {
-  $items['admin/structure/block/manage/%'] = array(
+  $items['admin/structure/block/manage/%block'] = array(

Excellent.

xjm’s picture

No, the add form doesn't use this in HEAD

Right. I added it for the edit form in HEAD, though. I just forgot it for the add form. ;)

xjm’s picture

  1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
    @@ -0,0 +1,236 @@
    +/**
    + * Form controller for the user account forms.
    + */
    +class BlockFormController extends EntityFormController {
    

    No it isn't. ;) Also, verb.

  2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
    @@ -0,0 +1,236 @@
    +      '#default_value' => !$entity->isNew() ? $entity->label() : $definition['subject'],
    

    Hey wait, wasn't it label now? Or I guess that's only on the entity, and the plugin definition still uses subject.

  3. +++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
    @@ -0,0 +1,90 @@
    +/**
    + * Provides a Block access controller.
    + */
    +class BlockRenderController implements EntityRenderControllerInterface {
    

    No it doesn't. ;)

  4. +++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
    @@ -0,0 +1,90 @@
    +  /**
    +   * Implements \Drupal\Core\Entity\EntityRenderControllerInterface::buildContent().
    +   */
    +  public function buildContent(array $entities, array $displays, $view_mode, $langcode = NULL) {
    +    return array();
    +  }
    

    Hm, why empty? Also, block view modes, trippy.

  5. +++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
    @@ -0,0 +1,90 @@
    +   * @param string $langcode
    +   *   (optional) For which language the entity should be prepared, defaults to
    +   *   the current content language.
    ...
    +  protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
    

    This says it's optional, but no default is provided in the signature. Also "The language for which..." rather than "For which language..."

  6. +++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
    @@ -0,0 +1,90 @@
    +        'subject' => $entity->label(),
    

    Ah, here's subject again. So it goes subject -> label -> subject -> label -> title or something? That's not confusing. At all. Yes, I know there's #1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer for this. ;)

  7. +++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
    @@ -0,0 +1,90 @@
    +      // Allow blocks to be empty, do not add in the defaults.
    

    You and your comma splices. :P

  8. +++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
    @@ -0,0 +1,90 @@
    +      // All blocks, even when empty, should be available for altering.
    +      $id = str_replace(':', '__', $entity->get('plugin'));
    +      list(, $name) = $entity->id();
    +      drupal_alter(array('block_view', "block_view_$id", "block_view_$name"), $build[$entity_id], $entity);
    

    #1874576: Improve the documentation and naming of hook_block_view_alter() hooks for this hunk.

  9. +++ b/core/modules/block/lib/Drupal/block/BlockStorageController.phpundefined
    @@ -0,0 +1,56 @@
    +  public function load(array $ids = NULL) {
    +    $entities = parent::load($ids);
    +    // Only blocks with a valid plugin should be loaded.
    +    return array_filter($entities, function ($entity) {
    +      return $entity->getPlugin();
    +    });
    
    +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -0,0 +1,182 @@
    +  public function getPlugin() {
    +    if (!$this->instance) {
    +      // Throw an exception if no plugin string was provided.
    +      if (!$this->plugin) {
    +        throw new PluginException(format_string("The block '@block' did not specify a plugin.", array('@block' => $this->id())));
    +      }
    

    Ah, that's kind of elegant. Is this failing silently, though? #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies).

    Edit: Ah, there it is. Hmm.

  10. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -0,0 +1,182 @@
    + * @Plugin(
    + *   id = "block",
    + *   label = @Translation("Block"),
    + *   module = "block",
    + *   controller_class = "Drupal\block\BlockStorageController",
    + *   access_controller_class = "Drupal\block\BlockAccessController",
    + *   render_controller_class = "Drupal\block\BlockRenderController",
    + *   form_controller_class = {
    + *     "default" = "Drupal\block\BlockFormController"
    + *   },
    + *   config_prefix = "plugin.core.block",
    + *   fieldable = FALSE,
    + *   entity_keys = {
    + *     "id" = "id",
    + *     "label" = "label",
    + *     "uuid" = "uuid"
    + *   }
    + * )
    

    I'm pretty sure this is how I ordered the keys in EntityManager, but I think we also tentatively decided to alphabetize annotations for readability, except for id and module. (Doesn't really matter since I'm sure all the other entities are exactly like this; just a thought.)

  11. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -0,0 +1,182 @@
    +   * The id of the block.
    

    ID, not id.

  12. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -0,0 +1,182 @@
    +  /**
    +   * The region this block is placed in.
    +   *
    +   * The 'disabled' region is represented by -1.
    +   *
    +   * @var string
    +   */
    +  protected $region = -1;
    ...
    +  /**
    +   * The status of this block.
    +   *
    +   * @var bool
    +   */
    +  protected $status = TRUE;
    

    Isn't there a constant for this? (If not there probably should be => "If SCOTCH fails" followup.)

    Also, aren't these two defaults contradictory?

  13. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -0,0 +1,182 @@
    +  /**
    +   * Settings to control the block visibility.
    +   *
    +   * @var array
    +   */
    +  protected $visibility = array();
    

    Huh, this has its own array? Is that new?

  14. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -0,0 +1,182 @@
    +    return $this->instance;
    

    I don't see $instance declared on the class; should it be?

  15. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -0,0 +1,182 @@
    +    $value = parent::get($property_name, $langcode);
    +    if ($property_name == 'theme' && !$value) {
    +      list($value) = explode('.', $this->id());
    +    }
    

    More theme explosion fun.

  16. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockCacheTest.phpundefined
    @@ -192,15 +192,10 @@ function testCachePerPage() {
    -    $this->assertEqual($config['cache'], $cache_mode, t("Test block's database entry updated to DRUPAL_NO_CACHE."));
    +    $settings = $this->block->get('settings');
    +    $settings['cache'] = $cache_mode;
    +    $this->block->set('settings', $settings);
    +    $this->block->save();
    

    Heh, someone didn't update an assertion message. :) We're not adding back this assertion, though; is it superfluous to the test?

So I've reviewed the whole patch now. I've a few lingering questions (mostly above), but overall I think this is a huge DX improvement, and much more recognizably architected.

Next I'll manually test it just to make sure nothing's gone strange (since we still have a few "missing test coverage" issues open).

tim.plunkett’s picture

FileSize
3.45 KB
184.57 KB
PASSED: [[SimpleTest]]: [MySQL] 49,262 pass(es). View

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.

xjm’s picture

I did a bunch of manual testing. Most things behaved, including stuff that doesn't have coverage like placing multiple instances. There were a couple minor oddities that both have followup issues. One, blocks with blank titles just say "Are you sure you want to delete the block ?" on confirmation forms. This should be fixed by #1875260: Make the block title required and allow it to be hidden.

blank_title.png

delete_works.png

Two, the "Custom Block" default title is back as suspected, but I've no problem with waiting for #1871772: Convert custom blocks to content entities to fix that again.

custom_block_title.png

tim.plunkett’s picture

FileSize
3.36 KB
2.87 KB
185.01 KB
PASSED: [[SimpleTest]]: [MySQL] 50,434 pass(es). View

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.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

tim.plunkett’s picture

FileSize
15.82 KB
195.18 KB
PASSED: [[SimpleTest]]: [MySQL] 50,440 pass(es). View

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().

EclipseGc’s picture

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.

xjm’s picture

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.

I definitely don't think this is a blocker, at all. I think it's out of scope. It had no test coverage, no implementation, and was never documented as a requirement. If there is a usecase that comes up later, we file an issue then.

Background for not-EclipseGc:

  • In the original patch, the BlockBase class provides a double set of methods, e.g. form() and blockForm().
  • Only the first set of methods (with no block* prefix) is on the BlockInterface. This method provides (e.g.) the full form for configuring an instance of the block, the full access restrictions, etc.
  • In practice, currently, the interface methods are only implemented on BlockBase. form() provides form elements that are common to all block plugins, like the title field, machine name, page visibility settings, etc.; access() provides the access control to restrict visibility by language, page, or role.
  • The block*() methods appear on the base class and are called by the other methods. E.g., BlockBase::access()checks BlockBase::blockAccess(); BlockBase::form() wraps special elements from BlockBase::blockForm(), etc.
  • In core, block plugins only override the block*() methods.
  • EclipseGc's concern is that, with the existing architecture, it is possible for MyBlockPlugin to override BlockBase::form(), choosing to build a different form for a specific block type, instead of adding elements to the normal form. Or, to override access() so that per-role, per-language, and per-page visibility settings could be ignored. This is not done for any core plugin.
  • In the current patch (where the entity controllers are used for access, rendering, building the form, and so on), replacing the whole form is no longer possible in quite that way. It would have to be changed with an alter hook.

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.

What custom block API? There was no API for block CRUD. :) As for the unused methods, this is a problem for config entities generally, that we planned to address once the conversions were done and we could see what really belonged only to content entities. But also out of scope here. (Discussing this in IRC this evening, a lot of the concerns I heard were legitimate about config entities in general, but not concerns about this patch.)

Edit: All that said, I was also not expecting the use of the controllers that this patch added. However, I found the method()/blockMethod() pattern hard to explain as well, and there's a lot of API cleanup happening here with the use of the added controllers.

IMO, the biggest reason for this patch (aside from the DX++ that the API is consistent with everything else that stores configured instances of stuff to CMI) is that there are a lot of complicated problems with the configuration system, bugs we are already working out for config entities and issues that are made a lot simpler if we can assume the only dynamically created configuration goes through the entity API. If we use a separate, custom API for blocks, we're risking having to fix those bugs twice, or not fixing them at all, and we make things more complicated if we have to support two different patterns of dynamically creating configuration objects.

tim.plunkett’s picture

FileSize
194.6 KB
PASSED: [[SimpleTest]]: [MySQL] 50,516 pass(es). View

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.

tim.plunkett’s picture

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()
catch’s picture

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.

heyrocker’s picture

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?

EclipseGc’s picture

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

tim.plunkett’s picture

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.

effulgentsia’s picture

+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.

EclipseGc’s picture

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

tim.plunkett’s picture

FileSize
29.52 KB
2.82 KB
18.07 KB
181.04 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

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.

tim.plunkett’s picture

FileSize
2.5 KB
181.54 KB
FAILED: [[SimpleTest]]: [MySQL] 49,979 pass(es), 4 fail(s), and 5 exception(s). View

Added some typehinting/docblocks

tim.plunkett’s picture

Issue summary: View changes

adding followup

msonnabaum’s picture

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;
tim.plunkett’s picture

FileSize
34.97 KB
163.79 KB
FAILED: [[SimpleTest]]: [MySQL] 50,470 pass(es), 44 fail(s), and 76 exception(s). View

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.

tim.plunkett’s picture

FileSize
1.2 KB
164.66 KB
PASSED: [[SimpleTest]]: [MySQL] 50,565 pass(es). View

Yay for tests.

582b827 Fix ViewsBlock::__construct().

yched’s picture

+++ 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...)

msonnabaum’s picture

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.

tim.plunkett’s picture

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).

tim.plunkett’s picture

FileSize
2.22 KB
160.64 KB
PASSED: [[SimpleTest]]: [MySQL] 50,693 pass(es). View

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

I think we're done here.

yched’s picture

"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

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

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

effulgentsia’s picture

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.

msonnabaum’s picture

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.

EclipseGc’s picture

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

webchick’s picture

Title: Convert block instances to configuration entities to resolve architectural issues » Change notice: Convert block instances to configuration entities to resolve architectural issues
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

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. :)

Sutharsan’s picture

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..

Sutharsan’s picture

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.

webchick’s picture

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.

webchick’s picture

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.

sun’s picture

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.

tim.plunkett’s picture

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...

msonnabaum’s picture

sun’s picture

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." ;)

tim.plunkett’s picture

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.

sun’s picture

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?

:-/

tim.plunkett’s picture

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.

sun’s picture

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

webchick’s picture

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

podarok’s picture

over 6 weeks now (

EclipseGc’s picture

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

podarok’s picture

xjm’s picture

Status: Postponed (maintainer needs more info) » Active

I don't think we don't get to pretend we don't have this documentation debt just because there are open issues to change things further.

xjm’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

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.

YesCT’s picture

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

catch’s picture

catch’s picture

Issue summary: View changes

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

xjm’s picture

Issue summary: View changes
Issue tags: +Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release.

The patch for this issue was committed on January 16, 2013, meaning a change record for this issue has been outstanding for more than a year. I wish we could threaten to remove the block module from core... :P

Gábor Hojtsy’s picture

Status: Active » Fixed
Issue tags: -Needs change record, -Missing change record

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)

Gábor Hojtsy’s picture

Title: Change notice: Convert block instances to configuration entities to resolve architectural issues » Convert block instances to configuration entities to resolve architectural issues

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.