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

pdrake’s picture

Assigned:Unassigned» pdrake
pdrake’s picture

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

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

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 ]
kim.pepper’s picture

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

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

kim.pepper’s picture

Issue tags:+Needs reroll
vijaycs85’s picture

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

Re-rolling...

dawehner’s picture

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

Cottser’s picture

Status:Needs review» Needs work
Issue tags:-Needs reroll
vijaycs85’s picture

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

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

vijaycs85’s picture

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you very much!

alexpott’s picture

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.

pguillard’s picture

Assigned:Unassigned» pguillard
pguillard’s picture

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.

helenkim’s picture

pguillard’s picture

StatusFileSize
new1.25 KB

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

helenkim’s picture

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

pguillard’s picture

rtbc ?

kim.pepper’s picture

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

kim.pepper’s picture

Issue tags:+Needs tests

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

AjitS’s picture

Status:Needs work» Needs review
AjitS’s picture

Status:Needs review» Needs work

Changed the status by mistake. Sorry.

ygerasimov’s picture

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.

tim.plunkett’s picture

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.

jibran’s picture

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?

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

Thank goodness for test coverage.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Well now it looks nice and clear.

alexpott’s picture

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

Patch no longer applies.

jibran’s picture

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

vijaycs85’s picture

+1 on RTBC.

jibran’s picture

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.

disasm’s picture

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

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

xjm’s picture

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.

disasm’s picture

Status:Needs review» Needs work
kgoel’s picture

Assigned:Unassigned» kgoel
kgoel’s picture

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.

tim.plunkett’s picture

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

kgoel’s picture

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

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.

kgoel’s picture

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

Status:Needs review» Reviewed & tested by the community

It is good to go.

tim.plunkett’s picture

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

kgoel’s picture

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

Status:Needs review» Reviewed & tested by the community

The feedback got adressed

alexpott’s picture

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

tim.plunkett’s picture

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.

neclimdul’s picture

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.

alexpott’s picture

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.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.