Sub-issue of #2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout

Problem/Motivation

#1535868: Convert all blocks into plugins introduced an abstracted API for creating user interface lists of any plugins. This API did not undergo technical review because of the unmanageable scope of that patch. It is unnecessarily complex and is not used anywhere other than the blocks UI.

This issue is major because:

Proposed resolution

Remove the abstracted Plugin UI API, and replace it with a block-specific implementation.

API changes

  1. The following classes are removed:

    • PluginUIManager
    • BlockPluginUI and its derivative class
    • SystemPluginUiCheck (access checker)

    The following classes are renamed:

    • SystemController to BlockAutocompleteController (routing controller)
  2. The following callbacks are removed:

    • system_plugin_ui_form()
    • system_plugin_ui_access()
  3. The system_forms() hook implementation is removed.

  4. The system_plugin_autocomplete route is removed and is replaced with a block-specific route for the new BlockAutocompleteController.

  5. The following services are removed:

    • access_check.system_plugin_ui
    • plugin.manager.system.plugin_ui
  6. The BlockListController no longer extends EntityListController, and therefore no longer takes an entity type in its constructor or its listing() method.

Additionally, the URLs for block listings and adding blocks per theme are changed.
Old: admin/structure/block/list/block_plugin_ui:bartik
New: admin/structure/block/list/bartik

Old: admin/structure/block/list/block_plugin_ui:bartik/modules:comment
New: admin/structure/block/list/bartik/add/comment

Files: 
CommentFileSizeAuthor
#38 pluginui-2056513-38.patch4.69 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,135 pass(es).
[ View ]
#30 block-2056513-30.patch47.25 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,974 pass(es).
[ View ]
#22 block-2056513-22.patch49.17 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,064 pass(es).
[ View ]
#22 interdiff.txt7.63 KBtim.plunkett
#14 block-2056513-14.patch49.19 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,580 pass(es).
[ View ]
#9 block-2056513-9.patch49.93 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,796 pass(es).
[ View ]
#9 interdiff.txt6.56 KBtim.plunkett
#1 block-2056513-1.patch49.72 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,902 pass(es).
[ View ]

Comments

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new49.72 KB
PASSED: [[SimpleTest]]: [MySQL] 57,902 pass(es).
[ View ]
tim.plunkett’s picture

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -55,18 +37,30 @@ public static function getSubscribedEvents() {
+      $route = new Route('admin/structure/block/list/' . $key . '/add', array(
+        '_form' => '\Drupal\block\Form\PlaceBlocksForm',
+        'theme' => $key,
+      ), array(
+        '_block_themes_access' => 'TRUE',
+      ));
+      $collection->add("block_admin_place.$key", $route);
+      $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_admin_place.category.$key", $route);

This is the only part that I know is wrong. Will have to find a way to remove the duplicate route.

tim.plunkett’s picture

Actually, we're going to be removing that whole route and URL-based filtering in #2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout anyway, so just adding this second route is not a problem.

xjm’s picture

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

xjm’s picture

Issue tags:+API change

Technically.

xjm’s picture

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

xjm’s picture

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

tim.plunkett’s picture

Issue tags:+Needs tests
StatusFileSize
new6.56 KB
new49.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,796 pass(es).
[ View ]

5.1 Yeah. That will have to be dealt with, but not this issue.
5.2 Reverting to 'block_plugin_ui' for now, since that's still what it is, and we're going to delete it anyway. No sense in changing it.
5.3
5.4 This is just moved code, but if you insist. :)
5.5 Doesn't inherit anything, will add a docblock

8 So the EntityListController is generic, and forces you to specify the entity type in the route definition. But we need a custom list controller since we pass in the current theme. And we override every method in EntityListController and never call parent:: for anything. So we might as well not force stupidity on the route definition.

I tracked down the need for the duplicate route. I had previously written it for #1938888: Convert user_admin_permissions to a new-style Form object, but we ended up justifying the need for two routes anyway. This unfortunately means this issue now needs tests.

tim.plunkett’s picture

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

I added the API changes to the summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Priority:Normal» Major

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

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

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

xjm’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

StatusFileSize
new49.19 KB
PASSED: [[SimpleTest]]: [MySQL] 57,580 pass(es).
[ View ]

No more blockers, this is done.

fubhy’s picture

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

tim.plunkett’s picture

xjm’s picture

  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?

xjm’s picture

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

tim.plunkett’s picture

This is straight as written in core, just moved slightly. I'll polish it a bit, but we don't usually hold moved code to this level of scrutiny.

xjm’s picture

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.

fubhy’s picture

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

tim.plunkett’s picture

StatusFileSize
new7.63 KB
new49.17 KB
PASSED: [[SimpleTest]]: [MySQL] 58,064 pass(es).
[ View ]

Thanks, most of the things I fixed are moved code, but xjm makes a good point in #20.

I'm not worried about list_themes() right now, this hasn't broken for me yet, and if there is a problem that other issue will handle it.

The RouteSubscriber was weird, switched the indenting to mimic field_ui's RouteSubscriber.

fubhy’s picture

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?

fubhy’s picture

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?

fubhy’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

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.

xjm’s picture

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

The last submitted patch, block-2056513-22.patch, failed testing.

jthorson’s picture

Status:Needs work» Needs review

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

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for requeuing. See you all over in #2058321: Move the 'place block' UI into the block listing soon!

tim.plunkett’s picture

StatusFileSize
new47.25 KB
PASSED: [[SimpleTest]]: [MySQL] 57,974 pass(es).
[ View ]
tim.plunkett’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

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.

webchick’s picture

Title:Remove PluginUI subsystem, provide block plugins UI as one-off» Change notice: Remove PluginUI subsystem, provide block plugins UI as one-off
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record, +Needs followup, +Approved API change

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

tim.plunkett’s picture

Title:Change notice: Remove PluginUI subsystem, provide block plugins UI as one-off» Remove PluginUI subsystem, provide block plugins UI as one-off
Priority:Critical» Major
Status:Active» Fixed
Issue tags:-Needs change record, -Needs followup

I checked the change notices for #1535868: Convert all blocks into plugins and did a pretty thorough search through the list, and found nothing.
Based on #1875942: Improve PluginUI documentation and #1875340: Review PluginUI architecture, I feel pretty safe saying this had no real docs.

Therefore, not adding a notification for something we never documented as existing :)

The follow-up issue is #2058321: Move the 'place block' UI into the block listing

effulgentsia’s picture

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

xjm’s picture

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

xjm’s picture

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

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
tim.plunkett’s picture

Status:Fixed» Needs review
StatusFileSize
new4.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,135 pass(es).
[ View ]

Whoops. Left the base class and interface in!

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Remove all the (PluginUI) things.

Dries’s picture

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.

Dries’s picture

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.

Dries’s picture

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.