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
#26 1987810-25-follow-up.patch592 byteststoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,552 pass(es).
[ View ]
#26 hide-descriptions.png42.58 KBtstoeckler
#22 system-1987810-22.patch9.32 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,444 pass(es).
[ View ]
#22 interdiff.txt746 bytestim.plunkett
#20 system-1987810-20.patch9.22 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,434 pass(es).
[ View ]
#20 interdiff.txt10.83 KBtim.plunkett
#18 1987810-route-system-config-18.patch4.69 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,589 pass(es), 51 fail(s), and 1 exception(s).
[ View ]
#17 drupal-1987810-6.patch9.39 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,844 pass(es).
[ View ]
#17 interdiff.txt1.42 KBdawehner
#15 drupal-1987810-15.patch9.83 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,474 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#15 interdiff.txt1.29 KBdawehner
#13 drupal-1987810-13.patch9.42 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,719 pass(es), 69 fail(s), and 17 exception(s).
[ View ]
#10 1987810-route-system-config-10.inderdiff.txt7.22 KBnick_schuch
#10 1987810-route-system-config-10.patch10.26 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 55,143 pass(es).
[ View ]
#4 1987810-route-system-config-4.patch7.38 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 55,277 pass(es).
[ View ]
#3 1987810-route-system-config-3.patch3.6 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 55,052 pass(es).
[ View ]
#1 1987810-route-system-config-1.patch6.68 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987810-route-system-config-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

vijaycs85’s picture

Status:Active» Postponed
StatusFileSize
new6.68 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987810-route-system-config-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
nick_schuch’s picture

Assigned:Unassigned» nick_schuch
nick_schuch’s picture

Status:Postponed» Needs review
StatusFileSize
new3.6 KB
PASSED: [[SimpleTest]]: [MySQL] 55,052 pass(es).
[ View ]

I have rerolled this against the latest head now that system_status() is in. Ready for a review.

nick_schuch’s picture

StatusFileSize
new7.38 KB
PASSED: [[SimpleTest]]: [MySQL] 55,277 pass(es).
[ View ]

Ah dam, only half the patch. Here we go.

Status:Needs review» Needs work

The last submitted patch, 1987810-route-system-config-4.patch, failed testing.

nick_schuch’s picture

Errors were these:

"exception 'Drupal\Core\Config\StorageException' with message 'Failed to write configuration file: sites/default/files/simpletest/98944/config_active/manifest.block.block.yml' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Config/FileStorage.php:102
Stack trace:"

Going to requeue.

nick_schuch’s picture

Status:Needs work» Needs review
nick_schuch’s picture

#4: 1987810-route-system-config-4.patch queued for re-testing.

dawehner’s picture

Status:Needs review» Needs work
+++ b/core/modules/system/lib/Drupal/system/Controller/ConfigController.phpundefined
@@ -0,0 +1,99 @@
+ * Definition of Drupal\system\Controller\ConfigController.

The documentation standard suggests to use "Contains \..."

+++ b/core/modules/system/lib/Drupal/system/Controller/ConfigController.phpundefined
@@ -0,0 +1,99 @@
+   * Menu callback; Provide the administration overview page.

Just drop the bit about the menu callback and maybe provide an @return

