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

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 ]

Assigned:Unassigned» nick_schuch

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.

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.

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.

Status:Needs work» Needs review

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

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.

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.

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.

Status:Reviewed & tested by the community» Needs work

Okay based on #11 needs work...

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.

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.

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.

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.

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.

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

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.

Status:Needs review» Reviewed & tested by the community

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

Priority:Normal» Critical
Issue tags:+blocker

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

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!

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 >

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.

Assigned:Unassigned» pfrenssen

I'll write a test.

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.