Even in Drupal 6 and before, the translation template extractor needed to have some hard-wired exceptions for some well-known hook_perm() implementations. In cases like node_perm(), which generated dynamic permission names, it was not possible to collect the name of the permissions for translation without running the extractor on a live site, and even then, permission names could change any time with renaming, adding or removing content types. The same problem applies to CCK permissions or any other wildcard permission for that matter which have dynamic parts in them.
The nice permissions descriptions improvement for Drupal 7 made this even worse by abstracting the permission name collection for certain core modules to node module. This makes static code analysis based permission name collection impossible for a considerable part of Drupal 7. So we need to look for a solution.
While the permission names are used in the database as keys (so they should be kept as their original English), we could use a dynamic string to display the permission name, which could allow for translating every known part of the permission name, specifically anything but the content type name in case of "create @type content", "edit own @type content" and such. The simplest way at this point to add this "permission title" is to make the description string a description array with the permission title and description the two expected items.
This in itself would make us write hook_perms() such as:
function example_perm() {
return array(
'my perm name' => array(t('my perm name'), t('Nice description...'))
);
}
This does not look nice when we need to repeat the permission name multiple times for multiple permissions. Any only to support dynamic permissions with placeholders. Oh, well. So I made up the user_localize_permissions() function, which does this automatically for us, and also serves as a marker for potx.module, so that it understands that the keys in the array wrapped by the user_localize_permissions() call are permission names to translate. So user_localize_permissions() is the same as if all those array keys would be wrapped around user_localize_permissions(). It is the job of potx.module to handle this of course. Therefore our code becomes much simpler:
function example_perm() {
return user_localize_permissions(array(
'my perm name' => t('Nice description...')
));
}
I've put this user_localize_permissions() into user module, because it is about user permission handling. We can rename it to t_perm() or something if desired and move elsewhere (eg. close to t() for example).
This patch also enforces the new hook_perm() format, since enforcing translatable permission names is in our best interest. We could try to enforce the translatable names without enforcing descriptions if we do:
function user_localize_permissions($permissions) {
$localized = array();
foreach ($permissions as $perm => $description) {
if (is_int($perm)) {
$localized[$description] = array(t($description), ''),
}
else {
$localized[$perm] = array(t($perm), $description);
}
}
return $localized;
}
Not sure keeping backwards compatibility here is worth the effort. With the user_localize_permissions() function it is already two possible formats to remember for hook_perm(), and if we keep supporting the old int indexed array format, it is going to be the third one, and we could end up with odd combinations.
Also note that this patch fixes all hook_perm() implementations I could find. It does not fix possible mentions of composite permission names in help text in core, which should also be looked at if this approach is agreed on. I don't expect much of a work there.
Comment | File | Size | Author |
---|---|---|---|
#13 | permission_localization_with_test.patch | 27.39 KB | meba |
#13 | permission_localization_without_test.patch | 27.17 KB | meba |
#9 | oldpermission.png | 76.79 KB | Gábor Hojtsy |
#9 | newpermissions.png | 77.66 KB | Gábor Hojtsy |
#8 | permissions_localization.patch | 27.25 KB | maartenvg |
Comments
Comment #1
hass CreditAttribution: hass commentedSubscribe
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commenteduser_localize_permissions
really looks like a hack. I suggest we standardize permissions in a clean array instead:*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().Comment #3
Gábor HojtsyLet's not make defines out of permission names in this patch, ok? :) Focus on the localization aspect. I am totally fine with a more complex array structure, whether it uses # or not, whether it is not a straight reuse of the permission name but somehow capitalized or not. My original patch is a "least effort for module developers" approach, which the permission code looked like taking (especially with the backwards compatible support on hook_perm() return values). As long as we keep the same name for permission titles and internal names, it is a hassle for developers to repeat the same string. As soon as we start diverging there, it is a requirement to specify them separately either exactly as or somewhat like how Damien Tournoud suggests.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedDon'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.
Comment #5
maartenvg CreditAttribution: maartenvg commentedI 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.
Comment #6
Gábor HojtsyGreat, I am all for it. Could someone please help, go through and make these changes into a patch? :)
Comment #7
maartenvg CreditAttribution: maartenvg commentedAlready on it :)
Comment #8
maartenvg CreditAttribution: maartenvg commentedOk, here it is.
This patch changes all hook_perm() declerations to the form
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!
Comment #9
Gábor HojtsyWow, looks great. Also love, that you run the tests on them ;) Just two quick shots before and after the patch applied.
Before:
After:
Comment #10
meba CreditAttribution: meba commentedApplies smoothly, works smoothly. Please commit this! :-)
Comment #11
Gábor HojtsyMultiple people verified it works great.
Comment #12
nedjoVery nice patch. A simple solution to a longstanding issue.
In this block of code the functionality changes:
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:
Otherwise very clean, ready to go.
Comment #13
meba CreditAttribution: meba commentedPrevious 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! :-)
Comment #14
webchickSubscribe! Somehow this failed to be on my radar. It's been a crazy week. :(
Comment #15
meba CreditAttribution: meba commentedStill applies to HEAD (with an offset)
Comment #16
Gábor HojtsySo to summarize:
- This patch gives us nicer permission titles, which could use proper capitalization instead of the odd "starts with lowercase" policy we have now.
- This patch allows us to finally be able to predictably translate permission names, even if they are composite (contain content type name for example).
- This patch opens the way for other permission related patches to add more metadata on permissions since the arrays would already be in place for them to add new items into. As Damien said above: "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."
Comment #17
RobLoachSubscribing! Making permissions awesomer is awesome.
Comment #18
webchickTo paraphrase catch on some patch, This is a lovely, lovely, patch filled with loveliness.
Small change: Can we please remove this hunk?
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?
Comment #19
meba CreditAttribution: meba commentedOK, 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 :)
Comment #20
maartenvg CreditAttribution: maartenvg commentedSince it was a misunderstanding, the status can be as before #18.
Comment #21
webchickLOL oops. :) I misread.
Committed! Yay!!
Docs please. ;D
Comment #22
Gábor HojtsyExisting update on permissions docs modified to say this, API docs still need to be updated:
Permissions are required to have titles and descriptions
(issue) and (issue) In the implementation of hook_perm(), the returned array format changed.
Drupal 6 supported this format:
Drupal 7 code should use the following format:
Previously, the values were the permission names; now permission names are the keys and the corresponding value is an array with permission title and description. If you happen to have composite permission names, which contain dynamic values such as content type names, look at the array format generated by node_list_permissions() which is reused by node_perm() and hook_perm() implementations of other core node modules.
Comment #23
lilou CreditAttribution: lilou commentedSee also this pending issue : #281621: Update hook_perm doc for Drupal 7
Comment #24
Gábor HojtsyI am tracking that bug as well, and the existence of it makes this fixed.
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.