Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Postponed
FileSize
6.68 KB
nick_schuch’s picture

Assigned: Unassigned » nick_schuch
nick_schuch’s picture

Status: Postponed » Needs review
FileSize
3.6 KB

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

nick_schuch’s picture

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

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

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

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

vijaycs85’s picture

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

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

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