Mistake in hook_uninstall

Clearly it's a mistake not to flush the cache.
While some developers prefer the API variable_del() which is not supported in Drupal 8, I think db_del() makes it easy to maintain the .install file.
Thanks.

Makes sense to me. I wonder if we could just clean these up by doing the variable_delete instead of a direct query to the variables table. Direct queries might hit variables that are not well named and are not related to this module.

I thought there were maybe more variables that might exist depending on the content types that are on a site - is that not true?

+++ b/comment_notify.inc
@@ -325,14 +325,3 @@ function comment_notify_variable_registry_get($name) {
-function comment_notify_variable_registry_set($name, $value) {
-  return variable_set("comment_notify_" . $name, $value);
-}

Good spot @eugene.ilyin, you're right the function is not used anywhere.

@greggles I think you're referring to comment_anonymous_article, and comment_anonymous_' . $type

I hope the second one doesn't require much work.

Yes, that's right, things like comment_anonymous_' . $type will need some more advanced effort.

Without any testing I imagine it is something like this:

$node_types = node_type_get_names();
$types = variable_get('comment_notify_node_types', array());

foreach ($types as $type => $name) {
  if (in_array($type, $node_types)) {
    $vars[] = comment_anonymous_' . $type;
  }
}

Then we add $vars to our $variables array. Not sure though.

Without checking what comment_notify_node_types() holds, I think it should be:

$node_types = node_type_get_names();
$types = variable_get('comment_notify_node_types', array());

foreach ($types as $type => $name) {
  if (array_key_exists($type, $node_types)) {
    $vars[] = 'comment_anonymous_' . $type;
  }
}

Hi Eugene,

I can confirm the variable 'comment_anonymous_' . $type is set in the module file, please refer to the
function comment_notify_settings() and you will notice it returns system_settings_form($form).
Hence, it saves the values given to variable_get() for the checkbox which I believe is assigned default values.

The variables are created for all content types except the article. Even if comment notify is not enabled for a content type the variable is still created against that content type; it is a boolean for the check-box.

So, I think we can get all the content types minus the article content type. Then we can add them to the existing $variables array or we can do variable_delete('comment_anonymous_' . $type) in a separate loop.

Thanks,

Yousif

Setting this to needs work. You may want to assign the issue to yourself until you supply an updated patch and don't hesitate to ask I'm sure Greg also can be helpful.

Variable comment_anonymous_* used in module comment. I don't think that we can remove it in module comment_notify.
Also you can check that this variable deleted in function comment_uninstall()

You're perfectly right, I had another look and they are set by the comment module which should be responsible for removing them.

For me this is RTBC, however, let's have Greg's input.

We should not delete the variables provided by the core comment module, hence #6 looks good to me.

Got it, thanks for the work folks!