Variables aren't cleaned up on uninstall

Thanks for submitting this, braahm. I'd also include a note about 'why not use direct database delete of variables starting with nodeapi_example', but I guess this module may not be the best example for that.

I agree with you about the developer being responsible of deleting the variables, +1 to include it. Patch seems fine, minor cosmetic changes would do.

Now that I see the purpose in the phpdoc header of the uninstall hook, I can't recall any examples for hook_enable or hook_disable methods, and I'm unsure if it is enough to understand that hook_uninstall only runs when the module is completely uninstalled, not only disabled.

While you're at it, please mention in the comment *not* to do "delete from variable where name like 'module_%'"" because it deletes unintentional things.

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
+ * hook_uninstall will only be called when uninstalling a module,
+ * not when disabling a module.

1) Missing () after function name.

2) Wraps too early (before 80 chars).

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
+ * We need to cleanup our data when uninstalling
+ * our module.

Wraps too early. But also, isn't this documented in hook_uninstall() already? If it's going to be kept, it can be merged with the first paragraph (actually should be prepended to it).

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
 function nodeapi_example_uninstall() {
+

No leading blank lines, please.

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
+  // Delete the nodeapi module variables because we don't need them anymore.

1) The actual module name seems to be "nodeapi_example", not "nodeapi".

2) Never ever use shortcuts such as "don't" in code comments. Not everyone is a native speaker, and even more importantly, not using them enforces a plain and strict language on you.

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
+  // created a nodeapi_example_contentType variable

1) Missing quotes around the variable name.

2) Dynamic parts in function and variables are usually denoted with all-uppercase letters, or alternatively [variable].

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
+  // for each contenttype in the variable table.

Content type are two words.

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
+  // This means we need to delete the variables when uninstalling
+  // the module.

Repetition?

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
+  // To delete our variables we call variable_del.

1) Missing ().

2) "Call foo() to do bar." would sound more natural to me.

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
+  // Never delete the variables from the variable table manually with a ¶
+  // delete-operation like "delete from variable where name like ¶
+  // 'nodeapi_example_%'" because it deletes unintentional things.

1) Trailing white-space.

2) The reasoning needs a bit more explanation to be valid. Doing a wildcard delete on "nodeapi_example_*" surely won't delete unintentional things...

+++ nodeapi_example/nodeapi_example.install
@@ -17,8 +17,33 @@ function nodeapi_example_install() {
+  $availableContentTypes = node_type_get_types();
+  $availableContentTypes = array_keys($availableContentTypes);
+
+  foreach($availableContentTypes as $contentType) {
+    variable_del('nodeapi_example_' . $contentType);
+  }

1) This code can be cleaned and shortened into:

foreach (node_type_get_types() as $name => $type) {

...whereas $name is the type name and $type is a node type object.

2) Missing space after foreach.

3) This uninstallation code only takes current node types into account. Does this nodeapi_example module implement the Node Content Type API hook to delete a potentially existing variable when a content type is deleted? (which can happen anytime)

So that being said, the wildcard variable delete approach is valid, at the very least, if your module stores data on behalf of other modules/factors that do not provide a clean CRUD/API to interact with, so your module might never ever have a chance to clean up the stuff it stored. Shit happens.

Powered by Dreditor.

I think we need a wildcard deletion here, because the admin could delete a node type that we have a variable for, and it would remain in the variable table unto eternity, even after we uninstall.

I'd wager this is also a problem in other examples, as well. Who volunteers to look? :-)

See also: #93528: Improve variable_del() by adding another function: variable_del_prefix()

Actually, the approach here is not a good example. In this case it's unlikely to fail, but there are many modules that can accidentally delete other module's variables.

The deletion should be based on an explicit list.

Well that would mean reworking the module so it stores a list of which node types to rate, rather than relying on a variable naming convention. Also a demonstration of how to cache that variable for the session. You volunteering? :-)

Hmm.

