Hello

I've found mistake in functionality for delete variables in hook_uninstall().
Variables of module comment_notify doesn't removed after uninstallation.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eugene.ilyin created an issue. See original summary.

eugene.ilyin’s picture

Status: Active » Needs review
FileSize
513 bytes

I've prepared patch to fix this bug.
Thank you

drupalove’s picture

Status: Needs review » Reviewed & tested by the community

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.

greggles’s picture

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.

eugene.ilyin’s picture

Ok, I'll prepare new patch soon.

eugene.ilyin’s picture

Done. Also I propose to remove function comment_notify_variable_registry_set, because it isn't used.

greggles’s picture

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

drupalove’s picture

+++ 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.

greggles’s picture

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

drupalove’s picture

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.

drupalove’s picture

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;
  }
}
eugene.ilyin’s picture

Mhh, I don't understand.

As I see the variable "comment_anonymous_*" isn't set in module comment_notify (only in test). And we shouldn't remove it.

comment_notify_node_types holds something like this "a:2:{s:7:"article";s:7:"article";s:4:"page";s:4:"page";}"

So, I think that my patch is correct and not requires additional efforts.

Please let me know if I'm not right.

drupalove’s picture

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

drupalove’s picture

Status: Reviewed & tested by the community » Needs work

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.

eugene.ilyin’s picture

Status: Needs work » Needs review

Mhh

Do you mean this element?

$form['comment_notify_settings']['comment_notify_node_types'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Content types to enable for comment notification'),
    '#default_value' => $enabled_types,
    '#options' => $checkboxes,
    '#description' => t('Comments on content types enabled here will have the option of comment notification.'),
  );

As I understand the $checkboxes is just options, but the real variable name is comment_notify_node_types.

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()

drupalove’s picture

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.

eugene.ilyin’s picture

Are there news?

drupalove’s picture

Status: Needs review » Reviewed & tested by the community

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

greggles’s picture

Status: Reviewed & tested by the community » Fixed

Got it, thanks for the work folks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.