Allow multiple values in a single variable_set() call
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Crell |
| Status: | patch (code needs work) |
Jump to:
This is a fork off of #138544: Use variable_* with an array, which has drifted to become concerned mostly with variable_del(). As discussed there, this patch enhances variable_set() to also accept an array of values to set, which will be done in a single batch operation. The primary advantage of doing so is that it only clears the cache once, so it's a bit faster if you're setting a lot of variables. The previous syntax is unaffected and still works exactly as before.
The only place it's likely to make a huge difference is on system_settings_form() pages, but I went through and converted core anyway. I skipped some unit tests because, well, I don't think it really makes a difference there. :-)
Oh yeah, and the unit tests for variables have been updated to test both syntaxes, and the tests pass. :-)
| Attachment | Size |
|---|---|
| variable_set.patch | 16.45 KB |
| Testbed results | ||
|---|---|---|
| variable_set.patch | failed | Failed: Failed to apply patch. Detailed results |
#1
Code style good, need to test.
#2
I just had another idea in that vein: instead of registering variables with yet another hook, if should be possible to extend the syntax of variable_set() beyond what you just did. Instead of having it
variable_set($name, $value = NULL)why not go further and acceptvariable_set($name, $value = NULL, $default = NULL). This would set the default if the third argument were present, or leave it unchanged otherwise. And maybe while we're at it, add the long-talked-about fourth argument defining prefetching to solve the problem outlined by DWW, and which other modules like chatroom have.From there, variable_get() could also be modified like variable_get($name, $default = NULL). And using variable_get($name) would now use the default defined upon variable_set'ting, still leaving the current syntax available, to serve two purposes: backwards compatibility, and ability to change defaults on the fly without otherwise affecting the stored default.
I tend to think this is more elegant than adding a hook_variables.
What do you think of it ?
#3
Hm. Actually I think tacking on still-more parameters to variable_set() is exactly the wrong way to go about it, is a step backward, and doesn't solve the underlying problem in the first place. It's also fundamentally flawed as you can never guarantee that you'd always be able to variable_set() before you'd variable_get().
So no, I'm much much MUCH more in favor of a registry-style hook_variables to centrally manage all of that sort of information. It's also well off-topic to this issue, which should be a nice and simple one. :-(
#4
I have to agree with the fact that it is slightly OT. But not with the other claims, especially the fact that "you can never guarantee": such variables should probably always be created in the .install file anyway, thus ensuring that they have a value at "run" time, while code running not running after the page full bootstrap process, could still use the second param in variable_get(). It would really reduce redundant code in a lot of places, and would still get us the same benefits as some hook_variables variant without the additional API clutter of yet one more core hook.
But you're right, your suggestion does not have to touch these larger issues, and can remain "atomic" as you suggested it.
#5
Lovely patch. Perhaps I am paranoid, but I would like to see a stronger 'access arguments' for the test menu item $items['system-test/variable-get']. i could imagine that this module get stuck enabled on someone's site and now their variables are exposed. After that, this needs a verify that the tests pass and then RTBC.
#6
Hm, valid point that. How does one lock down a menu item to only run during simpletest, though?
#7
In the test, create a user that has 'access administration pages' and login as that user. then you can increase the access arguments.
#8
http://drupal.org/node/138544#comment-1129772
#9
This is not a duplicate of that issue. It is a very deliberate fork as that issue was getting bogged down with other matters, as stated in the original post.
#10
The last submitted patch failed testing.