I was working on the Twitter OAuth task and had to install/uninstall the module a number of times to clean up things for clean testing. I noticed that the variables weren't being deleted on uninstall.

The attached patch does this. I'm pretty sure I got all the variables in the array.

CommentFileSizeAuthor
#10 493766_uninstall.patch424 bytessteinmb
#7 twitter-493766.patch815 bytessteinmb
twitter_install.patch980 bytesgold

Comments

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Needs work

Rather than listing all the possible variables, it's enough to use a query like DELETE FROM {variables} WHERE name LIKE 'twitter\_%' because all variable names start with the same substring.

gold’s picture

gold
they/them
Location 20 minutes in the future
commented
Status: Needs work » Active

This would have the potential to clash with anyone else that is creating variables that start with "twitter_".

Also, it would miss the oauth_twitter_% vars that are being used in the current twitter/oauth patch.

Wildcards like this in a uninstall would be very risky.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

I didn't notice any variable starting with oauth_, but if they are used in a proposed patch not yet applied to the module code, then the patch must be changed. Drupal variables used from a module must be prefixed with the name of the module; the only modules that don't follow that role are the Drupal core modules.

jergason’s picture

jergason commented

Any solutions in sight for this?

john franklin’s picture

john franklin commented

It doesn't appear to clean up permissions on uninstall either.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: clean up variables on uninstall » Clean up variables on uninstall
Status: Active » Needs work

The patch should delete Drupal variables using variable_del(), which is what is done by Drupal core modules.
In the case the code is using an SQL query, then it should probably use a query like DELETE FROM {variable} WHERE name IN ([list]), where [list] is the list of variable names separated by a comma.

steinmb’s picture

steinmb commented
Version: 6.x-2.x-dev » 6.x-3.x-dev
Priority: Minor » Normal
Status: Needs work » Needs review
StatusFileSize
new twitter-493766.patch815 bytes

We are still not cleaning up these variables on uninstall. This should also clean out variables from users that upgraded from 6.x.2 then non of the twitter_update() cleaned these out during the upgrade. This patch should clean stuff up.

juampynr’s picture

juampynr commented
Status: Needs review » Fixed

Applied a mix of the suggestions with variable_get and 'delete from' on the Drupal 6 version:

http://drupalcode.org/project/twitter.git/commitdiff/41d3e100fb8e9a97d5d...

In the Drupal 7 version, I experienced an unexpected behavior when I uninstalled twitter module: hook twitter_uninstall was not called. It seems that, because there are dependent modules such as twitter_post or twitter_signin, these need to be uninstalled too in order to make it work. There is more info about this here: http://drupal.org/node/123825. Below is the commit diff:

http://drupalcode.org/project/twitter.git/commit/03c9355f5b7bfdd66012395...

Cheers everyone for the help.

Status: Fixed » Closed (fixed)

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

steinmb’s picture

steinmb commented
Status: Closed (fixed) » Needs work
StatusFileSize
new 493766_uninstall.patch424 bytes

Reopening. Should we not also remove the twitter accounts?

juampynr’s picture

juampynr commented
Status: Needs work » Closed (fixed)

Tables defined in hook_schema are removed automatically in Drupal 7.

See http://api.drupal.org/api/drupal/modules--system--system.api.php/functio....