Convert block_admin_display() to a new style controller

Will work on after #1983844: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case

Intial patch..

Initial patch..

+++ b/core/modules/block/block.moduleundefined
@@ -116,10 +116,7 @@ function block_menu() {
-    'page arguments' => array($default_theme),

So the problem is this line. It passes a variable to the page callback, which the EntityListController cannot handle.

This will need something more custom.

It would be at least possible to extend the default EntityListController so there is just a single override needed for this function.

You'll also need to convert the foreach loop at the bottom into a RouteSubscriber. It currently is trying to use the same page callback.

+++ b/core/modules/block/block.services.ymlundefined
@@ -9,3 +9,11 @@ services:
+  block.route_subscriber:
+    class: Drupal\block\Routing\RouteSubscriber
+    tags:
+      - { name: event_subscriber}

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,52 @@
+    foreach (drupal_container()->get('plugin.manager.system.plugin_ui')->getDefinitions() as $plugin_id => $plugin) {

You can have the plugin UI manager injected via the services.yml

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,52 @@
+use \Drupal\Core\Routing\RouteBuildEvent;
+use \Drupal\Core\Routing\RoutingEvents;
+use \Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use \Symfony\Component\Routing\Route;

use statements don't need leading \

Otherwise, this looks great!

+++ b/core/modules/block/block.services.ymlundefined
@@ -13,6 +13,7 @@ services:
+    arguments: ['@service_container']

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -17,6 +19,21 @@
+  protected $container;
...
+    $this->container = $container;

@@ -35,7 +52,7 @@ static function getSubscribedEvents() {
+    foreach ($this->container->get('plugin.manager.system.plugin_ui')->getDefinitions() as $plugin_id => $plugin) {

Instead of getting the container, you can just get that single service injected. Sorry for the confusion

Is this

/**
  * @param \Drupal\Component\Plugin\PluginManagerInterface $plugin_manager
  *   The service container this object should use.
  */
  public function __construct(PluginManagerInterface $plugin_manager) {
    $this->plugin_manager = $plugin_manager;
  }

same as

/**
  * @param Drupal\system\Plugin\Type\PluginUIManager $plugin_manager
  *   The service container this object should use.
  */
  public function __construct(PluginUIManager $plugin_manager) {
    $this->plugin_manager = $plugin_manager;
  }

because both works or more specifically does it have anything to do with failed testing?

BlockStorageUnitTest needs to add 'system' to the public static $modules array.

+++ b/core/modules/block/block.routing.ymlundefined
@@ -4,3 +4,11 @@ block_admin_block_delete:
+    _controller: '\Drupal\block\Controller\BlockListController::listing'

This should use _content instead of _controller.

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockListController.phpundefined
@@ -0,0 +1,31 @@
+    $default_theme = $theme ?: config('system.theme')->get('default');

Please inject the config settings into the controller.

This looks perfect, commit if green!
Thanks @vijaycs85.

AFAICS this should remove the block_admin_display() function as once the patch has been applied it is not used.

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockListController.phpundefined
@@ -0,0 +1,67 @@
+   * @return array|string

So why should this be a string?

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,69 @@
+  /**
...
+  public function __construct(PluginManagerInterface $plugin_manager) {

Has some missing docs.

Just a few code standards issues... nothing major.

+++ b/core/modules/block/block.moduleundefined
@@ -117,10 +117,7 @@ function block_menu() {
+    'route_name' => 'block_admin_display'

Last item in an array should have a comma.

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,72 @@
+  protected $plugin_manager;

This should be $camelCase.

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,72 @@
+  static function getSubscribedEvents() {

Needs a scope modifier (i.e. public).

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,72 @@
+        ),array(

Space after the comma required.

+++ b/core/modules/block/lib/Drupal/block/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,72 @@
+        $collection->add('block_admin_display.' . $plugin_id , $route);

Space before comma should be removed.

With those changes, this would be RTBC, in my opinion.

Currently blocking #2014191: Implement the block-by-theme listing page.

I've reviewed and tested this as part of #2014191: Implement the block-by-theme listing page, and I can't see any further issues with it. Looking good.

+++ b/core/modules/block/block.routing.ymlundefined
@@ -4,3 +4,11 @@ block_admin_block_delete:
+    _content: '\Drupal\block\Controller\BlockListController::listing'
+    entity_type: 'block'

there is a new _entity_list key :) added in #2008356: Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form

Correct, since the block list needs $theme passed along, it cannot use _entity_list.

Committed 40471c8 and pushed to 8.x. Thanks!