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

fajerstarter - July 23, 2007 - 19:22

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

fgm - July 23, 2007 - 20:20

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

fajerstarter - July 23, 2007 - 21:18
Status:active» by design

As I see it is should be handled by core. You could open an issue about this in the Drupal project.

#4

fgm - July 24, 2007 - 18:48
Status:by design» needs review

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.

AttachmentSize
INSTALL.txt_3.patch 1.95 KB

#5

fajerstarter - July 25, 2007 - 06:16
Status:needs review» needs work

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

fgm - July 25, 2007 - 07:07

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 .install files include variable_del, and the few I read had them in hook_uninstall.

This may be because the documentation http://drupal.org/node/64279#uninstall saying In module.install, you can now place an "uninstall" hook, which drops tables and deletes any variables that the module adds. And, of course, having the code perform cleanup means instructions are unnecessary.

Random sample is weather.install

#7

fajerstarter - July 25, 2007 - 07:31
Status:needs work» won't fix

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.

 
 

Drupal is a registered trademark of Dries Buytaert.