[Regression] Module permission links missing from module list page

1. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -270,7 +270,8 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +    $permission_handler = \Drupal::service('user.permissions');
   

   Let's inject the user.permissions service so we can use $this->permissionHandler instead.

2. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -270,7 +270,8 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +    if ($module->status && \Drupal::currentUser()->hasPermission('administer permissions') && $permission_handler->moduleProvidesPermissions($module->getName())) {
   

   We already have $this->currentUser, so no need for \Drupal::currentUser() here. (That has nothing to do with your patch, but it's an easy thing to fix here.)

Also, since testbot didn't catch it, that means we probably need a test.

Sure enough, \Drupal\system\Tests\Form\ModulesListFormWebTest checks for a configure link, but not a permissions link.

Here is a tests-only patch and a full patch.

For the sake of completeness, we should also test that the "Help" link is shown. Some day we'll want to get rid of hook_help(), too.

#11 is out-of-scope. Add another issue for that.

Added #2516690: Missing test for "Help" links per module on "Extend" admin page for testing the Help link.

--- /dev/null
+++ b/core/modules/system/tests/modules/system_test/system_test.permissions.yml
@@ -0,0 +1,2 @@
+system test:
+  title: 'Administer system test'

This is strange -- there's a previously existing system_test.permissions.yml file, but it's in the "src" directory. Does that even work? Git tells me it comes from #2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag.

That is strange. Permissions files don't go there.

It may be that *both* files are being read. Can you see if PageCacheTest fails without the other permisisons.yml file present? I think it will.

PageCacheTest passes even without any system_test.permissions.yml.

Technically, permissions.yml isn't required for testing; a permission check is just looking for a string, and that string is programmatically applied to the test user directly in the test method:

    user_role_grant_permissions(RoleInterface::ANONYMOUS_ID, ['pet llamas']);

So I'd venture to say that the YAML file in "src" isn't doing anything at all.

So I'd venture to say that the YAML file in "src" isn't doing anything at all.

I don't want to just remove the file in this issue because #2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag intended it to be there, right or wrong. I think this calls for another novice issue and #9 is still open for review.

#9 is well-formatted, narrowly scoped, and has verified tests. Looks good to me.

Added novice task #2517114: Remove needless, wrongly-placed system_test.permissions.yml file, postponed on this issue.

Nice catch!

Committed and pushed to 8.0.x. Thanks! Go TC Drupal Go! :D

Thanks!
Added new line at the end of system_test.permissions.yml

Another follow-up issue: #2521758: Module anchors on Permissions pages do not get you where you would expect

changing as per #26

@naveenvalecha It has already been committed. Open a follow-up issue to fix any missed problems.

Oh a committer knows about it. Ignore #29.

@naveenvalecha please open a follow-up issue - doing follow-ups on issue pollutes issue credit history.

And once the follow-up is created and linked can this issue by set to fixed.

Thanks! @alexpott! Fine to create the followup. Added followup https://www.drupal.org/node/2521774