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.

Change notice is at here

Files: 
CommentFileSizeAuthor
#80 interdiff.txt2.79 KBtim.plunkett
#80 system-1987814-80.patch26.73 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,422 pass(es). View
#75 interdiff.txt915 bytestim.plunkett
#75 system-1987814-75.patch26.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es). View
#70 system-1987814-70.patch26.06 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,125 pass(es), 3 fail(s), and 0 exception(s). View
#70 interdiff.txt2.6 KBtim.plunkett
#67 interdiff.txt767 bytestim.plunkett
#67 system-1987814-67.patch24.01 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,340 pass(es), 2 fail(s), and 0 exception(s). View
#65 system-1987814-65.patch23.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,399 pass(es), 2 fail(s), and 0 exception(s). View
#62 system-1987814-62.patch22.87 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,556 pass(es), 2 fail(s), and 0 exception(s). View
#62 interdiff.txt1.24 KBtim.plunkett
#60 system-1987814-60.patch22.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,490 pass(es), 9 fail(s), and 816 exception(s). View
#60 interdiff.txt8.47 KBtim.plunkett
#54 drupal-system-1987814-54.patch17.13 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 56,408 pass(es), 1 fail(s), and 0 exception(s). View
#54 interdiff.txt2.91 KBjuampynr
#48 drupal-system-1987814-48.patch11.85 KBkushrohra
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#47 drupal-system-1987814-47.patch17.11 KBjuampynr
PASSED: [[SimpleTest]]: [MySQL] 58,064 pass(es). View
#47 interdiff.txt568 bytesjuampynr
#45 drupal-system-1987814-45.patch16.69 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 58,385 pass(es), 1 fail(s), and 0 exception(s). View
#45 interdiff.txt2.3 KBjuampynr
#43 drupal-system-1987814-43.patch16.15 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 58,111 pass(es), 2 fail(s), and 0 exception(s). View
#43 interdiff.txt568 bytesjuampynr
#41 drupal-system-1987814-41.patch15.74 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 58,283 pass(es), 2 fail(s), and 0 exception(s). View
#41 interdiff.txt927 bytesjuampynr
#36 drupal-system-1987814-36.patch15.59 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 58,068 pass(es), 2 fail(s), and 0 exception(s). View
#36 interdiff.txt1.16 KBjuampynr
#33 drupal-system-1987814-33.patch15.5 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 58,626 pass(es), 2 fail(s), and 0 exception(s). View
#32 drupal-system-1987814-32.patch15.49 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 58,138 pass(es), 130 fail(s), and 36 exception(s). View
#30 drupal-system-1987814-30.patch15.5 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 55,911 pass(es), 2 fail(s), and 0 exception(s). View
#28 drupal-system-1987814-28.patch16.47 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-system-1987814-28.patch. Unable to apply patch. See the log in the details link for more information. View
#28 interdiff.txt532 bytesjuampynr
#26 drupal-system-1987814-26.patch16.47 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-system-1987814-26.patch. Unable to apply patch. See the log in the details link for more information. View
#23 drupal-system-1987814-23.patch17 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 54,972 pass(es), 5 fail(s), and 0 exception(s). View
#23 interdiff.txt1.11 KBjuampynr
#22 drupal-system-7391414-21.patch16.97 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#21 drupal-system-7391414-20.patch0 bytesjuampynr
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#16 drupal-system-7391414-16.patch16.94 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 57,105 pass(es), 110 fail(s), and 44 exception(s). View
#16 interdiff.txt3.37 KBjuampynr
#14 drupal-system-7391414-14.patch15.55 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 56,057 pass(es), 5 fail(s), and 0 exception(s). View
#10 drupal-system-7391414-10.patch15.48 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 56,063 pass(es), 5 fail(s), and 0 exception(s). View
#10 interdiff.txt5.24 KBjuampynr
#5 drupal-system-7391414-5.patch11.04 KBlliss
FAILED: [[SimpleTest]]: [MySQL] 55,499 pass(es), 7 fail(s), and 0 exception(s). View
#2 drupal-system-7391414-2.patch9.25 KBlliss
FAILED: [[SimpleTest]]: [MySQL] 55,423 pass(es), 48 fail(s), and 1 exception(s). View

Comments

lliss’s picture

Assigned: Unassigned » lliss
lliss’s picture

FileSize
9.25 KB
FAILED: [[SimpleTest]]: [MySQL] 55,423 pass(es), 48 fail(s), and 1 exception(s). View

