Posting this here for comment.

In the function user_role_grant_permissions(); I believe there is a foreach() missing. The variable $modules is an array but the origional code does not account for it. Found this working on the updating process.

function user_role_grant_permissions($rid, array $permissions = array())
{
  $modules = user_permission_get_modules();
  // Grant new permissions for the role.
  foreach ($permissions as $name) {
     db_merge('role_permission')
        ->key(array(
          'rid' => $rid,
          'permission' => $name,
          'module' => $modules[$name],
      ))
        ->execute();
  }
}

The merge is failing since $module[$name] doesn't seem to have a value, $modules does but it is an array. $name doesn't look correct in the function either.

I replaced the code with

function user_role_grant_permissions($rid, array $permissions = array()) {
  $modules = user_permission_get_modules();
  // Grant new permissions for the role.
  foreach ($permissions as $name) {
    foreach ($modules as $value) {
      db_merge('role_permission')
        ->key(array(
          'rid' => $rid,
          'permission' => $name,
          'module' => $value,
        ))
        ->execute();
    }
  }

got the upgrade to continue beyond a breakpoint and the table was written to. Not posting a diff yet, just checking with those more familiar with the function, might be a better way to do this.

Comments

Cursory glance, but I don't see anything obviously wrong with the current code. Pasted the current function body here with additional inline comments:

<?php
 
// Fetch an associative array in the format $permission => $module.
 
$modules = user_permission_get_modules();
 
// Grant new permissions for the role.
 
foreach ($permissions as $name) {
    
db_merge('role_permission')
        ->
key(array(
         
'rid' => $rid,
         
'permission' => $name,
         
// $modules[$name] will be the name of the module implementing the permission we are granting.
         
'module' => $modules[$name],
      ))
        ->
execute();
  }
?>

$modules is just a look-up table to get the module name that implements the permission with name $name. There is no need to cycle over each of those items.

The replacement code in the OP looks really suspicous to me. It basically creates rid-permission-module sets containing mismatching permission - module combinations.

Category:bug» support
Status:Active» Postponed (maintainer needs more info)

Let's move this out of the bug queue. If you are still experiencing issues, please write up detailed steps to reproduce on a clean install. It's possible that the behavior you are seeing is caused by a bug elsewhere. Otherwise, feel free to close this issue.

Title:user_role_grant_permissions() possibly brokenuser_role_grant_permissions() broken when used for a disabled module's permission
Category:support» bug
Status:Postponed (maintainer needs more info)» Active

The merge can be reliably made to fail if you call user_role_grant_permissions() with a permission that does not exist in any currently-enabled module.

For example, calling something like this:

<?php
user_role_grant_permissions
(DRUPAL_AUTHENTICATED_RID, array('access statistics'));
?>

leads to a fatal error if the Statistics module is not enabled when that code is called. The error is:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module' cannot be null:

It's not entirely clear what the correct fix is. We could pass in an empty string for the module, of course, but then the permission will not have its module correctly recorded like the others.

This seems to be a bit of an API regression.

We could pass in an empty string for the module, of course, but then the permission will not have its module correctly recorded like the others.

I forgot to mention that I'm not totally sure what the consequences of getting the module name wrong are - maybe they are not too bad. It feels like a bad idea to leave that blank, though.

Status:Active» Needs review
StatusFileSize
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 18,736 pass(es).
[ View ]

Should we fix that? Calling user_role_grant_permissions() with a non-existing permission is indicative of a bug or oversight in the caller, in which case I think the fatal error is preferred over the silent empty module name column.

Patch attached with a suggested fix if we do want to fix this.

I agree with mr.baileys: depending on what conclusion we end up agreeing upon at #773828: Upgrade of disabled modules fails, this issue might not have to deal with the case of the upgrade with disabled module (say for example if we include all disabled modules' .module file automatically).

I think including all disabled modules' .module files actually would not fix the case where this happens during an upgrade - since user_role_grant_permissions() ultimately gets its allowed list from invoking hook_permission(), which requires the module to be enabled to pick up its permission list.

So unless we wind up doing something else at #773828: Upgrade of disabled modules fails (e.g., actually enabling the module before upgrading it, or otherwise somehow forcing its hook to be called), then that is actually a good example of where this error would be reached even though there is no bug in the calling code. The patch here would fix the error, but would prevent the permission from actually being assigned, which doesn't seem right - especially for a case like http://api.drupal.org/api/function/contact_update_7002/7 (the one discussed in the other issue).

I think there are use cases where you want to be able to assign a permission to a disabled module, so that the module behaves the way you want it to once it's enabled. There are other workarounds that can be done for that, but to me they seem kind of awkward compared to just assigning the permission.

StatusFileSize
new1.56 KB
PASSED: [[SimpleTest]]: [MySQL] 20,344 pass(es).
[ View ]

I think that throwing an exception is actually better behavior. Otherwise it ends up being yet another Drupal WTF where you have no idea what happened. Also, we need exception classes, but I would rather err on the side of noisy than silent here.

This patch is from http://drupal.org/node/794184#comment-2972432, and also creates a new function which doesn't validate that the permission is added to the module map.

Not sure if we want to go this route, @chx has another patch on that same issue which attempts to build all the disabled modules into the permissions list. Maybe that's cleaner?

Anyway, if interested...

About the patch in #9: I don't understand why user_role_grant_permissions() (plural) throws a permission, and why user_role_grant_permission() (singular) doesn't. That seems inconsistent behavior to me.

Well, the grant_permissions one takes a list of permissions w/o module names, and it attempts to find out which modules they belong to. The grant_permission one simply makes an insert given 3 required params. If grant_permissions doesn't find the existing module, it should throw an exception because it is not operating as expected and you as the caller have no way of knowing that.

Breaking w/ tradition (which I'm not recommending here) I'd actually require the module name be passed along with the permission to user_role_grant_permissions(). Why? because a function should do only one thing, and if this function grants permissions, I think a different function maps permissions to the modules they are from. It's okay if those two things are combined in one function, but perhaps the name should suggest that, and it should call 2 other functions which just does each thing (map to modules and do the actual granting).

Best,
Jacob

btw, we could cause the singular version to throw an exception as well, but since it is just a db_insert or update, and all the params are required, it will anyway throw a warning if you don't pass a module in, and the DB query will fail if you pass in bad values (i.e. an array for $permission).

-J

I think Jacob's approach here might wind up being the best we can do in Drupal 7 - although it's not really solving the problem so much as working around it; the function is just as broken as before, but we'd now be telling people not to use it :)

The only other option I can think of is that if you look at why this 'module' column was added to the database in the first place, it was to solve #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced, and in particular to allow Drupal 7 to properly clear out a module's permissions from the database when the module is uninstalled. So if that is the only reason it exists, perhaps we only need to ensure that that column is populated when a module is disabled - i.e. do it in user_modules_disabled() so that the data is there only if/when the module is later uninstalled, not when a permission is inserted (and then rename the column from 'module' to 'disabled_module' or what not). That probably still has its own issues too, though. We need a better overall way to deal with disabled modules in Drupal 8...

@chx's solution on the other issue will not work in the case of dynamically-generated permissions (e.g. taxonomy module), by the way.

By the way, it occurs to me that http://api.drupal.org/api/function/user_update_7006/7 is likely pretty broken too as a result of this bug; it's not using user_role_grant_permissions() but rather the helper function user_permission_get_modules() but that's the same issue; thus, it will miss assigning any disabled module's permissions as well, leaving the D6->D7 upgrade in an inconsistent state with the 'module' column empty in those cases.

Status:Needs review» Needs work

More exceptions! That's what we need, exceptions thrown in update.php! That's surely going to help.

Status:Needs work» Needs review

12. Rule of Repair: When you must fail, fail noisily and as soon as possible.
- Eric Raymond, "Art of Unix programming"

Isn't that preferable to not throwing an exception, letting the update continue and potentially leave the site in a broken state with the schema numbers updated?

So I run the update, no error is thrown, just silent failure.

Then, the schema number gets updated in the system table because it thinks it worked.

Now, I can never run that update again to fix my site.

At least w/ an exception you have a chance of fixing it, you also have a chance of handling it silently if you want. This is after all why we use Exception classes instead of die('you screwed up').

If you don't want a fatal, at least the caller has a choice. Obviously, we have no exception classes in D7, so it makes it harder to figure out what went wrong), but an update hook could silently allow it to succeed or at least return a pretty error and exit.

Can we not make the modules column NOT NULL instead of defaulting to '', then the merge would fail. I realise this would be a schema change but it seems like this should be enforced in the database (which would then throw a PDO exception anyway), rather than having to be handled in PHP.

While that's not a bad idea (we should also do that), because it reflects reality, it's failing a little too late IMO. If we know the query will fail, we should catch it first so we can show something useful to the user and not make them go find this thread to realize disabled mods' perms won't get loaded :)

-J

Personally, I think it is OK to thrown an exception. That is what exceptions are for. ;-)

