I am attempting to make the best hook_uninstall ever :D Sorry in advance for the long post.

As is the case with a lot of modules, mine makes many variables per node type it is enabled for. It would be tiresome and inefficient to delete every one of these variables with variable_del.

A common solution that appears to have its roots in Drupal 6 is db_query("DELETE FROM {variable} WHERE name LIKE 'mymodule_%'");
Or the Drupal 7 converted version, which is technically incorrect, being db_delete('variable')->condition('name', 'mymodule_%', 'LIKE')->execute();
Or the even better and correct D7 version db_delete('variable')->condition('name', db_like('mymodule_') . '%', 'LIKE')->execute();

This doesn't fully replicate variable_del though, as variable_del also flushes the cache and removes the variables from the global $conf array. Clearing the cache is simple, just add the same bit of code cache_clear_all('variables', 'cache_bootstrap');

Clearing the variables from the $conf array is a bit trickier, and there are a few methods that I can see.

  1. We could do a db_select before the db_delete to obtain a list of keys to remove from the $conf array. This involves 2 db queries, so probably not the most efficient (unless there is a way to combine the select and delete into 1 that I am not aware of?)
  2. Search through the $conf array for keys that start with 'mymodule_', much the same as the db like query. I tested a few different methods for this, which gave varying results (timed with microtime() and displayed as difference*1000).
    1.  $conf = array_intersect_key($conf, array_flip(preg_grep("/^mymodule_.*/", array_keys($conf), PREG_GREP_INVERT)));
      // Result 0.41699409484863 
    2. foreach($conf as $key => $value) {
        if (substr($key, 0, strlen('mymodule_')) === 'mymodule_') {
          unset($conf[$key]);
        }
      } // Result 1.2187957763672
      
    3. foreach($conf as $key => $value) {
        if (0 === strpos($key, 'mymodule_')) {
          unset($conf[$key]);
        }
      } // Result 0.69212913513184
      
    4. foreach($conf as $key => $value) {
        if (preg_match("/^mymodule_.*/", $key) === 1) {
          unset($conf[$key]);
        }
      } // Result 1.2669563293457
      

    So 2.1 is the fastest, but it does a lot. 2.3 is 50% slower, but is a lot easier to understand.

So, here is my final code (using 2.1). My questions to those smarter than me are: will it correctly modify the global $conf (or does it need to be @$conf of something?), is it the best method, and do you have any other comments?

global $conf;
$module_string = "mymodule_";

db_delete('variable')->condition('name', db_like($module_string) . '%', 'LIKE')->execute();
cache_clear_all('variables', 'cache_bootstrap');

$conf = array_intersect_key($conf, array_flip(preg_grep("/^{$module_string}.*/", array_keys($conf), PREG_GREP_INVERT)));

Comments

jaypan’s picture

Personally, I would use variable_del, and set up a few loops to handle the relevant elements. It's not good practice to bypass the APIs when they exist.


Contact me to contract me for D7 -> D10/11 migrations.
Or anything really. I'm friendly, and open to work or conversation.
stefan lehmann’s picture

remember that variables can hold arrays as well. So it's not necessary to have 1 variable for 1 value.

neRok’s picture

True, but my module hooks into the node type edit form, which saves each setting field you add to the form as an individual setting, suffixed with the node type. See function node_type_form_submit. So my 10xsettings per node type could easily be 40 settings. Doing a variable_del 40 times is a lot of database calls.

  $variables = $form_state['values'];

  // Remove everything that's been saved already - whatever's left is assumed
  // to be a persistent variable.
  foreach ($variables as $key => $value) {
    if (isset($type->$key)) {
      unset($variables[$key]);
    }
  }

  unset($variables['form_token'], $variables['op'], $variables['submit'], $variables['delete'], $variables['reset'], $variables['form_id'], $variables['form_build_id']);

  // Save or reset persistent variable values.
  foreach ($variables as $key => $value) {
    $variable_new = $key . '_' . $type->type;
    $variable_old = $key . '_' . $type->old_type;

    if (is_array($value)) {
      $value = array_keys(array_filter($value));
    }
    variable_set($variable_new, $value);

    if ($variable_new != $variable_old) {
      variable_del($variable_old);
    }
  } 
jaypan’s picture

Doing a variable_del 40 times is a lot of database calls.

Sure, but it's a page that will only ever be run once on a given server. The number of database calls per page on pages that are regularly accessed is something that should always be considered, but for a page like this, run by an admin one time, it's better to prioritize the APIs over the page execution time. You want to keep your system clean, and by bypassing the APIs, you may be orphaning elements in your system that are deleted by the API, but missed by you.


Contact me to contract me for D7 -> D10/11 migrations.
Or anything really. I'm friendly, and open to work or conversation.
neRok’s picture

OK, you've twisted my arm enough!

What would be the best way to populate a list of variables?

  1. Get a list of all node types, 'ping' 1 variable for each type and if it exists just presume the rest do for that node type and variable_del them all.
  2. db_select to get a list of variables, then variable_del them.
  3. Read global $conf and get a list of variables from it, then variable_del them.
joshi.rohit100’s picture

Hey @neRok, @jaypan is right. If you have apis and wrappers, then use them. Otherwise what is the use of CMS.

pmichelazzo’s picture

Why not just few lines using the API correctly?

  // Remove all variables with some prefix
  $sql = db_query('SELECT name 
                  FROM variable 
                  WHERE name LIKE :prefix',
                  array(':chlt' => 'prefix_%'))->fetchCol();
  $count = count($sql);
  if ($count != 0) {
    for ($i = 0; $i <= $count; $i++) {
      variable_del($sql[$i]);
    }
  }
cosolom’s picture

I found next way

/**
 * Implements hook_uninstall().
 */
function forward_uninstall() {
  // Remove variables
  global $conf;
  foreach (array_keys($conf) as $key) {
    // Find variables that have the module prefix
    if (strpos($key, 'forward_') === 0) {
      variable_del($key);
    }
  }
}
jaypan’s picture

That's somewhat dangerous - if any other modules happen to use forward_ as the start of their variable, you will delete those as well.


Contact me to contract me for D7 -> D10/11 migrations.
Or anything really. I'm friendly, and open to work or conversation.
cosolom’s picture

It's bad practice naming variables not started from module name. From coding standards:

Persistent variables (variables/settings defined using Drupal's variable_get()/variable_set() functions) should be named using all lowercase letters, and words should be separated with an underscore. They should use the grouping/module name as a prefix, to avoid name collisions between modules.

Somewhat dangerous maybe only if will be other module started from "forward_". For example, forward_node, etc.

jaypan’s picture

It's bad practice naming variables not started from module name.

I don't disagree, but not everyone realizes/follows best standards, and as you say, another module may also start with forward.


Contact me to contract me for D7 -> D10/11 migrations.
Or anything really. I'm friendly, and open to work or conversation.