link directly to the right section of permissions from the admin "by modules" page

The patch works well and I like it. It makes that admin page more user-friendly. After I apply the patch I wonder why that feature is not the part of the system by the begining.

Awesome.

Two things:

1. The attribute should be id="" and not name="" for XHTML compliance.

2. You can't do simply id="module" because there are already classes declared for #menu and such which will screw up the way the table looks. So something like id="perm-module" or something will work fine.

Let's write 'permission' or 'permissions' rather than 'perm'.

Do we need the 'perm-' to begin with? Why not just the module name?

Dries: Because some of these module names (like menu, iirc) are already IDs in CSS (#menu) that have their own formatting. So if we just use id="menu", the table gets distorted because a few oddball modules get right-aligned or what have you.

At least this is what I remember from when I worked on merlin's big administration patch where this functionality first showed up.

This patch just syncs and adds a space to the line in system.module to conform with coding standards. Great work Greg. Works as specified and does improve usability. I'm going to RTBC with the assumption that Dries' concerns have been addressed.

Do you think <a id="permission-@module">@module module</a> should be translatable? If someone does, it could break the desired functionality. This is not a language-specific string and it shouldnt be wrapped in t() IMO.

This patch adds the "id=..." stuff via #prefix/#suffix, so no translator will see it. Only the translatable part is still wrapped in t().

Might not hurt to wrap $module in a check_plain.

Not sure this is really needed, but added check_plain.

If you wrap one in check_plain, do it with the other as well.

+1 on this patch. Looks good to go.

Corrected a whitespace issue and added a comma to the last array element.

AFAIK, these check_plain()s are not needed.

You can use #10. Its the same patch, but without the check_plain()s.

profix: #10 actually fails ... needs to be re-rolled, I'm afraid.

re-rolled

Removed the whitespace again.

@kkaefer: Thanks and Sorry for 'again' :)

Ah, the capitalization patch. Made the "Configure" uppercase just in the original.

+1 to the patch. Tested on HEAD, did not apply cleanly though. Just one question, would it be slightly misleading if the module names in the access control page inherits link styles? Like having link colors and having on hover styles. Apart from that, looks RTBC to me :)

Lets set this RTBC and see what happens ... we all did enough re-rolls, I think.

Please use an id on an existing element instead of throwing in an extra a tag.

Aren't these modules, not permissions? I think the id should be module-name.

Ok, let's try this.

webchick's patch can't work because there are not yet any existing IDs for each module on the permissions page.

Huh? It does work..? I add the ID to the table cell.

Tested in Firefox and Safari.

Doesn't seem to be working in IE. Most likely this is IE being stupid about implementing something. Either way, it look like we have to work around it and may need to go with the <a ... anchor instead of embedding it in an existing tag to support IE.

This works for me in IE6.0.2 on Windows XP (and all the other browsers). Having any element's ID used as an anchor is a standard HTML 4 trait so not sure what's going on with your browser.

Not sure what the issue is here, but this works fine for me. +1

After a short discussion with webchick, I'm fine with this patch.

Works also in Safari.

I don't know either. It was IE 6.0.2 but if everyone else is working then I'm not going to argue. Its the first time I've used IE in a couple years so its just as likely I was overlooking something. The id thing seems pretty standard so lets get this thing committed.

woops, that wasn't my fault. I agree, RTBC.

Committed to HEAD.