Move system_get_module_admin_tasks() into a service and deprecate it

Mixed feelings about trait
it does 3 things
- transforms menu links a-ka http://cgit.drupalcode.org/drupal/tree/core/modules/system/src/Plugin/Bl...
- check access to links
- build links

I bet it fits both controllers and blocks

Maybe makes sense to move it to render element?

$build['menu_tree'] =[
  '#type' => 'menu_tree',
  '#extension' => 'help',
  '#transform' => [], // could define tree transformations additionally to defaults or override
];

1. +++ b/core/modules/help/src/Controller/HelpController.php
   @@ -136,7 +139,7 @@ public function helpPage($name) {
          // Only print list of administration pages if the module in question has
          // any such pages associated with it.
   -      $admin_tasks = system_get_module_admin_tasks($name, system_get_info('module', $name));
   +      $admin_tasks = static::getModuleAdminTasks($name);
          if (!empty($admin_tasks)) {
            $links = [];
            foreach ($admin_tasks as $task) {
   
   +++ b/core/modules/system/src/Controller/AdminController.php
   @@ -27,7 +30,7 @@ public function index() {
        foreach ($module_info as $module => $info) {
          // Only display a section if there are any available tasks.
   -      if ($admin_tasks = system_get_module_admin_tasks($module, $info->info)) {
   +      if ($admin_tasks = static::getModuleAdminTasks($module)) {
            // Sort links by title.
            uasort($admin_tasks, ['\Drupal\Component\Utility\SortArray', 'sortByTitleElement']);
   

   similar controllers to render links finally

2. +++ b/core/modules/system/src/AdminHelperTrait.php
   @@ -0,0 +1,93 @@
   +trait AdminHelperTrait {
   ...
   +      $parameters = new MenuTreeParameters();
   +      $parameters->setRoot('system.admin')->excludeRoot()->onlyEnabledLinks();
   +      static::$menuTree = $menu_link_tree->load('system.admin', $parameters);
   +      $manipulators = [
   +        ['callable' => 'menu.default_tree_manipulators:checkAccess'],
   +        ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
   +        ['callable' => 'menu.default_tree_manipulators:flatten'],
   +      ];
   +      static::$menuTree = $menu_link_tree->transform(static::$menuTree, $manipulators);
   

   This is tree transformation, which waiting for alter in system menu block, issue #2854013: Allow SystemMenuBlock tree manipulators to be altered

3. +++ b/core/modules/system/src/AdminHelperTrait.php
   @@ -0,0 +1,93 @@
   +    foreach (static::$menuTree as $element) {
   +      if (!$element->access->isAllowed()) {
   ...
   +      if ($link->getProvider() !== $module) {
   ...
   +      $admin_tasks[] = [
   +        'title' => $link->getTitle(),
   +        'description' => $link->getDescription(),
   +        'url' => $link->getUrlObject(),
   

   actually render element filtered by module(extension?)

4. +++ b/core/modules/system/src/AdminHelperTrait.php
   @@ -0,0 +1,93 @@
   +    /** @var \Drupal\user\PermissionHandlerInterface $permission_handler */
   +    $permission_handler = \Drupal::service('user.permissions');
   ...
   +    if ($permission_handler->moduleProvidesPermissions($module)) {
   +      /** @var \Drupal\Core\Access\AccessManagerInterface $access_manager */
   +      $access_manager = \Drupal::service('access_manager');
   +      if ($access_manager->checkNamedRoute('user.admin_permissions', [], \Drupal::currentUser())) {
   +        $url = new Url('user.admin_permissions');
   +        $url->setOption('fragment', 'module-' . $module);
   +        $admin_tasks["user.admin_permissions.$module"] = [
   +          'title' => t('Configure @module permissions', ['@module' => \Drupal::moduleHandler()->getName($module)]),
   +          'description' => '',
   +          'url' => $url,
   

   user namespace?! sounds should live in core

Re-rolling for 9.1.x-dev.

Fixed deprecation messages (looks it needs better wording - usage will throw exception?)
And fixed tests + clean-up

Looking through changes I still not sure we can make it trait without injecting so many services

1. +++ b/core/modules/system/src/AdminHelperTrait.php
   @@ -0,0 +1,93 @@
   +   *
   +   * @return array
   +   *   An array of task links.
   +   */
   +  public static function getModuleAdminTasks($module) {
   +    if (!isset(static::$menuTree)) {
   +      $menu_link_tree = \Drupal::menuTree();
   +      $parameters = new MenuTreeParameters();
   +      $parameters->setRoot('system.admin')->excludeRoot()->onlyEnabledLinks();
   +      static::$menuTree = $menu_link_tree->load('system.admin', $parameters);
   +      $manipulators = [
   

   Do we really need to make it a trait if it's static? Or it could just be be a separate, standalone service. Then we can properly inject things and call it from different places. And it can be a non-static property and installing modules will automatically invalidate it.

2. +++ b/core/modules/system/tests/src/Unit/SystemAdminModuleTasksTest.php
   @@ -0,0 +1,52 @@
   +   * @expectedDeprecation Using drupal_static_reset() with 'system_get_module_admin_tasks' is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\system\AdminHelperTrait::resetModuleAdminTasksCache() instead. See https://www.drupal.org/node/3038972
   +   * @see drupal_static_reset()
   +   */
   +  public function testSystemGetModuleAdminTasksResetCacheDeprecation() {
   +    require_once $this->root . '/core/includes/bootstrap.inc';
   +    drupal_static_reset('system_get_module_admin_tasks');
   +  }
   

   I think we don't do require_once of include files in unit tests, so this probably needs to be a kernel test. You can mock stuff there too if necessary.

and deprecation message update of course. Feels like we should have way to automate that ;)

There's MR

Fine with whatever is the most practical solution. user and system are both required modules, they're full of interdependencies, that's really nothing new and only an issue during initial installation bootstrapping and tests.

Wouldn't it be sufficient use an optionally injected dependency for those?

MR needs rebase to 9.5.x

It needs re-roll and suggestion was to move to trait #2466933-46: Change $info array argument to system_get_module_admin_tasks() to $name

Reroll of MR !928. Used a patch as I don't have permission to rebase it on 11.x, and creating a new MR and losing the code comments is probably more confusing for reviewers.

I replaced usage of ModuleHandler::getName() as there is an issue to deprecate that in favour of ModuleExtensionList::getName().

The existing deprecation for passing a string instead of an array to system_get_module_admin_tasks() is no longer needed as we are deprecating the whole function. Removed this and the corresponding deprecation test.

Forgot to replace a usage of system_get_module_admin_tasks() and removed an unrelated change.

Remaining tasks:

• Add tests for the new services
• Consider renaming to \Drupal\system\ModuleAdminLinkHelper and \Drupal\user\ModulePermissionsLinkHelper
• Consider using \Drupal\Core\Cache\MemoryCache\MemoryCache to keep the ability to flush the cache externally

Test is failing because link change from /admin/people/permissions/module/locale to /admin/people/permissions#module-locale. Seems like a valid fail?

Looks like this was older than the module specific permissions page?? https://www.drupal.org/node/3223123

Fix missing $name arg to getModulePermissionsLink()

• Renamed to \Drupal\system\ModuleAdminLinkHelper and \Drupal\user\ModulePermissionsLinkHelper
• Wrote kernel tests for the new services

Remaining task:

• Use \Drupal\Core\Cache\MemoryCache\MemoryCache to keep the ability to flush the cache externally

Fixes:

• Use \Drupal\Core\Cache\MemoryCache\MemoryCache to keep the ability to flush the cache externally

Updated the IS and CR

+++ b/core/modules/user/src/ModulePermissionsLinkHelper.php
@@ -0,0 +1,61 @@
+      if ($this->accessManager->checkNamedRoute('user.admin_permissions.module', ['modules' => $module])) {
+        $url = new Url('user.admin_permissions.module', ['modules' => $module]);

this is the only reason to keep it separate module/service (usage of user's module route)

But I think it could be implemented in system module's service just checking the route presence

One more typo

+++ b/core/modules/help/tests/src/Functional/HelpTest.php
@@ -137,8 +137,11 @@ protected function verifyHelp($response = 200) {
-        $admin_tasks = system_get_module_admin_tasks($module, $info['name']);
+        $admin_tasks = \Drupal::service('system.module_admin_links_helper')->getModuleAdminLinks($module);
+        if ($module_permissions_link = \Drupal::service('user.module_permissions_link_helper')->getModulePermissionsLink($module, $name)) {

it should use $info['name'] instead of $name

#42

But I think it could be implemented in system module's service just checking the route presence

Ah ok. Not sure what service provides that?

Re: #43 Fixed.

Re: #42 We are using \Drupal\user\PermissionHandlerInterface::moduleProvidesPermissions() here. I'm reluctant to add another circular dependency between core, system, and user modules.

Random fail

Reviewing #45

Deprecation appears to be up to date
Good test coverage
Reviewed the change record, the example was very useful and imagine will help those update later.

LGTM.

Fix missing service aliases.

I think it safe to put this back to RTBC

random failure

+++ b/core/modules/system/src/Controller/AdminController.php
@@ -18,14 +20,42 @@ class AdminController extends ControllerBase {
+
+  /**
+   * The module permissions link service.
+   *
+   * @var \Drupal\user\ModulePermissionsLinkHelper
+   */
+  protected ModulePermissionsLinkHelper $modulePermissionsLinks;
+

This shows up a general issue that we have dependencies on user module in system module, and dependencies on system module in.. a lot of things but probably also including user.

It might make sense at some point to move the admin ui building out of system module into a system_ui module that's just responsible for building admin pages. That module could then depend on system and user without issue.

There's no workaround though that I can think of except for making that third module, so committed/pushed to 11.x, thanks!

Published change record.