Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#42 1987638-convert-block-admin-display-42.patch9.37 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,892 pass(es).
[ View ]
#39 1987638-convert-block-admin-display-37.patch9.39 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 56,215 pass(es).
[ View ]
#39 1987638-diff-35-37.txt2.55 KBvijaycs85
#35 1987638-convert-block-admin-display-35.patch9.38 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,348 pass(es).
[ View ]
#35 1987638-diff-32-35.txt1.42 KBvijaycs85
#32 1987638-convert-block-admin-display-32.patch9.34 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 53,083 pass(es), 128 fail(s), and 35 exception(s).
[ View ]
#32 1987638-diff-29-32.txt752 bytesvijaycs85
#29 1987638-convert-block-admin-display-29.patch8.61 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 56,570 pass(es).
[ View ]
#29 1987638-diff-27-29.txt2.66 KBvijaycs85
#27 1987638-convert-block-admin-display-27.patch7.59 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 56,254 pass(es).
[ View ]
#27 1987638-diff-17-27.txt600 bytesvijaycs85
#17 1987638-convert-block-admin-display-17.patch7.01 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 55,285 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 1987638-diff-15-17.txt2.66 KBvijaycs85
#15 1987638-convert-block-admin-display-15.patch7.02 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 55,326 pass(es).
[ View ]
#15 1987638-diff-13-15.txt2.55 KBvijaycs85
#13 1987638-convert-block-admin-display-13.patch6.44 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 56,021 pass(es).
[ View ]
#13 1987638-diff-10-13.txt5.24 KBvijaycs85
#10 1987638-convert-block-admin-display-10.patch2.25 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 55,591 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#10 1987638-diff-5-10.txt1.51 KBvijaycs85
#5 convert-block-admin-display.patch1.29 KBmanu4543
FAILED: [[SimpleTest]]: [MySQL] 56,071 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#2 convert block-admin-display.patch1.29 KBmanu4543
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert block-admin-display.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

tim.plunkett’s picture

manu4543’s picture

Status:Active» Needs review
StatusFileSize
new1.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert block-admin-display.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Intial patch..

manu4543’s picture

Initial patch..

Status:Needs review» Needs work

The last submitted patch, convert block-admin-display.patch, failed testing.

manu4543’s picture

StatusFileSize
new1.29 KB
FAILED: [[SimpleTest]]: [MySQL] 56,071 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
manu4543’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, convert-block-admin-display.patch, failed testing.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
+++ 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.

dawehner’s picture

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

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
new2.25 KB
FAILED: [[SimpleTest]]: [MySQL] 55,591 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Will it help?

Status:Needs review» Needs work

The last submitted patch, 1987638-convert-block-admin-display-10.patch, failed testing.

tim.plunkett’s picture

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.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new5.24 KB
new6.44 KB
PASSED: [[SimpleTest]]: [MySQL] 56,021 pass(es).
[ View ]

Thanks for the quick reply @tim.plunkett. I have updated to routeSubscriber. I can see a TODO for D8 in foreach,

// Block administration is tied to the theme and plugin definition so
  // that the plugin can appropriately attach to this URL structure.
  // @todo D8: Use dynamic % arguments instead of static, hard-coded theme names
  //   and plugin IDs to decouple the routes from these dependencies and allow
  //   hook_menu_local_tasks() to check for the untranslated tab_parent path.
  // @see http://drupal.org/node/1067408
  $themes = list_themes();
  foreach (drupal_container()->get('plugin.manager.system.plugin_ui')->getDefinitions() as $plugin_id => $plugin) {

but not sure what that means. Hopefully this patch makes the patch green at least.

tim.plunkett’s picture

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

vijaycs85’s picture

StatusFileSize
new2.55 KB
new7.02 KB
PASSED: [[SimpleTest]]: [MySQL] 55,326 pass(es).
[ View ]

Updating patch to reflect comments in #14

tim.plunkett’s picture

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

vijaycs85’s picture

StatusFileSize
new2.66 KB
new7.01 KB
FAILED: [[SimpleTest]]: [MySQL] 55,285 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

sorry, my mistake. injected just plugin manager.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1987638-convert-block-admin-display-17.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1987638-convert-block-admin-display-17.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion
manu4543’s picture

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?

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1987638-convert-block-admin-display-17.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion

The last submitted patch, 1987638-convert-block-admin-display-17.patch, failed testing.

tim.plunkett’s picture

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

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new600 bytes
new7.59 KB
PASSED: [[SimpleTest]]: [MySQL] 56,254 pass(es).
[ View ]

Thanks @tim.plunkett. That solves the issue (Can confirm here).

dawehner’s picture

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

vijaycs85’s picture

StatusFileSize
new2.66 KB
new8.61 KB
PASSED: [[SimpleTest]]: [MySQL] 56,570 pass(es).
[ View ]

thanks @dawehner. Here is the fix for #28.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new752 bytes
new9.34 KB
FAILED: [[SimpleTest]]: [MySQL] 53,083 pass(es), 128 fail(s), and 35 exception(s).
[ View ]

alexpott++. just missed the basic purpose of moving stuffs :) Did a code look up for any other reference to block_admin_display() and seems we are good to remove it.

Status:Needs review» Needs work

The last submitted patch, 1987638-convert-block-admin-display-32.patch, failed testing.

dawehner’s picture

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

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new9.38 KB
PASSED: [[SimpleTest]]: [MySQL] 57,348 pass(es).
[ View ]

Will it help?

xtfer’s picture

Status:Needs review» Needs work

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.

xtfer’s picture

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

xtfer’s picture

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new2.55 KB
new9.39 KB
PASSED: [[SimpleTest]]: [MySQL] 56,215 pass(es).
[ View ]

Thanks for your review @xtfer. Here is the patch that fixes all review comments in #36.

xtfer’s picture

Status:Needs review» Reviewed & tested by the community

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.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new9.37 KB
PASSED: [[SimpleTest]]: [MySQL] 57,892 pass(es).
[ View ]

Just re-roll as we can't use _entity_list in blockListContoller.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

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