Convert block_admin_demo() to a new style controller

#4: drupal-convert_block_admin_demo-1987636-4.patch queued for re-testing.

The other theme is stilled picked up, during manual testing.

+++ b/core/modules/block/block.moduleundefined
@@ -229,9 +225,8 @@ function block_page_build(&$page) {
+  if ($item['path'] != 'admin/structure/block/demo/%' || !isset($item['map'][4]) || $item['map'][4] != $theme) {

Can we add a documentation what's going on here?

+++ b/core/modules/block/block.moduleundefined
@@ -251,26 +246,23 @@ function block_page_build(&$page) {
+      '#href' => 'admin/structure/block' . (config('system.theme')->get('default') == $theme ? '' : '/list/' . $theme),

Config is a service so let's use Drupal::config() instead.

+++ b/core/modules/block/lib/Drupal/block/Controller/AdminController.phpundefined
@@ -0,0 +1,36 @@
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

This dependency is not needed

+++ b/core/modules/block/lib/Drupal/block/Controller/AdminController.phpundefined
@@ -0,0 +1,36 @@
+   *

just remove this empty line.

+++ b/core/modules/block/lib/Drupal/block/Controller/AdminController.phpundefined
@@ -0,0 +1,36 @@
+    drupal_add_css(drupal_get_path('module', 'block') . '/block.admin.css');

Can you open a follow up to convert this into a library?

+++ b/core/modules/block/lib/Drupal/block/Controller/AdminController.phpundefined
@@ -0,0 +1,36 @@
+    return new Response();

Can't we also just return '' ?

Final nitpick: There should be an empty line between the last two } } of a file.

Thank you very much!

+++ b/core/modules/block/block.admin.incundefined
@@ -9,19 +9,6 @@
-  return array(
-    '#attached' => array(
-      'css' => array(drupal_get_path('module', 'block') . '/css/block.admin.css'),
-    ),
-  );

+++ b/core/modules/block/lib/Drupal/block/Controller/AdminController.phpundefined
@@ -0,0 +1,38 @@
+    drupal_add_css(drupal_get_path('module', 'block') . '/block.admin.css');
+    return '';

We should be return the same array as before... using drupal_add_css in a controller should not be done.

I removed that changes to the patch.

#17: 1987636-convert_block_admin_demo-14.patch queued for re-testing.

Forgot to put the interdiff (My First git diff interdiff :-)) , here it is.

Check the #17's patch from #15. It looks O.K. Thanks you.

rtbc ?

I found a bug.

Steps to reproduce:

• click on seven tab (path is /admin/structure/block/list/block_plugin_ui%3Aseven)
• click on 'demonstrate block regions'
• see a mostly blank screen, no regions demo'd (see screenshot)
• click 'Exit block region demonstration' (path is /admin/structure/block/list/seven)
• get a 'Page not found' error (see screenshot)

Seems to be an issue with paths and the non-default theme.

Flagging as "needs tests" as this wasn't caught by the current tests.

#4: drupal-convert_block_admin_demo-1987636-4.patch queued for re-testing.

Changed the status by mistake. Sorry.

I see the issue with Seven theme and blocks demo described in #22 even without applying any patches. It is a separate bug.

Opened issue #2042879: Theme Seven has "Demonstrate block regions" broken. Shall this new issue be blocking this one? I believe so.

I believe the bug described in #22 was resolved when I revamped the Block UI.
Anyway, here's a reroll with some fixes. The old patch didn't apply, so no interdiff.

Some minor issues.

1. +++ b/core/modules/block/lib/Drupal/block/Access/DemoThemesAccessCheck.php
   @@ -0,0 +1,33 @@
   +    return drupal_theme_access($request->attributes->get('theme'));
   

   :S

    function drupal_theme_access($theme) {
     if (is_object($theme)) {
       return !empty($theme->status);
     }
     else {
       $themes = list_themes();
       return !empty($themes[$theme]->status);
     }
   }

   Why are we depending on theme.inc? We can easily write the logic here.

