Remove PluginUI subsystem, provide block plugins UI as one-off

Loving the diffstat.

Manually tested, and there's a slight regression: If no category is selected, the list of block plugins to place starts off empty rather than "all". Of course the whole page will be removed soon enough, but worth investigating.

I also read through the patch; pretty much just inane observations.

1. +++ b/core/modules/block/block.moduleundefined
   @@ -61,14 +61,14 @@ function block_help($path, $arg) {
      if ($arg[0] == 'admin' && $arg[1] == 'structure' && $arg['2'] == 'block' && (empty($arg[3]) || $arg[3] == 'list') && empty($arg[5])) {
   

   Yech. (Out of scope, just... yech.)

2. +++ b/core/modules/block/custom_block/custom_block.moduleundefined
   @@ -221,9 +221,9 @@ function custom_block_add_body_field($block_type_id, $label = 'Block body') {
   + * Implements hook_form_FORM_ID_alter() for block_admin_place().
   

   block_admin_place() is kind of a funny name. :) I realized eventually that it refers to "place" blocks, but it's like... the admin place. :) You know, on the internet thing.

   Also, I think we need to change our form alter doc standards... there is no block_admin_place() callback. There is the block_admin_place form, or PlaceBlocksForm.

3. +++ b/core/modules/block/lib/Drupal/block/Controller/BlockAutocompleteController.phpundefined
   @@ -0,0 +1,74 @@
   +   * @param Request $request
   

   I don't think Request is in the global namespace?

4. +++ b/core/modules/block/lib/Drupal/block/Controller/BlockAutocompleteController.phpundefined
   @@ -0,0 +1,74 @@
   +   *   The matched plugins as json.
   

   JSON.

5. +++ b/core/modules/block/lib/Drupal/block/Controller/BlockAutocompleteController.phpundefined
   @@ -0,0 +1,74 @@
   +    $string_typed = $request->query->get('q');
   +    $string_typed = Tags::explode($string_typed);
   +    $string = Unicode::strtolower(array_pop($string_typed));
   +    $matches = array();
   +    if ($string) {
   +      $titles = array();
   +      foreach($this->manager->getDefinitions() as $plugin_id => $plugin) {
   +        $titles[$plugin_id] = $plugin['admin_label'];
   +      }
   +      $matches = preg_grep("/\b". $string . "/i", $titles);
   +    }
   +
   +    return new JsonResponse($matches);
   

   Comment or two in here would be good.

6. +++ b/core/modules/block/lib/Drupal/block/Form/PlaceBlocksForm.phpundefined
   @@ -0,0 +1,143 @@
   +  public function row($display_plugin_id, array $display_plugin_definition) {
   

   I need a docblock (probably just an {@inheritdoc}).

Technically.

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -221,9 +221,9 @@ function custom_block_add_body_field($block_type_id, $label = 'Block body') {
-function custom_block_form_block_plugin_ui_alter(&$form, $form_state) {
+function custom_block_form_block_admin_place_alter(&$form, $form_state) {

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -231,7 +231,7 @@ public function submit(array $form, array &$form_state) {
-    $form_state['redirect'] = 'admin/structure/block/list/block_plugin_ui:' . $entity->get('theme');
+    $form_state['redirect'] = 'admin/structure/block/list/' . $entity->get('theme');

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockListController.phpundefined
@@ -51,17 +57,15 @@ public static function create(ContainerInterface $container) {
-  public function listing($entity_type, $theme = NULL) {
+  public function listing($theme = NULL) {

+++ /dev/nullundefined
@@ -1,29 +0,0 @@
-class BlockPluginUI extends DerivativeBase {

+++ /dev/nullundefined
@@ -1,229 +0,0 @@
-class BlockPluginUI extends PluginUIBase {

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -19,24 +19,6 @@
-  protected $pluginManager;
...
-  public function __construct(PluginManagerInterface $plugin_manager) {

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockAdminThemeTest.phpundefined
@@ -38,14 +38,14 @@ function testAdminTheme() {
-    $this->drupalGet('admin/structure/block/list/block_plugin_ui:bartik');
+    $this->drupalGet('admin/structure/block/list/bartik');

+++ /dev/nullundefined
@@ -1,55 +0,0 @@
-class SystemPluginUiCheck implements AccessCheckInterface {

+++ /dev/nullundefined
@@ -1,56 +0,0 @@
-class SystemController extends ContainerAware {

+++ /dev/nullundefined
@@ -1,51 +0,0 @@
-class PluginUIManager extends PluginManagerBase {

+++ b/core/modules/system/system.moduleundefined
@@ -964,84 +964,10 @@ function system_menu() {
-function system_plugin_ui_form($form, &$form_state, $plugin, $facet = NULL) {
...
-function system_plugin_ui_access($plugin, $facet = NULL) {
...
-function system_forms() {

+++ b/core/modules/system/system.routing.ymlundefined
@@ -213,10 +213,3 @@ system_timezone:
-system_plugin_autocomplete:

+++ b/core/modules/system/system.services.ymlundefined
@@ -3,14 +3,6 @@ services:
-  access_check.system_plugin_ui:
...
-  plugin.manager.system.plugin_ui:

The API changes.

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockListController.phpundefined
@@ -7,15 +7,15 @@
-class BlockListController extends EntityListController {
+class BlockListController implements ControllerInterface {

@@ -24,17 +24,23 @@ class BlockListController extends EntityListController {
+  protected $blockListController;
...
-  public function __construct(EntityManager $entity_manager, ConfigFactory $config_factory) {
-    $this->entityManager = $entity_manager;
+  public function __construct(EntityListControllerInterface $block_list_controller, ConfigFactory $config_factory) {
+    $this->blockListController = $block_list_controller;

Oh. This is interesting. Why?

Updated issue summary.

Updated issue summary.

I added the API changes to the summary.

Updated issue summary.

This issue is a child issue of a critical; however, I don't think ripping out the unneeded complexity and abstraction is release-blocking--it's just a really good idea and will make our lives saner. Setting to major.

I closed these issues as dupes:
#1875340: Review PluginUI architecture
#1874840: Add explicit test coverage for the BlockPluginUI
#1875942: Improve PluginUI documentation

Updated issue summary.

Updated issue summary.

This issue also reduces what we still need to document in #1535868: Convert all blocks into plugins.

Updated issue summary.

Oh yes, please. I never understood why this went in. Going to give it an in-depth review tomorrow.

1. +++ b/core/modules/block/lib/Drupal/block/Controller/BlockAutocompleteController.phpundefined
   @@ -0,0 +1,78 @@
   +    $string_typed = Tags::explode($string_typed);
   

   Nice, I didn't realize this had been componentized alredy.

2. +++ b/core/modules/block/lib/Drupal/block/Form/PlaceBlocksForm.phpundefined
   @@ -0,0 +1,154 @@
   +  /**
   +   * @var string
   +   */
   +  protected $theme;
   

   Missing the one-line summary.

3. +++ b/core/modules/block/lib/Drupal/block/Form/PlaceBlocksForm.phpundefined
   @@ -0,0 +1,154 @@
   +   * @param string $display_plugin_id
   ...
   +   * @param array $display_plugin_definition
   ...
   +  public function row($display_plugin_id, array $display_plugin_definition) {
   

   Why are these display plugin IDs?

4. +++ b/core/modules/block/lib/Drupal/block/Form/PlaceBlocksForm.phpundefined
   @@ -0,0 +1,154 @@
   +    $row[] = check_plain($display_plugin_definition['admin_label']);
   

   Is check_plain() sufficient and correct here? Either the admin label has already been sanitized, or we'll get escaped HTML entities in it. (This might just be moved code?)

5. +++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
   @@ -55,18 +37,26 @@ public static function getSubscribedEvents() {
   +    foreach (list_themes(TRUE) as $key => $theme) {
   

   So, how early will this method be called? I've had some trouble with the results from list_themes() not being reliable in #2040037: The requested page "/admin/appearance/settings/bartik" could not be found after installation which is a very similar situation.

6. +++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
   @@ -55,18 +37,26 @@ public static function getSubscribedEvents() {
   +      // The block entity listing page.
   +      $route = new Route('admin/structure/block/list/' . $key, array(
   +        '_controller' => '\Drupal\block\Controller\BlockListController::listing',
   +        'theme' => $key,
   +      ), array(
   +        '_block_themes_access' => 'TRUE',
   +      ));
   +      $collection->add("block_admin_display.$key", $route);
   +
   +      // The block plugin listing page.
   +      $route = new Route('admin/structure/block/list/' . $key . '/add/{category}', array(
   +        '_form' => '\Drupal\block\Form\PlaceBlocksForm',
   +        'category' => NULL,
   +        'theme' => $key,
   +      ), array(
   +        '_block_themes_access' => 'TRUE',
   +      ));
   +      $collection->add("block_plugin_ui.$key", $route);
   

   The indentation here is funky; is this copied from somewhere?

Oh yes, please. I never understood why this went in. Going to give it an in-depth review tomorrow.

#1535868-294: Convert all blocks into plugins

The route subscriber is not moved code as far as I can tell, which is why I commented on it. And given that the plugin UI went in without review initially, it makes sense to review whatever bits of it we are retaining now.

+++ b/core/modules/block/block.moduleundefined
@@ -136,28 +136,28 @@ function block_menu() {
+    $items['admin/structure/block/list/' . $key] = array(
...
+      'route_name' => 'block_admin_display.' . $key,
...
+    $items['admin/structure/block/list/' . $key . '/add'] = array(
...
+      'route_name' => 'block_plugin_ui.' . $key,
...
+    $items['admin/structure/block/demo/' . $key] = array(

I know that you didn't add that here but I always hate this single quote string concatenation for strings with simple variables. Can we use the double-quote version with heredoc syntax? It's so much nicer to read.

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockAutocompleteController.phpundefined
@@ -0,0 +1,78 @@
+   * Constructs a new PlaceBlocksForm object.

Nope. :)

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockListController.phpundefined
@@ -51,17 +57,15 @@ public static function create(ContainerInterface $container) {
     $default_theme = $theme ?: $this->configFactory->get('system.theme')->get('default');
...
+    return $this->blockListController->render($default_theme);

$default_theme is not really the right variable name there. Should simply stay $theme (not added here, I know, but it's misleading).

+++ b/core/modules/block/lib/Drupal/block/Form/PlaceBlocksForm.phpundefined
@@ -0,0 +1,154 @@
+
+  /**
+   * @var string
+   */
+  protected $theme;

The active theme? The theme for which to render the form? ;)

+++ b/core/modules/block/lib/Drupal/block/Form/PlaceBlocksForm.phpundefined
@@ -0,0 +1,154 @@
+  public function buildForm(array $form, array &$form_state, $theme = NULL, $category = NULL) {

Meh, I hate FormInterface :)

Looking good otherwise.

Hmm, actually... I am a little bit concerned about list_themes(). We don't rebuild anything when we enable a new theme or do we?

Hmm, actually... I am a little bit concerned about list_themes(). We don't rebuild anything when we enable a new theme or do we?

Updated issue summary.

Yah #2040037: The requested page "/admin/appearance/settings/bartik" could not be found after installation is the issue for that. Ideally themes need an installation status and to be handled like modules, and then the tentacled procedural mass that is under list_themes() needs to be refactored as a module handler method or something. But out of scope here, and the fix in my issue should fix both.

And this is ready to go, assuming green. I feel like a weight has just been taken off my shoulders. :)

We had a community-sponsored bot go AWOL. Re-queued.

Updated issue summary.

We reviewed this patch one more time and noted some minor things that can be followups.

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockAutocompleteController.phpundefined
@@ -0,0 +1,78 @@
+class BlockAutocompleteController implements ControllerInterface {

We observed it might make sense to have an AutocompleteControllerBase -- scanning the autocomplete() method, the two things that would probably be different are the regex and the list of available definitions. Maybe, maybe not. Followup question either way.

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockAutocompleteController.phpundefined
@@ -0,0 +1,78 @@
+      $matches = preg_grep("/\b". $string . "/i", $titles);

One missing space here after "\b" -- Let's file a followup for that.

Ok. @xjm walked me through this issue for a good half hour, and it looks really good.

I think this was a nice idea in theory, but in practice it doesn't really work. When we examine the facilities in core/contrib most likely to benefit from generic UI handling code like this, pretty much all of them are better off with their own custom UI that's better-oriented to the task(s) at hand (e.g. Views, Blocks, Panels...). Maybe if we'd totally knocked block UI out of the park, and it represented a significant UX improvement that could have benefit in other parts of the system (similar to the benefit we've received from entity listings being generalized), but I don't think that's the case here. Tagging as "Approved API change" in response.

There's a lot of pretty ugly stuff removed from system.module, which is great, and lines like:

-    $form_state['redirect'] = 'admin/structure/block/list/block_plugin_ui:' . $entity->get('theme');
+    $form_state['redirect'] = 'admin/structure/block/list/' . $entity->get('theme');

...and:

-/**
- * Implements hook_forms().
- */
-function system_forms() {

...make me happy.

The one thing that would be nice is a generic facility for autocompletion that other modules could use to avoid boilerplate code like Block module has to do, but that can be a follow-up.

In reviewing, I noticed a couple of small details like off concatenation, check_plain() instead of String::check_plain(), but I don't think those need to hold up this patch; most of these will go away in other patches.

Committed and pushed to 8.x. Thanks!

Let's also make sure that the original blocks-as-plugins change notice gets any updates it needs as a result of this patch. (There might not be any, but let's check.)

The one thing that would be nice is a generic facility for autocompletion that other modules could use to avoid boilerplate code like Block module has to do, but that can be a follow-up.

#2059955: Standardize and simplify autocomplete controllers and services

Also, we need that followup for adding a single space. ;) Forthcoming.

Actually #2058321: Move the 'place block' UI into the block listing already addresses the single space issue by deleting the line. ;) So disregard.

Remove all the (PluginUI) things.

Looks like we have to commit both #20 and #30. All in all, this looks like a good win.

Given that #2058321: Move the 'place block' UI into the block listing is marked 'critical', and this patch is marked 'major', I'm inclined to commit #2058321 first. That may require this patch to be re-rolled, not sure yet.

Committed #2058321: Move the 'place block' UI into the block listing. Asking for a re-test to make sure this still applies and works correctly.

#38: pluginui-2056513-38.patch queued for re-testing.

D'oh. :P Committed and pushed to 8.x. Thanks!