Add permissionByModule to user.permissions service

So there seems to be indeed a usecase out there, but yeah I am not sure whether this is semantically a bug but rather a task/feature?

1. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -250,4 +250,17 @@ protected function systemRebuildModuleData() {
      }
    
   +  public function permissionsByModule($module_name)
   +  {
   ...
   +    if( $this->moduleProvidesPermissions($module_name)) {
   ...
   +    };
   ...
   +  }
   
   +++ b/core/modules/user/src/PermissionHandlerInterface.php
   @@ -54,5 +54,12 @@ public function getPermissions();
   +   *
   +   * @param $module_name
   +   * @return array
   +   */
   +  public function permissionsByModule($module_name);
    }
    
   

   Let's fix the codestyle and documentation here.

2. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
   @@ -391,6 +391,12 @@ public function testPermissionsYamlStaticAndCallback() {
   +    foreach ($modules as $module) {
   +      $perms = $this->permissionHandler->permissionsByModule($module);
   +      $this->assertNotEmpty($perms, "Permissions for module '$module' found.");
   +    }
   

   As you expand the test coverage let's put @covers at the top.

There are a couple of problems:

• Your implementation currently calls getPermissions twice (yeah implementation detail, but I think we can avoid that)
• The tests gets more ugly and ugly over time, let's prevent that directly

There we go.

1. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -98,16 +98,7 @@ public function getPermissions() {
   +    return (bool) $this->permissionsByModule($module_name);
   

   Do you think !empty() would be more self descriptive here?

2. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -250,4 +241,21 @@ protected function systemRebuildModuleData() {
   +  public function permissionsByModule($module_name) {
   ...
   +    foreach ($permissions as $permission_name => $permission) {
   +      if ($permission['provider'] == $module_name) {
   

   We have a mixture between module and provider here. Do you think we should convert everything to provider? That seems to be the direction we are heading in?

3. +++ b/core/modules/user/src/PermissionHandlerInterface.php
   @@ -54,5 +54,16 @@ public function getPermissions();
   +   * Provide list of permissions for given module.
   

   Provides

4. +++ b/core/modules/user/src/PermissionHandlerInterface.php
   @@ -54,5 +54,16 @@ public function getPermissions();
   +   *   An array of permissions, see getPermissions() for possible values.
   

   Should we just use an @see there?

5. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
   @@ -207,6 +163,27 @@ public function testBuildPermissionsYaml() {
   +    $this->assertEquals(['access_module_a' => [
   +      'provider' => 'module_a', 'title' => 'single_description', 'description' => '',
   +    ]], $this->permissionHandler->permissionsByModule('module_a'));
   

   I don't really mind, but I would rather just see the expected in a separate variable so the assertEquals is easier to read.

6. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
   @@ -410,6 +387,61 @@ protected function assertPermissions(array $actual_permissions) {
   +   * Setups a permission handler with static permissions.
   

   Sets up*

We have a mixture between module and provider here. Do you think we should convert everything to provider? That seems to be the direction we are heading in?

Well yeah but should we really rename also the moduleProvidesPermissions? I think we should do a better naming in #2338469: Add support for a core.permissions.yml as well as core.routing.yml

Should we just use an @see there?

I would vote to have both, because, well the inline reference helps to understand it.

I don't really mind, but I would rather just see the expected in a separate variable so the assertEquals is easier to read.

Sure, let's do it.

+++ b/core/modules/user/src/PermissionHandler.php
@@ -250,4 +242,21 @@ protected function systemRebuildModuleData() {
+  public function permissionsByModule($module_name) {

This is more permissions FOR module? By would imply I get a list grouped by module..

+++ b/core/modules/user/src/PermissionHandlerInterface.php
@@ -54,5 +54,18 @@ public function getPermissions();
+   * @return array

@return array is never ever good documentation. Always specify what exactly is in the array. Is it an array of arrays? Then use @return array[].

Patch no longer applies, and the drush issue mentioned in the summary has been fixed by other means, so I'm not sure if this is still applicable.

Working on it.

Patch was not applicable. Just rerolling for now.

Looks pretty fine

Re-submitting the prior patch for the bots to review.

Part of the new review initiative this one came up.

Wonder after 7 years if its still a valid bug report.

If so can we also get an updated issue summary.

Since there has been no activity going to close for now.

If still a valid bug please reopen updating issue summary.

Thanks all!