Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hello
I've found mistake in functionality for delete variables in hook_uninstall().
Variables of module comment_notify doesn't removed after uninstallation.
Comment | File | Size | Author |
---|---|---|---|
#6 | comment_notify-mistake-in-hook_uninstall-2576139-6-D7.patch | 1.71 KB | eugene.ilyin |
#2 | comment_notify-mistake-in-hook_uninstall-2576139-2-D7.patch | 513 bytes | eugene.ilyin |
Comments
Comment #2
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and commentedI've prepared patch to fix this bug.
Thank you
Comment #3
drupalove CreditAttribution: drupalove as a volunteer commentedClearly 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.
Comment #4
gregglesMakes 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.
Comment #5
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and commentedOk, I'll prepare new patch soon.
Comment #6
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedDone. Also I propose to remove function comment_notify_variable_registry_set, because it isn't used.
Comment #7
gregglesI thought there were maybe more variables that might exist depending on the content types that are on a site - is that not true?
Comment #8
drupalove CreditAttribution: drupalove as a volunteer commentedGood 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.
Comment #9
gregglesYes, that's right, things like
comment_anonymous_' . $type
will need some more advanced effort.Comment #10
drupalove CreditAttribution: drupalove as a volunteer commentedWithout any testing I imagine it is something like this:
Then we add $vars to our $variables array. Not sure though.
Comment #11
drupalove CreditAttribution: drupalove as a volunteer commentedWithout checking what comment_notify_node_types() holds, I think it should be:
Comment #12
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedMhh, 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.
Comment #13
drupalove CreditAttribution: drupalove as a volunteer commentedHi 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
Comment #14
drupalove CreditAttribution: drupalove as a volunteer commentedSetting 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.
Comment #15
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedMhh
Do you mean this element?
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()
Comment #16
drupalove CreditAttribution: drupalove as a volunteer commentedYou'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.
Comment #17
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedAre there news?
Comment #18
drupalove CreditAttribution: drupalove as a volunteer commentedWe should not delete the variables provided by the core comment module, hence #6 looks good to me.
Comment #19
gregglesGot it, thanks for the work folks!