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

CommentFileSizeAuthor
#80 interdiff.txt2.79 KBtim.plunkett
#80 system-1987814-80.patch26.73 KBtim.plunkett
#75 interdiff.txt915 bytestim.plunkett
#75 system-1987814-75.patch26.95 KBtim.plunkett
#70 system-1987814-70.patch26.06 KBtim.plunkett
#70 interdiff.txt2.6 KBtim.plunkett
#67 interdiff.txt767 bytestim.plunkett
#67 system-1987814-67.patch24.01 KBtim.plunkett
#65 system-1987814-65.patch23.89 KBtim.plunkett
#62 system-1987814-62.patch22.87 KBtim.plunkett
#62 interdiff.txt1.24 KBtim.plunkett
#60 system-1987814-60.patch22.89 KBtim.plunkett
#60 interdiff.txt8.47 KBtim.plunkett
#54 drupal-system-1987814-54.patch17.13 KBjuampynr
#54 interdiff.txt2.91 KBjuampynr
#48 drupal-system-1987814-48.patch11.85 KBkushrohra
#47 drupal-system-1987814-47.patch17.11 KBjuampynr
#47 interdiff.txt568 bytesjuampynr
#45 drupal-system-1987814-45.patch16.69 KBjuampynr
#45 interdiff.txt2.3 KBjuampynr
#43 drupal-system-1987814-43.patch16.15 KBjuampynr
#43 interdiff.txt568 bytesjuampynr
#41 drupal-system-1987814-41.patch15.74 KBjuampynr
#41 interdiff.txt927 bytesjuampynr
#36 drupal-system-1987814-36.patch15.59 KBjuampynr
#36 interdiff.txt1.16 KBjuampynr
#33 drupal-system-1987814-33.patch15.5 KBjuampynr
#32 drupal-system-1987814-32.patch15.49 KBjuampynr
#30 drupal-system-1987814-30.patch15.5 KBjuampynr
#28 drupal-system-1987814-28.patch16.47 KBjuampynr
#28 interdiff.txt532 bytesjuampynr
#26 drupal-system-1987814-26.patch16.47 KBjuampynr
#23 drupal-system-1987814-23.patch17 KBjuampynr
#23 interdiff.txt1.11 KBjuampynr
#22 drupal-system-7391414-21.patch16.97 KBjuampynr
#21 drupal-system-7391414-20.patch0 bytesjuampynr
#16 drupal-system-7391414-16.patch16.94 KBjuampynr
#16 interdiff.txt3.37 KBjuampynr
#14 drupal-system-7391414-14.patch15.55 KBjuampynr
#10 drupal-system-7391414-10.patch15.48 KBjuampynr
#10 interdiff.txt5.24 KBjuampynr
#5 drupal-system-7391414-5.patch11.04 KBlliss
#2 drupal-system-7391414-2.patch9.25 KBlliss
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lliss’s picture

Assigned: Unassigned » lliss
lliss’s picture

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

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

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

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

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

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

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

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

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

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

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

Rerolling.

juampynr’s picture

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

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

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

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

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

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

kushrohra’s picture

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

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

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

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

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