Normalize permissions table

token comment to get me subscribed to this thread...

* can't use raw SQL because you want permissions to be translatable.

We'd store untranslated permissions, just like we do now. Is that a problem?

* need to handle the case when there is a new version of module with more or less permissions.

What do you mean with that? How does that differ from the current behavior?

We'd store untranslated permissions, just like we do now.

yep, that should work.

need to handle the case when there is a new version of module with more or less permissions.

i'm pretty sure a module's .install file can handle the necessary database updates to make this work

To solve the translation issue, we could add a @translation "Here is the string." to the PHPDoc comments of the function and extend extractor.php to include these. Then we store the plain english string in the database and t() it on run-time.

Looks like this needs work.

Assigning to self as per convo in #drupal

Note that I am not hapy with the proposed solution. Currently, the query that gets a user's permission is run only once. Now it will get run once per permission that is checked on a page.

We should at least check the average amount of queries this will add to Drupal per page and/or think about a caching solution.

Here is a !!work in progress!!.
I haven't finished rewriting user_admin_perm yet, so it's kind of crap now..

MySQL install: tested
Postgresql install: untested

Upgrade path: not yet

yes, needs benchmarking. this was tried once before without success but i can't find the issue. anyway, all has changed since then.

once nice byproduct of this patch is that we have a logical place to put permission descriptions - http://drupal.org/node/30984

Still no love for the administer roles page, but I split out the table update thingie into a proper function and cleaned up my spacing and removed a couple of lines. Now it should update permissions when you enable a module.. (Tested by truncating permissions table and enabling comment module)

And here's the access control page done the other way around, by permission instead of role.

Thoughts?

Patch no longer applies...

Rerolled

• Load all permissions for a user on the first call to user_access.
• Move the permission management functions to user.module. (Ideas for better names welcome!)
• Change algorithm for finding new permissions, use a lot less queries...

Can we properly prefix the new tables with 'user_'?

Hmmm. That would give us user_role and users_roles, wouldn't it? Sounds confusing. And then the joining table between user_permission and user_role becomes user_permission_user_roles. :P I think the permission and role names are fine, personally...

In *MY* opinion, the most logical place would be a module called "role.module" marked as an essential module.... Then prefixing the tables with role_. ;-)

Modules are not for code organization. They are for feature organization. Roles and permissions certainly belong in the user module.

If you want additional code organization, add modules/user/role.inc to be included by user.module as necessary.

The last patch was technically CNR, but I forgot to set the status...

I don't understand what this patch buys us -- except a lot more code and a potential performance penalty.

What problem are we trying to fix, and why are we trying to fix it this way?

- adding a new role does not use db_next_id(), new role rid is always 0.

- viewing admin/user/access warns:

warning: array_merge() [function.array-merge]: Argument #1 is not an array in \modules\user\user.module on line 1885.

Line #1885 is: '#header' => array_merge(t('Permission'), $role_names),

Should probably use a array cast, or something like:
'#header' => array_merge(array(0 => t('Permission')), $role_names),

- in user_update_permissions(), the:
db_query("INSERT INTO {permission} (pid,perm,module) VALUES (%d,'%s','%s')",db_next_id('{permission}_pid'), $permission, $module);

Isn't required anymore as the user_permission_add_permission() call in the line before already adds the permission if it doesn't exist.

- The patch needs a bit of a tidy (mainly spacing) to meet Drupal coding standards.

@Dries #27: I don't understand what this patch buys us.

It makes it much easier to manipulate the permissions associated with a role, directly via SQL queries, instead of grabbing a huge string and doing string manipulations with comma magic to make it work correctly. For example, the DB updated needed over at http://drupal.org/node/153998 is about 10 times more complicated that it should be since we just store a pile of permissions in a big string.

Plus, as moshe pointed out in #16, we could store (and therefore display) descriptions for each permission, which would be a *HUGE* usability win.

So, I think it's a mistake to "won't fix" this issue just because it didn't end up being a critical requirement for the D6 menu system. We should reconsider this once development opens up again for D7. As a developer, it really is *quite* a pain in the ass to manipulate roles and permissions programmatically while they're all smooshed together in a comma-separated string.

I might make this my first D7 patch. We'll see.

Nevermind. My itch is over at http://drupal.org/node/30984. But big +1 to anyone who chooses to take this on. :)

