Fix broken localization for permission names

Subscribe

user_localize_permissions really looks like a hack. I suggest we standardize permissions in a clean array instead:

$perms = array(
  'translate content' => array(
     '#title' => t('Translate content'),
     '#description' => t('Translate website content.'),
  )
);

*Kitten killer alert:* And while we are at it, we could even make 'translate content' a define() to improve developer experience (autocomplete...) and have a less bug-prone user_access().

Don't worry, Gábor, I'm not so keen of killing too much kittens today.

As I see it, the array structure I suggest in #2 is not much more complex as what we already have. It's the logical extension to the permission description patch. As an added bonus, having properly capitalized permissions will make the permission page look nicer.

Having proper keys (with the hash or not) will also allow extensibility down the road, and is something that could really help other patches to go forward, like #182023: Add a third default role to core for handling Administrative duties, #310494: Allow grouping of permissions and #248598: Label permissions which are warned about in the user interface.

I definitely prefer the way described in #2 above the original proposed method, as this is what we see in menu's, form's, etc, and creates possibilities for enhancing the permissions system. Using this structure consistently is good from a module development PoV. Using hashes is part of that consistency.

Seeing as other improvements for the permissions system depends on this, we might want to put this in first gear and get it in quickly.

Already on it :)

Ok, here it is.

This patch changes all hook_perm() declerations to the form

  $perms = array(
    'permission internal name' => array(
      'title' => t('Permission title'),
      'description' => t('The permissions does this and that.'),
    ),
}

and alters the permissions settings form to read these values. This method requires no changes to the database as the internal names are kept default.

You may notice I've dropped the hashes, because contrary to my previous statement only Froms API uses it, al other systems (links, menus, blocks, nodes, etc) use hashless keys. Besides the capitalization on the permissions page and being able to tranlate the permissions, nothing changed visually. On a final note, I ran all tests, and got no errors.

Happy reviewing!

Applies smoothly, works smoothly. Please commit this! :-)

Very nice patch. A simple solution to a longstanding issue.

In this block of code the functionality changes:

-      foreach ($permissions as $perm => $description) {
-        // Account for permissions lacking a description.
-        if (is_int($perm)) {
-          $perm = $description;
-          $description = NULL;
+      foreach ($permissions as $perm => $perm_item) {
+        if (is_int($perm) || !array($perm_item)) {
+          // Pre Drupal 7.x code, should not be there.
+          continue;

Prior to the patch, old-style permissions were accepted. After the patch, old-style permissions are tested for but not supported.

I see the point of this change and agree we should be dropping backward compatibility, but I think it's better just to drop the test. We don't usually test for broken updates. So I suggest dropping these four lines from the patch:

       if (is_int($perm) || !array($perm_item)) {
         // Pre Drupal 7.x code, should not be there.
         continue;
      }

Otherwise very clean, ready to go.

Previous patch now applies with an offset. Rerolling for head and attaching another version without this test. Let webchick or Dries decide which one to commit. Lets get this in! :-)

Subscribe! Somehow this failed to be on my radar. It's been a crazy week. :(

Still applies to HEAD (with an offset)

Subscribing! Making permissions awesomer is awesome.

To paraphrase catch on some patch, This is a lovely, lovely, patch filled with loveliness.

Small change: Can we please remove this hunk?

+        if (is_int($perm) || !array($perm_item)) {
+          // Pre Drupal 7.x code, should not be there.
+          continue;
         }

I realize I am partially to blame for this code, since it snuck in during the permission descriptions, but IMO this was a bad call, and we shouldn't coddle broken code. If huge, ugly errors are spewed everywhere when you're using a module that doesn't conform to hook_perm()'s rules, then GOOD. :) It needs to be fixed.

Also, the one that says "with test" doesn't actually seem to have a test. Please fix?

OK, this is a misunderstanding. You should look for http://drupal.org/files/issues/permission_localization_without_test.patch which means "without pre 7.x test" - the code you requested :)

Since it was a misunderstanding, the status can be as before #18.

LOL oops. :) I misread.

Committed! Yay!!

Docs please. ;D

See also this pending issue : #281621: Update hook_perm doc for Drupal 7