Status:Needs review» Reviewed & tested by the community

Exceptions are a nightmare which I have complained about in many issues and I am sure we will get many headaches from exceptions thrown in update.php (you WILL get half-updated schemas, good luck fixing that) but sure go ahead. Exceptions are part of OOP so they must be great, right? (No. I have said we should yank all exceptions from Drupal 7 and I still think so)

But it's actually not worse than the nightmare we are descending into already so let's go with this.

I think pretty much all modern languages have exceptions. I don't think this is about using class() anywhere, it's just a way to handle an error from the call site; let the call site decide how to handle the error and not through a monolithic end-of-the-road handler. How is this related to the OOPacolypse? :)

-J

Status:Reviewed & tested by the community» Needs work

If we throw an exception, let's add some PHPdoc about not using that function in module updates at the very least. I can't believe people complain about throwing errors too late, then expect people to read the actual code to find out that one will be thrown at all.

Also I'd like to see at least a followup for the schema change opened before this gets committed.

Subscribing.

Here's a snippet for those of you who may be running into a similar problem where the node is enabled but the node type cache is not yet up to date, as is the case just after a new node type has been inserted. You need to clear the node cache or else you will also run into the problems in this thread..

/**
* Implements hook_node_type_insert($info).
*/
function mymodule_node_type_insert($info) {
  // The node type cache needs to be reset first so that the newly inserted
  // module has permissions defined for it
  node_type_cache_reset();
  // now go ahead and grant permissions...
  user_role_grant_permissions($foo, $bar)
}

