Allow multiple variable deletes in variable_del()

Here is the patch.

I took cache_clear_all() out of the loop, though: I don't see what the point is in keeping it within.

ths is much needed - variable_del is quite expensive we definately should batch them like this. you can rewrite so we only execute one delete query WHERE name IN (x,.z,...).

Here is the patch rerolled with just one query.

I seem to remember someone mentioning an issue with security an SELECT .. IN queries, though, but can't remember what it was.

well done.

for symetry with variable_get(), i think variable_del() should also handle a string as a param. just check is_string($names) and if true, $names = array($names).

@moshe: The code to handle a string is already in the patch ...

While we're at it, why not go further and add array syntax to the other two functions:

variable_set(array(VAR_FOO, VAR_BAR), array($foo, $bar));
// ....
list($foo, $bar) = variable_get(array(VAR_FOO, VAR_BAR), array(DEF_FOO, DEF_BAR));
// ...
variable_del(array(VAR_FOO, VAR_BAR);

In each of these cases, the existing syntax using just strings can be maintained, so existing code would not be affected, but new code could take advantage of this mechanism to reduce the overall number of queries.

With variable_get using a static array, changing it would just be a matter of symmetry, not performance, but variable_set would also benefit from this change by reducing the number of queries being submitted.

Nice idea, but we're far past code-freeze at this point.

This still works against head, although there is some offset.

$ patch -p0 < bootstrap.inc__6.patch
(Stripping trailing CRs from patch.)
patching file includes/bootstrap.inc
Hunk #1 succeeded at 474 (offset 5 lines).

Rerolled against today's HEAD.

Should I add equivalent code to variable_set / variable_get ?

Regarding patch in #9, the implode within the query should be db_placeholders instead.

I would update the patch, but I don't have my dev environment setup right now.

I would also advise against using the array syntax for variable_get(). It's just very confusing, and not really necessary due to caching.

For variable_set(), if you wanted to use an array syntax for performance I would recommend using an associative array format:

variable_set(array('var_a' => 'value a', 'var_b' => array('value_b1', 'value_b2')));

New patch version replaces the manual expansion by db_placeholders as per #11. Thanks for the suggestion.

Added some vertical spacing and reworded the comments as per #10.

I encourage you to add support for variable_set() and not add for variable_get(). multiple variable_set() calls have same problem as delete - a cache clear happens for each one.

Regarding variable_set, a decision has to be taken regarding the API. There are basically at least three choices:

1. maintain a strict compatibility with the scalar syntax, by passing parallel arrays like variable_set(array(VAR1, VAR2, VAR3), array(VAL1, VAL2, VAL3)). This is a natural extension of the scalar syntax: variable_set(VAR1, VAL1)
2. change for a more natural syntax, like variable_set(array(VAR1 => VAL1, VAR2 => VAL2, VAR3 => VAL3)), as suggested by Crell
3. extend the scalar syntax with a variable-length argument list, like variable_set(VAR1, VAL1, VAR2, VAL2, VAR3, VAL3)
4. possibly other variations (which ones ?)

The second syntax is probably simpler and a better overall fit with most uses of the function, but it is really unnatural and error-prone due to the difference it exhibits with the scalar case. Which would be better ?

#2 is by far easiest for humans to parse. It also matches well with syntax in other areas like t(), FAPI, Menu system....

I would be very -1 to any of the other proposed changes.

I agree with #2 - and I would be happy to write a regular expression to change all instances to use it.

In the mean time, I changed around variable_del to add a little safeguard for un-necessary queries, and I updated all calls to variable_del to use this new syntax.

Just to clarify, you can quite easily support both old and new style variable_set() syntax, and both should be kept. The array syntax is nice if you are doing multiple operations, while the traditional syntax is frequently all you need.

function variable_set($vars, $val = NULL) {
  if (!is_array($vars)) {
    $vars = array($vars => $val);
  }
  foreach ($vars as $var => $val) {
    // Write to the database.
  }
  cache_clear_all() // and other cleanup stuff.
}

variable_set('foo', 'bar');

variable_set(array('foo' => 'bar', 'baz' => 'boing'));

subscribing

I'd be very interested in variable_del() wildcard functionality. For modules with a lot of configuration settings it's easy to miss a variable or forget to add it to the uninstall function.

Cheers,
Stella

For variable_set(), I've split that off to a separate issue: #317930: Allow multiple values in a single variable_set() call

If some form of hook_variables gets in -- #145164: DX: Use hook_variable_info to declare variables and defaults then we can use hook_modules_uninstalled() to clear variables and module developers will no longer need to clear their own variables during hook_uninstall().

I would prefer to see concentration on hook_variables() than a wildcard delete. Because with hook_variables, you can then just call the variables hook, get the array, and run a DBTNG delete statement if you need to batch delete vars outside of the uninstall.

:-/ THIS IS VERY VERY VERY STUPID IDEA!
Your bunny wrote!

The only real useful usage of this proposal is when we delete many variables at uninstall module process. Another using isn't important in life.

At first, there is not symmetric with variable_set and variable_get. Using arrays in parameters of variable_set and variable_get is incorrect: not intuitive and confuse lumers like me. Pandora's box.

All yours variables must have suffix with module name for escape namespace conflict:

your_module_name_option1
your_module_name_option2
your_module_name_option3

As you suggest if we want delete them we must enumerate ALL variables in parameters:

$variables = array(
'your_module_name_option1'
'your_module_name_option2'
'your_module_name_option3'
);
variable_del($variables) {

Code is swelled. It's too many lines (mnogo bukav).

If all variables begins from 'your_module_name' so we can delete them just make query

  db_query("DELETE FROM {variable} WHERE name LIKE '%s'", $name);

This solution is very well implemented in #93528: Improve variable_del() by adding another function: variable_del_prefix().

   variable_del('your_module_name_', TRUE) {

It's short and beautiful!

Patch #93528: Improve variable_del() by adding another function: variable_del_prefix() is more powerful, simplest, quickly, and don't require change another functions.

P.S.

Otherwise if you have many variables you can just do union their to the object an write to one variable:

  $options = new stdObject();
  $options->option1 = 211;
  $options->option2 = 211;
  $options->option3 = 211;
  $options->option4 = 211;
  variable_set('module_name_options', $options);

KISS!

Variable registry is at #145164: DX: Use hook_variable_info to declare variables and defaults. Needs some help. :)

While the note up at #24 makes some really good points, I thought I would go ahead and re-apply this patch to head. I caught one syntax error (didn't close an array correctly) while fixing the amazing amount of fuzz in the most recent patch. It now applies cleanly.

The code is simply taking the variable_del() function and making it handle (and expect?) arrays for deletion. After going through and reading every line and fixing the patch where it was broken, it almost always seemed to add about three or four lines of code, but it made long recurring statments of "variable_del" ancient history. A lot of modules got around this problem by implementing foreach loops.

I'm not absolutely positive about the DBTNG syntax for the "IN" clause, but I tried my best. Perhaps the test bot will tell us more.

Delete queries should always use the query builder. I think what you're after is:

db_delete('variable')
  ->condition('name', $delete, 'IN')
  ->execute();

I'm not sure about making the array form required. Available, sure, but like my earlier variable_set() multi-value patch the array can be an extra complication when you don't need it.

Crell! Thanks for the tips and the DBTNG magic -- I looked for documentation on using things like "IN" but to no avail. Well, actually I was trying to figure out the syntax to use for the condition function. api.d.o wasn't much help in that department.

I have made your suggestions and re-rolled the patch. Arrays are not required. If you hand it a simple string, it will deal with it just fine.

Josh

I've got some ideas on how to really clean this patch up. And apparently I need to actually run an *AMP stack and make sure the PHP syntax doesn't fail... ;-)

Crell - guess that was a bad use of a status?

Attached is a newly optimized patch that good rid of a lot of the unnecessary changes to core modules. As I said before, the function does not require an array for it to work. Thus, I removed all instances where the previous programmer added extra array()s around single values. I think it cut the patch down considerably.

Hopefully the community will see this as a very simple addition to a very simple function. It's just deleting variables, but as you can see when you look at the patch, there was a very a real need for it to handle multiple values.

For the robot who will be checking this for me -- I guarantee that I could not find ANY php invalid syntax in the patch. Though, even after installing MAMP, setting up a database and going through a Drupal install, I still managed to miss syntax issue.

Question: Should I write a simpletest for this additional functionality? Where do I go to learn how to write a simpletest?

changing the status...

@#36: I'm fully in favor of this patch. For unit tests, have a look at the forked issue: #317930: Allow multiple values in a single variable_set() call. That one is about variable_set(), but includes some unit tests. You can use that as a model. Yes we do want unit tests for this. :-)

@38 - fixed.
@39 - Your testing patch and the folks in #drupal helped me get the test function written.

Everything in this release has not been tested, including the new test function, because, while I can run php scripts with XAMPP and cygwin, I cannot seem to get any version of D7 to install without crashing apache. (another issue, i'm sure) I've also added the Performance tag to this issue, as this will increase performance in some fashion. I believe less function calls can result in all kinds of performance increases.

The new test function looks like thus:

  /**
   * Makes sure that we can delete an array of variables.
   */
  testVariableDeleteMultiple() {
    // Setup
    $numberToTest = rand(2,15);
    
    // Create associative array for testing
    for ($i = 1; $i <= $numberToTest; $i++) {
      $deleteMe[$i] = $this->randomName(6,'variable_del_');
      // variables to delete
      variable_set($deleteMe[$i], $this->randomName(2));
    }
    
    // Delete the variables
    variable_del($deleteMe);
    
    // Assert that they have been deleted
    for ($i = 1; $i <= $numberToTest; $i++) {
      // Variable_get() should return NULL on non-existant variables
      $this->assertNull(variable_get($deleteMe[$i]), t('Deleting multiple values: Deletion successful for test variable #'.$i));
    }
    
  }

whoops... forgot the "function" declaration...

Man, I need to get an IDE so these kind of errors are easier to find...

I discussed with Josh on #drupal:

- I'm not sure this patch leads to any signifiant performance gain (the cache is cleared several times, but that's a quite cheap operation... we don't rebuilt it in that request)
- the only point is DX, and based on PHP syntax unset($var1, $var2), we should support variable_del('var1', 'var2')

Thanks Damien... It is definitely more about DX than performance.

I can see the argument for following unset(). However, there's a sister issue for variable_set (#317930: Allow multiple values in a single variable_set() call) for which an array is the only logical form. So we may want to follow that pattern as well.

Arguably I suppose we could support both forms without too much trouble. I'm open to that if others don't think that would cause too much confusion.

Yup.

Is there really any reason to reject either syntax ? After, it is just /extremely/ simple to improve DX by accepting both with something like:

foreach (func_get_args() as $name) {
  $to_delete = array();
  if (is array($name)) {
    $to_delete += $name;
  }
  else {
    $to_delete[] = $name;
  }
  // and from there do the actual deletion

We've had this in db_query() for years until DBTNG, and I don't think this was actually a problem. Of course PHP-level signature homogeneity would favor vdel(x, y, ..;), but overall Drupal signature homogeneity is screaming for arrays. And vdel(array(x, y, ...)) is not /that/ different from vdel(x, y, ...) but more consistent with Drupal in most respects.

And this little extra code handles a healthy (or not) mix of arrays and vars, not just for the first argument. It's a bit like PHP fundamental choices: if you can make sense of something the coder wrote, do so. More rigor may be valuable, and targeted by other languages, just not this one. Drupal is not written in Java or C#, why not got with the spirit of the language it was created in instead of fighting it ?

Arguably I suppose we could support both forms without too much trouble. I'm open to that if others don't think that would cause too much confusion.

I'm actually against supporting the unset($var1, $var2, $var3) syntax. It becomes very unconventional in the cases where you actually do want to pass it an array. In addition, func_get_args() does cause a slight performance hit, and gives us the same result anyway.

For example, check this out:

// With the unset() syntax
$variablesToDelete = module_invoke_all('delete_variables');
call_user_func_array('variable_del', $variablesToDelete); // EW WTF BBQ OMG

// With the array vs string syntax
$variablesToDelete = module_invoke_all('delete_variables');
variable_del($variablesToDelete);

The majority of core does not support this syntax, so we should probably stick with the array vs string argument.

see also #975118: Allow variable functions (variable_del, variable_get, variable_set) to receive an array of name/value pairs

here is patch from #975118: Allow variable functions (variable_del, variable_get, variable_set) to receive an array of name/value pairs

Issue #987768: [PP-1] Optimize variable caching is being developed to allow incremental variable caching. If that patch hits, it will require changes to the patches proposed here. I hope though that the multiple variable set()/del() functionality will be applied soon thereafter.

Reading through all of the comments above, it appears that the desired calling patterns should be:

* variable_set(string, $value) or variable_set(array)
* variable_del(string) or variable_del(array)

Is this correct? Or does it make sense to introduce plural functions variables_set(array) and variables_get(array) to handle bulk operations and leave the current singular format of variable_set()/get()/del() as is?

My hope is that we can reach agreement on what we want the function profiles to be and then quickly can implement an updated patch once #987768: [PP-1] Optimize variable caching has landed.

For reference, it appears that many agree that bulk variable operations are needed as indicated by the following issues:
#93528: Improve variable_del() by adding another function: variable_del_prefix()
#317930: Allow multiple values in a single variable_set() call
#975118: Allow variable functions (variable_del, variable_get, variable_set) to receive an array of name/value pairs

Adjusting the title and issue tags

@Berdir over in comment #20 of #317930: Allow multiple values in a single variable_set() call suggests:

I think using separate functions sounds like a good idea, we've been trying to do similar things elsewhere too.

However, my naming suggestion would be variable_(get|set|del)_multiple(), then it is very similar to entity load functions for example and IMHO easier to find than with a different prefix.

Just like node_load() and node_load_multiple(), variable_get() could then simply be a wrapper function around variable_get_multiple().

This is a much better suggestion than my variables_set(array) and variables_del(array) functions. The resulting function profiles would be:

* variable_get(string, default)
* variable_get_multiple(array)

* variable_set(string, value)
* variable_set_multiple(array)

* variable_del(string)
* variable_del_multiple(array)

There would be no need for a variable helper function to bulk load from data base to static cache as that now would be covered by variable_get_multiple().

In keeping with recent Drupal trend not to abbreviate, should the variable_del() functions be renamed to variable_delete()?

Thoughts?

Let's keep having multiple variable_set() and multiple variable_del() as separate issues to keep the patches small. Larry originally created #317930: Allow multiple values in a single variable_set() call with that intent at #20.

I have no problem with keeping the patches small by having a separate issues for variable_get()/variable_set()/variable_del(). My hope is that we can agree on what the function profiles should be and then get the patches promptly done once #987768: [PP-1] Optimize variable caching lands.

Does it make sense to rename the title to get that focus and then change to variable_del() once consensus is reached?

• Removed the variable_set stuff
• Scoped the test in its own function
• Uses variable_del(array) in all hook_uninstall calls

We should probably hit #987768: [PP-1] Optimize variable caching first, but let's see what testing bot says.

+++ b/modules/aggregator/aggregator.install
@@ -9,14 +9,16 @@
+    'aggregator_clear'
+    'aggregator_category_selector'
+    'aggregator_fetcher'
+    'aggregator_parser'
+    'aggregator_processors'

commas?

Powered by Dreditor.

Good catch.

variable_* functions will not exist anymore in D8