Move system_get_module_admin_tasks() into a service and deprecate it

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

The last submitted patch, 29: 3038971-29.patch, failed testing. View results →

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 →

The last submitted patch, 28: 3038971-28.patch, failed testing. View results →

The last submitted patch, 33: 3038971-33.patch, failed testing. View results →

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.

The last submitted patch, 45: 3038971-45.patch, failed testing. View results →

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.

The last submitted patch, 45: 3038971-45.patch, failed testing. View results →

Fix missing service aliases.

I think it safe to put this back to RTBC

The last submitted patch, 50: 3038971-50.patch, failed testing. View results →

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!

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

Published change record.