Use Case: Building an Install Profile.
Action: Copy the text format & role assembly code.
Edge Case: Decide not to support comments. So don't turn that module on.

You get stuff that looks like #1163190: Integrity constraint violation: 1048 Column 'module' cannot be null at the end of installing all the modules.

Title:user_role_grant_permissions() broken when used for a disabled module's permissionuser_role_grant_permissions() throws PDOException when used for a disabled module's permission or with non-existent permissions
Version:7.x-dev» 8.x-dev
Status:Needs work» Needs review
StatusFileSize
new778 bytes
new922 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,268 pass(es).
[ View ]

A PDOException can be thrown by user_role_grant_permissions in the following cases :
- one of the modules as been disabled and one of the granted permissions belong to it
- one of the granted permissions does not exist

This can occur when some permissions are dynamically generated from objects in the database with one cache layer.
This often occurs when enabling of reverting Features.
When this happens during the site install process, this one is immediately interrupted.

Here is a quick patch (and its backport to D7) to avoid this Exception to be thrown in these cases.
I hope you will find it usefull.

Patch #26 works on d7.19

Also confirming patch at #26 on D7. Has resolved multiple issues with permissions defined in features when installing an installation profile.

Status:Needs review» Reviewed & tested by the community

Hi,
the patch in #26 worked well for me on 7.19 and last codebase on Drupal 8 branch.
Thanks

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

#1901636: Importing a disabled format that is assigned to a role causes a fatal has tests for this, as well as some open ended discussion on the true cause. Though it is interesting that it is a problem in D7.
Not marking either a duplicate yet.

This issue seems to duplicate the existing #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced to a pretty good extent.

#26 works for me on 7.19

This wouldn't apply to 7.22 so I re-rolled it. Applying it solved my issues, which were mainly to do with a Feature that contained field permissions for node types defined in other Features. The permissions still work as well. My simpletests for this Feature and its field permissions, which crash due to the fatal if I don't use this patch, are not only able to run, but they are all passing when the patch is applied. I do not experience a silent failure that later breaks my permissions or anything like that.

Status:Needs work» Needs review

Rerolling so it applies cleanly from root folder.
Also, testing it indicates it works, although it doesn't set the permissions, with no error messages.

Works for me too.

Status:Needs review» Reviewed & tested by the community

as for #37

Status:Reviewed & tested by the community» Needs work

We need some tests to ensure we don't break this again...

Just a reminder: current patch fixes the error thrown, now it continues *without setting the permissions*.
So, AFAICT, this needs work. I'd expect the patch to actually set the permissions, somehow.

Status:Needs work» Needs review
StatusFileSize
new1.93 KB
PASSED: [[SimpleTest]]: [MySQL] 56,752 pass(es).
[ View ]

