Uninstalling should remove variables
fgm - July 23, 2007 - 11:36
| Project: | Admin message |
| Version: | 5.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | won't fix |
Description
The current hook_uninstall implementation drops the tables created by the module, but does not remove the persistent variables node_options_admin_message and comment_admin_message.
It should probably remove them since no other code will be using them afterwards.

#1
I decided to not remove the installed content type "Admin message" when uninstalling since it will leave the nodes created without a matching type.
And since the variables are part of the content type created by Drupal is should be handled by core when the user decides to remove it (currently the are not removed, if you feel they should you should probably report it to Drupal core).
#2
I think this needs to go further: what will drupal do with these nodes once the module is uninstalled ? The default node handling does not necessarily cover what should be done with them. Uninstall should probably also remove the existing nodes, or maybe fail if any such nodes exist, to prevent this case of nodes remaining without the module handling them, hence being inconsistent.
At this point (no remaining nodes), removing the variables would be more logical. What do you think of it ?
#3
As I see it is should be handled by core. You could open an issue about this in the Drupal project.
#4
I think core will have trouble knowing what it should safely do with non-core-defined content, but if you say this is by design then so be it.
In that case, maybe outlining these optional steps in INSTALL.txt would be helpful to users who would otherwise think the module will uninstall without leaving traces ? The proposed patch adds such a possible set of instructions.
#5
Do you know of any other modules, core or contrib, which uninstall content (or comes with instructions like this)? Not that Admin message can't be first, but it would bi nice to know how other handles this.
As far as the instructions goes I think it's enough just to inform the user that conent is not deleted, maybe a message upon uninstalling.
#6
I didn't know how to look for instructions in INSTALL.txt files without spending a lot of times, but regarding variable deletion, this appears to be actually quite common. In contrib DRUPAL-5--1-1, 78
.installfiles includevariable_del, and the few I read had them in hook_uninstall.This may be because the documentation http://drupal.org/node/64279#uninstall saying . And, of course, having the code perform cleanup means instructions are unnecessary.
Random sample is weather.install
#7
The question here is not whether its standard or not to delete variables, of course modules should delete variables that they introduce. The question is instead whether a module should delete content or not, and there research on common practice would be interesting.
As said before, the two variables you mentioned are tightly coupled with the content type, and the content type is tightly coupled with the content (nodes). Hence, it doesn't make sense to remove them if the content and/or the content type is removed. Also, maybe I wasn't clear on this, but the variables are *not* directly introduced by this module, they are probably created with
node_type_save. That's why I say that they should be likewise handled by core when the content type is deleted.Please open a new issue with some research on common practice on content deletion if that is what you are after.