Each time we want to set a value to a variable, we need to :
- write the database
- clear the cache
- set conf[$name]
But in some case, there is no need to do all these tasks. See :
function variable_set($name, $value) {
global $conf;
if ($conf[$name] !== $value) {
db_merge('variable')->key(array('name' => $name))->fields(array('value' => serialize($value)))->execute();
cache_clear_all('variables', 'cache_bootstrap');
$conf[$name] = $value;
}
}
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | variable_set_should_rely_on_conf_before_merge_907524.d7.14.patch | 907 bytes | elusivemind |
| #4 | 907524-variable_set-avoid_db-rerolled.patch | 591 bytes | musicnode |
| #1 | 907524-variable_set-avoid_db.patch | 568 bytes | franz |
Comments
Comment #1
franzInteresting, this might go for some speed gain.
I improved logic a bit, testing for with isset() first to avoid warnings.
This patch is for D8, but can be easily back-ported to D7, if that's the case.
Comment #2
franzComment #3
xjmThis is one of those things that seems so obvious once someone notices it. :)
Just a minor comment cleanup -- "If the value is set already, we avoid a database call." (Period at the end.)
We'll need to reroll this on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades. Tagging as novice for the task of rerolling the patch.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #4
musicnode commentedRe-rolled, added the period, needs review.
Comment #5
xjmThank you for the reroll. I can't think of a reason not to RTBC this pending tests.
Aside: assign the to yourself while you're working on the patch, and unassign it after if you're not continuously working on it. :) (Rather than the other way around.)
Comment #6
catchI don't think we can do this necessarily.
If I have a variable in settings.php, I may want to set it in the database before removing it from settings.php, this code would make that impossible to do with variable_set().
I've seen lots of contrib modules (and core too) that blindly call variable_set() dynamically without checking the value first, but that should really be up to the caller I think.
If people think this is overly defensive, then I'd be happy to commit it though, would solve a lot of performance issues in the wild, but putting back to CNR so more people can chime in.
Comment #7
chx commentedIs that a challenge to find reasons not to RTBC the patch :D ?
One could test this behaviour: with current HEAD if i have a unit test and I do a variable_set I willget an exception which can be caught. with this one, if I set $GLOBALS['conf'] then this no longer throws the exception.
Also, if a variable is set beforehands in settings.php then this will stop it from entering the database. Good idea? I am not 100%.
Comment #8
xjmOkay I couldn't, but fortunately others can.
Comment #9
xjmxpostses!
Comment #10
chx commentedWhat you want to do is to capture $variables in variable_initialize before conf is loaded into it (store it into drupal_static('variables_database') perhaps) and compare to that not conf. What I said about testing stands.
Comment #11
catchThat last one is the approach taken in #987768: [PP-1] Optimize variable caching to avoid cache clears when not necessary (although it still writes to the database regardless, but doesn't clear cache unless there's a change).
Comment #12
ianthomas_ukRemoving the novice tag, as we've already seen some non-obvious side effects of the proposed changes.
This doesn't apply to 8.x any more, as variable_set is due to be removed and its replacement (state/config system) works totally differently. I'll move it back to 7.x in case someone wants to work in it there, but I think this is probably a won't fix.
Comment #13
elusivemind commentedI know I'm late to the party.
One of the side effects of this code NOT being in place is that if you set a variable on a load balanced system, all of the web heads will attempt to populate the variable at once causing a cascade and backing up the database. Are there suggested fixes for this? Right now we have this patch in place on our install of Drupal and it resolves this issue.
I acknowledge the side effects above, but is there anything that can be done to mitigate? Drupal itself isn't checking variables before setting them, so to expect module developers to do this as an SOP seems inconsistent. With all due respect.
Comment #14
elusivemind commentedProposing a revised patch which provides an optional argument to variable_set which can be used in the edge cases described in #8. I would like some feedback if possible. Thank you.
Comment #15
elusivemind commentedComment #16
catchMarking as duplicate of #973436: Overzealous locking in variable_initialize(). There are also other issues dealing with variable_get() stampedes.
Comment #18
catch