+++ b/core/modules/system/lib/Drupal/system/Controller/ConfigController.phpundefined
@@ -0,0 +1,99 @@
+    if ($system_link = entity_load_multiple_by_properties('menu_link', array('link_path' => 'admin/config', 'module' => 'system'))) {
...
+        $menu_links = menu_link_load_multiple($result);

You can inject the menu link entity storage controller and load the entities from there.

+++ b/core/modules/system/lib/Drupal/system/Controller/ConfigController.phpundefined
@@ -0,0 +1,99 @@
+      $query = \Drupal::entityQuery('menu_link')

Please inject the entity query into the controller.

+++ b/core/modules/system/lib/Drupal/system/Controller/ConfigController.phpundefined
@@ -0,0 +1,99 @@
+          $block['content'] = '';
+          $block['content'] .= theme('admin_block_content', array('content' => system_admin_menu_block($item)));
...
+      return theme('admin_page', array('blocks' => $blocks));
...
+      return t('You do not have any administrative items.');

What about converting it to a render array here, I guess this should be possible.

+++ b/core/modules/system/system.routing.ymlundefined
@@ -137,3 +137,9 @@ system_php:
+    _controller: 'Drupal\system\Controller\ConfigController::overview'

this should be _content here.

nick_schuch’s picture

Status:Needs work» Needs review
StatusFileSize
new10.26 KB
PASSED: [[SimpleTest]]: [MySQL] 55,143 pass(es).
[ View ]
new7.22 KB

Ok, here is a patch that I believe addresses the items in #9. I also updated all the _controller references to _content.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community
+++ b/core/modules/system/lib/Drupal/system/Controller/ConfigController.phpundefined
@@ -0,0 +1,140 @@
+    $blocks = array();
+    $query = $this->queryFactory->get('menu_link')
+      ->condition('link_path', 'admin/config')
+      ->condition('module', 'system');
+    $result = $query->execute();
+    if ($system_link = $this->entityManager->getStorageController('menu_link')->load($result)) {
+      $system_link = reset($system_link);
+      $query = $this->queryFactory->get('menu_link')
+        ->condition('link_path', 'admin/help', '<>')
+        ->condition('menu_name', $system_link->menu_name)
+        ->condition('plid', $system_link->id())
+        ->condition('hidden', 0);
+      $result = $query->execute();
+      if (!empty($result)) {
+        $menu_links = $this->entityManager
+          ->getStorageController('menu_link')

It would be cool to have some comment what the hell is happening here.

+++ b/core/modules/system/lib/Drupal/system/Controller/ConfigController.phpundefined
@@ -0,0 +1,140 @@
+          $block['content'] = theme('admin_block_content', array('content' => system_admin_menu_block($item)));

We could just switch to a render array here.

+++ b/core/modules/system/system.routing.ymlundefined
+++ b/core/modules/system/system.routing.ymlundefined
@@ -1,13 +1,13 @@

@@ -1,13 +1,13 @@
system.cron:
   pattern: '/cron/{key}'
   defaults:
-    _controller: '\Drupal\system\CronController::run'
+    _content: '\Drupal\system\CronController::run'
   requirements:
     _access_system_cron: 'TRUE'
system.machine_name_transliterate:
   pattern: '/machine_name/transliterate'
   defaults:
-    _controller: '\Drupal\system\MachineNameController::transliterate'
+    _content: '\Drupal\system\MachineNameController::transliterate'
   requirements:
     _permission: 'access content'

@@ -91,7 +91,7 @@ date_format_edit:

@@ -91,7 +91,7 @@ date_format_edit:
system_run_cron:
   pattern: '/admin/reports/status/run-cron'
   defaults:
-    _controller: '\Drupal\system\CronController::runManually'
+    _content: '\Drupal\system\CronController::runManually'
   requirements:
     _permission: 'administer site configuration'

@@ -112,28 +112,34 @@ date_format_localize_reset:

@@ -112,28 +112,34 @@ date_format_localize_reset:
system_theme_disable:
   pattern: '/admin/appearance/disable'
   defaults:
-    _controller: 'Drupal\system\Controller\ThemeController::disable'
+    _content: 'Drupal\system\Controller\ThemeController::disable'
   requirements:
     _permission: 'administer themes'

system_theme_enable:
   pattern: '/admin/appearance/enable'
   defaults:
-    _controller: 'Drupal\system\Controller\ThemeController::enable'
+    _content: 'Drupal\system\Controller\ThemeController::enable'
   requirements:
     _permission: 'administer themes'

system_status:
   pattern: '/admin/reports/status'
   defaults:
-    _controller: 'Drupal\system\Controller\SystemInfoController::status'
+    _content: 'Drupal\system\Controller\SystemInfoController::status'
   requirements:
     _permission: 'administer site configuration'

system_php:
   pattern: '/admin/reports/status/php'
   defaults:
-    _controller: 'Drupal\system\Controller\SystemInfoController::php'
+    _content: 'Drupal\system\Controller\SystemInfoController::php'
   requirements:
     _permission: 'administer site configuration'

Yes right these ones should be replacing _controller by _content, but ideally this should have been a new issue. It just saves kittens.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Okay based on #11 needs work...

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new9.42 KB
FAILED: [[SimpleTest]]: [MySQL] 56,719 pass(es), 69 fail(s), and 17 exception(s).
[ View ]

Let's just do that.

Status:Needs review» Needs work

The last submitted patch, drupal-1987810-13.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.29 KB
new9.83 KB
FAILED: [[SimpleTest]]: [MySQL] 56,474 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

urgs.

Status:Needs review» Needs work

The last submitted patch, drupal-1987810-15.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new9.39 KB
PASSED: [[SimpleTest]]: [MySQL] 56,844 pass(es).
[ View ]

SO using render arrays would need changes in more then one place.

vijaycs85’s picture

Issue tags:+LONDON_2013_JULY
StatusFileSize
new4.69 KB
FAILED: [[SimpleTest]]: [MySQL] 57,589 pass(es), 51 fail(s), and 1 exception(s).
[ View ]

Re-roll...

Status:Needs review» Needs work

The last submitted patch, 1987810-route-system-config-18.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new10.83 KB
new9.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,434 pass(es).
[ View ]

Rerolled, I took a slightly different approach.

jibran’s picture

Nothing big.

  1. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -7,21 +7,122 @@
    +            '#content' => system_admin_menu_block($item),

    @todo here.

  2. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -32,9 +133,9 @@ public static function create(ContainerInterface $container) {
    -    return new RedirectResponse(url('<front>', array('absolute' => TRUE)));
    +    return $this->redirect('front');

    This is awesome. I completely missed it.

  3. +++ b/core/modules/system/system.admin.inc
    @@ -400,7 +346,7 @@ function theme_admin_block($variables) {
    +    $output .= '<div class="body">' . render($block['content']) . '</div>';

    It is in the theme function I think it is fine.

PS: https://twitter.com/JibranIjaz/status/366753152608903169

tim.plunkett’s picture

Assigned:nick_schuch» Unassigned
StatusFileSize
new746 bytes
new9.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,444 pass(es).
[ View ]

Added the todo. This now blocks #1987814: Convert system_admin_menu_block_page() to a new style controller, let's get it in.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for the change. Code looks great let's get it in.

tim.plunkett’s picture

Priority:Normal» Critical
Issue tags:+blocker

All of these are criticals, but I'm actually marking this one since it blocks another.

webchick’s picture

Status:Reviewed & tested by the community» Fixed
+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
@@ -7,21 +7,124 @@
+class SystemController extends ControllerBase implements ControllerInterface {

Man, we so desperately need to rename that interface. :P

Committed and pushed to 8.x. Thanks!

tstoeckler’s picture

Status:Fixed» Needs review
Issue tags:+Needs tests
StatusFileSize
new42.58 KB
new592 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,552 pass(es).
[ View ]
+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
@@ -32,9 +135,9 @@ public static function create(ContainerInterface $container) {
+    return $this->redirect('front');

The route is in fact called <front>. This is currently broken in 8.x, which you can verify by clicking the "Hide descriptions" link on admin/config. You will get a nice helpful error that the 'front' route does not exist. (See also attached screenshot.) That's why I'm re-opening this instead of opening a follow-up. Setting to needs review so the attached patch gets a date with the bot, but the fact that 8.x is currently passing tests is that we're missing test coverage for this.

Edit: Fixed HTML for < front >

tstoeckler’s picture

Priority:Critical» Major
Status:Needs review» Needs work

Now back to needs work for tests.

Also this is no longer critical. It's sort of major though due to the current state in 8.x.

pfrenssen’s picture

Assigned:Unassigned» pfrenssen

I'll write a test.

tim.plunkett’s picture

Assigned:pfrenssen» Unassigned
Priority:Major» Critical
Status:Needs work» Fixed
Issue tags:-Needs tests

#2076551: SystemController::compactPage uses an invalid route name for the front page was opened for this, I thought pwolanin had linked it here.

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