user_role_grant_permission fails when module is missing [#1249952]

When passing an array of permissions into user_role_grant_permission, the following error is generated if no module owns the permission.

WD php: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module' cannot be null: UPDATE {role_permission} SET module=:db_update_placeholder_0                                                          [error]
WHERE ( (rid = :db_condition_placeholder_0) AND (permission = :db_condition_placeholder_1) ); Array
(
    [:db_update_placeholder_0] => 
    [:db_condition_placeholder_0] => 4
    [:db_condition_placeholder_1] => use text format member_filter
)
 in user_role_grant_permissions() (line 3030 of modules/user/user.module).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column &#039;module&#039; cannot be null: UPDATE {role_permission} SET module=:db_update_placeholder_0
WHERE ( (rid = :db_condition_placeholder_0) AND (permission = :db_condition_placeholder_1) ); Array
(
    [:db_update_placeholder_0] => 
    [:db_condition_placeholder_0] => 4
    [:db_condition_placeholder_1] => use text format member_filter
)
 in user_role_grant_permissions() (line 3030 of modules/user/user.module).

This can happen when trying to load a previously saved list of permissions and a module was disabled in the meantime. This is helpful for people with multiple instances of Drupal (dev, test, live etc) and wanting to copy data between them.

Suggest placing (if ($modules[$name]) { continue; } in the foreach loop to ignore permissions for missing modules

Comment	File	Size	Author
#2	user_role_grant_permission_fails_when_module_is_missing-1249952-2.patch	515 bytes	juampynr
#2
#1	user_role_grant_permission_fails_when_module_is_missing-1249952-.patch	508 bytes	johnennew
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

johnennew CreditAttribution: johnennew commented 15 August 2011 at 17:02

File	Size
user_role_grant_permission_fails_when_module_is_missing-1249952-.patch	508 bytes

Here's a patch

Comment #2

juampynr CreditAttribution: juampynr commented 27 October 2011 at 16:02

Priority:	Normal	» Major
Status:	Needs review	» Reviewed & tested by the community

File	Size
user_role_grant_permission_fails_when_module_is_missing-1249952-2.patch	515 bytes

Tested and added an isset() evaluation to avoid PHP warnings. Applies cleanly and fixes the issue.

I experienced this error while enabling a feature.

Comment #3

catch

he/him

English

CreditAttribution: catch commented 27 October 2011 at 17:14

Version:	7.7	» 8.x-dev
Status:	Reviewed & tested by the community	» Needs work
Issue tags:		+Needs backport to D7

Just suppressing the warning means there's no feedback if this condition happens for any other reason.

The patch should be against 8.x and tagged for backport.

Please don't mark your own patches RTBC.

Comment #4

juampynr CreditAttribution: juampynr commented 27 October 2011 at 21:56

OK for setting it as needs review, but I also said that I tested it myself and fixed the reported error.

Comment #5

JeremyFrench CreditAttribution: JeremyFrench commented 1 November 2011 at 10:35

Looking at this issue I can see a couple of options.

1. Keep things as they are. This would force implementing modules to check that a module is enabled before setting a permission. (or cause errors). I'm not 100% sure this is a bad thing.

2. Implement a variation of the patch above which does a drupal_set_message() and/or a watchdog. So errors are skipped. This could cause problems if modules are later enabled as permissions are not as expected.

3. Enable permissions to be created for modules which are not yet installed.

I'm not sure that 2 is the best option. At best it will punt errors further down stream and cause problems when someone does enable a module later.

I'm tempted to go with option 3.

Allow null on the module column, which will allow for permissions to be added for modules not yet installed.

When modules are installed check to see if there are matching permissions with null and alter them (also set a flag that they were implemented outside of the module).

When modules are uninstalled set the column to null again.

It is more complicated than option 2 but possibly more reliable.

Comment #6

johnennew CreditAttribution: johnennew commented 14 November 2011 at 14:33

I think making the field able to be null sounds reasonable though I am not sure about everywhere this is used.

This is something that can happen when using features and may be due to the order that features enables each 'feature' of a feature. I wonder if enabling a module which both defines a permission and defines those permissions attached to roles is causing the exception.

Comment #7

JeremyFrench CreditAttribution: JeremyFrench commented 15 November 2011 at 09:06

I started working on a patch for option 3 above, It kind of works but it is a bit of a rabbit hole and leads to some weird inconsistencies.

So I looking at option 2. One simple change, along with the set message, is to return the number of permissions actually changed. So a calling module has visibility as to if the roll change worked as expected.

It would also not be too tricky to implement a module which has 'sticky permissions' which can be set outside of modules being enabled. So that may be a better approach to the original use case.

Comment #8

sburlacu CreditAttribution: sburlacu commented 4 December 2011 at 10:24

Status:

Needs work

» Needs review

#1: user_role_grant_permission_fails_when_module_is_missing-1249952-.patch queued for re-testing.

Comment #9

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 16 January 2012 at 19:02

Priority:

Major

» Normal

I don't think this is a bug, nor do I think we need to do anything here. Callers need to ensure that the permissions they pass to user_role_grant_permissions() exist. The caller is best placed to decide what needs to happen if the permission does not exist (display a warning, throw an error, ignore the permission, whatever...)

So my feeling is that this is a won't fix.

In any case, as per Priority levels of issues, this does not seem major to me.