Subscribe. I'm tracking this against my own hook_configuration() issue at node/230059

I told chx I'd work on this after I got better. (Got the flu. :-/)

Still not better, but I spent a bit of time tinkering with this today.

I probabaly shouldn't be coding while feeling like this, but whatever.

Here's a re-think of things.

I left some notes and questions in the patch marked with @TODO.

+1 on concept. Some visual review of the patch, although I've not tried running it yet:

+  // Copy the permissions from the old {permission} table to the new {role_permission} table.
+  $result = db_query("SELECT rid, perm FROM {permission} ORDER BY rid");
+  while ($role = db_fetch_object($result)) {
+    $permissions = explode(', ', $role->perm);
+    foreach ($permissions as $permission) {
+      db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", $role->rid, $permission);
+    }
+  }

It's more robust to explode on just the comma and use array_map('trim', $array) on the result, like so:

$permissions = array_map('trim', explode(',', $role->perm);

Regarding some of the TODOs: Yes, we want to be able to show/update just a subset of permissions/roles, so the current code there is correct and the TODO can be changed into a normal doc explaining why we do it that way. That said, there may be a more efficient way to do the delete/insert cycle by deleting just the roles/perms that were shown and rebuilding that. There should be fewer delete queries that way. (OTOH, it's not like this is a particularly common page so a few extra queries isn't a real problem.)

I don't think for the dataset we're working with the DISTINCT will make any appreciable difference, and neither will the duplicate iteration, so it doesn't really matter which way we go, performance-wise. I personally prefer the DISTINCT myself, but that's just stylistic.

Regarding #41:

1) I am not sure I follow. Which IDs are we looking up?

2) The delete/rebuild cycle is a very common idiom for this sort of task, although as I note above it could probably be optimized a little. Not a huge issue given how rarely that form is executed.

3) We need to know the full list of modules/permissions quite rarely, so the cost of the module_invoke_all() is not a problem. Besides, that should get faster soon anyway. :-)

Setting to CNR so that others know to try it out.

After some discussion with justinrandell and webchick in #drupal, here's what I hope is a patch that's pretty much the minimum change needed to give us this better table structure.

I think a couple core tests need to be patched too - but feedback on this patch appreciated before I make that effort.

This patch mostly fixes the tests, but I can't really be 100% sure yet, due to this ciritical error: http://drupal.org/node/252920

@justinrandell - since all the permission handling is based on the strings, I think it would just be a PITA to link it to another table. Storage of a few short strings doesn't cost us much and makes all the queries simple and obvious.

so, chx says (paraphrasing) in #drupal "the table structure is good if the benchmarks are good"

