We should remove *all* variables when unistalling, based on avpoll_*, instead of specific variable names.

CommentFileSizeAuthor
#6 advpoll_del_valiables.patch1.33 KBanders.fajerson
#4 advpoll_del_variables.patch1.39 KBanders.fajerson

Comments

ChrisKennedy’s picture

ChrisKennedy commented

What's your reasoning?

ChrisKennedy’s picture

ChrisKennedy commented

Also, what do other good modules do?

anders.fajerson’s picture

anders.fajerson commented

The reasonig was mainly that we didn't have to remember including variables to the install file every time we added them, and that it would be shorter code. We have quite a few variables now. But, on the other hand, variables is not added that often so we might as well write them out.

The main point though is that we should remove all variables.

anders.fajerson’s picture

anders.fajerson commented
Status: Active » Needs review
StatusFileSize
new advpoll_del_variables.patch1.39 KB

Here is a patch with uses variable_del(). The other option is to simply use db_query("DELETE FROM {variable} WHERE name LIKE 'advpoll_%'"); (used by fivestar). I'm not sure which I prefer, but I'm leaning towards the approach used in this patch.

ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs review » Needs work

Ah now I see what you mean about all the content type vars.

I also prefer using variable_del().

This patch sounds good, I would just abstract out 'advpoll_' because it's at the beginning of every content type variable and will be in any new variable we add. It'll make the list a lot shorter too.

anders.fajerson’s picture

anders.fajerson commented
Status: Needs work » Needs review
StatusFileSize
new advpoll_del_valiables.patch1.33 KB

Yes, of course. New patch with shorter list.

ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs review » Reviewed & tested by the community

woot

anders.fajerson’s picture

anders.fajerson commented
Title: Remove all variable on unistall » Remove all variables on unistall
Status: Reviewed & tested by the community » Fixed

Thanks. Fixed: http://drupal.org/cvs?commit=89780

anders.fajerson’s picture

anders.fajerson commented
Title: Remove all variables on unistall » Remove all variables on uninstall
Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)

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