2. +++ b/core/modules/block/lib/Drupal/block/Controller/AdminController.php
   @@ -0,0 +1,39 @@
   +  public function demo(Request $request) {
   ...
   +    $theme = $themes[$request->attributes->get('theme')];
   

   This doesn't make sense. $request->attributes->get('theme') should be passed to function not $request according to route definition. Am I missing something?

2 is definitely a good point. I'll sleep on 1.

Okay, I did both of #28.
It turns out we had duplicate access checkers, so I simplified all of that.

Thank goodness for test coverage.

Well now it looks nice and clear.

Patch no longer applies.

3-way merge

Replaced t with $this->t in AdminController on @alexpott suggestion.

#37: drupal8.block-module.1987636-37.patch queued for re-testing.

+++ b/core/modules/block/block.module
@@ -214,9 +210,10 @@ function block_page_build(&$page) {
+  // Make sure the blocks are not getting rendered for demo page
+  // or theme specific site building page.
+  if ($item['path'] != 'admin/structure/block/demo/%' || !isset($item['map'][4]) || $item['map'][4] != $theme) {

Can't we just check of if (\Drupal::request()->attributes->get(RouteObjectInterface::ROUTE_NAME) != 'block_admin_demo' ?

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Did reroll first and after bot passes than i will be working on fixes.

1. +++ b/core/modules/block/block.module
   @@ -134,14 +134,6 @@ function block_menu() {
   -      'theme callback' => '_block_custom_theme',
   -      'theme arguments' => array($key),
   

   I think you'll need to leave this until #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system is in.

2. +++ b/core/modules/block/block.routing.yml
   @@ -1,7 +1,7 @@
   -    _content: '\Drupal\block\Controller\BlockController::demo'
   +    _content: '\Drupal\block\Controller\AdminController::demo'
   
   +++ b/core/modules/block/lib/Drupal/block/Controller/AdminController.php
   @@ -0,0 +1,39 @@
   +class AdminController extends ControllerBase {
   
   +++ /dev/null
   @@ -1,23 +0,0 @@
   -class BlockController {
   

   I think this can reasonably leave this as BlockController

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockController.php
@@ -7,17 +7,33 @@
-    return block_admin_demo($theme);

Removal of this function from block.admin.inc file is missing.

It is good to go.

1. +++ b/core/modules/block/block.module
   @@ -135,7 +135,6 @@ function block_menu() {
   -      'title' => check_plain($theme->info['name']),
   
   +++ b/core/modules/block/lib/Drupal/block/Controller/BlockController.php
   @@ -7,17 +7,33 @@
   +      '#title' => $this->t($themes[$theme]->info['name']),
   

   This is switched from check_plain to t(), why? Should probably be String::checkPlain

2. +++ b/core/modules/block/lib/Drupal/block/Controller/BlockController.php
   @@ -7,17 +7,33 @@
   +   *   THe name of the theme.
   

   Wrongly capitalized H in THe

The feedback got adressed

+++ b/core/modules/block/block.module
@@ -193,9 +191,10 @@ function block_page_build(&$page) {
+  // Make sure the blocks are not getting rendered for demo page
+  // or theme specific site building page.
+  if ($item['path'] != 'admin/structure/block/demo/%' || !isset($item['map'][4]) || $item['map'][4] != $theme) {

Lets check the route name here instead of path. This formulation looks fragile. I think the new if means that $item['map'][4] == $theme then blocks will not be rendered... regardless of what is in $item['path'].

We can just use the route name.

I also added tests, because we were missing a use statement, which caused the page to fatal.

+++ b/core/modules/block/block.module
@@ -193,9 +192,7 @@ function block_page_build(&$page) {
+  if (\Drupal::request()->attributes->get(RouteObjectInterface::ROUTE_NAME) != 'block.admin_demo') {

This look questionable because we where ignoring the parameter but tim assured me we don't care and reviewing further that seems true.

This looks pretty straight forward. Back to RTBC with the assumption testbot comes back green.

Committed c9e8fe3 and pushed to 8.x. Thanks!