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

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.

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

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

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

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

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

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.

juampynr’s picture

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.

juampynr’s picture

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.

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

juampynr’s picture

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.

juampynr’s picture

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.

juampynr’s picture

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.

juampynr’s picture

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.

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

vijaycs85’s picture

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

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

juampynr’s picture

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.

juampynr’s picture

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.

juampynr’s picture

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

kushrohra’s picture

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.

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

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

tim.plunkett’s picture

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.

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

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

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

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

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

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.