A quick test looks fine. I used ab with a session cookie for a normal user (not user #1) across the local net. The test machine is running PHP 5.2.4, MySQL 5.0.45, Darwin 10.4.11, apache 1.3.41

I ran each test 2x, so you can get a sense of the variability.

Without patch:

$ ab -n200 -v1 -c5 -C SESS***=*** http://192.168.1.2/drupal-7/admin

Time taken for tests: 31.387279 seconds
Requests per second: 6.37 [#/sec] (mean)

Time taken for tests: 31.437647 seconds
Requests per second: 6.36 [#/sec] (mean)

$ ab -n200 -v1 -c5 -C SESS***=*** http://192.168.1.2/drupal-7/admin/user/permissions

Time taken for tests: 27.789632 seconds
Requests per second: 7.20 [#/sec] (mean)

Time taken for tests: 28.213168 seconds
Requests per second: 7.09 [#/sec] (mean)

WITH patch from #48:

$ ab -n200 -v1 -c5 -C SESS***=*** http://192.168.1.2/drupal-7/admin

Time taken for tests: 30.527532 seconds
Requests per second: 6.55 [#/sec] (mean)

Time taken for tests: 30.688854 seconds
Requests per second: 6.52 [#/sec] (mean)

$ ab -n200 -v1 -c5 -C SESS***=*** http://192.168.1.2/drupal-7/admin/user/permissions

Time taken for tests: 27.980566 seconds
Requests per second: 7.15 [#/sec] (mean)

Time taken for tests: 27.727507 seconds
Requests per second: 7.21 [#/sec] (mean)

So, bottom line is that I see the same or a slightly faster benchmark with the patch.

here's a revised patch that is exactly the same except for omitting the changes to profile.test - there is a patch here which fixes that: http://drupal.org/node/252920#comment-827565

With the combination of this patch with that one, tests work.

the profile.test fixes are committed.

Re-rolled to clean up some minor whitespace problems, plus add more return output during the update.

I reviewed the code and it looks RTBC. My tests ran fine with it.

I wonder if it makes sense to add some additional tests to test the "permission API". As far as I could see, we don't test permissions being revoked/deleted. Breaking the permission API might results in all kinds of security issues. It seems like an area that we'd want to test well. This might be a good time to go the extra mile and to write some more tests and/or clean up the API if necessary.

ok, we; there's not really a permissions API to add/remove them except the big form. I'll see about writing a functional test for that page since it does not seem that there is any such test currently.

Also, thinking a little more about the patch this morning, it seems that perhaps the update should be in system.install rather than user.install. Normally I'd want to keep core updates with the respective module, but if I recall correctly, system.install's updates are always run first. Thus, any other module updating its permissions could use the new, easier table structure for updates if the update is done there.

We should probably introduce a proper API for playing with the permission settings; that would help install profiles, too, and make it more testable. Whether we do that as part of this patch or not I don't know.

Thanks Peter, that would be helpful.

setting to CNW until I have a chance to rewrite the update and get a basic test in place.

partly done

Seems to be working - update moved to system.install, basic test added to user.test

I'm not 100% on all the different layers of caching, and caching in static variables in particular. It's prone to errors. It is becoming impossible to remember when you need to flush what cache.

@justinrandell -

#1 - yes, that may be a bug.

#2 - I think a LEFT JOIN is preferred, since even if a role has no permissions we want to get a results back. Otherwise we keep issuing a query.

fixed #1

confirmed correctness of LEFT JOIN w/ davidstrauss.

oops- clashed with registry patch - re-rolled

also, davidstrauss suggested in IRC that the query could alternately use an INNER JOIN, but then we would need to change the loop logic in function user_role_permissions() to set empty entries for any rid that returns no results.

Tested patch from #68 (the patch from #67 works too and is probably identical, but I haven't taken a close look at the code).

The new test class UserPermissionsTestCase looks good, I think it covers what we need for this patch to be committed. Tests are run without any failures or exceptions.

The rest of the changes seem ready as well, but a little more attention wouldn't hurt for a patch of this importance, so I'm leaving it as CNR.

After further thought- maybe the INNER JOIN is the better choice. The code for that function seems even a little simpler this way.

Reviewed, tested, looks great. Committed to CVS HEAD. Thanks Peter.

Wow, I go away for a little while and look at all the goodness that happens. ;) Cool.

However, it seems like this deserves a mention over at http://drupal.org/node/224333, no?

@chx - I agree generally module developers won't need to take any action. However, it's probably worth adding a short section for those who are changing their module's permissions, or for any modules that try to directly manipulate permissions.

seems to me that some functions that are in the test ought to be in core. for example:
drupalCreateRole
drupalCreateUser

Moshe: those functions are modified by the patch but they're already in core.

I reckon a note to explain the change ought to be fine for this, but agree it's worth having one just in case.

As has been pointed out elsewhere, an "API" is more than a set of function names and parameters. It's also the underlying data schema, how the data is stored, used, etc. Given that there is no "API" for manipulating roles and permissions with functions, module authors have only had the option of directly manipulating the DB tables. Those tables are now different, so the upgrading from 6.x to 7.x doc needs to mention this.

2 general points:

1) I'd much rather there was "too much" info in the upgrade docs and we mentioned things that will only bite a small handful of users (or folks upgrading custom code for their own sites), instead of not mentioning changes that "shouldn't" effect people (which inevitably will).

2) If there's any chance of a change breaking someone's code, no issue should be marked "fixed" until it's documented in the upgrade guide. This is an extremely small price to pay for our drop-is-always-moving philosophy. But, given that philosophy, this must be a hard and fast rule, (dare I say it) even more strictly enforced than "not committed without the tests".

Cheers,
-Derek

@chx: Thanks. ;) I would have just done it myself, but I wanted to make the more general point...

@chx - thanks, I just didn't have time the last few days for much of anything.