Despite module system name differing, if two modules have the same name only one of their hook_permission() implementations is shown on admin/people/permissions. While system name of course has to be different, can't module names be the same? Words like "System" are general enough to get reused.

For example, my module (my_module.info) with name = System and an implementation of hook_permission() will not be exposed.

I'm working on a test case that'll fail and then a patch to fix but you can apply the following patch to test for the time being. It's the start of a hidden module with name = User and hook_permission() implementation that doesn't show.

How should the permissions page distinguish between modules with the same name, if at all?

CommentFileSizeAuthor
#10 740258-module-name-hook-permission-10.patch3.55 KBcoltrane
#8 740258-module-name-hook-permission-8.patch3.58 KBcoltrane
#6 740258-module-name-hook-permission-6.patch2.38 KBcoltrane
#5 user_admin_permissions.patch883 bytesmr.baileys
#2 740258-module-name-hook-permission.patch624 bytescoltrane
#1 740258-module-name-hook-permission.test2.67 KBcoltrane
module-name-hook-permission.test1.08 KBcoltrane
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented
FileSize
740258-module-name-hook-permission.test2.67 KB

Full test case for this issue, will fail till I find the offending code.

Patch adds new test case and mock module user_test.

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented
Status: Active » Needs review
FileSize
740258-module-name-hook-permission.patch624 bytes

Simple as not keying by module name and instead using module system name. User's role permissions test case passes with this patch but I'll leave the rest of testing up to the bot.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Needs review » Needs work

Current patch basically rolls back the changes from #647250: Permissions page not in alphabetical order. Maybe not that simple :)

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented

The ksort() is still in place, but it's possible that they could not be in alphabetical order, good catch. Could possibly use module name as value and use regular sort.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Needs work » Needs review
FileSize
user_admin_permissions.patch883 bytes

Sounds good, something like this?

Tested with a custom module with name = System, both "system system" and "custom system" permissions are displayed, and the list is alphabetical.

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented
FileSize
740258-module-name-hook-permission-6.patch2.38 KB

Looks good, mr.baileys, thanks.

Here's #5 and the test from #1 as a combined patch.

Status: Needs review » Needs work

The last submitted patch, 740258-module-name-hook-permission-6.patch, failed testing.

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented
Status: Needs work » Needs review
FileSize
740258-module-name-hook-permission-8.patch3.58 KB

Oops, #6 was missing added mock module.

retester2010’s picture

retester2010 CreditAttribution: retester2010 commented
Status: Needs review » Needs work 
+++ modules/simpletest/tests/user_test.module	14 Mar 2010 23:33:36 -0000
@@ -0,0 +1,19 @@
+}
\ No newline at end of file
Index: modules/user/user.admin.inc

there should be a new line

+++ modules/user/user.test	14 Mar 2010 23:33:37 -0000
@@ -1028,6 +1028,32 @@ class UserPermissionsTestCase extends Dr
+  }
+  ¶
+  function setUp() {
+    parent::setUp('user_test');
+    $this->admin_user = $this->drupalCreateUser(array('administer permissions', 'access user profiles', 'administer site configuration', 'administer modules', 'administer users'));
+  }
+  ¶

trailing white spaces

40 critical left. Go review some!

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
740258-module-name-hook-permission-10.patch3.55 KB

Marking "major" because this issue affects permissions and as maintainers update their projects this could be a blocker.

Stevel’s picture

Stevel CreditAttribution: Stevel commented
Status: Needs review » Reviewed & tested by the community

Looks good, it's never a good idea to use the display name as the key instead of the machine name.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

#10: 740258-module-name-hook-permission-10.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed

Mmmm. I really don't know about this. It sounds like you have to be doing completely unholy things to even run across this bug. Definitely not major. I talked to Ben about it here at BADCamp, though, and he pointed out that he might want to make his own custom toolbar_replacement.module with the human readable name "Toolbar." I think this causes a UX nightmare, personally. ;P

However, I agree that keying permissions by a human readable name is pretty silly, so this seems like a bug worth fixing either way.

Committed #10 to HEAD. Thanks!

Status: Fixed » Closed (fixed)

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