'roleassign_roles' variable being erroneously added to users table 'data' column [#123317]

any time you use a custom form field in any of the user forms hooks, you must explicitly unset it during the _update or _insert op if you do not want it saved in the serialized data column of the users table. roleassign fails to do this, and roleassign_roles gets saved there.

this means that every time a user object is loaded from the database, the code has to go through the extra step of unserializing that unnecessary variable in the data column. attached patch corrects this, and provides a database update which will clean our that var for all users.

tested and working perfectly.

Comment	File	Size	Author
	roleassign_3.patch	1.67 KB	hunmonk

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

TBarregren CreditAttribution: TBarregren commented 28 February 2007 at 17:42

Status:

Needs review

» Fixed

Thank you for pointing this out. I had completely missed that one.

Your patch worked great, except for roleassign_update_1() where you missed the return value.

I have committed the patch, along with some other improvements of the source code, to both the 4.7 and 5 branches.

Comment #2

hunmonk CreditAttribution: hunmonk commented 28 February 2007 at 19:15

out of curiosity, why did you change the approach on variable deletion? the new method seems more complex and prone to errors. a simple variable_del('name_of_variable'); handles both the removal of the variable from the variables table, and the clearing of the cache. also, using that LIKE clause to pull the variables for the module could lead to problems of pulling variables you shouldn't be deleting (although i'll admit that's probably not going to happen). seems cleaner to just explicitly delete the necessary variables using variable_del.

just my two cents.

Comment #3

TBarregren CreditAttribution: TBarregren commented 28 February 2007 at 21:48

I have been working on two other modules, which will be release here on drupal.org. (I am just awaiting an accept from my customer.) When working with these modules, I got tired on updating the uninstall hook every time I renamed a variable, either directly or indirectly by changing a form. So I wrote the new uninstall hook, which automagically delete all variables in the module's "namespace".

Admittedly, there was no urgent reason to also introduce this automagically uninstall hook into RoleAssign. The code worked as it was. But I decided to incorporate it anyway, along with some other improvements, of which the adaption to Drupal's coding standard is most notably, just for the gratification of knowing that the source code has become a better place to live (for a programmers like myself, that is :-)).

In fact, I wonder if it wouldn't be a good idea to incorporate the automagically variable delete code into core. I have not investigate it, but I guess that most contributed modules don't remove their variables when unistalled. That is of course not a big issue, but it is a little bit annoying. Considering that many variables are created automagically, it perhaps not a bad idea to also delete them automagically. My code shows how that can be accomplished.

The code is admittedly more complex, but I don't agree that it is more prone to errors. Only variables in the module's "namespace" are deleted. Following the information hiding principle, these variables are private to the module, and hence it must be regarded as safe to delete them. Drupal is built upon this assumption. Not only variables, but forms, functions and database tables, depends on this principle.