Here's the patch from #26 re-rolled with a very rudimentary test that essentially copies the role assignment logic from the standard install profile. It has no assertions, but when run without the patch from #26 it fails (as the PDO exception is thrown, uncaught).

Regarding #40 this issue isn't about assigning the missing permission--in fact, Drupal cannot possibly assign the permission, because it is unaware of permissions from missing or disabled modules.

With the patch all code calling this function should verify that there is an enabled module for each permission it passes in or risk missing some permissions.
Without the patch all code calling this function should verify that there is an enabled module for each permission it passes in or risk a crash.

So should the responsibility of verifying the permissions always be with the calling function?
With code like this?

<?php
$permissions
= array('access content', 'access comments');
$valid_permissions = array_intersect_assoc($permissions, array_keys(user_permission_get_modules()));
// Do something in case one or more permissions are not available.
if (count($valid_permissions) < count($permissions)) {
 
$invalid permissions = array_diff($permissions, $valid_permissions);
 
$message = 'Attempt to assign invalid permissions @perms';
 
$args = array('@perms' => implode(', ', $invalid_permissions);
 
drupal_set_message(t($message, $args, 'error');
 
watchdog('mymodule', $message, $args), WATCHDOG_ERROR);
}
// Grant the permissions to the role.
user_role_grant_permissions($rid, $valid_permissions);
?>

In that case I'd say we commit the patch and add documentation stating that "The caller of user_role_grant_permissions() is responsible for verifying the permissions it passes are available. user_role_grant_permissions will completely ignore all permissions that are not provided by an enabled module at the time of calling."

Status:Needs review» Needs work

@JvE:
Regarding the question in #42 I'm setting this back to CNW.

Just skipping grants without a notice would leave quite some responsibility at contrib side and will lead to very inconsistent error handling and WTF situations where the grant simply disappears and nobody knows why.

It would be much better if user_role_grant_permissions() threw the error. But because it is called once per role, we'd get an error message per role and not an error message per failing permission.
For D7 we probably can't do better.

But user_role_grant_permissions() seems to be tailored to the role permissions admin page. However, there is a couple of cases (with features being the most prominent one), where permissions are set just the other way around: many roles per permission. It would be nice if we could make these API functions more flexible:

user_role_grant_permissions($rid, array $permissions)
would become:
user_role_grant_permissions(array $grants)
with $grants being an array of permissions keyed by rid, or the other way around.
Same with user_role_revoke_permissions().

and:
user_role_change_permissions($rid, array[array] $permissions)
would become:
user_role_change_permissions(array $grants, array $revokes)

The caller would then be able to pass in arbitrary pairs of rids and permissions, while on the callee's side, we could easily group by permission.
If there is support for this idea, I would happily file a patch in a separate issue.

I'm starting to lean towards a "won't fix".
The problem here is the `module` column which is apparently a solution from #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced.

As David_Rothstein notes in #13 this only seems to be required so Drupal can know which permissions to clean up when a module is uninstalled.

To me it seems a proper fix would be to make drupal permissions namespaced so that the module is always known. Then it does not matter if modules are disabled or even not installed, the permission can be stored anyway.

Without changing the API the current issue is not going to be resolvable.
Which means that indeed all code calling user_role_grant_permissions() or user_role_change_permissions() has these options:

  1. Ensure all requested permissions are available before calling.
  2. Catch and handle exceptions that may be thrown from these functions
  3. Bypass these functions and update the `role_permission` table themselves.
  4. Don't care about errors or integrity at all.

Features (my use case) is somewhat attempting to solve this as part of #1265168: Rebuild the file list properly when a feature is enabled or disabled.
For core #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed may eventually solve this.

For D9 I think Pancho's idea of deprecating the current user_role_*_permissions() in favor of more flexible function is the way to go. Think options for caring about which permission belongs to which module, taking enabled/disabled in consideration, differently keyed/groups arrays, etc.

@JvE:
We're in feature freeze, but I don't think the API was frozen, too.
As you say, my proposal in #43 enables us to correctly fix a bug. So maybe a task, but definitely no feature.
I'm gonna start this new route with a new patch because I agree that the other route doesn't lead us anywhere. This wouldn't be a massive change.

#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed got in some time ago, so it is no longer possible to disable modules.

Title:user_role_grant_permissions() throws PDOException when used for a disabled module's permission or with non-existent permissionsuser_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module)
Issue summary:View changes

It doesn't matter that you can't disable modules any more, as the same applies to uninstalled modules. I've tweaked the title to reflect this.