First go at it. Patch attached.

lliss’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-system-7391414-2.patch, failed testing.

lliss’s picture

Status: Needs work » Needs review
FileSize
11.04 KB
FAILED: [[SimpleTest]]: [MySQL] 55,499 pass(es), 7 fail(s), and 0 exception(s). View

Okay. Tried to get to clever and use a single route to handle a bunch of different paths. But these started clobbering the others further down the url path. admin/config/content would work but admin/config/content/something would not. So I'm just declaring a single route for each item now. This seems redundant.

Status: Needs review » Needs work

The last submitted patch, drupal-system-7391414-5.patch, failed testing.

kim.pepper’s picture

lliss, are you still working on this?

lliss’s picture

I'm willing to but I couldn't make sense of the last round of failures from the test bot. Any guidance would be appreciated.

tim.plunkett’s picture

I'm not sure if they were added since the last patch, but system_admin_menu_block_page() is used as a page callback in both core/modules/user/user.module and core/modules/system/tests/modules/menu_test/menu_test.module.

Those would need to be converted.

juampynr’s picture

FileSize
5.24 KB
15.48 KB
FAILED: [[SimpleTest]]: [MySQL] 56,063 pass(es), 5 fail(s), and 0 exception(s). View

Converted remaining callbacks. Also removed function bookRender().

I am not sure about how to change the following since now we have several routes pointing to the same controller.

// core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php

/**
   * Loads system menu link as needed by system_get_module_admin_tasks().
   *
   * @return array
   *   An array of menu link entities indexed by their IDs.
   */
  public function loadModuleAdminTasks() {
    $query = $this->buildQuery(NULL);
    $query
      ->condition('base.link_path', 'admin/%', 'LIKE')
      ->condition('base.hidden', 0, '>=')
      ->condition('base.module', 'system')
      ->condition('m.number_parts', 1, '>')
      ->condition('m.page_callback', 'system_admin_menu_block_page', '<>');
    $ids = $query->execute()->fetchCol(1);

    return $this->load($ids);
  }

juampynr’s picture

Status: Needs work » Needs review

Changing status.

Status: Needs review » Needs work

The last submitted patch, drupal-system-7391414-10.patch, failed testing.

longwave’s picture

Many of the comment blocks in #10 need reformatting so they wrap correctly at 80 characters.

juampynr’s picture

Status: Needs work » Needs review
FileSize
15.55 KB
FAILED: [[SimpleTest]]: [MySQL] 56,057 pass(es), 5 fail(s), and 0 exception(s). View

Fixed comment lengths. I still need some help with #10.

dawehner’s picture

Status: Needs review » Needs work

This is really cool.

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -0,0 +1,51 @@
+   * @return
+   *   The output HTML.
...
+      $output = theme('admin_block_content', array('content' => $content));
...
+      $output = t('You do not have any administrative items.');
...
+    return $output;

What about just returning a render array?

+++ b/core/modules/system/system.routing.ymlundefined
@@ -1,3 +1,75 @@
+system_admin:
+  pattern: '/admin'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_structure:
+  pattern: '/admin/structure'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_reports:
+  pattern: '/admin/reports'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_config_media:
+  pattern: '/admin/config/media'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_config_services:
+  pattern: '/admin/config/services'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_config_development:
+  pattern: '/admin/config/development'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_config_regional:
+  pattern: '/admin/config/regional'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_config_search:
+  pattern: '/admin/config/search'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_config_system:
+  pattern: '/admin/config/system'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_config_ui:
+  pattern: '/admin/config/user-interface'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_config_workflow:
+  pattern: '/admin/config/workflow'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_config_content:
+  pattern: '/admin/config/content'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:

This is pretty much awesome! In controller for all content

+++ b/core/modules/user/user.routing.ymlundefined
@@ -26,6 +26,13 @@ user_autocomplete_anonymous:
+user_accounts:
+  pattern: '/admin/config/people'
+  defaults:
+    _form: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'

I'm wondering how we can get rid of this additional coupling between user and system module.

juampynr’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
16.94 KB
FAILED: [[SimpleTest]]: [MySQL] 57,105 pass(es), 110 fail(s), and 44 exception(s). View

Now using a render array to return the contents.

Also moved the logic to SystemManager class.

Status: Needs review » Needs work

The last submitted patch, drupal-system-7391414-16.patch, failed testing.

kim.pepper’s picture

One minor nitpick:

