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 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 56,408 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#54 interdiff.txt2.91 KBjuampy
#48 drupal-system-1987814-48.patch11.85 KBkushrohra
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#47 drupal-system-1987814-47.patch17.11 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 58,064 pass(es).
[ View ]
#47 interdiff.txt568 bytesjuampy
#45 drupal-system-1987814-45.patch16.69 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,385 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#45 interdiff.txt2.3 KBjuampy
#43 drupal-system-1987814-43.patch16.15 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,111 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#43 interdiff.txt568 bytesjuampy
#41 drupal-system-1987814-41.patch15.74 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,283 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#41 interdiff.txt927 bytesjuampy
#36 drupal-system-1987814-36.patch15.59 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,068 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#36 interdiff.txt1.16 KBjuampy
#33 drupal-system-1987814-33.patch15.5 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,626 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#32 drupal-system-1987814-32.patch15.49 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,138 pass(es), 130 fail(s), and 36 exception(s).
[ View ]
#30 drupal-system-1987814-30.patch15.5 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 55,911 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#28 drupal-system-1987814-28.patch16.47 KBjuampy
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 bytesjuampy
#26 drupal-system-1987814-26.patch16.47 KBjuampy
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 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 54,972 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#23 interdiff.txt1.11 KBjuampy
#22 drupal-system-7391414-21.patch16.97 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#21 drupal-system-7391414-20.patch0 bytesjuampy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#16 drupal-system-7391414-16.patch16.94 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 57,105 pass(es), 110 fail(s), and 44 exception(s).
[ View ]
#16 interdiff.txt3.37 KBjuampy
#14 drupal-system-7391414-14.patch15.55 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 56,057 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#10 drupal-system-7391414-10.patch15.48 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 56,063 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#10 interdiff.txt5.24 KBjuampy
#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

Assigned:Unassigned» lliss

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

First go at it. Patch attached.

Status:Active» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new11.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.

lliss, are you still working on this?

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.

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.

StatusFileSize
new5.24 KB
new15.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.

<?php
// 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);
  }
?>

Status:Needs work» Needs review

Changing status.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new15.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.

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.

Status:Needs work» Needs review
StatusFileSize
new3.37 KB
new16.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.

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.

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.

StatusFileSize
new0 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.

StatusFileSize
new16.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.

Status:Needs work» Needs review
StatusFileSize
new1.11 KB
new17 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.

Assigned:lliss» juampy

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.

Status:Needs work» Needs review
StatusFileSize
new16.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.

Status:Needs work» Needs review
StatusFileSize
new532 bytes
new16.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.

Status:Needs work» Needs review
StatusFileSize
new15.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.

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

Rerolling.

StatusFileSize
new15.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.

+++ 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()

Status:Needs work» Needs review
StatusFileSize
new1.16 KB
new15.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.

2 fails:

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

<?php
 
// 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 :(

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

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

Status:Needs work» Needs review
StatusFileSize
new927 bytes
new15.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.

Status:Needs work» Needs review
StatusFileSize
new568 bytes
new16.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.

Status:Needs work» Needs review
StatusFileSize
new2.3 KB
new16.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.

Status:Needs work» Needs review
StatusFileSize
new568 bytes
new17.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).

StatusFileSize
new11.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.

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.

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!

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!

Status:Needs work» Needs review
StatusFileSize
new2.91 KB
new17.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.

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.

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

I will have a look again.

Status:Needs work» Needs review
StatusFileSize
new8.47 KB
new22.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.

Status:Needs work» Needs review
StatusFileSize
new1.24 KB
new22.87 KB
FAILED: [[SimpleTest]]: [MySQL] 57,556 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Whoops.

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

Assigned:juampy» Unassigned
Status:Needs work» Needs review
StatusFileSize
new23.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,399 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new24.01 KB
FAILED: [[SimpleTest]]: [MySQL] 58,340 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new767 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).

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.

Status:Needs work» Needs review
StatusFileSize
new2.6 KB
new26.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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new26.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
new915 bytes

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

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

Yep.

Status:Postponed» Active
Issue tags:+Needs reroll

And it is in.

Status:Active» Needs work

Working on it now.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new26.73 KB
PASSED: [[SimpleTest]]: [MySQL] 58,422 pass(es).
[ View ]
new2.79 KB

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

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

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

Status:Needs review» Reviewed & tested by the community

Ok then. Let's get this in.

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

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!

Assigned:tim.plunkett» Unassigned

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

Issue summary:View changes

Updated issue summary.