As you know, the problem is that one module's uninstall can delete another module's variables. I saw this actually happen once. An example would be amazon module cleaning up its variables... and accidentally deleting amazon_store's variables.

Can you think of an authoritative way we can query the variables in this case without making that mistake?

One way to do it might be to have a hash of the module name instead of the module name. So b8402bd1149e4f204fb7fd0ad733c59c_NODETYPE if it's md5. Utterly unreadable.... so perhaps the first five chars, plus module name as self-documentation: b8402_example_nodeapi_NODETYPE.

For the Amazon examples it would be: 2d0d4_amazon_stuff and 9681d_amazon_store_stuff. That's unique enough to prevent the problem.

So if the spec is 'create an over-designed best-practice for naming your variables,' then here's a patch with a solution. Drupal really should give us a way to do this.

Sorry. I think I'm being annoying.

I do not want to change the variable naming convention. I just want to adhere to best practice of not accidentally deleting somebody else's variables. I'll take a look at this in a little bit and see if I can come up with a reasonable solution.

Wow that applied REEEEEELY cleanly. :-)

Let's try again (again).

There's no way around it other than to change the naming convention, or to change the way the variables are stored. Alternately there could be another table, but variables are so handy and already in the cache.

Also I think my solution is pretty reasonable. :-)

Really sorry, because I'm not trying to make you jump through hoops. It's important to keep this as simple as possible, and introducing hashes won't do that.

Could we compromise on your #6 with a variable naming scheme of nodeapi_example_node_type_$nodetype ? That way a module would have to exist with the name nodeapi_example_node_type (probably) to have a collision, and it's unlikely.

Then just a little note in the uninstall explaining that querying the table is unusual, but we have to do it here because we have no record of what variables have been created, and that a normal module would just do a variable_del on variables it knows it's created. (block_example_uninstall() render_example_uninstall() and image_example_uninstall() are examples of the more normal case)

So in this code, it means changing a form element name to nodeapi_example_node_type, which is not descriptive of the form element. Swap it out for something like nodeapi_example_nodetype_settings and it works both ways, but it's still awkward.

I'll let you do the honors. :-)

OK, ever so sorry. This is built on #6 with just a little bit of fudging here are there.

And I really didn't understand the problem here (dynamically created variables) when I made my first comment, or perhaps my responses would have been more intelligent.

+++ b/nodeapi_example/nodeapi_example.install
@@ -7,6 +7,8 @@
+ *
+ * @ingroup nodeapi_example

This section has already been added by the commit for issue #767204: API.drupal.org: Rework Doxygen comments so people can navigate the comments, so the patch doesn't apply.

So I couldn't test it. Otherwise this patch looks good to me. Just a couple of minor issues with the comments.

+++ b/nodeapi_example/nodeapi_example.install
@@ -42,3 +44,39 @@ function nodeapi_example_schema() {
+ * Our implementation of nodeapi_example_form_alter() automatically
+ * created a nodeapi_example_node_type_<contentType> variable
+ * for each node type the user wants to rate.
+ *
+ * To delete our variables we call variable_del for our variables'
+ * namespace, 'nodeapi_example_'. Note that an average module would have

I wonder if the namespace is not actually 'nodeapi_example_node_type_', not just 'nodeapi_example_' as it appears here.

+++ b/nodeapi_example/nodeapi_example.install
@@ -42,3 +44,39 @@ function nodeapi_example_schema() {
+ * not when disabling a module. This allows our data to say in the

Typo. 'This allows our data to STAY in the database...'

If someone can confirm my namespace issue in #19, I'll re-roll the patch.

Yes, you're correct, the namespace should be 'nodeapi_example_node_type_'. Thanks for the re-roll. :-)

Here's the patch.

I checked this out and it seems to work fine. The testbot likes it. I'll commit tomorrow unless someone says there's a reason not to.

Thanks so much for your work on this.

Pushed to both 7.x-1.x and 8.x-1.x.