+++ b/core/modules/system/lib/Drupal/system/SystemManager.phpundefined
@@ -0,0 +1,43 @@
+   * @return
+   *   The output HTML.

Should be

@return array
A render array suitable for drupal_render.
juampynr’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI-conversion

#16: drupal-system-7391414-16.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal-system-7391414-16.patch, failed testing.

juampynr’s picture

FileSize
0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Nice catch @kim.pepper! Fixed. I have run the tests and confirmed that they are broken because of the changes of the patch, so I am going to dive into them to see what is broken.

juampynr’s picture

FileSize
16.97 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Oops, uploaded the wrong version of the patch. Here it is.

juampynr’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
17 KB
FAILED: [[SimpleTest]]: [MySQL] 54,972 pass(es), 5 fail(s), and 0 exception(s). View

Whoa! and I was using wrong issue numbers patch. I think I need to sleep :S

I figured out why there were so many failing tests: I needed to add the SystemManager to the SystemController with a use statement and make a method public in the former.

Looking forward to see how many remaining tests are to be fixed. I still have the issue I mentioned at #10.

Status: Needs review » Needs work

The last submitted patch, drupal-system-1987814-23.patch, failed testing.

juampynr’s picture

Assigned: lliss » juampynr

Great, I am back to 5 failing assertions. I am going to figure out how #10 works.

Tips are more than welcome.

Also, I am assigning this ticket to myself.

juampynr’s picture

Status: Needs work » Needs review
FileSize
16.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-system-1987814-26.patch. Unable to apply patch. See the log in the details link for more information. View

Re-rolling.

Status: Needs review » Needs work

The last submitted patch, drupal-system-1987814-26.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
532 bytes
16.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-system-1987814-28.patch. Unable to apply patch. See the log in the details link for more information. View

Fixed a permission thanks to @tim.plunkett.

Status: Needs review » Needs work

The last submitted patch, drupal-system-1987814-28.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
15.5 KB
FAILED: [[SimpleTest]]: [MySQL] 55,911 pass(es), 2 fail(s), and 0 exception(s). View

Re-rolling again.

Status: Needs review » Needs work

The last submitted patch, drupal-system-1987814-30.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
15.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,138 pass(es), 130 fail(s), and 36 exception(s). View

Rerolling.

juampynr’s picture

FileSize
15.5 KB
FAILED: [[SimpleTest]]: [MySQL] 58,626 pass(es), 2 fail(s), and 0 exception(s). View

ControllerInterface has changed its namespace. Rerolling again.

Status: Needs review » Needs work

The last submitted patch, drupal-system-1987814-33.patch, failed testing.

