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.

Test process :
-Apply patch
-Refresh Cache
-Check that block pages like :
-- /admin/structure/block/demo/bartik
-- /admin/structure/block/demo/seven
are still ok

Files: 
CommentFileSizeAuthor
#55 block-conversion-1987636-55-FAIL.patch4.33 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,698 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#55 block-conversion-1987636-55-PASS.patch4.5 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,928 pass(es).
[ View ]
#55 interdiff.txt2.59 KBtim.plunkett
#52 block-conversion-1987636-52.patch3.31 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 58,778 pass(es).
[ View ]
#52 interdiff.txt1014 byteskgoel
#49 block-conversion-1987636-49.patch3.3 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 58,971 pass(es).
[ View ]
#49 interdiff.txt713 byteskgoel
#47 block-conversion-1987636-47.patch2.61 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 58,933 pass(es).
[ View ]
#47 interdiff.txt3.31 KBkgoel
#44 block-conversion-1987636-44.patch3.76 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/block/block.module.
[ View ]
#37 drupal8.block-module.1987636-37.patch8.66 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,521 pass(es).
[ View ]
#37 interdiff.txt1000 bytesjibran
#35 drupal8.block-module.1987636-35.patch8.59 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,023 pass(es).
[ View ]
#32 block-1987636-32.patch8.59 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,492 pass(es).
[ View ]
#32 interdiff.txt708 bytestim.plunkett
#30 interdiff.txt7 KBtim.plunkett
#30 block-1987636-30.patch8.53 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,501 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#27 block-1987636-27.patch5.57 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,459 pass(es).
[ View ]
#22 seven-block-demo.png21.53 KBkim.pepper
#22 page-not-found.png84.67 KBkim.pepper
#19 interdiff.1987636.13.17.txt1.25 KBpguillard
#17 1987636-convert_block_admin_demo-14.patch7.08 KBpguillard
PASSED: [[SimpleTest]]: [MySQL] 58,048 pass(es).
[ View ]
#13 1987636-convert_block_admin_demo-13.patch7.78 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,968 pass(es).
[ View ]
#13 1987636-diff-11-13.txt1.03 KBvijaycs85
#11 1987636-convert_block_admin_demo-11.patch7.78 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,458 pass(es).
[ View ]
#8 1987636-convert_block_admin_demo-8.patch7.58 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,352 pass(es).
[ View ]
#4 drupal-convert_block_admin_demo-1987636-4.patch7.59 KBpdrake
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-convert_block_admin_demo-1987636-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 drupal-convert_block_admin_demo-1987636-3.patch7.25 KBpdrake
PASSED: [[SimpleTest]]: [MySQL] 56,008 pass(es).
[ View ]
#2 drupal-convert_block_admin_demo-1987636-2.patch4.85 KBpdrake
PASSED: [[SimpleTest]]: [MySQL] 56,074 pass(es).
[ View ]

Comments

Assigned:Unassigned» pdrake

Status:Active» Needs review
StatusFileSize
new4.85 KB
PASSED: [[SimpleTest]]: [MySQL] 56,074 pass(es).
[ View ]

StatusFileSize
new7.25 KB
PASSED: [[SimpleTest]]: [MySQL] 56,008 pass(es).
[ View ]

StatusFileSize
new7.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-convert_block_admin_demo-1987636-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

The last submitted patch, drupal-convert_block_admin_demo-1987636-4.patch, failed testing.

Issue tags:+Needs reroll

Status:Needs work» Needs review
StatusFileSize
new7.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,352 pass(es).
[ View ]

Re-rolling...

Assigned:pdrake» Unassigned

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

Status:Needs review» Needs work
Issue tags:-Needs reroll

Status:Needs work» Needs review
StatusFileSize
new7.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,458 pass(es).
[ View ]

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

StatusFileSize
new1.03 KB
new7.78 KB
PASSED: [[SimpleTest]]: [MySQL] 57,968 pass(es).
[ View ]

Thanks for the review @dawehner. Updated Coding standard page to reflect this standard (https://drupal.org/node/608152/revisions/view/2686632/2739353).

Status:Needs review» Reviewed & tested by the community

Thank you very much!

Status:Reviewed & tested by the community» Needs work

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

Assigned:Unassigned» pguillard

Assigned:pguillard» Unassigned
Status:Needs work» Needs review
StatusFileSize
new7.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,048 pass(es).
[ View ]

I removed that changes to the patch.

StatusFileSize
new1.25 KB

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 ?

Status:Needs review» Needs work
StatusFileSize
new84.67 KB
new21.53 KB

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.

seven-block-demo.pngpage-not-found.png

Issue tags:+Needs tests

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new5.57 KB
PASSED: [[SimpleTest]]: [MySQL] 58,459 pass(es).
[ View ]

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.

Status:Needs review» Needs work

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

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

Assigned:Unassigned» tim.plunkett

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

Status:Needs work» Needs review
StatusFileSize
new8.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,501 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new7 KB

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

Status:Needs review» Needs work

The last submitted patch, block-1987636-30.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new708 bytes
new8.59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,492 pass(es).
[ View ]

Thank goodness for test coverage.

Status:Needs review» Reviewed & tested by the community

Well now it looks nice and clear.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new8.59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,023 pass(es).
[ View ]

3-way merge

+1 on RTBC.

Assigned:tim.plunkett» Unassigned
StatusFileSize
new1000 bytes
new8.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,521 pass(es).
[ View ]

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

Status:Reviewed & tested by the community» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, drupal8.block-module.1987636-37.patch, failed testing.

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

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

Status:Needs review» Needs work

Assigned:Unassigned» kgoel

Status:Needs work» Needs review
StatusFileSize
new3.76 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/block/block.module.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, block-conversion-1987636-44.patch, failed testing.

Status:Needs work» Needs review
  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

StatusFileSize
new3.31 KB
new2.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,933 pass(es).
[ View ]

Status:Needs review» Needs work

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

Assigned:kgoel» Unassigned
Status:Needs work» Needs review
StatusFileSize
new713 bytes
new3.3 KB
PASSED: [[SimpleTest]]: [MySQL] 58,971 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

It is good to go.

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

Status:Needs work» Needs review
StatusFileSize
new1014 bytes
new3.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,778 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

The feedback got adressed

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.59 KB
new4.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,928 pass(es).
[ View ]
new4.33 KB
FAILED: [[SimpleTest]]: [MySQL] 58,698 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

We can just use the route name.

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Fixed

Committed c9e8fe3 and pushed to 8.x. Thanks!

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

Issue summary:View changes

Updated issue summary.