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
FileSize
4.85 KB
PASSED: [[SimpleTest]]: [MySQL] 56,074 pass(es). View
pdrake’s picture

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

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

FileSize
1.03 KB
7.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
FileSize
7.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,048 pass(es). View

I removed that changes to the patch.

helenkim’s picture

pguillard’s picture

FileSize
1.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
FileSize
84.67 KB
21.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.png

page-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
FileSize
5.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

     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
FileSize
8.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,501 pass(es), 1 fail(s), and 0 exception(s). View
7 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
FileSize
708 bytes
8.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
FileSize
8.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
FileSize
1000 bytes
8.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
FileSize
3.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

FileSize
3.31 KB
2.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
FileSize
713 bytes
3.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
FileSize
1014 bytes
3.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
FileSize
2.59 KB
4.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,928 pass(es). View
4.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.