dawehner’s picture

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -0,0 +1,46 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static($container->get('system.manager'));
...
+  /**
+   * Constructs a SystemController object.
+   */
+  public function __construct(SystemManager $systemManager) {

__construct needs some docs. While you are here, it might be cooler to have __construct() before create()

juampynr’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
15.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,068 pass(es), 2 fail(s), and 0 exception(s). View

Re-rolled and made the suggested changes at #35. I also saw that one of the remaining two tests failing passed locally so I am giving it another try for the test bot.

Status: Needs review » Needs work

The last submitted patch, drupal-system-1987814-36.patch, failed testing.

vijaycs85’s picture

2 fails:

1. This might not be valid test case anymore. But still out for discussion.

  // file: /core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.php

  /**
   * Test that 'page callback', 'file' and 'file path' keys are properly
   * inherited from parent menu paths.
   */
  function testFileInheritance() {
    $this->drupalGet('admin/config/development/file-inheritance');
    $this->assertText('File inheritance test description', 'File inheritance works.');
  }

2.
HTTP response expected 403, actual 404 Browser MenuTest.php 635 Drupal\menu\Tests\MenuTest->verifyAccess()

don't see anything in that line number of that file :(

juampynr’s picture

The related menu item is not inheriting the controller. Has hook_menu() changed how inheritance works in Drupal 8?

dawehner’s picture

+++ b/core/modules/user/user.moduleundefined
@@ -963,15 +963,13 @@ function user_menu() {
+    'route_name' => 'user_accounts',

+++ b/core/modules/user/user.routing.ymlundefined
@@ -26,6 +26,13 @@ user_autocomplete_anonymous:
+user_accounts:

let's find a better route name like user_admin_index or something similar. user_accounts really reminds you on user/{} or admin/people

juampynr’s picture

Status: Needs work » Needs review
FileSize
927 bytes
15.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,283 pass(es), 2 fail(s), and 0 exception(s). View

Changed user_accounts route to user_admin_index as requested at #40.

Status: Needs review » Needs work

The last submitted patch, drupal-system-1987814-41.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
568 bytes
16.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,111 pass(es), 2 fail(s), and 0 exception(s). View

Fixed one of the failing tests.

Status: Needs review » Needs work

The last submitted patch, drupal-system-1987814-43.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
16.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,385 pass(es), 1 fail(s), and 0 exception(s). View

Discussed it with @dawehner. We do not need to check for callback inheritance since Controllers do not support it. I am removing the testing hook_menu() entry and the related test.

Status: Needs review » Needs work

The last submitted patch, drupal-system-1987814-45.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
568 bytes
17.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,064 pass(es). View

Adding again help module as a dependency of the failing test (that is why it fails).

kushrohra’s picture

FileSize
11.85 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Re Rolled patch no 47 and create a new patch to solve above problem

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, drupal-system-1987814-48.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review

#48: drupal-system-1987814-48.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal-system-1987814-48.patch, failed testing.

disasm’s picture

This review applies to #47 which doesn't need rerolled, it applies cleanly. The patch in #48 looks like it might have lost some code from #47 because the file size is smaller. Also, has an ambiguous statement about solving a problem, but no interdiff showing what was done, so my opinion is future work should be done based off of #47.

+++ b/core/modules/system/system.routing.ymlundefined
@@ -1,3 +1,75 @@
+system_admin:
+  pattern: '/admin'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_structure:
+  pattern: '/admin/structure'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:

Not going to select the entire file, but from previous conversions I've worked on, putting a blank line between each route makes it much more legible. This whole file should have this done to it.

Other than that one nitpick, I don't see anything wrong with this patch and it passes all tests. I think it's ready for RTBC!

disasm’s picture

This review applies to #47 which doesn't need rerolled, it applies cleanly. The patch in #48 looks like it might have lost some code from #47 because the file size is smaller. Also, has an ambiguous statement about solving a problem, but no interdiff showing what was done, so my opinion is future work should be done based off of #47.

+++ b/core/modules/system/system.routing.ymlundefined
@@ -1,3 +1,75 @@
+system_admin:
+  pattern: '/admin'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:
+    _permission: 'access administration pages'
+system_admin_structure:
+  pattern: '/admin/structure'
+  defaults:
+    _content: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+  requirements:

Not going to select the entire file, but from previous conversions I've worked on, putting a blank line between each route makes it much more legible. This whole file should have this done to it.

Other than that one nitpick, I don't see anything wrong with this patch and it passes all tests. I think it's ready for RTBC!

juampynr’s picture

Status: Needs work » Needs review
FileSize
2.91 KB
17.13 KB
FAILED: [[SimpleTest]]: [MySQL] 56,408 pass(es), 1 fail(s), and 0 exception(s). View

Added blank lines between each new route at system.routing.yml.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, drupal-system-1987814-54.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review

#54: drupal-system-1987814-54.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal-system-1987814-54.patch, failed testing.

disasm’s picture

juampynr’s picture

Weird, I tried that test locally and it passed, hence I hit re-test.

I will have a look again.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.47 KB
22.89 KB
FAILED: [[SimpleTest]]: [MySQL] 57,490 pass(es), 9 fail(s), and 816 exception(s). View

The fail is legitimate and reproducible. I'm not sure why.

Rerolled, with some additions.

Status: Needs review » Needs work

The last submitted patch, system-1987814-60.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
22.87 KB
FAILED: [[SimpleTest]]: [MySQL] 57,556 pass(es), 2 fail(s), and 0 exception(s). View

Whoops.

jibran’s picture

+++ b/core/modules/system/system.admin.incundefined
@@ -16,7 +16,8 @@
+  if ($system_manager->checkRequirements() && user_access('administer site configuration')) {

Drupal::request()->attributes->get('_account')->hasPermission('administer site configuration');

Status: Needs review » Needs work

The last submitted patch, system-1987814-62.patch, failed testing.

tim.plunkett’s picture

Assigned: juampynr » Unassigned
Status: Needs work » Needs review
FileSize
23.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,399 pass(es), 2 fail(s), and 0 exception(s). View
dawehner’s picture

  1. +++ b/core/modules/system/lib/Drupal/system/SystemManager.php
    @@ -124,4 +148,82 @@ public function getMaxSeverity(&$requirements) {
    +    $item = menu_get_item();
    +    if ($content = $this->getAdminBlock($item)) {
    

    Are we sure that menu_get_item will still work once the menu router is removed? We want to have the current request, so \Drupal::request() is the way to do it?

  2. +++ b/core/modules/system/lib/Drupal/system/SystemManager.php
    @@ -124,4 +148,82 @@ public function getMaxSeverity(&$requirements) {
    +   * @return array
    +   */
    

    Missing docs ...

  3. +++ b/core/modules/system/lib/Drupal/system/SystemManager.php
    @@ -124,4 +148,82 @@ public function getMaxSeverity(&$requirements) {
    +    if ($item['tab_root'] != $item['path']) {
    +      $item = menu_get_item($item['tab_root_href']);
    +    }
    +
    

    We also need to support the new router ...

  4. +++ b/core/modules/system/system.admin.inc
    @@ -16,7 +16,8 @@
    +  if ($system_manager->checkRequirements() && user_access('administer site configuration')) {
    

    user_access should be replaced \Drupal::currentUser()->hasPermisssion, but yeah this is kind of out of scope ...

tim.plunkett’s picture

FileSize
24.01 KB
FAILED: [[SimpleTest]]: [MySQL] 58,340 pass(es), 2 fail(s), and 0 exception(s). View
767 bytes

No idea about the new router stuff, but this is 100% moved code.
1, 3, and 4 are out of scope.

Fixed the docs (2).

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Well, at least we know now that menu_get_item() should simple not be used at all.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, system-1987814-67.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.6 KB
26.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,125 pass(es), 3 fail(s), and 0 exception(s). View

The authentication providers were only properly restricted on legacy paths, not routes. This fixes it.

In addition, I'm removing an obscure hook_menu test that relied on this entire set of paths being page callbacks. It's time to cut the cord.

Status: Needs review » Needs work

The last submitted patch, system-1987814-70.patch, failed testing.

jibran’s picture

Some minor issues.

  1. +++ b/core/modules/system/lib/Drupal/system/SystemManager.php
    @@ -124,4 +148,83 @@ public function getMaxSeverity(&$requirements) {
    +    $item = menu_get_item();
    ...
    +      $item = menu_get_item($item['tab_root_href']);
    

    @todo with the issue id will be nice here.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -16,7 +16,8 @@
    +  if ($system_manager->checkRequirements() && user_access('administer site configuration')) {
    

    hmmm user_access can be replaced.

  3. +++ b/core/modules/system/system.admin.inc
    @@ -44,7 +45,7 @@ function system_admin_config_page() {
    +        $block['content'] .= theme('admin_block_content', array('content' => $system_manager->getAdminBlock($item)));
    

    I think we can't call theme function directly now.

tim.plunkett’s picture

There is no issue for 72.1, until menu_get_item() itself is fixed. It's not specific to this usage at all.

72.2 and 72.3 are in #1987810: Convert system_admin_config_page() to a new style controller.

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
26.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es). View
915 bytes

It was a wrong assumption in AuthTest. I ran this change by klausi and linclark.

jibran’s picture

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs review » Postponed

Yep.

jibran’s picture

Status: Postponed » Active
Issue tags: +Needs reroll

And it is in.

tim.plunkett’s picture

Status: Active » Needs work

Working on it now.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
26.73 KB
PASSED: [[SimpleTest]]: [MySQL] 58,422 pass(es). View
2.79 KB

Okay, there was one remaining conversion, and the actual function itself. Not too bad!

jibran’s picture

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php
@@ -51,7 +51,13 @@ public function enhance(array $defaults, Request $request) {
-        $request->attributes->set('_account', drupal_anonymous_user());
+        $anonymous_user = drupal_anonymous_user();
+        $request->attributes->set('_account', $anonymous_user);
+
+        // The global $user object is included for backward compatibility only
+        // and should be considered deprecated.
+        // @todo Remove this line once global $user is no longer used.
+        $GLOBALS['user'] = $anonymous_user;

I don't know much about this change but i think we can replace it with current_user service. Not sure. Other then this it is RTBC for me.

tim.plunkett’s picture

@jibran That is inside the subsystem that creates the current_user. Basically, current_user is broken, and I'm fixing it.
If we call it there, we'd get an infinite loop.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Ok then. Let's get this in.

tim.plunkett’s picture

#80: system-1987814-80.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Other than the hunk jibran pointed out in #81, answered in #82, this looks good to me. It's also a bit weird we're doing t() instead of $this->t() but this currently doesn't extend from any base classes, so that's fine.

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.