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

FileSize
7.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
FileSize
10.26 KB
PASSED: [[SimpleTest]]: [MySQL] 55,143 pass(es). View
7.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
FileSize
9.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
FileSize
1.29 KB
9.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
FileSize
1.42 KB
9.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
FileSize
4.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
FileSize
10.83 KB
9.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
FileSize
746 bytes
9.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
FileSize
42.58 KB
592 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.