the admin/user/settings page has a bunch of rather large variables (all the bodies and subjects for the various welcome emails, password reset email, etc). it's about to get even more as soon as http://drupal.org/node/144859 lands. there's a nice method in the code for gracefully handling the hard-coded defaults for these (_user_mail_text()). however, one annoying property of this settings page is that as soon as you hit "Save configuration", we populate the {variable} table with *all* of the (rather huge) settings, even if (as is usually the case) most or all of the email bodies and subjects are identical to the defaults.

i had a potentially nice idea to solve this problem, so that we only put stuff in {variable} that's actually customized (which can help performance, since we have to read all these values from the DB and unserialize them on every page load)...

we should just have a simple little custom submit handler on this form, before the default submit handler from the system_settings_form() comes in. for all of the email settings, we just compare the value in $form_values with the hard-coded default. if they're the same, we just unset() that from $form_values before we pass it on to the default submit handler.

if folks don't think this is insane, i'll take a stab at a patch. should be quite trivial.

actually, maybe an even better solution would be to change the submit handler for system_settings_form() to *always* compare the value in $form_values with the #default_value from the form itself, and if they're the same, not to save it into the {variable} table (since any code using this setting has to be written to assume that if it's not in the DB, it should use the hard-coded default value, just like after someone did a "Reset to defaults" operation on a settings page). that'd be quite slick, IMHO. maybe i'll try a patch for that, instead. eaton tells me D6 FAPI passes the original $form array into the submit handlers, and that still has #default_value, so this should be incredibly easy, and should provide a nice reduction in the {variable} table footprint.

Files: 
CommentFileSizeAuthor
#278 _user_mail_text-multilanguage-145164-278-D6.patch671 bytesNBZ4live
#278 _user_mail_text-multilanguage-145164-278-D7.patch654 bytesNBZ4live
#236 drupal_variables.diff5.63 KBadrian
#217 145164_hook_variables_135.patch37.04 KBmatt2000
Failed: Failed to run tests. View
#215 145164_hook_variables_134.patch37.17 KBBerdir
Failed: Failed to run tests. View
#213 145164_hook_variables_133.patch37.17 KBBerdir
Failed: Failed to run tests. View
#210 145164_hook_variables_132.patch37.26 KBBerdir
Failed: Invalid PHP syntax in modules/simpletest/tests/common.test. View
#207 145164_hook_variables_131.patch67.11 KBBerdir
Failed: Failed to apply patch. View
#206 145164_hook_variables_130.patch67.09 KBBerdir
Failed: Failed to run tests. View
#204 145164_hook_variables_130.patch69.14 KBBerdir
Failed: 10790 passes, 2 fails, 0 exceptions View
#200 145164_hook_variables_129.patch66.87 KBBerdir
Failed: Failed to apply patch. View
#175 145164_hook_variables_128.patch66.87 KBDavid_Rothstein
Failed: Failed to apply patch. View
#173 145164_hook_variables_127.patch67.3 KBBerdir
Failed: 10686 passes, 1 fail, 0 exceptions View
#169 145164_hook_variables_126.patch66.81 KBBerdir
Failed: Failed to apply patch. View
#165 145164_hook_variables_125.patch66.85 KBBerdir
Failed: Failed to install HEAD. View
#158 145164_hook_variables_124.patch66.54 KBBerdir
Failed: Failed to apply patch. View
#150 145164_hook_variables_124.patch66.54 KBBerdir
Failed: Failed to apply patch. View
#149 145164_hook_variables_123.patch66.54 KBBerdir
Failed: Failed to apply patch. View
#146 145164_hook_variables_122.patch52.02 KBBerdir
Failed: Failed to apply patch. View
#141 145164_hook_variables_121.patch51.96 KBBerdir
Failed: 10212 passes, 231 fails, 37 exceptions View
#139 145164_hook_variables_120.patch51.86 KBBerdir
Failed: Failed to install HEAD. View
#137 145164_hook_variables_119.patch51.68 KBBerdir
Failed: 10562 passes, 1 fail, 0 exceptions View
#134 varcach.patch50.72 KBRobLoach
Failed: 10562 passes, 1 fail, 0 exceptions View
#132 varcach.patch50.45 KBRobLoach
Failed: 10562 passes, 1 fail, 0 exceptions View
#127 145164_hook_variables_118.patch50.12 KBBerdir
Failed: 10563 passes, 1 fail, 0 exceptions View
#125 145164_hook_variables_117.patch50.09 KBBerdir
Failed: 10552 passes, 12 fails, 0 exceptions View
#123 145164_hook_variables_116.patch48.2 KBBerdir
Failed: 10545 passes, 19 fails, 0 exceptions View
#120 145164_hook_variables_115.patch48.16 KBBerdir
Failed: 10538 passes, 26 fails, 9312 exceptions View
#118 145164_hook_variables_114.patch46.85 KBBerdir
Failed: Failed to install HEAD. View
#114 145164_hook_variables_113.patch43.03 KBdeviantintegral
Failed: Failed to install HEAD. View
#103 145164.patch43.21 KBRobLoach
Failed: Failed to apply patch. View
#85 145164.patch43.24 KBRobLoach
Failed: Failed to apply patch. View
#84 145164.patch51.04 KBRobLoach
Failed: Failed to apply patch. View
#77 variable_defaults_d7.patch309.42 KBmaartenvg
Failed: Failed to apply patch. View
#74 145164.patch293.64 KBRobLoach
Failed: Failed to apply patch. View
#73 145164.patch292.3 KBRobLoach
Failed: Failed to apply patch. View
#72 145164.patch292.28 KBRobLoach
Failed: Failed to apply patch. View
#71 variable_defaults.patch305.66 KBagentrickard
Failed: Failed to apply patch. View
#64 variable_defaults_07.patch279.75 KBcwgordon7
Failed: Failed to apply patch. View
#61 variable_defaults_06.patch279.64 KBcwgordon7
Failed: Failed to apply patch. View
#54 variable_defaults_05.patch274.84 KBcwgordon7
Failed: Failed to apply patch. View
#52 variable_defaults_03.patch265.43 KBcwgordon7
Failed: Failed to apply patch. View
#49 variable_defaults_02.patch265.13 KBcwgordon7
Failed: Failed to apply patch. View
#47 variable_defaults.patch265.49 KBcwgordon7
Failed: Failed to apply patch. View
#37 variables_new_api_06.patch171.67 KBJose Reyero
#29 variables_new_api_05.patch186.16 KBJose Reyero
#28 variables_new_api_04.patch169.57 KBJose Reyero
#26 variables_new_api_03.patch164.16 KBJose Reyero
#21 variables_new_api_02.patch155.68 KBJose Reyero
#20 variables_new_api_01.patch153.79 KBJose Reyero
#17 variable_get.txt41.57 KBJose Reyero
#14 variables_new_api_00.patch13.89 KBJose Reyero

Comments

eaton’s picture

This is a SLICK idea. Huge kudos!

webchick’s picture

I vote the last idea. Elegant.

kbahey’s picture

A while ago there was talk about having variables by realm. The realm would be essentially the module that defines this variable. This tiered approach to variables can make loading them more granular.

Having said that, the code freeze is upon us and low hanging fruit must quickly be picked.

Go for it.

dww’s picture

Title: don't waste space in {variable} table for default settings » new variable API
Category: task » feature

thinking about this more, i've got even more ambitious plans. just relying on #default_value means if you change a variable from the default, to something else, back to the default, we'd still have a copy in the DB, since there's know way in the submit handler to know what's the hard-coded default. furthermore, it seems crazy that every call site of variable_get() has to handle it's own version of what to use if the value isn't in the DB. as far as i can tell, everyone just has to hard-code this value everywhere in their module, and hope they keep it consistent, or use php define() constants. there's the problem of hook_uninstall, and needing variables tied to specific modules (realms, as kbahey calls them).

i propose that we introduce hook_variable() to define all the defaults for all the variables for a given module, and change the API for variable_get() to not take a $default parameter anymore:

/**
 * Registry of default values for variables.
 *
 * @see variable_get()
 * @see variable_set()
 *
 * @return
 *   Array of name to value mappings for each variable defined by this module.
 *
 */
function hook_variable() {
  return array(
    'name' => t('Default value for name'),
    'foo' => 2,
    ...
  );
}

/**
 * Return a persistent variable.
 *
 * @param $name
 *   The name of the variable to return.
 * @return
 *   The value of the variable.
 */
function variable_get($name) {
  global $conf;
  return $conf[$name];
}

variable_set() would remain the same API. however, if there was no default already defined via hook_variable() for a given $name, it would be a fatal error. this is a develper error -- any variable you're setting with variable_set() requires a definition via hook_variable().

variable_init() would call hook_variable() for all modules. it would prepend the $name keys returned by each module's hook with that modules name, and would initially populate the $conf array with those values. we'd have a minor schema change to add a realm or module column to the {variable} table, determined implicitly via hook_variable(), and then uninstall would automatically be able to drop all variables associated with a given module (not just relying on the project name prefix of the variable name itself).

the other incredibly slick thing this would allow (since the $name values in hook_module would by definition be unique), is that for forms that are built via system_settings_form(), we could automatically add a "reset this setting to its default" checkbox on every setting form element, and the submit handler would just be able to find the right form element based on the variable name that corresponds with the checkbox, and remove the entry from the DB (therefore, falling back to the value returned by hook_variable() in variable_get()).

a quick grep finds just under 500 call sites in core for variable_get() that would have to be inspected and changed, so this isn't going to be a quick task to port every core module and .inc file to this new hook and API, but i think it'd totally be worth it.

i don't suppose i could get away with a patch to bootstrap.inc to change the API, independently of changing all 500 call sites, huh? ;) any volunteers to help adopt certain core modules and port them to this API?

p.s. for extra credit, the schema upgrade would invoke hook_variable() (which it'll have to do anyway to initially populate the realm/module column during the conversion). at that time, if the value in the DB matches the value returned by hook_variable(), we just delete the record from the DB entirely, instead of inserting it with the right realm value.

Crell’s picture

I believe what dww is talking about was just discussed on the devel list in the context of translations:

http://lists.drupal.org/pipermail/development/2007-May/023665.html

This is yet another place where a variable-defaults hook, of whatever name, would be very useful. I thought it was a good idea then, and I think it's a good idea now. :-) It definitely needs to be a hook to handle dynamic variable names, but as discussed in that thread there are edge cases that need to be accounted for. I had a demo implementation idea in this message, which you are welcome to steal:

http://lists.drupal.org/pipermail/development/2007-May/023765.html

bjaspan’s picture

subscribing

I like the idea in general but will have to give it more thought. Once thought I had is that some modules call variable_get() with different or context-dependent defaults; that would not work in this scheme (would it?), although I guess variable_get could still have a $default arg which overrides hook_variables (and ultimately forces its default into the db).

It would be great if the new api did not require changing all modules and calls to variable_get so code could migrate to it incrementally. Perhaps if a var is not defined by hook_variables it is not a fatal error, it just reverts to previous behavior, and we make this ability deprecated to be removed in D7 or something.

Got to run, I'll think more about this later.

webchick’s picture

Well, now we're getting into the discussion dldelge and I have been having over the past little week. ;)

I just spent 5 minutes typing up this thing about how we could use the 'realms' idea (#3) in order to facilitate automatd cleanup on uninstall of a module's variables, but realized we wouldn't need it with dww's idea:

// somewhere in system_uninstall() or whatever
foreach (module_invoke($this_module, 'variable') as $name => $value) {
  db_query("DELETE FROM {variable} WHERE name = '%s'", $name);
}

Hmmm. :) Anyway, HUGE +1. What we have now is hella clunky. What's described in this issue would:
a) Cut down on the size of the variables table and the $conf array (better for site performance)
b) Make just ONE PLACE to specify defaults for variables (better for developers)
c) Make clean-up automatic (better for everyone :))

bjaspan’s picture

One additional thought: This would trade loading and unserializing all the variables in the variables table with either (a) loading and unserializing the cached sum of hook_variables or (b) calling YAH (yet another hook) during each page request. Both (a) and (b) involve dealing with every variable that every module *might* set whereas the current system only deals with variables that *are* set.

We should be careful to make sure it is actually a performance improvement.

dww’s picture

a few more thoughts and replies to comments above:

  • sorry i missed a related discussion on devel... i can't keep up with all those threads. ;)
  • my primary concern is actually developer-friendliness and possible admin UI improvements. i agree that the performance implications won't necessarily be a big win (we'll have to see), but so long as they're not significantly worse, i still think this would be worth doing.

    i'm not really a performance expert, but i get the sense that the main cost associated with a hook is that you have to read all the files, parse all the php, etc. however, we have to do that during bootstrap, anyway. ;) once we've got all that code in RAM, the actual invocation of the hook isn't that terrible, is it? and, maybe i was being dumb, but i imagined we'd just invoke this once during variable_init() and populate the $conf array, so variable_get() just always returns the value in $conf. so, it wouldn't reduce the size of $conf, if anything, it'll be bigger.

  • webchick's right that if we had the hook, we wouldn't necessarily need to change the schema for {variable} and add a realm column. we would still want a DB update to call the hook for all modules, and purge the {variable} table of all entries that match the values returned by the hook.
  • @bjaspan: [One] thought I had is that some modules call variable_get() with different or context-dependent defaults; that would not work in this scheme (would it?) -- yeah, i wasn't sure if that was going to be the case. i really need to inspect more of those 500 call sites to see how we're using variable_get() to be sure. i'd have to see some concrete examples to make up my mind on this part, but my gut tells me that context sensitive defaults are probably not necessary, and are potentially confusing and wrong. ;) that's just speculation, so real examples where we do this would be welcome.
  • i'd really rather avoid the $default arg to variable_get() if at all possible. i know changing all the call sites is going to be a pain in the ass, but i think it'll be confusing if we have both hook_variable() and the $default arg. maybe a transitional API in D6 and a more thorough break in D7 is a good compromise, but i think it'd be cleaner to just switch entirely now. that's a big part of the motivation for this.

    also, this really confuses me: (and ultimately forces its default into the db). huh? no way would i go for that. variable_set() puts stuff in the DB. variable_get() gives you an answer. never the two shall swap roles, IMHO. ;)

  • obviously, it'd be fine if the default value returned by some module's hook_variable was NULL, to specify a variable it owns and will be managing, setting, etc, but which there is no reasonable default. as in, a required site-wide setting that the admin must specify.
Gábor Hojtsy’s picture

Derek, we are exactly in the same tracks as in the devel list discussion I started. People brought up automatic variable uninstallation, reduction of stored variables, and so on. It is a pleasure to hear that someone would take up the initial work at least :) Go for it!

dww’s picture

wow, i just read the whole devel list thread. tee hee. ;) looks like i'm "blazing a new trail" through some very well-traveled paths. i guess the good news is that, in my own little bubble, i came up with almost exactly the same design as gabor, moshe, larry and others have been advocating in that thread. therefore, it *must* be a good idea. ;)

now, let's see if i have the time/energy to crank out the patch so we can benchmark it before the freeze... *sigh*

Jose Reyero’s picture

I think the idea of the hook_variable is great. We need some more information there though.

For variables that have a localizable text value as default, we'd need to be able to get that default value not localized yet, because the actual default for that one will be the English value translated to the default language, which we cannot get if we are browsing the site in some other language.

This is specially true since we are now supporting multilingual requests with locale module, so we may need to be ready to get the variable value -and the default- in several languages in the same request. See http://drupal.org/node/143249, recently committed.

Also, if we return an array of values for each variable, this mechanism will be much easier to extend in the future, so I'd propose something like this:

function hook_variable() {
  return array(
    'name' => array(
        'default' => 'Default value for name', // This is the English hardcoded default
        'localize' => TRUE,
        'type' => 'text'  // May be some useful information too
        .....
    );
    ...
  );
}
dww’s picture

Assigned: dww » Unassigned

if we're going to do a full array of meta data like that, it should definitely use the standard #keyword (FAPI-style) array, which would open the door for hook_variables_alter() and other similar insanity. ;)

that said, my time before the code freeze is running short, and we still don't have an official 5.x-2.0 release of update_status out. :( that seems like a higher priority for me, so i'm unassigning myself, since i don't want to leave people with the impression i'm actively working on this right now.

i *really* hope someone decides to adopt this and try to get an initial patch together before the dreaded freeze sets in...

Jose Reyero’s picture

Status: Active » Needs review
FileSize
13.89 KB

Ok, here's the first try. Just to get feedback, only the 'Site information settings' implemented. Please, feedback

This patch

  • Introduces a new hook_variable() that returns variable metadata, only implemented as a sample for a few settings in system module. Variable metadata is an array with the form
array(
  'variable_name' => array(
     type => VARIABLE_TEXT, // Special constants defined in bootstrap.inc which provide information abt the variable
     'default' => 'Default value'
  );
);
<li>Reworked variable API to automatically handle default values</li>
<code>
function variable_get($name, $default = NULL, $langcode = NULL) {
  // Default value is not required anymore. Language code needed in some cases for localizing defaults.
}
/**
 * Get default information about a variable
 *
 * @param $name
 *   Variable name
 * @param $langcode
 *   Language code, as some default values need localization
 * @param $process
 *   Whether to return processed value or raw data
 */
function variable_get_default($name, $langcode = NULL, $process = TRUE) {
  // This function collects and returns default values for variables
  // It handles localization when needed. Variables to be localized have some special variable type.
}
  • Improved settings form management to automatically populate default values for variables and do automatic validation for known variable types.
  •   $form['site_name'] = array(
        '#variable' => TRUE, // We mark this setting as variable, no need to provide default value
        '#type' => 'textfield',
        '#title' => t('Name'),
        '#description' => t('The name of this website.'),
        '#required' => TRUE
      );
    
  • Bonus: Variables defined in the settings file -$conf array- are now disabled in settings forms. For that I've renamed the global $conf to $variables so we can know which variables are predefined and which ones are loaded.
  • Optimized variable_set(), so it doesn't blindly saves values anymore, only the ones changed. Also, when a variable is set to it's default value, it is deleted from the table
  • Automatically removes module variables on module uninstall.
  • Special considerations:

    • Default value, for settings form, is the variable default value localized -if needed- for the default language.
    • variable_get in bootstrap inc still needs default values. If modules provide default values for its variables, we cannot access them before module loading.
    • I was tempted to retrieve all the variable information -included the full form element- into hook_variable() but thinking twice, we don't need to have a gigantic array -may be hundreds of variables- collected for each page, so I've reduced the variable information to the minimum needed and defined that special variable constants to keep it small.
    • The hook_variable() name clashes with devel_variable() defined in devel module. So either disable devel module or rename that function when testing this patch.
    • Performance If we remove all default values for variables from the code, some speed-up is expected because of a) shorter code and b) We save a lot of t() calls that were used in providing default values for variables

    To better illustrate this last point:

      variable_get('anonymous', t('Anonymous));
      // This calls the localization function t() to provide a default value for the variable each time. With the new system it will be:
      variable_get('anonymous');
     // and the t() function will only be called when we actually need to localize the default value.
    
    Gábor Hojtsy’s picture

    Title: new variable API » New variable defaults to improve performance and help developers

    Wow, let me collect the performance gains:
    - slightly less code to parse on each page view
    - less t() calls to invoke, it only needs to be invoked at most one time for a value
    - less data to load in and unserialize from variable cache, as defaults are not saved
    - less updates to variables table because if value is unchanged it is not updated
    - less variables stored because uninstalled modules get their variables uninstalled automatically

    These are very nice! Additionaly to this, we have a "typed" variable system with centralized defaults, so developers don't need to repeat their defaults every time.

    We should have this in Drupal 6! Great work!

    Some notes on the patch:
    - documentation on each constant introduces should be added, just like with other constants
    - why is variable_get() still supporting a default value?
    - the new $langcode parameter should be documented there
    - "Get default information about a variable", hm what is a "default information"? Maybe you mean "Get metadata about a variable"
    - "Whether to return processed value or raw data", hm, what is a "processed value"?
    - coding style issues like "} elseif" and "} else {"
    - renaming the runtime $conf to $variables in seems to be a good idea, but then renaming $conf to $custom_variables would be even better for settings.php
    - the simlification changes to language_default() could be used elsewhere too (oftentimes we only need the language code, but it is not for this patch)
    - document $property on language_default()
    - coding style issues with "foreach(array_keys"
    - some changes have the variable name in the value of #variable, some have TRUE... I think you meant TRUE everywhere
    - "$form['#variable_list'] = system_settings_form_elements($form);" should have a comment on what it does (like the comment on top of the implementation of system_settings_form_elements()
    - "@param $elements" should be "@param $forms"
    - I am not sure if it is good to disable settings defined in the settings PHP file, but if we do so, we should inform users that it is disabled because it is defined in PHP already... now users have no clue why it is disabled
    - centralized validation is very cool! but add a break; after the last case, so if someone adds more, it is not forgotten
    - system_settings_form_submit() has the default language retrieved, but then not used at all

    And a language related remark: do I understand it right, that you have the t() on the variable defaults separated to have the t() always run with the default language and not with the language used to administer the site at the moment? (We will need the extractor know this new special case for localized strings again, that should be noted, but it is out of scope for this patch.)

    Gábor Hojtsy’s picture

    Let me point out one more thing. I don't know what is the status of phptemplate_variables() and _phptemplate_variables() but this used to be a well-known way to mangle with template variables, and the name clashes with the proposed hook. Anyway, there does not seem to be any of the two functions in Drupal 6 as it seems, so it might be safe with going this name forward.

    Jose Reyero’s picture

    FileSize
    41.57 KB

    Gabor, to the question
    - why is variable_get() still supporting a default value?
    Not sure about this, but I still don't know wether we can get rid of that argument. One reason is variables in bootstrap.inc. The other one is some variables not having a hardcoded default value, but a computed one -guess.. another good use for callbacks :-)
    Anyway, I attach a complete list of variable_get() functions in Drupal core, if someone wants to help me figure out this.

    going through all the rest of the comments in the meanwhile...

    moshe weitzman’s picture

    subscribe ... great patch, jose.

    killes once argued for keeping the current complicated variable_get() with differring defaults if developer wants but i think this is an extremely rare need and thus we can ignore it in favor of simplicity. dev can work around it if needs varying default.

    Gábor Hojtsy’s picture

    Jose, I was especially thinking about these two use cases to consider for variable_get() default parameter:
    1. let the whole code work with the not yet converted modules :)
    2. do something about bootstrap

    Indeed bootstrap is an interesting question here. If the variables used in bootstrap can be isolated, those would need defaults outside of this callback system, as module source code is not yet available at that time.

    Jose Reyero’s picture

    FileSize
    153.79 KB

    Here's the patch improved and extended.

    Changes:

    • Removed $default parameter from variable_get()
    • Provided defaults for all needed bootstrap variables in _drupal_defaults(). These can be overridden by $conf. Changes in variable_init() to just merge values in the right order, for overrides.
    • New variable_get_metadata function to retrieve variables metadata
    • New variable_get_default function that admits a default, like old 'variable_get'. This is useful in some special cases through the code, where there's not really a default value, but some value that needs to be returned. Also, there are some variables with 'changing names', like content type properties or some filter variables for which we cannot have hardcoded defaults.

    And I've done the replacement for all the '/include/*' files plus the required modules ('block', 'filter', 'node', 'system', 'user') and the ones enabled by the default profile ('color', 'comment', 'help', 'taxonomy', 'dblog') so this can be really tested.

    Notes:
    - One special case is user.module, where all the _user_mail_texts are now in the hook_variable(). This has the extra advantage of being able to retrieve this texts localized for different languages.
    - There's a leftover debug line in variable_get to help until we change all the variable_gets out there.
    - There are some small functions that were used only for these default values, like language_default. We can get rid of them now unless we find some other use for them.
    - There are a lot of defaults defined in bootstrap. They could also be defined in the settings file, or 'variable_get_default' may be used for some cases. Anyway, I think is good to have in the same place as many defaults as possible defined
    - This patch does break backwards compatibility, which can be good because ideally we should go through all the modules collecting defaults and centralizing them in hook_variable(). Anyway, a quick fix -for testing purposes only- is s/variable_get(/variable_get_default(/

    Jose Reyero’s picture

    FileSize
    155.68 KB

    Updated the patch for latest Drupal HEAD, fixed some minor issues, and some improvements:
    - Store bootstrap defaults in $variables['defaults']. They will be used now as the last fallback but they won't be merged with database variables.
    - Reworked variable_get for this new fallback. Now the precedence for variable_get is:
    1. $conf array in settings file
    2. Database stored values, when available.
    3. Defaults provided by modules, when loaded
    4. Defaults defined in bootstrap.inc:_drupal_defaults()
    - Optimized variable_del. Now it only throws a db update when the variable was actually stored in the database.
    - Optimized variable_set. Checks the value against current value and defaults before updating the database
    - Reworked variable_get_metadata(), now it needs to be called once without parameters, after module loading for initialization. This fixes some previous issues and allows more control of the startup system.

    With all these optimizations we should be able to reduce to a minimum the variable's database updates and cache refresh, which should improve performace still more.
    Note: Still some debug code in variable_get to help catching old variable_get calls. I've cleaned them all for default modules and install system though.

    Gábor Hojtsy’s picture

    Hm, some remarks:

    - the constants on top of bootstrap.inc still lack *individual* documentation. these are not simple easy to understand things. like I really don't know from there, what a "localize" modifier would mean, or if I mark my variable email, it will get validated as such
    - I would name _drupal_defaults as _variable_defaults(), because that is what it does. (small errors in the function phpdoc, also this function defines default variable *values*, not default variables, so document as such)
    - variable_get() has some coding style errors, brackets on wrong lines... anyway, it should cache the variable metadata IMHO, t() should receive an array() if there are no $args, and all the cases could use comments on what happens there
    - "This function behaves pretty much as the old Drupal 5 variable_get() function" hm... evil grin.... this is incentive to just s/variable_get/variable_get_default/ when I update my modules... anyway, better document here the usefulness of this function *instead*... in what cases should it be used, why it is kept (I see you used it some placed, but the pattern/reason is not entirely clear to me, and will not be to developers for sure)...
    - I would add a pointer to the phpdoc() of variable_get_metadata() that where are "before module loading" variable defaults accessible
    - I don't get how the $load static works... "Enable loading in the next try" is very unclear
    - bracket coding style issues in variable_set()
    - as I have said before, now that you renamed the runtime used $conf to $variables, there is a "developer mind" disconnect between $conf and $variables... these look really different (by their name), but $conf is really just the "static PHP configuration file defaults/values for $variables"
    - the foreach still has coding style issues in drupal_uninstall_module()

    I did not look through all of the module changes, since they are mostly removing the default value parameters, but two things hit my eyes:

    - you still use quite a few variable_get_default()s... maybe the enhanced documentation on it's usefulness will elighten me, why is it good to have these
    - you have some VARIABLE_DATA for values which are basically numbers like 'user_register'... maybe improved documentation on the meaning of constants will enlighten me here

    I have also seen you are using variable_get_default() on dynamic named variables, like filter format related variables. Why don't you do an SQL query on the formats and build a dynamic list of variable defaults with the names in filter_variables()? (I noticed there is even no filter_variables() implementation because of this).

    Although the patch looks big, most of the things are simple replacements following a rule, so I would urge people to discuss this and test. Simple performance benchmarks would be nice to see, although that might not be easy to do because we also need different data sets (variables) to test, as the patch changes what variables are actually stored.

    chx’s picture

    Please, please, pretty please use -F^f or -p in the future when rolling monsters like this (actually, any patch) but it's almost impossible to say what changed...

    chx’s picture

    drupal_unset_globals will kill $variables, i think. i might be wrong.

    Gábor Hojtsy’s picture

    Hm, I thought a bit more about dynamically named variables, and indeed it is not a good idea performance-wise to have then calculated from SQL requests (eg for filter formats) on all page views. So we either keep the current variable_get_default() calls as used, or invent some caching there for hook_variable() results, which might help anyway. That cache should be invalidated whenever something which affects the number of variables changes (ie. module enabled/disabled, filter added/removed and so on). I am not sure this is feasible, but it turned out in my thinking that we cannot put all variable defaults into hook_variables() otherwise while keeping the performance improvements.

    Jose Reyero’s picture

    FileSize
    164.16 KB

    Here it is. Addressed most of Gabor's remmarks in #22, but reworked all of the variable API.
    chx, sorry, still looking for that option in Eclipse. For most of this code though, I think the important thing are the variable names, and otherwise, the patch would be really *huge*.

    Main changes:

    • Introduced a new type of variable, VARIABLE_GROUP, that is meant to group all the different variables that used dynamic names, format names (filter) -the part I've used it for first- or content types -to do-
    • I.e. all the 'allowed_html_$format' variables for filters will be now a single array variable, 'allowed_html_format' indexed by $format. This way we can get rid of dynamic variable names which were an ugly thing IMHO and also the maintenance of the variable table will be much easier.

    • Introduced variable_get/set/del/_element functions to access spare elements of these variable arrays.
    • All these variable arrays are handled automatically in settings forms, just need to mark the form element or the parent element with #variable_element => $format. See _filter_html_settings().
    • Reworked the ugly variable_get_metadata() function. Now it is much cleaner, see documentation, an also provides the _variable_defaults() values. It allows incremental module loading like the one used in bootstrap_invoke_all().

    I reworked all filter.module variables with these new features to illustrate the whole thing. Other modules -like node.module and content types- pending. Anyway, the installation system and main features of default modules work if someone wants to test.

    Please, feedback!!!!. Let's move on before the code changes too much, updating all this is really a lot of job :-(

    moshe weitzman’s picture

    i tested and variables still work AFAICT, even the variable override in settings.php. filter varables work too. nice.

    yes, i can see how keeping this patch up is very hard.

    this patch adds some complexity but the benefits list is really compelling. +1.

    - variable_get has debug code
    - variable_set_element(). would it simplify code or understanding if all unspecified variables got placed in to a 'system' group? just an idea.

    Jose Reyero’s picture

    FileSize
    169.57 KB

    Hi Moshe,
    I think I better keep the debug code till we replace all the variables and have a definitive version of the patch.
    About variable_set_element, the elements were stored in the same variable as a huge array, but not anymore.

    In this new version:
    - Improved storage adding a 'element' field to the variable table. Btw, the schema api is really great :-)
    This new storage will allow to clean up the table much easily when uninstalling some modules. Now this different elements for the same variable name are stored as different rows, which allows some real improvement.
    - Reworked the 'variable_get_metadata', looks much simpler now. New 'variable_metadata' function for loading the data.
    - Support for multi part variable names like 'content_type:story', or 'filter:1'... which are handled now nicely with default values
    - Moved the form functions to form.inc so they can be re-used for other forms using variables that don't use system_settings_form(). Next: the content type form.

    Example, for filter formats, which is the part already implemented:

    /**
     * Implementation of hook_variable().
     */
    function filter_variable() {
      return array(
        'filter_default_format' => array('type' => VARIABLE_VALUE, 'default' => 1),
        // These are variable groups
        'filter:html' => array('type' => VARIABLE_GROUP | VARIABLE_VALUE, 'default' => FILTER_HTML_STRIP),
        'filter:allowed_html' => array('type' => VARIABLE_GROUP | VARIABLE_STRING, 'default' =>  '<a> <em> <strong> <cite> <code> <ul> <ol> <li> <dl> <dt> <dd>'),
        'filter:html_help' => array('type' => VARIABLE_GROUP | VARIABLE_BOOL, 'default' => 1),
        'filter:html_nofollow' => array('type' => VARIABLE_GROUP | VARIABLE_BOOL, 'default' => FALSE),
        'filter:url_length'  => array('type' => VARIABLE_GROUP | VARIABLE_NUMBER, 'default' => 72)
      );
    }
    

    This defines variable groups with complex names like 'filter:html_help'. Now for filter format 1, we'll use:

    variable_get_element('filter:1', 'html_help');
    variable_set_element('filter:1', 'allowed_html', '<a><p>...');
    

    The storage in the variable table looks like

    name      | element     | value
    ---------------------------------
    'filter:1'   | html_help  | 1
    'filter:1'   | allowed_html | '<a><p>..'
    

    Now 'filter:1' will behave pretty much as an old variable. We can also use:

    variable_get('filter:1'); // will return the whole 'filter:1' properties array
    variable_del('filter:2'); // will remove the variable filter:2 and all of it's elements 
    

    And also we can use the variable_xx_element() API to get/set individual properties. This is handled automatically in settings forms, just marking variables and variable elements.

    Jose Reyero’s picture

    FileSize
    186.16 KB

    I've done some improvements, added comments everywhere, and cleared things a little bit. I've done the most complex cases with this variable system -upload settings, content type variables, filters..- , and I'm pretty confident that with this latest changes and aditions, this variable system can support all use cases in Drupal core:

    • Moved back from previous complex variable names.
    • Before: variable_get_element('filter:1', 'html_help');
      Now: variable_get_element('filter_html_help', 1);
      so variable names are now fixed and there's an 'element' value that may change for content types, filter formats, etc...

    • Moved the form rewriting from the system.module to form.inc. It now rewrites forms and perform validations automatically for forms and form elements marked as 'variables'. This was needed because there may be other modules adding variables to the forms on form_alter, so the form API takes care of everything now.
    // We mark the form
    $form['#variable'] = TRUE;
    
    // And then we mark each element:
      $form['site_name'] = array(
        '#variable' => TRUE,
        '#type' => 'textfield',
          ....
      );
    
    // Example from content type form. Not messing anymore with changing variable names.
    $form['workflow'] = array(
        '#variable_element' => $type->type,
        '#type' => 'fieldset',
    .....
      );
      $form['workflow']['node_options'] = array('#type' => 'checkboxes',
        '#variable' => TRUE,
    
    // More complex one from upload settings. Here variable names are different to field names:
    
      foreach ($roles as $rid => $role) {
        $form['settings_role_'. $rid] = array(
          '#variable_element' => $rid,
          '#type' => 'fieldset',
    ...
        );
        $form['settings_role_'. $rid]['upload_extensions_'. $rid] = array(
          '#variable' => TRUE, '#variable_name' => 'upload_extensions',
    ....
          '#maxlength' => 255,
    
    // And that is all it takes to have default fields populated, values validated and variables saved in settings forms, even with changing variable names.
    
  • New variable type, VARIABLE_FALLBACK which uses another variable name as a fallback for default values. I needed this for upload settings and roles
  • /**
     * Implementation of hook_variables().
     */
    function upload_variable() {
      return array(
    ...
        // Default maximum file size per upload
        'upload_uploadsize_default' => array('type' => VARIABLE_NUMBER, 'default' => 1),
        // Default total file size per user
        'upload_usersize_default' => array('type' => VARIABLE_NUMBER, 'default' => 1),
    ....
    
        // Group variables, per role. All they use another variable as fallback
    
        // Permitted file extensions.
        'upload_extensions' => array('type' => VARIABLE_GROUP | VARIABLE_STRING | VARIABLE_FALLBACK, 
          'default' => 'upload_extensions_default'),
        'upload_uploadsize' => array('type' => VARIABLE_GROUP | VARIABLE_NUMBER | VARIABLE_FALLBACK,
          'default' => 'upload_uploadsize_default'),
    .....
      
      );
    }
    

    Replaced variables for all default modules, include/*.inc, content types, filters, upload.... I think these are the most complex cases for variable usage.

    David Strauss’s picture

    This may have already been suggested, but wouldn't this new hook fit best in .install?

    Dries’s picture

    1. These names are poorly chosen:

    +define('VARIABLE_VALUE', 0x0020); // Value from a predefined range
    +define('VARIABLE_DATA', 0x0008); // Other types of data
    

    They're not descriptive.

    2. The coding style needs work.

    3. The code comments need work.

    4.

    +define('VARIABLE_GROUP', 0x0100); // Defines a group of variables
    +define('VARIABLE_LOCALIZE', 0x1000); // The default of this variable must be localized
    +define('VARIABLE_FALLBACK', 0x2000); // This variable will use another one as a fallback for defaults

    These defines should be explained better so one understands when to use them. We should probably have a short paragraph of practical documentation for each of them.

    5. The terminology 'element' and 'group' is a bit confusing. Looking at the code, it seems to behave as an 'array'? Do all dynamic variables need to go into a 'group' even if there is only one of them?

    6. Is the fallback mechanism really needed? If so, what for?

    7. I don't understand:

    } elseif ($element) {
    +        // There may be a variable with a special name, appending _default
    +        return variable_get($name.'_default', $langcode);
    +      } else {
    

    Why is this needed? It seems bad practice to have such conventions?

    In general, I think the feature this patch introduces are compelling, but at the same time, the complexity seems to shoot through the roof. What once was a simple operation is now rather complex (with good reason). Unfortunately, the documentation didn't help me understand the "variables flow" and "discovery mechanisms" so I'll have to spend more time looking at the _code_ to truly understand what is going on or how to properly use this.

    This patch is definitely going to raise the barrier to entry so I'd start with (i) writing better documentation and (ii) being critic about the features/APIs. If we can simplify this, or remove parts of this patch, we should.

    Jose Reyero’s picture

    Dries,

    Yes, I share your concerns, really the complexity of this has escalated too much. It started simple but when trying to support all the use cases for variables on Drupal core...

    Though at this point I've done quite a thorough review of variables use in Drupal and I think a lot of things can be simplified.

    Ok about 1., 2. 3., 4., I'll fix that. I'm improving comments with each new version of this patch.

    5. The terminology 'element' and 'group' is a bit confusing. Looking at the code, it seems to behave as an 'array'? Do all dynamic variables need to go into a 'group' even if there is only one of them?

    Yes, agree, looking for something better for naming...
    The thing is that if we want to have a solid variable system with defaults for all the variables, and be able to clean it up on module uninstall, we shouldn't use these old 'fuzzy' variable names anymore, like 'filter_html_$format'.
    So the need for this feature looks quite clear to me. A different thing is the naming and the proposed api, about which I completely agree this patch is not quite there yet.

    6. Is the fallback mechanism really needed? If so, what for?
    7. I don't understand:

    +        return variable_get($name.'_default', $langcode);
    

    These two are about the same. I needed the fallback mechanism to get rid of these '_default' names, so that was really leftover code.
    The case for this were:
    a) Some defaults needed to be defined in bootstrap, as they were used by the filter.module which in turn, because of the install system and the maintenance theme, needs to work without loading other modules.
    b) These defaults also being needed as a default for other 'group' variables, so this basically was for not needing to write defaults twice, and also being able to override these defaults in settings file.

    The good news is that if we move the _variable hook to .install and load the defaults into the database -see below about this-, that may not be needed anymore, which is one of the points for simpifying.

    Some more thoughts:
    - The current system does a big number of not really needed db updates, cache refreshing and localization calls -for variable defaults- that make it far from optimal.
    - We cannot really rely on module hook to provide defaults at run time because there are too many cases of modules using variables defined by other modules that may not be loaded or even enabled. So a way to work this out may be to move the hook to .install, as David suggested in the previous comment and load the defaults in the database.
    - Getting rid of the $default parameter for variable_get has caused not few troubles, and also it may be needed for some bootstrap code -or include code in general-, the install system, and some special cases. So I think we better keep it for now, though it doesn't need to be required parameter and if at some point later we see we can drop it easily, after most of the variable_get have been patched, it will be a really simple patch. And the size of simple 'proof of concept patches' sky rockets :-( as I have to do all the replacing for all default apis and modules :-(

    So based on all of this, I'll be posting some really simplified version soon.

    killes@www.drop.org’s picture

    I've got a simple question:

    Assumign I used to have a variable "foo" and I had a select field in the module settings where a user could assign a value to this variable.

    Doing if (variable_get('foo', NULL) === NULL) I am then able to see if a user has chosen a setting for this variable.

    How would I do this in this new system?

    hass’s picture

    subscribing, i'd need to know what killes has asked, too.

    webchick’s picture

    @killes: I think you could just compare against the default value, no? If they don't match, then they picked something.

    Gábor Hojtsy’s picture

    @webchick: even if the value is the default, the user might picked that one. In this system, the value is not stored if it is the default, so we don't know whether it is because the user did not pick a value or because the user picked the default value. Whether this is loss of some important information with the possibly considerable performance gains in sight is in question.

    Jose Reyero’s picture

    FileSize
    171.67 KB

    New version, reworked a few things:
    - Variable metadata is stored in the variable table on module install/uninstall. We use the group_variable_set() api for that. So we can use all variables even from disabled modules now.
    - Got back the variable_get() default parameter, Renamed some functions: 'variable_get_element' to 'group_variable_get' -- does it help?
    - Removed _variable_defaults() from bootstrap, for the few cases of bootstrap variables that need a default we can use now variable_get() with a default.
    - The variable metadata can be defined as a simple value, which will be the default value, this simplifies it for now.

    So I think we better keep the default value, which doesn't bother too much anyway as long as it's not required until we've agreed on the variable api and we prove it can be really removed for all variables without making this too complex.

    About storing all the metadata in the variable table, which seems to grow it too much at the beginning, I think that will be compensated by keeping it clean in the long term, and also -for performance- because this system avoids an incredible number of database queries and variables cache refreshing.

    About the question of whether we can know if a variable has been set or not with this new system, the answer is no, we can't. It seems pretty obvious that either we store default values or not. And if not, that's it, we cannot know if a variable has been set to it's default value.
    But the question raises other issues about the abstraction level of our variable system. As long as the 'variable_get()' function retrieves the right value, any other thing is actually metadata about variables, and if it's about metadata, this new system provides much more metadata than the old one, so this is really about which data we need and which one we can spare.
    So please, any use case for that 'needing to know if the variable has been set or not'?

    About the patch, it's far from complete, but again, a proof of concept of the new api. If we agree on the api, the variable types we need etc, then we can do the full variable replacement through all the code. Otherwise it will be a waste of time.

    Crell’s picture

    The only use case I can think of for knowing the difference between default value that the user set and default value that the use didn't set is for "Hey dummy, you need to go to the settings page first" type messages. For that, there's at least 2 other options. 1) Have a meaningless default value (NULL, -1, 0, etc.) that you can check against but don't expose to the user. 2) Have another hidden boolean variable that defaults to false, and on the settings page has a 'value' element with a value of true. You then know that if that variable is set to true, the user has submitted that form at least once.

    There may be other cases I'm not thinking of, but I'm fine with losing direct access to that info as we can get it other ways in the rare cases that we need it.

    keith.smith’s picture

    Status: Needs review » Needs work

    patch does not apply any longer...

    # patch -p0 < variables_new_api_06.patch
    patching file modules/block/block.module
    patching file includes/locale.inc
    Hunk #1 succeeded at 440 (offset 16 lines).
    Hunk #2 succeeded at 897 (offset 18 lines).
    Hunk #3 succeeded at 1702 (offset 107 lines).
    patching file includes/common.inc
    Hunk #7 succeeded at 1351 (offset 7 lines).
    Hunk #8 FAILED at 1541.
    Hunk #9 succeeded at 2155 (offset 34 lines).
    Hunk #10 succeeded at 2145 (offset 7 lines).
    Hunk #11 succeeded at 2204 (offset 34 lines).
    Hunk #12 succeeded at 2315 (offset 7 lines).
    Hunk #13 succeeded at 2362 (offset 34 lines).
    Hunk #14 succeeded at 2392 (offset 5 lines).
    Hunk #15 succeeded at 2505 (offset 41 lines).
    1 out of 15 hunks FAILED -- saving rejects to file includes/common.inc.rej
    patching file includes/file.inc
    patching file includes/form.inc
    Hunk #1 FAILED at 294.
    Hunk #2 succeeded at 399 (offset -7 lines).
    Hunk #3 succeeded at 1286 (offset -8 lines).
    Hunk #4 succeeded at 1975 (offset -8 lines).
    1 out of 4 hunks FAILED -- saving rejects to file includes/form.inc.rej
    patching file includes/path.inc
    Hunk #2 succeeded at 218 (offset -2 lines).
    patching file includes/bootstrap.inc
    Hunk #3 succeeded at 442 (offset 1 line).
    Hunk #5 succeeded at 477 (offset 1 line).
    Hunk #7 succeeded at 615 (offset 1 line).
    Hunk #9 succeeded at 1112 (offset 1 line).
    Hunk #11 succeeded at 1151 (offset 1 line).
    Hunk #13 succeeded at 1224 (offset 1 line).
    Hunk #15 succeeded at 1315 (offset 1 line).
    patching file includes/cache.inc
    patching file includes/language.inc
    patching file includes/theme.inc
    Hunk #1 succeeded at 675 (offset 18 lines).
    Hunk #2 succeeded at 1391 (offset -1 lines).
    Hunk #3 succeeded at 1511 (offset 18 lines).
    Hunk #4 succeeded at 1505 (offset -1 lines).
    Hunk #5 succeeded at 1534 (offset 18 lines).
    patching file includes/install.inc
    patching file includes/image.inc
    patching file modules/drupal/drupal.module
    Hunk #1 FAILED at 42.
    Hunk #2 succeeded at 61 (offset 2 lines).
    Hunk #3 succeeded at 338 (offset 60 lines).
    1 out of 3 hunks FAILED -- saving rejects to file modules/drupal/drupal.module.rej
    patching file modules/blog/blog.module
    Hunk #1 succeeded at 96 (offset 1 line).
    patching file themes/chameleon/chameleon.theme
    patching file modules/upload/upload.install
    patching file modules/upload/upload.module
    Hunk #4 succeeded at 265 (offset 2 lines).
    Hunk #6 succeeded at 339 (offset 2 lines).
    Hunk #8 succeeded at 418 (offset 2 lines).
    Hunk #9 succeeded at 630 (offset 9 lines).
    patching file modules/taxonomy/taxonomy.module
    Hunk #1 succeeded at 1270 (offset 2 lines).
    Hunk #3 succeeded at 1420 (offset 2 lines).
    patching file modules/user/user.module
    Hunk #1 succeeded at 495 (offset 42 lines).
    Hunk #2 succeeded at 547 (offset 12 lines).
    Hunk #3 succeeded at 603 (offset 42 lines).
    Hunk #4 succeeded at 615 (offset 12 lines).
    Hunk #5 succeeded at 676 (offset 42 lines).
    Hunk #6 succeeded at 670 (offset 12 lines).
    Hunk #7 succeeded at 740 (offset 10 lines).
    Hunk #8 FAILED at 1050.
    Hunk #9 succeeded at 1306 with fuzz 2 (offset 21 lines).
    Hunk #10 succeeded at 1377 (offset 6 lines).
    Hunk #11 succeeded at 1405 (offset 21 lines).
    Hunk #12 succeeded at 1449 (offset 7 lines).
    Hunk #13 succeeded at 1484 (offset 21 lines).
    Hunk #14 succeeded at 1485 (offset 7 lines).
    Hunk #15 succeeded at 1509 (offset 21 lines).
    Hunk #16 succeeded at 1675 (offset 15 lines).
    Hunk #17 succeeded at 2420 (offset 17 lines).
    Hunk #18 succeeded at 2436 (offset 15 lines).
    Hunk #19 succeeded at 2458 (offset 17 lines).
    Hunk #20 succeeded at 2480 (offset 15 lines).
    Hunk #21 succeeded at 2502 (offset 17 lines).
    Hunk #22 succeeded at 2520 (offset 15 lines).
    Hunk #23 succeeded at 2547 (offset 17 lines).
    Hunk #24 succeeded at 2570 (offset 15 lines).
    Hunk #25 succeeded at 2595 (offset 17 lines).
    Hunk #26 succeeded at 2610 (offset 15 lines).
    Hunk #27 succeeded at 2634 (offset 17 lines).
    Hunk #28 FAILED at 2730.
    Hunk #29 succeeded at 2993 (offset -17 lines).
    Hunk #30 succeeded at 3068 (offset 17 lines).
    Hunk #31 succeeded at 3085 (offset -17 lines).
    2 out of 31 hunks FAILED -- saving rejects to file modules/user/user.module.rej
    patching file modules/comment/comment.module
    Hunk #1 succeeded at 397 (offset 1 line).
    Hunk #3 succeeded at 466 (offset 1 line).
    Hunk #5 succeeded at 564 (offset 1 line).
    Hunk #7 succeeded at 753 (offset 1 line).
    Hunk #9 succeeded at 1075 (offset 1 line).
    Hunk #10 succeeded at 1436 (offset 27 lines).
    Hunk #11 succeeded at 1419 (offset 1 line).
    Hunk #12 succeeded at 1454 (offset 27 lines).
    Hunk #13 succeeded at 1452 (offset 1 line).
    Hunk #14 succeeded at 1571 (offset 27 lines).
    Hunk #15 succeeded at 1554 (offset 1 line).
    Hunk #16 succeeded at 1622 (offset 27 lines).
    Hunk #17 succeeded at 1648 with fuzz 1.
    Hunk #18 succeeded at 1890 (offset 17 lines).
    Hunk #19 succeeded at 1966 (offset -1 lines).
    patching file modules/comment/comment.install
    patching file modules/node/content_types.inc
    patching file modules/node/node.module
    Hunk #10 succeeded at 1920 (offset 64 lines).
    Hunk #12 succeeded at 2088 with fuzz 1 (offset 20 lines).
    Hunk #13 succeeded at 2161 (offset 45 lines).
    Hunk #14 succeeded at 2471 (offset 19 lines).
    patching file modules/system/system.module
    Hunk #1 succeeded at 371 (offset 1 line).
    Hunk #3 succeeded at 499 (offset 1 line).
    Hunk #5 succeeded at 547 (offset 1 line).
    Hunk #6 succeeded at 604 with fuzz 2.
    Hunk #24 succeeded at 1258 (offset 5 lines).
    Hunk #26 succeeded at 1306 (offset 5 lines).
    Hunk #28 succeeded at 2269 (offset 30 lines).
    patching file modules/system/system.install
    Hunk #1 FAILED at 1.
    Hunk #2 FAILED at 3342.
    2 out of 2 hunks FAILED -- saving rejects to file modules/system/system.install.rej
    patching file modules/system/system.schema
    Hunk #1 FAILED at 186.
    1 out of 1 hunk FAILED -- saving rejects to file modules/system/system.schema.rej
    patching file modules/dblog/dblog.module
    patching file modules/dblog/dblog.install
    patching file modules/ping/ping.module
    patching file modules/filter/filter.module
    Hunk #5 succeeded at 610 (offset 2 lines).
    Hunk #7 succeeded at 806 (offset 2 lines).
    Hunk #9 succeeded at 1007 (offset 2 lines).
    Hunk #11 succeeded at 1073 (offset 2 lines).
    Hunk #13 succeeded at 1523 (offset 2 lines).
    patching file install.php
    Hunk #1 FAILED at 15.
    Hunk #2 FAILED at 25.
    Hunk #3 succeeded at 846 (offset 9 lines).
    Hunk #5 succeeded at 923 (offset 11 lines).
    2 out of 5 hunks FAILED -- saving rejects to file install.php.rej
    patching file modules/menu/menu.install
    patching file modules/color/color.module
    patching file modules/locale/locale.module
    patching file modules/aggregator/aggregator.module

    Jose Reyero’s picture

    Well, this happened to be a gigantic task and looks like we are not even closer, so I'm leaving this one for now, to work on some other more urgent multilingual related features.
    If someone else wants to take over and make something in time for Drupal 6....

    catch’s picture

    Version: 6.x-dev » 7.x-dev

    Bumping this to drupal 7. Same issue, different approach over at: http://drupal.org/node/79008

    I have a feeling there may be a third issue where this was fixed, but not closing these until I find it.

    mlncn’s picture

    Subscribing. Getting rid of the need to define identical defaults where the variable is set, and where it is used, alone makes this proposal worthwhile. Translatable variables: yay! How all these moving parts are fitting together: head hurts.

    benjamin, Agaric Design Collective

    RobLoach’s picture

    Component: base system » user system

    Subscribings. Patches like this are why I regret Drupal is using CVS and not something like GIT which is really good with merging differences.

    earnie’s picture

    subscribe

    Why is this a component of "user system" instead of "base system"?

    catch’s picture

    Component: user system » base system
    yhager’s picture

    Component: user system » base system

    subscribe

    cwgordon7’s picture

    FileSize
    265.49 KB
    Failed: Failed to apply patch. View

    Same idea as above patch(es), only now for HEAD, and with a somewhat different implementation. Localization is not occurring right now, but believe me, the patch is big enough on its own, localization is an easy follow-up patch.

    No auto-deletion of variables either, but again, easy follow-up. I am incredibly reluctant to add more stuff to this patch, for the obvious reason that by the time you're reading this, this will need a re-roll. :/

    If you are feeling in a nice mood you will re-roll the patch for me instead of just stating it needs a re-roll. :)

    cwgordon7’s picture

    Assigned: Unassigned » cwgordon7
    Status: Needs work » Needs review
    cwgordon7’s picture

    FileSize
    265.13 KB
    Failed: Failed to apply patch. View

    Erm, take two. Kindly disregard the last patch. :)

    catch’s picture

    Status: Needs review » Needs work

    A few typos - extra bracket in profiles/default/default.profile and missing function_exists('drupal_alter') in variable_default(). When installing, I get to the 'Drupal is already installed' screen rather than initial admin/site settings. $language is no longer cast to object in language_default(). I see it's cast to object in system_variables() but that's not working - presumably it's called in the bootstrap before system module is loaded, or something.

    I haven't quite tracked down where everything is so no re-roll I'm afraid. Overall things look pretty good though.

    cwgordon7’s picture

    Status: Needs work » Needs review

    Re-rolled as per #50.

    cwgordon7’s picture

    FileSize
    265.43 KB
    Failed: Failed to apply patch. View

    Er. Try again?

    cwgordon7’s picture

    Status: Needs review » Needs work

    Lots of test failures with above patch. Re-rolling...

    cwgordon7’s picture

    Status: Needs work » Needs review
    FileSize
    274.84 KB
    Failed: Failed to apply patch. View

    Patch re-rolled. Test failures now correspond to pre-patch levels. Side effect of the new patch is the unauthorized node view test has been cleaned up a bit as I struggled to figure out why it was failing (which is a whole other issue...). I have now implemented the "Delete variables on uninstall automatically" feature. Localization will have to wait for a followup still. I have been incredibly lucky and there have been no interim commits between the last patch and this one, so at the moment it should apply to HEAD.

    earnie’s picture

    I really appreciate this patch. I've eyeballed about 1/4 of it and can see its many benefits. One question I have concerns the uninstall feature. Since many of the contrib modules do not have a hook_uninstall will the uninstall tab contain the uninstall checkbox for the module anyway? Otherwise, we loose lose that benefit.

    RobLoach’s picture

    This variable system will delete the variables listed in hook_variable when the user UNINSTALLS the module. It will not effect the variables in the database if the module is just disabled. Thanks a lot for the patch update, Charlie.

    earnie’s picture

    Rob, yes, that is exactly my point. And the modules data without a hook_uninstall will always remain in variables table unless the module is listed in the uninstall UI regardless of hook_uninstall.

    dww’s picture

    @earnie: Yes, that's a good point -- admins must be able to select modules to uninstall even if there's no hook_uninstall(). However, a) I'm not sure if that's a problem now (someone would have to test it and/or review the code), and b) if it is, it belongs in a separate issue. This patch is already enormous, and we don't need to delay/complicate it any further with fall out changes like this one. Whether or not the uninstall UI shows modules without a hook_uninstall() can be handled after this lands.

    cwgordon7’s picture

    We've been lucky enough to go 6 days without any commits, so this patch still stands - but it won't soon, so we need quick reviews in order to get this in with minimal cvs conflicts! Please, please review this now!

    dww’s picture

    Status: Needs review » Needs work

    Before I slam the patch too much, lemme give props to Charlie for taking on this monster. ;) I lost patience for trying to get patches this big into core long ago. Kudos for the young and ambitious among us.

    That said... ;)

    A) This comment in function variable_default() is wonky:

      // Intensional usage of function_exists().
    

    "Intensional" is misspelled, and that comment isn't all that helpful. Don't you mean something like:

    // We have to use function_exists() because this function is called before the code registry is initialized.
    

    ???

    B) I can't find it explained in the comments in this issue, nor in comments in the patch, but WTF does it mean for a variable's default type to be "variable" and how does that work without potentially causing infinite recursion looking for the default value?

    C) Ditto, I see no explanation for the variable_ancestor() functionality, nor motivation for why we'd want it. If you're going to reroll someone's monster patch and make big API/behavioral changes like this, please alert us about it and justify your changes. Expecting people to closely review just to tease out changes like this (or hope they slide in under the rader) is a little slippery. ;)

    D) In fact, the entire #type system for what a default value can be seems weird. Why would you ever need a callback? Isn't that what hook_variable() is -- a function that's invoked that gives your module a chance to tell the system what the defaults should be. If your module needs to compute the current phase of the moon or something to decide an appropriate default, why can't it just do that directly in hook_variable(), instead of telling hook_variable() "ask me again and I'll tell you later"? I notice blogapi.module does this, but it's not immediately obvious why. Is it because hook_variable() is invoked so early during bootstrap that something like node_get_types() isn't yet available? If so, that should really be commented somewhere.

    E) Why does book_variable() use type '#value' like this?

    +function book_variables() {
    +  return array(
    +    'book_allowed_types' => array(
    +      '#type' => 'value',
    +      '#value' => array('book'),
    +    ),
    +    'book_child_type' => 'book',
    +    'book_block_mode' => 'all pages',
    +  );
    +}
    

    Looking at the implementation of variable_default(), it seems like this would do:

    +function book_variables() {
    +  return array(
    +    'book_allowed_types' => array('book'),
    +    'book_child_type' => 'book',
    +    'book_block_mode' => 'all pages',
    +  );
    +}
    

    ?? Because if it's an array, we assume it's got a '#type' field somewhere I didn't notice? Hrm. :/ So, you can return anything you want as a default value, except an array (which we do a fair bit of the time), and then you have to do a few handstands to return your array. Can't we just assume if there's no #type that it's a value, even if it's an array? That's what it *looks* like we're doing in variable_default(). Similar badness in many other core modules like color_variables() (which is mostly trying to return empty arrays as the defaults), etc.

    F) Here you changed the name of a variable and didn't provide a DB update function to repair existing sites:

    -      if ($screenshot = variable_get('color_' . $theme . '_screenshot', NULL)) {
    +      if ($screenshot = variable_get('color_screenshot_' . $theme)) {
    

    Either that's a typo in the patch, or you intentionally changed this name but left people's sites without a data migration path. Ditto 'color_stylesheets_', 'color_logo_'and 'color_palette'. Perhaps the undocumented motivation for variable_ancestor() comes into play? ;) We still need to do something to preserve people's existing settings.

    G) I know you said "Localization will have to wait for a followup still." but have you given that any thought yet at all? Seems like Jose spent quite a bit of effort on that, but I didn't follow it closely, and haven't given it any thought myself. Naively, I'd assume I should wrap user-facing text in t() calls inside my hook_variable() implementations, such as:

    +    'contact_form_information' => t('You can leave a message using the contact form below.'),
    

    Why not? Because we don't have locale stuff initialized yet at hook_variable() time? More docs, please. ;) Developers *will* get this wrong.

    H) I don't understand what this change to a modules/contact/contact.test has to do with this patch:

    -    $this->drupalGet('user/' . $web_user2->uid . '/contact');
    +    drupal_set_message($this->drupalGet('user/' . $web_user2->uid . '/contact'));
    

    I) This part really belongs in another patch that should be backported to D6, right?

    -  db_query('DROP TABLE {forum}');
    +  drupal_uninstall_schema('forum');
    

    J) None of the changes to modules/node/node.test make any sense relative to this patch.

    K) [Probably outside the scope of this patch]: Why is the variable called 'node_cron_views_scale' if it's "owned" by statistics.module?

    L) It's not clear why the definition of DRUPAL_HASH_COUNT had to move to system.module but the related DRUPAL_MIN_HASH_COUNT still lives in password.inc. Lemme guess, password.inc isn't included when hook_variable() is invoked. ;) Sucks to split those two related constants into separate files like this. Without much thought, I think I'd vote for moving both constants to user.module and password_count_log2 to user_variable(). passwords are inherently about users, so it seems more natural that this variable's default (and related constants) live in user.module, if they can't live in password.inc.

    M) You did weird things again with the names of the variables related to the notifications user.module sends on status changes. This time, not only isn't there a DB update path, but you left both the old and new formats in user_variables():

    +    'user_mail_status_activated_notify' => TRUE,
    +    'user_mail_status_blocked_notify' => FALSE,
    +    'user_mail_status_deleted_notify' => FALSE,
    ...
    +    'user_mail_notify' => TRUE,
    +    'user_mail_notify_status_deleted' => FALSE,
    +    'user_mail_notify_status_blocked' => FALSE,
    

    The new ones seem incomplete, too, since there's no longer a default for "status_activiated", unless you're relying on the ancestor stuff again, in which case, it needs a comment.

    N) There's no API documentation for hook_variable() anywhere in the patch. :(

    O) What happened to Jose's work on the slick #variable FAPI element for system_settings_form() et al? That seemed like one of the really nice features of this patch that required little code, and it's been silently dropped with your "Same idea as above patch(es), only now for HEAD, and with a somewhat different implementation.". :/

    Note to other reviewers: I didn't carefully go through and ensure that every default was correctly moved (without silent alteration *grin*) into the hook_variable() implementations in each module. Either we can assume Charlie got it all right, or someone still needs to verify all that. ;)

    cwgordon7’s picture

    FileSize
    279.64 KB
    Failed: Failed to apply patch. View

    First of all, thanks to dww for reviewing this monster patch. :)

    Let me document the changes between this patch and the original patch here, as well as in the inline documentation in the rerolled patch:

    "Variable variables" are now handled by placing the variablity at the end, and variable defaults are searched for in the manner:

    foo_bar_baz_32
    foo_bar_baz
    foo_bar
    foo

    If none of those variables are found as defaults, NULL is returned as the default. This does mean some renaming of dynamic variables (apologies for lack of update functions in initial patch).

    Now let me address each of your (completely valid) points below (all are addressed in the new patch):

    A) // Intensional usage of function_exists(). Comment cleaned up to: // We use function_exists() here, because the registry may not be available early on in the page process.

    B) As explained above. Inline documentation also improved in the new patch.

    C) ALERT, API CHANGE! ;) I didn't really like the way variable variables were handled in Jose's patch, so I made the variable_ancestor() functionality work as a way to cleanly do variable variables. Inline documentation for this is also improved.

    D) I initially thought the #type system was necessary, at the very least because some variables may default to other variables, which would cause infinite recursion. However, I now realize that with the variable ancestry system, this is unnecessary, as variables can simply be magically named to become the defaults of other variables when undefined. So the entire #type system has been dropped as per your suggestion, and I believe we now have a much cleaner API. :)

    E) Addressed with #D.

    F) Ugh. Update functions written now.

    G) It's an interesting question. I checked the order of the page process, and it turns out that locale is initialized before the variable_defaults() are, so we can use t() (the exception being when we use dynamic placeholders, which of course need to be done at runtime so user-set variables work too.) Patch includes updates to use t().

    H) /me hides. Debug code removed.

    I) Yes. Removed.

    J) /me hides again. Irrelevant code removed.

    K) Outside the scope of this patch, but because it is an implementation of a node-specific hook, I'd guess. Probably needs another issue, like #I.

    L) Sure, makes sense. Done.

    M) Old variables removed, comment added, (as well as update functions for everything).

    N) I would like to document hook_variable(), but I am unsure the proper place to do it - surely I can't put a sample of hook_variable() in core. I'm not sure how hook examples end up on api.drupal.org. In any case, here is an example hook with inline docs that should be able to be used when someone tells me what to do with them ;).

    /**
     * Defines the default values for variables in case no user-set value is found.
     * Variable variables can be handled through variable ancestry: if the variable
     * foo_bar_5 isn't found, the system will then check the variable foo_bar for a
     * default value if no overriding value is found. The variable defaults need to
     * follow the form 'variable name' => 'default value'.
     *
     * @return
     *   The return value should be an array of variables defined by this module in
     *   the form 'variable name' => 'default value'.
     */
    function hook_variable() {
      return array(
        'my_module_limit' => 500,
        'my_module_node_types' => array('page'),
        'my_module_enable_cool_feature' => FALSE,
        // The filter settings will follow the form my_module_filter_settings_[ID],
        // so we will use the variable ancestry system to set the default values of
        // all variables following the form my_module_filter_settings_*, where * is
        // anything.
        'my_module_filter_settings' => array('[[', ']]'),
      );
    }
    

    O) I thought most of it was not needed now that I had simplified the API, but I guess it makes an even cleaner API to remove the '#default_value' where unnecessary. I'm moving this work to system_settings_form(), though, where I think it belongs.

    SimpleTest failures remain at previous levels with this new patch.

    Thanks again for your review! :)

    cwgordon7’s picture

    Status: Needs work » Needs review
    Crell’s picture

    Just a note, since this patch removes a parameter from the end of variable_get() but does not replace it with anything, variable_get('foo', 'some default') will still continue to work and function correctly. It may be easier to make a patch that just changes the API as needed and get that tested and committed, then strip out the 'some default' entries in later patch(es). That should make chasing head with this patch far easier, and reduce the collision with other patches currently pending. It would also make the patch easier to review, as there would only be 30 KB, not 300 KB. :-)

    We did that for the Drupal 6 page callback split and I am doing it for the Drupal 7 Database API revamp, so I see no reason we can't do it here. It would also give us a whole bunch of DROP tasks to give to new contributors that should be really easy to do, but also really important. Score for everyone. :-)

    Also, at a quick glance I note you're using a recursive call to variable_get() for variable_ancestor(). Recursion in PHP is not particularly fast. If that can be refactored into an iterative loop, especially if not a loop over a function call, that should be a nice performance bump.

    cwgordon7’s picture

    FileSize
    279.75 KB
    Failed: Failed to apply patch. View

    Patch applies to latest HEAD with offset. Rerolled.

    RobLoach’s picture

    Should we be adding a variable.test that would:

    1. Test the invocation of hook_variables
    2. Test variable_set
    3. Test variable_get with the defaults from hook_variables and overridden by variable_set

    Thoughts?

    sun’s picture

    Status: Needs review » Needs work

    Agreeing with Larry, let's focus on the API change first.

    That said, I would even vote for moving that variable_ancestor() feature into a separate follow-up issue/patch. The original intention of this issue was very simple and clear. IMHO, a hook_variables() alone is complex enough.

    Please correct me if I'm wrong, but the most important question seems to be: How do we prevent the new registry from loading all modules on all requests because of hook_variables() ? Moving this hook into a separate file will still have a negative performance impact.

    Derek's issue about t() not being available at the time variables are initialized is equally important. The earlier we invoke the locale/language system, the higher is the performance impact.

    Dave Reid’s picture

    Patch needs re-roll with DBTNG changes.

    maartenvg’s picture

    This has always been a nuisance for me, and this solution seems wonderful. I will review anything that comes by.

    webchick’s picture

    Chiming in to say I would gladly welcome the day this is RTBC. ;)

    agentrickard’s picture

    Working on the DBTNG and a patch upgrade. Here's the current patch status.

    [ ken ] : patch -p0 < variable_defaults_07.patch 
    patching file cron.php
    Hunk #1 succeeded at 13 with fuzz 2 (offset 5 lines).
    patching file install.php
    Hunk #1 succeeded at 641 (offset -15 lines).
    Hunk #2 succeeded at 697 (offset -15 lines).
    Hunk #3 succeeded at 706 (offset -15 lines).
    Hunk #4 succeeded at 780 with fuzz 1 (offset -4 lines).
    patching file update.php
    Hunk #1 succeeded at 534 (offset 45 lines).
    patching file includes/actions.inc
    patching file includes/bootstrap.inc
    Hunk #1 succeeded at 500 (offset 12 lines).
    Hunk #2 FAILED at 520.
    Hunk #3 succeeded at 532 (offset 8 lines).
    Hunk #4 succeeded at 748 (offset 8 lines).
    Hunk #5 succeeded at 1005 (offset 8 lines).
    Hunk #6 FAILED at 1077.
    Hunk #7 FAILED at 1106.
    Hunk #8 FAILED at 1116.
    Hunk #9 succeeded at 1202 (offset 23 lines).
    Hunk #10 succeeded at 1227 (offset 23 lines).
    Hunk #11 succeeded at 1263 (offset 23 lines).
    Hunk #12 succeeded at 1285 (offset 23 lines).
    4 out of 12 hunks FAILED -- saving rejects to file includes/bootstrap.inc.rej
    patching file includes/cache.inc
    Hunk #1 FAILED at 16.
    Hunk #2 FAILED at 28.
    Hunk #3 FAILED at 143.
    3 out of 3 hunks FAILED -- saving rejects to file includes/cache.inc.rej
    patching file includes/common.inc
    Hunk #1 succeeded at 329 with fuzz 2 (offset -2 lines).
    Hunk #2 succeeded at 345 (offset -2 lines).
    Hunk #3 succeeded at 374 (offset -2 lines).
    Hunk #4 succeeded at 418 (offset -2 lines).
    Hunk #5 succeeded at 620 (offset 8 lines).
    Hunk #6 succeeded at 1214 (offset 49 lines).
    Hunk #7 succeeded at 1226 (offset 49 lines).
    Hunk #8 succeeded at 1374 (offset 49 lines).
    Hunk #9 succeeded at 1524 (offset 49 lines).
    Hunk #10 succeeded at 1749 (offset 51 lines).
    Hunk #11 succeeded at 2101 (offset 51 lines).
    Hunk #12 succeeded at 2111 with fuzz 2 (offset 51 lines).
    Hunk #13 succeeded at 2376 (offset 51 lines).
    Hunk #14 succeeded at 2431 (offset 51 lines).
    Hunk #15 succeeded at 2464 (offset 51 lines).
    Hunk #16 succeeded at 2552 (offset 54 lines).
    Hunk #17 succeeded at 2585 with fuzz 1 (offset 54 lines).
    Hunk #18 succeeded at 2628 (offset 54 lines).
    Hunk #19 succeeded at 2673 (offset 54 lines).
    Hunk #20 succeeded at 3589 (offset 16 lines).
    can't find file to patch at input line 603
    Perhaps you used the wrong -p or --strip option?
    The text leading up to this was:
    --------------------------
    |Index: includes/database.mysql.inc
    |===================================================================
    |RCS file: /cvs/drupal/drupal/includes/database.mysql.inc,v
    |retrieving revision 1.91
    |diff -u -p -r1.91 database.mysql.inc
    |--- includes/database.mysql.inc	14 Apr 2008 17:48:33 -0000	1.91
    |+++ includes/database.mysql.inc	25 Jul 2008 05:34:09 -0000
    --------------------------
    File to patch: 
    Skip this patch? [y] y
    Skipping patch.
    3 out of 3 hunks ignored
    can't find file to patch at input line 637
    Perhaps you used the wrong -p or --strip option?
    The text leading up to this was:
    --------------------------
    |Index: includes/database.mysqli.inc
    |===================================================================
    |RCS file: /cvs/drupal/drupal/includes/database.mysqli.inc,v
    |retrieving revision 1.57
    |diff -u -p -r1.57 database.mysqli.inc
    |--- includes/database.mysqli.inc	14 Apr 2008 17:48:33 -0000	1.57
    |+++ includes/database.mysqli.inc	25 Jul 2008 05:34:09 -0000
    --------------------------
    File to patch: 
    Skip this patch? [y] y
    Skipping patch.
    3 out of 3 hunks ignored
    can't find file to patch at input line 671
    Perhaps you used the wrong -p or --strip option?
    The text leading up to this was:
    --------------------------
    |Index: includes/database.pgsql.inc
    |===================================================================
    |RCS file: /cvs/drupal/drupal/includes/database.pgsql.inc,v
    |retrieving revision 1.71
    |diff -u -p -r1.71 database.pgsql.inc
    |--- includes/database.pgsql.inc	9 May 2008 19:18:11 -0000	1.71
    |+++ includes/database.pgsql.inc	25 Jul 2008 05:34:10 -0000
    --------------------------
    File to patch: 
    Skip this patch? [y] y
    Skipping patch.
    1 out of 1 hunk ignored
    patching file includes/file.inc
    Hunk #1 succeeded at 96 (offset 15 lines).
    Hunk #2 succeeded at 399 (offset 8 lines).
    Hunk #3 succeeded at 587 (offset 32 lines).
    Hunk #4 succeeded at 1026 (offset 73 lines).
    Hunk #5 succeeded at 1065 (offset 71 lines).
    patching file includes/form.inc
    Hunk #2 succeeded at 1663 (offset 10 lines).
    patching file includes/image.inc
    patching file includes/install.inc
    Hunk #1 succeeded at 590 (offset 133 lines).
    patching file includes/language.inc
    patching file includes/locale.inc
    Hunk #1 succeeded at 459 (offset 6 lines).
    Hunk #2 succeeded at 963 (offset 27 lines).
    Hunk #3 succeeded at 1755 (offset 27 lines).
    Hunk #4 succeeded at 2061 (offset 27 lines).
    Hunk #5 succeeded at 2149 (offset 32 lines).
    patching file includes/mail.inc
    Hunk #2 FAILED at 172.
    1 out of 2 hunks FAILED -- saving rejects to file includes/mail.inc.rej
    patching file includes/menu.inc
    Hunk #4 FAILED at 1258.
    Hunk #5 FAILED at 1268.
    Hunk #6 succeeded at 2457 (offset 21 lines).
    2 out of 6 hunks FAILED -- saving rejects to file includes/menu.inc.rej
    patching file includes/password.inc
    patching file includes/path.inc
    patching file includes/session.inc
    Hunk #1 FAILED at 69.
    1 out of 1 hunk FAILED -- saving rejects to file includes/session.inc.rej
    patching file includes/theme.inc
    Hunk #2 succeeded at 884 (offset 3 lines).
    Hunk #3 succeeded at 925 (offset 3 lines).
    Hunk #4 succeeded at 1654 (offset 3 lines).
    Hunk #5 succeeded at 1818 (offset 3 lines).
    Hunk #6 succeeded at 1831 (offset 3 lines).
    Hunk #7 succeeded at 1842 (offset 3 lines).
    patching file includes/theme.maintenance.inc
    patching file modules/aggregator/aggregator.admin.inc
    Hunk #1 succeeded at 398 (offset 162 lines).
    patching file modules/aggregator/aggregator.install
    patching file modules/aggregator/aggregator.module
    Hunk #2 FAILED at 801.
    Hunk #3 succeeded at 936 (offset 23 lines).
    1 out of 3 hunks FAILED -- saving rejects to file modules/aggregator/aggregator.module.rej
    patching file modules/aggregator/aggregator.pages.inc
    Hunk #2 succeeded at 64 (offset -1 lines).
    Hunk #3 succeeded at 170 (offset -1 lines).
    Hunk #4 succeeded at 268 with fuzz 1 (offset -1 lines).
    Hunk #5 succeeded at 287 (offset -1 lines).
    Hunk #6 succeeded at 309 (offset -1 lines).
    Hunk #7 succeeded at 332 (offset -1 lines).
    Hunk #8 succeeded at 362 (offset -1 lines).
    Hunk #9 succeeded at 379 (offset -1 lines).
    Hunk #10 succeeded at 428 (offset -1 lines).
    patching file modules/aggregator/aggregator.test
    patching file modules/block/block.admin.inc
    patching file modules/block/block.module
    Hunk #4 succeeded at 539 (offset -3 lines).
    patching file modules/blog/blog.pages.inc
    patching file modules/blogapi/blogapi.module
    Hunk #3 succeeded at 584 (offset 11 lines).
    Hunk #4 succeeded at 755 (offset 11 lines).
    patching file modules/book/book.admin.inc
    patching file modules/book/book.module
    patching file modules/color/color.module
    Hunk #1 succeeded at 2 with fuzz 1.
    patching file modules/comment/comment.install
    Hunk #1 FAILED at 17.
    1 out of 1 hunk FAILED -- saving rejects to file modules/comment/comment.install.rej
    patching file modules/comment/comment.module
    Hunk #3 succeeded at 393 with fuzz 2.
    Hunk #4 succeeded at 581 (offset -2 lines).
    Hunk #5 succeeded at 1297 (offset -1 lines).
    Hunk #6 succeeded at 1320 (offset -1 lines).
    Hunk #7 succeeded at 1458 with fuzz 1 (offset -3 lines).
    Hunk #8 succeeded at 1721 (offset -4 lines).
    Hunk #9 succeeded at 2006 (offset -4 lines).
    patching file modules/contact/contact.admin.inc
    patching file modules/contact/contact.install
    patching file modules/contact/contact.module
    patching file modules/contact/contact.pages.inc
    patching file modules/dblog/dblog.admin.inc
    patching file modules/dblog/dblog.module
    patching file modules/dblog/dblog.test
    patching file modules/filter/filter.admin.inc
    patching file modules/filter/filter.module
    Hunk #2 FAILED at 179.
    Hunk #3 FAILED at 316.
    Hunk #7 FAILED at 673.
    Hunk #9 FAILED at 694.
    Hunk #11 succeeded at 1165 (offset -2 lines).
    4 out of 11 hunks FAILED -- saving rejects to file modules/filter/filter.module.rej
    patching file modules/forum/forum.admin.inc
    patching file modules/forum/forum.install
    patching file modules/forum/forum.module
    Hunk #12 FAILED at 485.
    Hunk #13 succeeded at 502 (offset -8 lines).
    Hunk #14 succeeded at 518 (offset -8 lines).
    Hunk #15 succeeded at 650 (offset -8 lines).
    Hunk #16 succeeded at 706 (offset -8 lines).
    Hunk #17 succeeded at 861 (offset -8 lines).
    Hunk #18 succeeded at 890 (offset -8 lines).
    1 out of 18 hunks FAILED -- saving rejects to file modules/forum/forum.module.rej
    patching file modules/forum/forum.pages.inc
    patching file modules/forum/forum.test
    patching file modules/locale/locale.module
    patching file modules/menu/menu.admin.inc
    Hunk #1 FAILED at 607.
    Hunk #2 FAILED at 625.
    2 out of 2 hunks FAILED -- saving rejects to file modules/menu/menu.admin.inc.rej
    patching file modules/menu/menu.module
    patching file modules/node/content_types.inc
    patching file modules/node/node.admin.inc
    patching file modules/node/node.module
    Hunk #3 succeeded at 1262 (offset 9 lines).
    Hunk #4 succeeded at 1380 (offset 9 lines).
    Hunk #5 succeeded at 1725 (offset 9 lines).
    Hunk #6 succeeded at 1786 (offset 9 lines).
    Hunk #7 succeeded at 1806 (offset 9 lines).
    Hunk #8 succeeded at 1817 (offset 9 lines).
    Hunk #9 succeeded at 1850 (offset 9 lines).
    Hunk #10 succeeded at 2380 (offset 40 lines).
    patching file modules/node/node.pages.inc
    Hunk #1 succeeded at 72 (offset 5 lines).
    Hunk #2 succeeded at 186 (offset 6 lines).
    patching file modules/openid/openid.module
    patching file modules/path/path.admin.inc
    Hunk #1 succeeded at 97 (offset 2 lines).
    Hunk #2 succeeded at 107 (offset 2 lines).
    patching file modules/profile/profile.install
    patching file modules/profile/profile.module
    patching file modules/search/search.admin.inc
    patching file modules/search/search.install
    patching file modules/search/search.module
    Hunk #2 succeeded at 334 (offset -3 lines).
    Hunk #3 succeeded at 362 (offset -3 lines).
    Hunk #4 succeeded at 439 (offset -3 lines).
    Hunk #5 succeeded at 448 (offset -3 lines).
    patching file modules/simpletest/drupal_web_test_case.php
    Hunk #1 succeeded at 681 (offset 55 lines).
    Hunk #2 succeeded at 705 (offset 62 lines).
    Hunk #3 succeeded at 765 (offset 62 lines).
    Hunk #4 succeeded at 797 (offset 63 lines).
    patching file modules/simpletest/simpletest.install
    patching file modules/simpletest/simpletest.module
    Hunk #1 succeeded at 2 with fuzz 1.
    patching file modules/simpletest/simpletest.test
    Hunk #1 succeeded at 50 (offset 9 lines).
    patching file modules/statistics/statistics.admin.inc
    Hunk #3 FAILED at 109.
    1 out of 5 hunks FAILED -- saving rejects to file modules/statistics/statistics.admin.inc.rej
    patching file modules/statistics/statistics.install
    patching file modules/statistics/statistics.module
    Hunk #3 succeeded at 80 with fuzz 2 (offset 5 lines).
    Hunk #4 succeeded at 202 with fuzz 2 (offset 14 lines).
    Hunk #5 FAILED at 211.
    Hunk #6 succeeded at 267 (offset 14 lines).
    Hunk #7 succeeded at 278 (offset 14 lines).
    Hunk #8 succeeded at 293 (offset 14 lines).
    Hunk #9 succeeded at 349 (offset 14 lines).
    1 out of 9 hunks FAILED -- saving rejects to file modules/statistics/statistics.module.rej
    patching file modules/syslog/syslog.module
    patching file modules/system/image.gd.inc
    patching file modules/system/system.admin.inc
    Hunk #6 succeeded at 1206 (offset 12 lines).
    Hunk #7 succeeded at 1280 (offset 12 lines).
    Hunk #8 succeeded at 1342 (offset 12 lines).
    Hunk #9 succeeded at 1351 (offset 12 lines).
    Hunk #10 succeeded at 1370 (offset 12 lines).
    Hunk #11 succeeded at 1382 (offset 12 lines).
    Hunk #12 succeeded at 1394 (offset 12 lines).
    Hunk #13 succeeded at 1457 (offset 12 lines).
    Hunk #14 succeeded at 1476 (offset 12 lines).
    Hunk #15 succeeded at 1499 (offset 12 lines).
    Hunk #16 succeeded at 1559 (offset 12 lines).
    Hunk #17 succeeded at 1566 (offset 12 lines).
    Hunk #18 succeeded at 1573 (offset 12 lines).
    Hunk #19 succeeded at 1582 (offset 12 lines).
    Hunk #20 FAILED at 1594.
    Hunk #21 FAILED at 1615.
    Hunk #22 FAILED at 1636.
    Hunk #23 succeeded at 1687 (offset 12 lines).
    Hunk #24 succeeded at 1694 (offset 12 lines).
    Hunk #25 succeeded at 1710 (offset 12 lines).
    3 out of 25 hunks FAILED -- saving rejects to file modules/system/system.admin.inc.rej
    patching file modules/system/system.install
    Hunk #2 FAILED at 171.
    Hunk #5 succeeded at 2018 (offset -4 lines).
    Hunk #6 succeeded at 2045 (offset -4 lines).
    Hunk #7 succeeded at 2070 (offset -4 lines).
    Hunk #8 succeeded at 2493 (offset -4 lines).
    Hunk #9 succeeded at 2506 (offset -4 lines).
    Hunk #10 succeeded at 2621 (offset -4 lines).
    Hunk #11 succeeded at 2685 (offset -4 lines).
    Hunk #12 succeeded at 2856 (offset -4 lines).
    Hunk #13 succeeded at 3048 (offset 6 lines).
    1 out of 13 hunks FAILED -- saving rejects to file modules/system/system.install.rej
    patching file modules/system/system.module
    Hunk #1 succeeded at 42 with fuzz 1.
    Hunk #4 succeeded at 799 (offset 7 lines).
    Hunk #5 succeeded at 810 (offset 7 lines).
    Hunk #6 succeeded at 840 (offset 7 lines).
    Hunk #7 succeeded at 855 (offset 7 lines).
    Hunk #8 succeeded at 901 (offset 7 lines).
    Hunk #9 succeeded at 945 (offset 7 lines).
    Hunk #10 succeeded at 960 (offset 7 lines).
    Hunk #11 succeeded at 1244 (offset 7 lines).
    Hunk #12 succeeded at 1269 (offset 7 lines).
    Hunk #13 succeeded at 1283 (offset 7 lines).
    Hunk #14 succeeded at 1365 (offset 7 lines).
    Hunk #15 succeeded at 1439 (offset 7 lines).
    Hunk #16 succeeded at 1996 (offset 6 lines).
    Hunk #17 succeeded at 2052 (offset 6 lines).
    Hunk #18 succeeded at 2146 (offset 6 lines).
    patching file modules/system/system.test
    patching file modules/taxonomy/taxonomy.admin.inc
    Hunk #1 succeeded at 666 (offset -23 lines).
    patching file modules/taxonomy/taxonomy.module
    Hunk #2 FAILED at 493.
    Hunk #3 succeeded at 1153 (offset 38 lines).
    Hunk #4 succeeded at 1175 (offset 38 lines).
    1 out of 4 hunks FAILED -- saving rejects to file modules/taxonomy/taxonomy.module.rej
    patching file modules/taxonomy/taxonomy.pages.inc
    Hunk #1 succeeded at 48 with fuzz 1.
    patching file modules/update/update.fetch.inc
    Hunk #1 FAILED at 51.
    1 out of 3 hunks FAILED -- saving rejects to file modules/update/update.fetch.inc.rej
    patching file modules/update/update.module
    Hunk #2 succeeded at 265 (offset -6 lines).
    Hunk #3 FAILED at 286.
    Hunk #4 succeeded at 347 (offset -4 lines).
    Hunk #5 succeeded at 416 (offset -4 lines).
    1 out of 5 hunks FAILED -- saving rejects to file modules/update/update.module.rej
    patching file modules/update/update.report.inc
    Hunk #1 FAILED at 26.
    1 out of 2 hunks FAILED -- saving rejects to file modules/update/update.report.inc.rej
    patching file modules/update/update.settings.inc
    patching file modules/upload/upload.admin.inc
    patching file modules/upload/upload.module
    Hunk #5 FAILED at 226.
    1 out of 6 hunks FAILED -- saving rejects to file modules/upload/upload.module.rej
    patching file modules/upload/upload.test
    patching file modules/user/user.admin.inc
    patching file modules/user/user.install
    Hunk #1 FAILED at 251.
    1 out of 1 hunk FAILED -- saving rejects to file modules/user/user.install.rej
    patching file modules/user/user.module
    Hunk #3 FAILED at 267.
    Hunk #5 FAILED at 459.
    Hunk #10 FAILED at 833.
    Hunk #14 succeeded at 1275 (offset 5 lines).
    Hunk #15 FAILED at 1358.
    Hunk #16 succeeded at 1482 (offset 8 lines).
    Hunk #17 succeeded at 1527 (offset 8 lines).
    Hunk #18 succeeded at 1542 (offset 8 lines).
    Hunk #19 succeeded at 1552 (offset 8 lines).
    Hunk #20 succeeded at 2042 (offset 8 lines).
    Hunk #21 succeeded at 2083 with fuzz 1 (offset 8 lines).
    Hunk #22 succeeded at 2147 (offset 8 lines).
    Hunk #23 succeeded at 2155 (offset 8 lines).
    Hunk #24 succeeded at 2272 (offset 8 lines).
    Hunk #25 succeeded at 2300 (offset 8 lines).
    Hunk #26 succeeded at 2316 (offset 8 lines).
    Hunk #27 succeeded at 2331 (offset 8 lines).
    Hunk #28 succeeded at 2386 (offset 8 lines).
    Hunk #29 succeeded at 2418 (offset 8 lines).
    4 out of 29 hunks FAILED -- saving rejects to file modules/user/user.module.rej
    patching file modules/user/user.test
    Hunk #1 FAILED at 44.
    Hunk #2 FAILED at 75.
    2 out of 6 hunks FAILED -- saving rejects to file modules/user/user.test.rej
    patching file profiles/default/default.profile
    patching file themes/chameleon/chameleon.theme
    
    agentrickard’s picture

    Status: Needs work » Needs review
    FileSize
    305.66 KB
    Failed: Failed to apply patch. View

    New patch. The system tests pass, but I have not run the entire testing suite.

    This version does not address Crell's notes above. But I think leaving the parameter in would lead to confusion.

    RobLoach’s picture

    Status: Needs review » Needs work
    FileSize
    292.28 KB
    Failed: Failed to apply patch. View

    Updated the variable tests (modules/simpletests/tests/bootstrap.text)...... The variable default test seems to fail. Any better way of writing this?

        // Retrieving default values for variables.
        $default_cron_key = 'drupal'; // The cron_key variable from System module.
        $cron_key = variable_get('cron_key');
        $this->assertIdentical($cron_key, $default_cron_key, t('Retrieving default values for variables'));
    
    RobLoach’s picture

    Status: Needs work » Needs review
    FileSize
    292.3 KB
    Failed: Failed to apply patch. View

    Ahhh, it's because the cron_key variable is changed to a random string on install. Switching it to the "anonymous" variable fixed it! Great work! RTBC it!

    RobLoach’s picture

    FileSize
    293.64 KB
    Failed: Failed to apply patch. View

    The cross request variable test was flawed. Now it retrieves the value and checks to see if the values are equal instead of just seeing if we got content back. Variable tests all pass.

      /**
       * testVariable
       */
      function testVariable() {
        // Retrieving default values for variables.
        $default_anonymous = 'Anonymous'; // The default anonymous variable from System module.
        $anonymous = variable_get('anonymous');
        $this->assertIdentical($anonymous, $default_anonymous, t('Retrieving default values for variables'));
        
        // Setting and retrieving values.
        $variable = $this->randomName();
        variable_set('simpletest_bootstrap_variable_test', $variable);
        $this->assertIdentical($variable, variable_get('simpletest_bootstrap_variable_test'), t('Setting and retrieving values'));
    
        // Make sure the variable persists across multiple requests.
        $retrieved_variable = $this->drupalGet('system-test/variable-get');
        $this->assertIdentical($retrieved_variable, $variable, t('Variable persists across multiple requests'));
    
        // Deleting variables.
        variable_del('simpletest_bootstrap_variable_test');
        $variable = variable_get('simpletest_bootstrap_variable_test');
        $this->assertIdentical($variable, NULL, t('Deleting variables'));
      }
    
    maartenvg’s picture

    Status: Needs review » Needs work

    Contact test failed, attached fixes that. (Missing "access user profiles" permissions for test users. I wonder why previously it didn't fail, because that code hasn't been changed)

    The following tests still fail:
    - Aggregator -> Categorize feed item functionality
    - Aggregator -> Update feed item functionality
    - Search -> Search engine ranking

    maartenvg’s picture

    Now with file attached.

    maartenvg’s picture

    FileSize
    309.42 KB
    Failed: Failed to apply patch. View

    Wow, that is strange... perhaps a filename problem.

    Dave Reid’s picture

    I don't understand why the contact.test would need to be changed, since there's no access check of any kind in the contact module for the 'access user profiles' permission...

    maartenvg’s picture

    Yeah, you are right, ignore patch at #77. I thought users needed access to profiles to see the user's contact form on the profile, because that was what fixed the error and seemed logical.

    In that case there are 22 fails in contact.

    Dave Reid’s picture

    Patch in #74 applied cleanly. Contact module test 271 passes, 0 fail for me. Now checking all the rest of the tests...

    Dave Reid’s picture

    Fresh patched install with full test suite: 5771 passes, 8 fails

    Node revisions: 24 passes, 2 fails, 1 exception

    Revision reverted.	Other	node.test	82	NodeRevisionsTestCase->testRevisions()
    Revision deleted.	Other	node.test	90	NodeRevisionsTestCase->testRevisions()

    Search engine ranking: 20 passes, 6 fails, 13 exceptions

    Undefined offset: 0	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Trying to get property of non-object	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Search ranking "sticky" order.	Other	search.test	346	SearchRankingTestCase->testRankings()	
    Undefined offset: 0	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Trying to get property of non-object	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Search ranking "promote" order.	Other	search.test	346	SearchRankingTestCase->testRankings()	
    Undefined offset: 0	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Trying to get property of non-object	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Search ranking "relevance" order.	Other	search.test	346	SearchRankingTestCase->testRankings()	
    Undefined offset: 0	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Trying to get property of non-object	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Search ranking "recent" order.	Other	search.test	346	SearchRankingTestCase->testRankings()	
    Undefined offset: 0	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Trying to get property of non-object	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Search ranking "comments" order.	Other	search.test	346	SearchRankingTestCase->testRankings()	
    Undefined offset: 0	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Trying to get property of non-object	Notice	search.test	346	SearchRankingTestCase->testRankings()	
    Search ranking "views" order.	Other	search.test	346	SearchRankingTestCase->testRankings()
    Dave Reid’s picture

    The node revisions test failing is not consistent for me. Sometimes it passes with the patch, sometimes not. Arg. The search engine ranking test is consistently failing with the patch.

    chx’s picture

    Just a note, since this patch removes a parameter from the end of variable_get() but does not replace it with anything, variable_get('foo', 'some default') will still continue to work and function correctly. It may be easier to make a patch that just changes the API as needed and get that tested and committed, then strip out the 'some default' entries in later patch(es). That should make chasing head with this patch far easier, and reduce the collision with other patches currently pending. It would also make the patch easier to review, as there would only be 30 KB, not 300 KB. :-)

    This still stands. There were many calls for the removal of variable_ancestor into a followup patch. I can't see that either. Clean this up please, it's unreviewable and thus uncommittable.

    RobLoach’s picture

    FileSize
    51.04 KB
    Failed: Failed to apply patch. View

    Recreated the patch without variable_ancestor, and the removal of a bunch of variable_get parameter changes. Variable SimpleTest tests are all passing, but I'm getting a bunch of other SimpleTest fails.

    RobLoach’s picture

    Status: Needs work » Needs review
    FileSize
    43.24 KB
    Failed: Failed to apply patch. View

    Removal of all variable_get changes.

    Dries’s picture

    The DX experience degrades with this patch -- the current system works exactly how other programming languages, like Java, work. If this is meant to improve performance, we need to do some benchmarks. Otherwise, it is a misleading argument and the end result is not necessarily an improvement, IMO.

    Crell’s picture

    Quoth Dries:

    the current system works exactly how other programming languages, like Java, work.

    I do not understand that statement. How does a settings API (what we're talking about here) map to how things work in a programming language's native syntax?

    Personally I'm primarily interested in single-sourcing my variable defaults, and in the ability to then _alter those defaults for cases where core does something stupid like default comments to ON for all node types. (I mean really, how does that make sense? :-) ) That improves DX.

    dww’s picture

    Yeah, I couldn't disagree more with #86. ;) Wearing only the developer experience (DX) hat, let me summarize the benefits of this issue:

    a) Having to correctly specify the same default at every call site when you're looking up a variable is a serious DX pain in the ass, duplication of effort, and an API that makes it easy to introduce bugs. Having a single place to define all your defaults makes it easier to use and less error prone to get this right.

    b) ability to alter defaults

    c) we can automatically clear out settings on module uninstall

    d) better i18n possibilities down the road

    e) It'd let us build something that automatically added a "reset to default" checkbox (or something) on every single setting, instead of a global button for an entire page of settings. I believe this would be a big UX win, to let admins selectively reset things to defaults without necessarily clobbering all of their configuration.

    f) Makes it easier to document all the settings and their default values, so we could finally have a useful document to point people to for what they can put in their settings.php file, etc.

    g) Helps the DX of reviewing/auditing/understanding a module -- at a glance you can see all the settings that it's introducing. Currently, you have to grep for all the variable_get() calls, and trying to keep it straight in your head which ones are from core, which ones are from other modules, and which ones are native to the module you're looking at. Note that a lot of modules don't even call variable_set(), since that's handled via the system_settings_form submit handler, so grepping for that doesn't help you.

    If you're just looking at the patch from #85, you're missing a lot of the point. Eventually, the sorts of changes from a patch like #74 are going to happen -- we just decided to split it out to make it possible to review/commit this patch in the first place, then we can fix all the call sites in much smaller followup patches that won't constantly need re-rolling.

    I hope this helps clarify why this is a big DX win.

    catch’s picture

    Status: Needs review » Needs work

    I agree with both Crell and dww, I think this'll make things much more self-documenting. I also have an old site upgraded to 5.x from 4.5.4 which at one point had over 1500 rows in the variables table - other than going through manually, there's no way to clear that stuff out, since you can't go through and uninstall modules retrospectively and you can't tell if a variable is valid or not without this patch.

    Having said that, this patch causes notices on install, and triggers 'Drupal is already installed' before the user/1 account creation screen, so needs some work.

    RobLoach’s picture

    What if we added back an optional variable_get $default parameter, and used that if a default for the value wasn't provided in the hook_variables, cache or $conf? Then we can use both the default or hook_settings, yet still allow for the default to be manually passed through.

    agentrickard’s picture

    Piling on the DX train here. I have started implementing an internal module version of this just to avoid confusion about setting the default for every function call. This is a huge improvement.

    It also ties in to the changes proposed by #253569: DX: Add hook_modules to act on other module status changes and #306027: user_modules_uninistalled() is missing, which will let us automatically clear module variables on uninstall, so the developer has even less work to remember to do in hook_uninstall().

    agentrickard’s picture

    Title: New variable defaults to improve performance and help developers » DX: Use hook_variable to declare variables and defaults
    agentrickard’s picture

    Title: DX: Use hook_variable to declare variables and defaults » DX: Use hook_variables to declare variables and defaults

    More accurate title.

    Gábor Hojtsy’s picture

    Title: DX: Use hook_variables to declare variables and defaults » New variable defaults to improve performance and help developers

    Rob: that would invalidate some of the good things in this patch, since it would allow people to not use the variable registry, and therefore would make it impossible again to do automated uninstall, i18n data defaults, etc. That would make most of the benefits of this patch completely optional.

    Gábor Hojtsy’s picture

    Title: New variable defaults to improve performance and help developers » DX: Use hook_variables to declare variables and defaults

    Cross-posted.

    Damien Tournoud’s picture

    There is a huge issue with this patch:

    +    $invoked = TRUE;
    +    $defaults += module_invoke_all('variables');
    +    drupal_alter('variable_defaults', $defaults);
    

    All hook_variables are loaded at potentially *every* page load. Whooo, this probably hurts performance very badly (thanks Ken for changing the title).

    So we should either:
    - cache the defaults: we could use the same cache entry as 'variable' to avoid hitting the database twice
    - prefix the variable names with the module name and load the default values on a module-by-module basis

    killes@www.drop.org’s picture

    Although my earlier concerns have been addressed (thanks!). I still feel that this is rather overengineering than an improvement.

    agentrickard’s picture

    @killes

    Well, I don't know. I have taken to writing the following in all my modules. A smart extension of this would be huge, IMO.

    function mymodule_variables($name) {
      $variables = array(
        'mymodule_var_one' => 'one',
        'mymodule_var_two' => array(),
      );
      return variable_get($name, $variables[$name]);
    }
    

    And then I never use variable_get in my code. I use mymodule_variable('mymodule_var_one') instead. Having this as a core registry would eliminate a lot of duplicate code for me.

    Caching the defaults seems like a good idea -- couldn't we set default values in the variables cache when we rebuild it?

    dww’s picture

    @killes: Is it too much to ask to provide some justification for your feeling that this is "overengineering"? Lots of us have tried to be very specific for why we think this is a good patch. Vague replies like yours make it difficult to move forward.

    RobLoach’s picture

    DamZ at #96:

    So we should either:
    - cache the defaults: we could use the same cache entry as 'variable' to avoid hitting the database twice
    - prefix the variable names with the module name and load the default values on a module-by-module basis

    I attempted sticking a cache_set and cache_get in variable_default, but that resulted in a blow up because the functions arn't available then.

    earnie’s picture

    The con for this wonderful feature is that you can't just declare a variable wherever you feel like it. However the pros for this feature far outweigh the affect of the con.

    agentrickard’s picture

    @earnie

    The same could be said for hook_theme(). So not much weight to the 'con' argument from me.

    We have been steadily moving Drupal in a more formal direction with regard to declaring module functions up-front. This continues in that same direction.

    RobLoach’s picture

    FileSize
    43.21 KB
    Failed: Failed to apply patch. View

    Keeping the fuzz in check. The cache can't be used so early in the bootstrap that the cache isn't available. Does anyone know where variable_get is being called when cache.inc isn't available?

    agentrickard’s picture

    Rob-

    Couldn't you just add the default variable loading to variable_init()?

    
    /**
     * Load the persistent variable table.
     *
     * The variable table is composed of values that have been saved in the table
     * with variable_set() as well as those explicitly specified in the configuration
     * file.
     */
    function variable_init($conf = array()) {
      // NOTE: caching the variables improves performance by 20% when serving cached pages.
      if ($cached = cache_get('variables', 'cache')) {
        $variables = $cached->data;
      }
      else {
        $result = db_query('SELECT * FROM {variable}');
        while ($variable = db_fetch_object($result)) {
          $variables[$variable->name] = unserialize($variable->value);
        }
        // NEW LOOP TO LOAD THE UNSET DEFAULTS.
        module_invoke_all('variables');
        // do some handling of the defaults.
     
       cache_set('variables', $variables);
      }
    
      foreach ($conf as $name => $value) {
        $variables[$name] = $value;
      }
    
      return $variables;
    }
    
    dww’s picture

    Assigned: cwgordon7 » Unassigned

    @agentrickard #104: Doesn't that just exacerbate the concern that we'd therefore have to load all modules and all of their settings on every page load, regardless of which modules are actually necessary to render a given page? Doesn't that undo most of the RAM/performance gains of the code registry, etc? I don't have a great alternative all figured out to describe yet, but if/when I come up with one, I'll do that. ;)

    @DamZ #96: "prefix the variable names with the module name and load the default values on a module-by-module basis" To make the auto-uninstall stuff possible, way back in my original #4 post I suggested we add a "module" or "realm" column to the {variable} table so we know what module "owns" each row. I still haven't had time to figure out what happened in the code registry patch, nor looked at any of the code, so apologies if this is totally unfeasible, but perhaps we could still go this route and lazy-load defaults.

    p.s. I'm setting this to unassigned since Charlie hasn't touched this issue in a few months, and it's obviously a team effort at this point.

    Crell’s picture

    @dww: If we always had to load this hook, then we'd have to always load the file in which this hook resides... which need not be the .module file. I'd actually argue that most hooks should no longer live in .module files in D7 because of the registry. If they lived in some other, smaller file then only that file would need to be always loaded.

    That said, I agree that finding a way to not even do that is a good thing. Registry hooks (what this becomes) should by nature not be needed very often.

    agentrickard’s picture

    @dww -- no, we only have to load it on page loads when the variables cache is not valid. Kind of like rebuilding update_status. ;-)

    R.Muilwijk’s picture

    Unfortunalty I was not really aware of what was already going on with the variable system. I've started on some work which might be a bit similar to this solution, though not completely. I'd appreciate if you guys could have a look at it and tell me your initial thoughs and or I should work it out some bit more.

    Do not load all variables every page load.

    RobLoach’s picture

    Issue tags: +Patch Spotlight

    It's quiet... Too quiet..... Would #254491: Standardize static caching help?

    RobLoach’s picture

    Status: Needs work » Postponed
    Jose Reyero’s picture

    I agree with should push this other patch to speed up this one.

    About variables and default values, there's always this issue with textual variables and localizing the defaults. Also there are way too many modules using variables to store configurable texts. The worst example is user.module and _user_mail_text().

    So I'm thinking if we could somehow treat differently these text variables, this would be much easier, as we know defaults for these ones need to be localized. I propose replacing all these variable_get() for texts by a string_get() function which will:
    - check for a variable override
    - use localization system if available
    - handle string replacement by itself
    Something like:

    string_get('user', 'password_reset_subject', $variables, $langcode);
    

    Something similar though not really the same to this feature, #365934: Handle big texts / module text files / readable and localizable / get rid of _user_get_text()

    moshe weitzman’s picture

    Priority: Normal » Critical
    Status: Postponed » Needs work

    The variable table is terribly bloated since it saves all variables that are submitted, even if default. I have nearly new sites that have 800 variabls and those need to be unserialized on every page view. marking this as critical.

    I would like to see an update function added where we delete variables whose values are same as default.

    Perhaps in this issue or another issue, I would like to see schema change where we store the module that owns the variable and do automatic uninstall so modules need not bother with that.

    Gribnif’s picture

    Consider this a "+1" for inclusion in D7.

    There's another benefit of using a hook that I don't think was mentioned before now: it could also prevent name collisions, though the most recent patch doesn't seem to do that. While it's good practice to prefix variable names with the module's name, I'm sure there are plenty of programmers who don't. This could help prevent some potentially nasty consequences.

    deviantintegral’s picture

    Status: Needs work » Needs review
    FileSize
    43.03 KB
    Failed: Failed to install HEAD. View

    Here is an updated patch which applies against HEAD. Now that #88264: variable_get($name, $default = NULL) has landed, I wasn't sure how that change would be integrated, so currently the patch removes the parameter entirely.

    All tests currently pass on my local install. I'd like to eventually be able to write a contrib module to be able to export changed variables to code in a custom module, similar to how Views handles it. I think this hook will go a long way in reaching that goal.

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    snufkin’s picture

    subscribe

    nonsie’s picture

    subscribing

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    46.85 KB
    Failed: Failed to install HEAD. View

    A few things I noticed with this patch:
    - It breaks horribly when the patch is applied and the registry doesn't know the new variables hooks :)
    - Removing the default completely does not work - There are many dynamically built variables, for which no default is available. Re-added default parameter, which is only used when the returned value by $conf or default_variable() is NULL. This might be solved in a cleaner way later on (for example another type of variable_get() which allows to pass in multiple possible keys, similiar to the theme pattern system) but that is imo out of the scope of this patch.
    - constants added in that patch (because of the default variables configuration were included twice, removed in password.inc
    - patch failed to apply changes in statistics.module, no idea why.
    - I had to add field_storage_module to the defaults in variable_default() because that seems to be called sometimes before the registry is available, it seems. The re-added $default would make it possible to remove that list, but I left it there for now. No idea what is better...
    - removed a default variable 'comment' in comment module, it was referencing to a non-existent constant and is not used anywhere..

    Re-rolled patch against latest HEAD. I have some weird timezone issues with my test env again, no idea if the patch works...

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    FileSize
    48.16 KB
    Failed: 10538 passes, 26 fails, 9312 exceptions View

    got it :)

    The installer uses a different default value to check if a site_name has been saved already. I had to change that to directly check in global $conf and ignore default_variable().

    Berdir’s picture

    Status: Needs work » Needs review

    setting back to needs review...

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    48.2 KB
    Failed: 10545 passes, 19 fails, 0 exceptions View

    Default timezone value was set to 0 but needs to be UTC. Another try...

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    50.09 KB
    Failed: 10552 passes, 12 fails, 0 exceptions View

    Ok, last fixes, the line numbers of the errors changed... :)

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    50.12 KB
    Failed: 10563 passes, 1 fail, 0 exceptions View

    Ok, another bugfix. variable_set() did not correctly set variables when there is no default value, when the new value is 0, FALSE, '' or something else that is equal to NULL. Changed the comparison to be type-safe, but this only a workaround, imho.

    RobLoach’s picture

    Is there any possible way we could cache the call to hook_variables() in variable_defaults()? I understand that in some instances the cache setting isn't available, but it seems like a good place where we could save a lot of function calls to implementations of hook_variables().

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    cwgordon7’s picture

    The theme.inc portion of the patch seems weird - was it put in there by mistake?

    Also, cache_set()/cache_get() should be used in variable_default() - as mentioned above, this would be a huge performance hit otherwise.

    Berdir’s picture

    1 fail? Oh noes :p

    The failing test is: "Correct picture field." According to the test, the value should be '' (empty string), but is actually 0, set by the following code in user_save()

        $edit['picture'] = empty($edit['picture']->fid) ? 0 : $edit['picture']->fid;
    

    I cannot see how this can be changed because of that patch. the user_picture_default value is only used in a single place in template_preprocess_user_picture() and in the test, the user is loaded directly with user_load_multiple().

    Any idea?

    - theme.inc: Yes that change was introduced by accident. I will remove that.

    - Caching: I just wanted to get the patch working again, before thinking about introducing new features. Actually, I had just the idea to merge $conf and $defaults, because with the current patch, we access another function for every default setting. Also, using cache_get/set means one additional query per request. Just an idea, I need to check if that is actually possible and makes sence...

    RobLoach’s picture

    Status: Needs work » Needs review
    FileSize
    50.45 KB
    Failed: 10562 passes, 1 fail, 0 exceptions View

    Like this?

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    RobLoach’s picture

    Status: Needs work » Needs review
    FileSize
    50.72 KB
    Failed: 10562 passes, 1 fail, 0 exceptions View

    Adds some code documentation in system.api.php and removes the theme.inc change. Failed attempt at the caching.

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    I think the function_exists('cache_set') call is not needed, because that will always be available if module_invoke_all and drupal_alter is there. for example, cache_get/set is used in variable_init and I know that module_invoke_all is not available there.

    Looks good, my idea was to go a step further and merge the defaults with variables, so that there is only one cache 'variables' instead of 'variables' and 'variables_defaults', as both need to be loaded anyway. But I could not yet get it working.

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    51.68 KB
    Failed: 10562 passes, 1 fail, 0 exceptions View

    Found the reason for the failing test.

    user_load_multiple does use the wrong default value for user_pictures, it should be 0 but is 1. With that patch, the submitted default value is not used anymore but the one defined in user_variables():

          if (!empty($picture_fids) && variable_get('user_pictures', 1) == 1) {
    

    This means that the test actually tests a wrong behavior. Updated patch.

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    51.86 KB
    Failed: Failed to install HEAD. View

    Turned out, the caching code from RobLoach did not work as expected, because the whole thing was only executed once as it was inside a empty($defaults). The first called did set the default values and it was never touched again.

    Updated, the function follows the following logic now:

    3 Static variables:
    $defaults => obvious
    $loaded => FLAG if cache_get() was called already
    $built => FLAG if module_invoke_all etc. was called already

    1. if $reset is TRUE, reset $built, $defaults and clears cache
    2. if $defaults is empty, set always needed variables
    3. if $loaded and $reset is FALSE, set $loaded to TRUE, check cache_get()
    4. if $built is FALSE, $name is not in $defaults and the needed functions are available, set $built to TRUE, call the hooks and save $defaults to cache
    5. if $name is in $defaults, return $defaults[$name]
    6. return NULL

    Hints:
    - $built and $loaded needs to be set BEFORE cache_get/set to prevent endless loops because these functions call variable_get() too
    - I am still wondering if we could "inject" the variable_defaults cache into the variables cache and by-pass variable_default() in the following requests almost completely. Problem: every dynamic variable, like 'setting_'. $node->type, will still be passed through variable_defaults() and unecessary execute all the hooks. Possible solutions:
    a) If $default variable to variable_get() is passed in, ignore variable_default() and use the provided $default value => un-intuitive, all existing variable_get() calls need to be changed before this can be implemented
    b) add a flag to $conf/variables, something like 'variable_defaults_cache_enabled'. if set, variable_get() will not call variable_default() at all. (It will still call it for very early calls (before variable_init())

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    51.96 KB
    Failed: 10212 passes, 231 fails, 37 exceptions View

    Interesting, cache_get/set is available while installing but the cache tables are obviously not..

    Added a few MAINTENAINCE checks, attempt 8 to pass the tests.. :)

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    cwgordon7’s picture

    This is failing tests, so that needs to be fixed, but other than that, just a few more things I noticed, not all of which necessarily need to be addressed in this patch:

    1. I'd like to see a comment that explains the value of aggregator_clear.
    2. For blogapi_node_types, why do the values need to be run through check_plain()? This would seem contrary to Drupal's philosophy of filtering on output.
    3. Why is the code comment "Implementation of hook_init()" added to dblog.module?
    4. Why are a few blank lines removed from dblog.module?
    5. The function field_variables() should have its proper "Implementation of hook_variables() comment.
    6. Certain search variables aren't prefixed with "search." This can wait for a followup patch, though.
    7. Something is odd about the capitalization in "Menu callback; SimpleTest Bootstrap Variable test." It should probably be either "Menu callback; SimpleTest Bootstrap Variable Test." or "Menu callback; SimpleTest bootstrap variable test."
    8. The code comment for system_variables() is indented one space too many.
    9. There should probably be a code comment explaining the value of statistics_flush_accesslog_timer.
    10. The @return comment for hook_variables() should be on it's own line, for example:
      /**
       * The construction of any default variables associated with the given module.
       *
       * @return
       *   An associative array holding all default variables regarding the module.
       */
      

      Also, however, I'd prefer a more explanatory code comment there, such as:

      /**
       * Define the default values of any variables used by this module.
       *
       * @return
       *   An associative array, where the keys are the names of the variables, and
       *   the values are the default values to be used if not overridden by the
       *   administrator.
       */
      

      (The indentation is off on the inline-code comments, but the text should be good).

    11. This is outside of the scope of the patch, but mime_extension_mapping is scary, especially for the already overburdened variables table - if I want to change one of those, I'm going to have to resave the entire array to the database? Perhaps they would be better off as individual variables. The least we could do, however, would be to move that variables' definition to its own little function, and thus have something like 'mime_extension_mapping' => mime_extension_mapping_defaults() rather than some huge array. It actually confused me at first when I was looking at the function - I thought that each of the individual keys of the array was a variable itself, before I saw that it was actually just part of an array.
    12. Why the variable_get() call change in upload.module? It seems like that would belong best in another patch.
    13. Eventually I'd like to see a single-line comment for each variable in hook_variables(), but that can wait for a followup patch.
    14. The variables seem to be shoved into the hook in an arbitrary order. Could we perhaps alphabetize them?

    Other than that, though, this looks very good! Keep up the awesome work! :D

    Jose Reyero’s picture

    On a quick review, the patch looks good. There are some localization issues though, but they look easy to fix.

    As some variables are strings and the defaults need to be localized, we should
    a) Cache per language (just add language code to the cache key).
    b) Add an aditional language code parameter to variable_get() so it can be passed in turn to language_default(), thus allowing the retrieval of (default) variables in other languages than the current one. This is needed i.e. for the mail composing part which sends out e-mails in the user's language instead of the current request language.

    Dave Reid’s picture

    @cwgordon
    1. See your #13.
    3. Why not add documentation that was supposed to be there in the first place? :) /me shakes fist at undocumented modules like openid and dblog
    4. Only one space between functions (coding style).
    8. I don't see how it's indented wrong. Remember patch lines with no change always have an extra space at the start of the line.
    12. We don't need the second parameter for that variable_get() since it is automatic now. Oh, I see what you mean. We're leaving changing all the variable_get() calls for a sep. patch.
    13. Yes, very much agreed that we'd need to discuss commenting every variable and separate patch. :)
    14. Yeah it probably would be nice to have them alphabetised.

    Berdir’s picture

    FileSize
    52.02 KB
    Failed: Failed to apply patch. View

    Ok, last fails were because there was a endless loop in some hook_variables() that use t(), because t() uses variable_get() (dynamic name, so there is no default value anyway) itself. Fixed by moving $built above the module_invoke_all() calls.

    I really hope this is it now, these tests drive me insane :) (Don't get me wrong, I love to have them, implementing something like this without a test suite would be almost impossible...)

    Thanks for the reviews, I'll try to address the things pointed out, but I will need help for some of these, I don't know anything about most of these variables, for example :)

    Berdir’s picture

    Status: Needs work » Needs review
    cwgordon7’s picture

    Status: Needs review » Needs work

    Several of my points from #143 still apply here, so this is still needs work - though hopefully not for long! :D

    @Dave Reid
    1. Agreed.
    3. This really belongs in its own patch (see http://webchick.net/please-stop-eating-baby-kittens) ;)
    4. Again, belongs in its own patch.
    8. Er, by system_variables() I meant statistics_variables().
    12. Yeah, we're not converting all the calls in the patch, so this change doesn't belong here.
    13. Very much agreed. :)
    14. Awesome.

    @Berdir, you're awesome, keep doing what you're doing! :D

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    66.54 KB
    Failed: Failed to apply patch. View

    Awwww, green. Now, the reviews...

    @cwgordon7
    2: The whole function is crazy, it doesn't makes any sense to load the names (even use check_plain()) and then just check if blog is enabled. Changed to only check the blog type. Good example why we need to simplify node_get_types() : #220233: Put node_get_types() out of its misery :)
    3 & 4: Removed
    5: Fixed.
    7: changed to "Menu callback; SimpleTest bootstrap variable test.", this is beyond my english knowledge, no idea what's better.
    8: Fixed.
    10: Fixed.
    11: Added mime defaults to a separate function drupal_mime_extension_mapping_defaults() and also used that in file.inc (so that we don't have that huge list twice). I am not sure if that should be done in this patch (thinking of kittens) but it does make sense imho. A better approach to handle that list might be a separate table, which could be queried directly. This list could also be extended, if necessary. But that is obviously far beyond that patch.
    12: Fixed.
    14: Done.

    @Jose Reyero
    Hm, I can see what you mean, but I'm not sure if that should be solved as part of that patch. Making the variable_default cache language-aware does not solve the problem, simply because the variables patch isn't language-aware too. So it would only work as long as the text is not changed. Currently, (in core) there are only two places which use t() in hook_variables(), that is blogapi (indirectly, because it calls _node_build_types()) and contact. Maybe these can be somehow changed, for example, translate the text returned by variable_get().

    Berdir’s picture

    FileSize
    66.54 KB
    Failed: Failed to apply patch. View

    Updated to fix 2 minor code style issues pointed out by cwgordon7.

    Crell’s picture

    Non-critical DX point: The hook_variables() hook looks like an ideal case for a registry-style hook. That's a common Drupal pattern for hooks that return an associative array that gets cached in some form or another. For better DX, it should be called hook_variables_info() (which we use in a few places to indicate that type of hook, and need to use in more places), and should have a corresponding alter hook. (I didn't check closely enough to see if one is already there; but it's a dead-simple addition that considerably improves DX.)

    Berdir’s picture

    There is a alter hook but the name is different from hook_variables, it's hook_variable_defaults (not my decision :) ). I don't really care how it's named. Did I understand you correctly that you suggest "hook_variables_info()" and "hook_variables_info_alter($variables)"?

    Crell’s picture

    Yes. I've been pushing for the past year for us to standardize on the _info() suffix for registry hooks, as it makes it dead simple for new developers to recognize them and the related features they have. hook_variable_defaults() is completely obscure as an alter hook. Alter behavior should be named foo_alter(). Otherwise it's not an alter hook, no matter what data it may have.

    catch’s picture

    it's hook_variable_defaults_alter() - I kinda see the reason to add the defaults there - since we're altering the defaults only, and it won't have any effect if the variable has been overridden in the database. We don't want people using hook_variables_alter() instead of variable_set() right?

    Crell’s picture

    @154: Hm, fair point. That might be a legit exception to make, but I'm worried about the wtf factor. I still find hook_theme_registry_alter() to be completely weird, since hook_theme() is the base hook. (And both also take weird extra parameters I've never quite figured out.) Which then I suppose begs the question of whether the hook should be named hook_variables_defaults_info() instead of just hook_variables[_info](). But I fear that drifts into heavy bikeshed territory, so I am not going to push TOO hard on it as I really want this patch. :-)

    Either way we'll want to document the alter behavior super-extensively.

    catch’s picture

    hook_variables_info() and hook_variables_info_alter() wouldn't be so bad - it suggests altering the information about the variables, rather than the current value of the variables themselves, so maybe that'd be enough.

    dww’s picture

    Title: DX: Use hook_variables to declare variables and defaults » DX: Use hook_variables_info to declare variables and defaults
    Status: Needs review » Needs work

    works for me...

    Berdir’s picture

    FileSize
    66.54 KB
    Failed: Failed to apply patch. View

    @ Crell: Yes, I meant hook_variable_defaults_alter, but that doesn't make sense either :)

    Edit: Ignore the patch, I uploaded the old version.

    sun’s picture

    I wanted to stay out of this naming discussion, but...

    We have hook_menu, hook_theme, hook_perm, and with this patch, we have hook_variables. However, the correlating functions are called variable_get()/variable_set(). And we even add a function variable_default().

    Can we rename to hook_variable(), ie. make it singular for consistency?

    Berdir’s picture

    @sun: Currently, variable_* only gets/sets/deletes a single value (there are patches to change that). While hook_variables() can be used to define multiple default values. There is also the function drupal_initialize_variables(). On the other side, there is variable_init() which loads all variables.

    http://api.drupal.org/api/search/7/variables

    Again, I don't have an opinion on that, just pointing out what we have...

    Edit: Ignore the above patch, I uploaded the old version.

    dww’s picture

    Title: DX: Use hook_variables_info to declare variables and defaults » DX: Use hook_variable_info to declare variables and defaults

    sure, whatever.

    RobLoach’s picture

    Issue tags: +Bikeshed

    So, hook_variable_info() and hook_variable_info_alter()?

    dww’s picture

    Issue tags: -Bikeshed

    Yes. And we're done "discussing" that part. ;)

    Berdir’s picture

    I'm working on a update but I have some issues with caching and the tests right now. Will provide a patch soon, I hope :)

    Berdir’s picture

    FileSize
    66.85 KB
    Failed: Failed to install HEAD. View

    Turned out that my registry was borked and the hooks were not correctly called.

    Renamed the hooks to hook_variable_info() and hook_variable_info_alter($variables).

    Berdir’s picture

    Status: Needs work » Needs review
    Xano’s picture

    Subscribing.

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    66.81 KB
    Failed: Failed to apply patch. View

    Untested re-roll of the patch.

    Berdir’s picture

    This issue is now pretty long and many things were changed, removed or newly introduced. I'm trying to describe what this patch currently does, what it doesn't and what needs to be handled, in this patch or in a follow-up. Even I haven't read every comment, If something is wrong, please comment or ping me in IRC.

    What this patch does:
    - hook_variable_info(): A hook which allows each module to declare the variables they use, returned as array
    - hook_variable_info_alter($variables): A hook which allows to alter the variables other modules provide
    - variable_default(): A new function which is called by variable_get(). If possible, it loads the default variables, stores them in cache and if possible, returns the default value. The function also defines a few variable default, which are needed before the hook system is ready to use.
    - hook_variable_info() implementations for the core modules.
    - The default value to variable_get() can still be provided but is only used when variable_default() returns NULL. This is currently needed for dynamic variable names, like "mymodule_variable_" . $node->type and so on. See below.
    - variable_set checks if the value is the default and does not store it in {variables}

    What this patch doesn't (yet) do: (Some things need to be changed as part of that patch, some things not.)
    1. Correctly document the alter hook in system.api.php.
    2. Remove $reset from variable_default() and use drupal_static(). There is still some dicussing going on about that.
    3. A upgrade function which removes all unecessary variables (when they use the default value) from {variables}
    4. Automatic removal of variables when a module is uninstalled. This is not yet done in this patch, but would be pretty easy. However, it would also make sense to remove all variable_del() calls in hook_uninstall() which would make this patch unecessary big.
    5. Convert all variable_get() to not provide the default value. This would make the patch *huge* and it was decided to do that in a follow-up. The provided value is ignored anyway unless variable_default() returns NULL.
    6. Handling of dynamic variable names. The proposed solution was to automatically remove the dynamic part. Example: A module does use the following variable "mymodule_variable_${var1}_${var2}". variable_get() would check if there is a variable, if not a variable default and if not, remove "_${var2}" from the name and repeat until it is down to "mymodule_variable".
    7. file.inc loads the huge mimetype array with variable_get(). This will be stored in the cache. A possible solution to that is described at #331171-31: Allow modules to alter the MIME extension mapping rather than setting a huge variable.

    I will update the points 1-3 in the next update. I think the others should be handled in follow-up patches but I'm open for other ideas.

    Advantages/Features/Possibilities this patch/hook provides (direct or indirect):
    - It is not necessary anymore to provide the default value for each variable_get() call. This would for example make it very easy to automatically fill the #default_value with system_settings_form().
    - Does not store default values in the database, makes the variables table much smaller. We still need to cache the default values, however.
    - We could easily provide settings for generic node settings, for example for comments. With the automatic loading of default values described at 6, it would just work, no special handling necessary.
    - Automatic removal of variables on module uninstall.
    - Idea from DamZ: We could maybe change variable_get($name, $default_value = NULL) to variable_get($module, $variable). This would enforce to prefix all variables with the name of the module and we could even have a variable_get_all($module), which would return all variables of a module. This would eliminate some discussions I've seen at http://www.drupaltoughlove.com/ about storing the variables as an array.
    - Alternative caching: This was an idea I had but I'm not sure if and how it would exactly work: Instead of having two different caches, we could load the defaults and then "inject" them into the variables cache. On the next request, they would be part of $conf. This would mean that we only have a single cache entry and accessing the defaults would be as fast as saved values.

    Xano’s picture

    I'd say that if I do variable_get('foo_bar', 'default_value'), I want this default value to be used instead of the one defined in hook_variable_info(). Just my way of thinking.

    Also, why not hook_variable() instead of hook_variable_info()?

    +1 on variable_get($module, $variable).

    +    // We need to check if a variable has been saved, because variable_get
    +    // returns a different default value than we need here.
    +    global $conf;
    

    Globals are usually called at the beginning of the function.

    Berdir’s picture

    > I'd say that if I do variable_get('foo_bar', 'default_value'), I want this default value to be used instead of the one defined in hook_variable_info(). Just my way of thinking.
    I agree. However, it would currently mean that variable_default() is never used. This would make it hard to test. For example, there are many different states when bootstrap is happening and variable_default() needs to work with all of them and only use caching, module_invoke_all() and so on when it's the functions are available. There were also places where the same variable_get() call was used with different default values which resulted in unexpected behavior as it used a different value with that patch.

    If we still want a way to have a $default_value param after the conversion of all the variable_get() calls, we could change it to work like you suggest after that.

    > Also, why not hook_variable() instead of hook_variable_info()?
    See #151 and the following comments (http://drupal.org/node/145164#comment-1390134)

    > Globals are usually called at the beginning of the function.
    True, I will update that. If we would have a "force this default value" parameter as you suggest, we could still use that there. But... ;)

    Another thing I'd like to do is to write a script which loads the defaults and then compares them with all the variable_get() calls and cries if there are either names calls which are not in hook_variable_info() or different values. I'm quite sure that there are such cases because these lists haven't been touched since quite some time.

    Berdir’s picture

    FileSize
    67.3 KB
    Failed: 10686 passes, 1 fail, 0 exceptions View

    Changes:

    - Changed 1, 2 from above
    - moved global $conf to the start of the function
    - fixed a few coding styles (false, true)

    I'd like to hear which points from #170 should be fixed in this patch too.

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    David_Rothstein’s picture

    Status: Needs work » Needs review
    FileSize
    66.87 KB
    Failed: Failed to apply patch. View

    Rerolled patch attached; this should fix the failing test. The failure was due to some combination of drupal_static() not being used correctly for multiple variables, as well as the fact that the $refresh parameter had been partially but not completely removed... I also took out some debugging code comments that had been left in the patch, but otherwise did not make any changes.

    Reviewing this patch and reading the entire issue above, I'm still not sure about this part:

    +  // Do not store default values in the database.
    +  if ($value === variable_default($name)) {
    +    variable_del($name);
    +  }
    

    Which ironically was actually the original driver for this issue :)

    Here's the deal: When I save a settings form, I want all the settings to be stored in the database, not just some of them. Otherwise, what happens when the code changes? Let's suppose a module author decides the default value of 'foo' should change from 2 to 3 and releases a new version of the module that does this. So even though I've been running the module on my site for many many years and I've submitted the settings form many many times and consciously decided that I want 'foo' to be 2 on my site because it's ultra-important for my desired functionality, suddenly the module's behavior changes out from under me?

    It's not clear to me from the above discussion if this issue has been solved, but I don't think it has. I guess we could say that it's up to the module author to provide an upgrade path that deals with this situation, but we'd probably want to document that carefully since I don't think too many people would even think about doing that otherwise. Personally, I would consider just leaving it out for now -- I think this patch has tons of other benefits without attempting to reduce the size of the variables table on top of that. If there are giant variables that you don't want loaded into memory on every page load, then the correct solution when writing your code is simply to not store them in the variables table :) See #391924: Use Fields to store text variables, especially those that require text format support for an issue that would attempt to make this easier to do in D7.

    Berdir’s picture

    Argh, I should not upload patches after 2am, I should really know how to use drupal_static() ;)

    Hm, your point is valid, however, the only difference to what we have now is that you assume that the default values are saved in the variables table. But this happens only when the settings form is actually submitted, normally, you only do that if you want to use a non-default (or revert back to the) value of atleast one variable of a specific form. And I don't think that you visit all forms your site has just to save the default values to the database :)

    Because of that, I'd suggest to leave that piece there and clearly document (probably in the apidoc of hook_variable_info?) that you need to take care of the upgrade path (and probably note it in the release notes). I thought about providing a helper function for that, for example variable_update_default($variable, $old, $new). But I'm not sure, because if you need to change the type of a variable (from a TRUE/FALSE value to multiple values from a select, for example), you need to have a custom update function anyway.

    moshe weitzman’s picture

    @David - Technically you make a good point. But we have a an epidemic right now with cruft in the variables table. This has just gotten worse and worse for sites that have been up for a few years. Unserializing and loading all of them is a major performance problem on the sites I work on. I think this performance problem outweighs the 'conscious decision' problem that you highlighted. We can't solve this one with education - its been tried and failed. So, I hope this part of the patch stays in, and we add some UI later that lets admins choose to stop following default.

    Berdir’s picture

    @177
    Note that with this patch, we still have to load all default variables, unless we can implement some sort of lazy-loading, and I'm not sure what would be faster. The only difference is that they are not stored in {variables} + cache anymore but in hooks + cache.

    hass’s picture

    Are we able to automatically drop dead variables in the variables table with this approach? I would be happy if it is possible to clean up this table... :-)))

    earnie’s picture

    Re: 179:

    The only way I can think of to know if a variable is still active is to stamp the variable table with the module using it. But that is a different issue.

    dww’s picture

    @earnie: No, all modules declare their variables via hook_variable_info(), so we know exactly which module is exposing which variable. It's hard to imagine a scenario where one module (foo) exposes a variable, which another module (bar) uses, and then foo.module is disabled before bar.module is. I think any module (bar) which depends on the variables provided by another module (foo) needs to list that dependency in the .info file to be enforced at the modules page. Generally, the variables shared by many modules are those provided by core required modules which will never be disabled, anyway.

    @Berdir: Thanks for all your work on this. Re: the "unfinished business" in #170, of the items you list, I think only 170.6 ("Handling of dynamic variable names") should be considered in this patch. All of the others are either new features we can build on top of this change, or things that should be done in phase 2 just to keep the size of this patch within the bounds of reason to actually review. I think this area is where we should focus our attention at this point.

    @David: I'm with moshe on this (probably goes without saying). ;) The counter-argument to your clever scenario (which I'd guess is more common, but neither of us have any data to actually know) is that the site admin might have tweaked a setting or two, but never looked too closely at the rest of the defaults, and is trusting the module maintainer's decisions. The maintainer decides to change a default for some advanced setting based on their intimate knowledge of the workings of the code and why the new default is better, and the site owner is happy to be using the new default when they upgrade, without having to think about it.

    earnie’s picture

    So any variable not in the list of variables can be removed from the table? I have to give that more thought. My first inclination is to say "I don't think so" but I can't come up with a good scenario that fits. The first scenario I thought of was a batch job using the Drupal API but if it uses the API it would use hook_variable_info. However, that process itself may not be registered as a module so the module_load doesn't load it.

    So, a batch process foo that uses the Drupal API but doesn't register itself as a module could indeed cause a variable to be stored but not listed in the list of variables as defined by hook_variable_info. Has anyone been stupid enough to use Drupal in this manner? Of course, me. I have a process that uses the Drupal API to update tables in a different database than the default Drupal DB. I use that data to create nodes and taxonomy using node_save. The process is a script and not a module.

    Berdir’s picture

    Regarding cleanup of stalled variables data. Simple answer is no. We can only delete the variables that are explicitly defined in hook_variable_info and their default value is saved in {variables}. This is especially true for the upgrade path 6.x -> 7.x and contrib modules in general. Because contrib modules will most probably not be available when core is upgraded and because of that, their defaults will not be available too.

    catch’s picture

    I think we could do a contrib module for this - delete everything not defined by hook_variable_info() at the current time - or at least show what's there and give the option of deleting. But automated delete in a core upgrade path isn't going to happen.

    deviantintegral’s picture

    It appears that a comment in the patch at #175 mentions a hook_variables() function which doesn't exist:

    + * The default value for dynamic variables. Static variables defaults should
    + * be declared in hook_variables().

    Should that be hook_variable_info instead?

    deviantintegral’s picture

    I wonder if it's possible to avoid the whole issue of "dynamic" and "static" variables completely. IMO modules should be *required* to describe their variables, just as we do for every other registry in core. Otherwise, I think there will be many modules which don't implement hook_variable_info, simply because code will still work anyways.

    What if there was some kind of placeholder system for describing variable names? It could be as simple as a wildcard (mymodule_*_enabled), or perhaps something modelled after the menu system loaders (mymodule_%contenttype_enabled). A more structured system would require both a way for modules to describe their own variable placeholders, and for core to provide placeholders for commonly used variables. When updating internal variables, modules would have to be smart enough to know to get the variable registry rebuilt. So, it might be best to simply allow wildcards.

    Thoughts?

    dww’s picture

    @deviantintegral: The problem is that this info hook is not just declaring the variables, it's defining the default values. Unless we can safely assume that all dynamic variables that are defined by a wildcard use the same default (and someone would have to do some careful research to see if that assumption is true), then just declaring them via a wildcard is insufficient. I just reread most of this entire issue -- while the subject of dynamic variables has come up a few different times, no one seems to have gone through and audited all of core for exactly how they're used and what the implications would be of forcing all dynamic variables to use the same default for a given "root".

    Berdir’s picture

    #185
    Correct, I will update that.

    Regarding dynamic variables, we also have to keep in mind that variable_get() needs to be as fast as possible. the menu has to evaluate the given parameter only once, variable_get() can be called dozens of times. While I like the idea of wildcard, I don't think we (or atleast I) can implement that in a fast way.

    I've not looked through core completetly but some types of dynamic properties I've seen include:
    - comment_default_mode_' . $form['#node_type']->type, 'upload_extensions_' . $rid -> these and similiar should be fine

    - 'theme_' . $key . '_settings', 'color_' . $theme . '_files', 'taxonomy_' . $vocabulary->vid . '_synonyms', 'user_mail_' . $op . '_notify' -> Is not at the end of the variable, this makes it hard to handle
    - 'upload_' . $form['#node_type']->type, 'comment_' . $form['#node_type']->type -> Is imho too short and could lead to conflicts, see #270302: System variables conflict.
    - 'locale_custom_strings_' . $langcode -> a default value for all languages makes no sense

    JohnAlbin’s picture

    wow. really long thread. subscribing since I was just about to look at theme setting variables.

    David Strauss’s picture

    Dynamically named variables make me cry.

    Xano’s picture

    That may be, but core uses them heavily and the only replacement I see for variables like 'upload' . $node_type is that the variable upload becomes an array.

    Berdir’s picture

    I don't think dynamic variables are bad, however, it's bad that we use them so inconsistently.

    I'd be so happy if all dynamic variables would follow the rule "mymodule_variable_$dynamic1_$dynamic2". I'd even say we should add that as a coding style, especially when this issue will have a "automatic substitution for defaults"-feature (can someone come up with a good name for that, please? :) ).

    I have a somewhat working patch, which would go through the above example as:
    1. Check if there is a variable for "mymodule_variable_$dynamic1_$dynamic2". If not, check if there is a default variable for "mymodule_variable_$dynamic1_$dynamic2"
    2. If nothing has been found, check if there is a variable (and if not, a default variable) for "mymodule_variable_$dynamic1".
    3. If nothing has been found, check if there is a variable (and if not, a default variable) for "mymodule_variable".
    4. If nothing has been found, return $default_variable.

    Notes:
    - Because I decided to not go "deeper" as "mymodule_variable" and as long as we have dynamic variables like "theme_$name_settings", we can't get completetly rid of $default_value.
    - There are variables with NULL as default value, however, I currently use NULL to decide if variable_default() has found something. Not sure how to solve that conflict.

    earnie’s picture

    I'd be so happy if all dynamic variables would follow the rule "mymodule_variable_$dynamic1_$dynamic2". I'd even say we should add that as a coding style, especially when this issue will have a "automatic substitution for defaults"-feature (can someone come up with a good name for that, please? :) ).

    You're referring to module namespace of the variables. I would be happy to see variable_get/_set to require the module name as the first parameter and that is used in the DB as a separate column or concatenated as a prefix to the variable name. The API should take care of this for the user of the API, IMO. I know there are those that suggest we don't babysit the user but for this issue I think it really matters. If we change the variable_get/_set API footprint so that module name is a required parameter then module A could define variable FOO as well as module B could define variable FOO and both will have the values they set due to the namespace requirement.

    variable_get('drupal', FOO, 'default') for the variables in the includes directory makes sense.
    variable_get('system', FOO, 'default') for the core system module makes sense.

    I prefer adding a column to the variable table than the concatenation but I can live with concatenation.

    Berdir’s picture

    Yes, that's one of the things I'm referring to :) Not only that, also the order (module_variable_dynamicstuff).

    Sounds fine to me, And I know that others agree too, it's mentioned many times in this issue, starting by #3. But I'm not sure if we should do it in in this patch.

    earnie’s picture

    But hook_variable_info would be key to the upgrade path anyway. I've created #427654: Add module name parameter requirement for variable_get/_set/_del for continued discussion of variable_get/_set. The clean up of the variable table discussion should happen there as well, IMO, since that is what will create the variables in the table anyway.

    Crell’s picture

    If we have dynamic names, it makes sense to follow the same pattern as the theme system and use a double-underscore for separating static from dynamic parts. That also would avoid issues like node types that have an underscore in their name. So the example above would become:

    mymodule_stuff__$nodetype__$fieldname
    mymodule_stuff__$nodetype
    mymodule_stuff

    Of course, that introduces its own complication of what to do if mymodule__$nodetype IS defined, but mymodule_stuff__$nodetype__$fieldname is not.

    I would really rather make that a second patch. Even if we just deal with non-dynamic variables for now that's a huge, huge step forward that we can get into core and get converting and polishing, and then we can add dynamic handling in a follow-up.

    Berdir’s picture

    Crell's suggestion with __ sounds nice.

    I also agree to do the dynamic stuff in a follow-up patches, that should be easier to handle.

    If we have this commited, we can start converting dynamic variables, removing unecessary default variables and so on.

    dww’s picture

    Yeah, so long as we're reasonably confident we can solve dynamic variables later, which it sounds like we are, I'm fine with just getting this in ASAP for static variables (which are the overwhelming majority of them), and then start with the smaller followup patches.

    Is there anything we know of in #175 that actually needs work at this point? Or should we turn all eyes on that and get this RTBC?

    earnie’s picture

    Status: Needs review » Needs work

    I don't see an answer to #185. We need the documentation corrected.

    Berdir’s picture

    FileSize
    66.87 KB
    Failed: Failed to apply patch. View
    Berdir’s picture

    Status: Needs work » Needs review

    Above patch is a re-roll of #175 with the updated comment.

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review

    Re-roll, there was a conflict with recent syslog commits.

    Berdir’s picture

    FileSize
    69.14 KB
    Failed: 10790 passes, 2 fails, 0 exceptions View
    Berdir’s picture

    Status: Needs review » Needs work

    That was actually the wrong patch...

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    67.09 KB
    Failed: Failed to run tests. View

    This one should be good, I accidently added some dynamic stuff I was experimenting with.

    Berdir’s picture

    FileSize
    67.11 KB
    Failed: Failed to apply patch. View

    Fixed minor DBTNG coding style issue with db_merge().

    Berdir’s picture

    Some performance tests, I'm not sure how accurate they are.

    Used command: ab -n 200 -c 1 http://site/, No cache

    Without patch:
    Requests per second: 11.96 [#/sec] (mean)
    Requests per second: 12.02 [#/sec] (mean)
    Requests per second: 12.21 [#/sec] (mean)

    With patch:
    Requests per second: 10.57 [#/sec] (mean)
    Requests per second: 10.54 [#/sec] (mean)
    Requests per second: 10.50 [#/sec] (mean)

    So this seems to be quite slower, there are several reasons
    - no cache means that the hooks are called every time
    - According to xdebug, 175 calls to variable_default, over 5% of the overall time...
    - there are also over 500 calls from variable_default to drupal_static, but they only take up 0,4% of the time or so
    - We have currently only the (performance related) disadvantages of the page, we need to remove the default variables and if possible the default value to actually gain anything.
    - Another possible improvement might be my earlier idea: instead of a second cache, "inject" the defaults into the normal variables cache. That should almost completely bypass variable_default, because it will only be called for dynamic variables.

    It seems that with enabled cache, the differences are even bigger, I can't receive any constant numbers from my tests though..

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    37.26 KB
    Failed: Invalid PHP syntax in modules/simpletest/tests/common.test. View

    - Re-roll due to a conflict in common.test
    - Removed mimetype array related things, as that is hopefully going to be handled at #331171: Allow modules to alter the MIME extension mapping rather than setting a huge variable

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review

    Forgot to fix a conflict in common.test, re-roll

    Berdir’s picture

    FileSize
    37.17 KB
    Failed: Failed to run tests. View

    Forgot to fix a conflict in common.test, re-roll

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    37.17 KB
    Failed: Failed to run tests. View

    New try...

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    matt2000’s picture

    Status: Needs work » Needs review
    FileSize
    37.04 KB
    Failed: Failed to run tests. View

    re-rolled against latest HEAD

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    liquixis’s picture

    (sorry for my english)

    "Do not store default values in the database"

    If variable is set to it's default value, it still must be stored in DB, just for case of default value change (for example in next versions of module).

    I implemented similar approach ( http://drupal.org/node/472306 ).
    I will also put it here.

    
    /**
     * This add-on allows for a module:
     * - To specify structure and default values of all module settings in one place: "hook_get_settings()".
     * - Use variable naming standards:  "_" .
     * - Use defined default values everywhere (with option to override default value)
     *   Example:
     *     Instead of "variable_get('my_module_var1', 456)" in several places of a code, you can just use "setting_get('my_module', 'var1')".
     *     To override default value use the same approach as for "variable_get": "setting_get('my_module', 'var1', 987)".
     * - Automatic check for existence of setting at the moment of "set" or "get" and in case if setting it's not defined raise an error.
     * - You will never forget about to delete some variable at "hook_uninstall()", because you can just call "setting_delete_all('my_module')".
     * - Automatic specify default values ('#default_value') for admin settings form by using "setting_prepare_settings_form('my_module', $form)",
     *   this will also automatically rename element names from format '' to format ' "_" '
     *   (for proper processing with "system_settings_form_submit()").
     */
    
    function setting_get_settings($module) {
      global $settings;
    
      if (!isset($settings[$module])) {
        $settings[$module] = module_invoke($module, 'get_settings');
        if (!isset($settings[$module])) {
          $settings[$module] = array();
        }
      }
    
      return $settings[$module];
    }
    
    function setting_check($module, $name) {
      $module_settings = setting_get_settings($module);
    
      if (isset($module_settings[$name])) {
        return TRUE;
      }
      else {
        trigger_error(t('Setting %name in module %module is not exist.', array('%name' => $name, '%module' => $module)), E_USER_WARNING);
      }
    }
    
    function setting_get($module, $name, $value = NULL) {
      if (setting_check($module, $name)) {
        if (!isset($value)) {
          $module_settings = setting_get_settings($module);
          $value = $module_settings[$name];
        }
        return variable_get($module.'_'.$name, $value);
      }
    }
    
    function setting_set($module, $name, $value) {
      if (setting_check($module, $name)) {
        variable_set($module.'_'.$name, $value);
      }
    }
    
    function setting_get_all($module, $values) {
      global $conf;
    
      $result = array();
      $module_settings = setting_get_settings($module);
      foreach ($module_settings as $name => $default_value) {
        $result[$name] = (array_key_exists($module.'_'.$name, $conf) ? $conf[$module.'_'.$name] : (array_key_exists($name, $values) ? $values[$name] : $default_value));
      }
      return $result;
    }
    
    function setting_reset_all($module) {
      $module_settings = setting_get_settings($module);
      foreach ($module_settings as $name => $value) {
        variable_set($module.'_'.$name, $value);
      }
    }
    
    function setting_delete_all($module) {
      $module_settings = setting_get_settings($module);
      foreach (array_keys($module_settings) as $name) {
        variable_del($module.'_'.$name);
      }
    }
    
    function setting_prepare_settings_form($module, $form) {
      $result_form = $form;
      foreach ($form as $name => $element) {
        // If this is a form element
        if (($name{0} != '#') && is_array($element)) {
          // If this is an input form element
          if (in_array($element['#type'], array('checkbox', 'checkboxes', 'date', 'file', 'password', 'radio', 'radios', 'select', 'textarea', 'textfield', 'weight', 'hidden'))) {
            // If "default_value" applicable
            if (!in_array($element['#type'], array('file', 'password'))) {
              $element['#default_value'] = setting_get($module, $name);
            }
    
            // Rename element name in result form
            $result_form[$module.'_'.$name] = $element;
            unset($result_form[$name]);
          }
          else {
            $result_form[$name] = setting_prepare_settings_form($module, $element);
          }
        }
      }
      return $result_form;
    }
    

    Use example (exerpts from BitTorrent module):

    /**
     * Implementation of hook_get_settings().
     */
    function bt_torrent_get_settings()
    {
      global $base_url;
    
      return array(
        // Settings (Public)
        'override_announce' => 1,
        'override_announce_url' => module_exists('bt_tracker') ? $base_url .'/announce.php' : '',
        'scrape_cron' => 0,
        'scrape_cron_interval' => 7200,
        'scrape_cron_count' => 3,
        'scrape_cron_timeout' => 10,
    
        // Variables (Private)
        'last_scrape' => 0,
        'last_scrape_position' => 0,
      );
    }
    
    //...
    
    function bt_torrent_admin_settings() {
      $form['announce'] = array(
        '#type' => 'fieldset',
        '#title' => t('Announce URLs'),
        '#description' => t('Control where the torrents\' URLs will point.'),
        '#collapsible' => TRUE,
        '#collapsed' => FALSE,
      );
      $form['announce']['override_announce'] = array(
        '#type' => 'radios',
        '#title' => t('Override Announce URL'),
        '#options' => array(
          t('Leave Intact'),
          t('Append'),
          t('Override'),
        ),
        '#description' => t('This determines whether or not the announce URLs of the torrents being downloaded should be changed. Append will simply add your URL to the torrent\'s list of announce URLs, where Override will replace the current announce URL(s)'),
      );
      $form['announce']['override_announce_url'] = array(
        '#type' => 'textfield',
        '#title' => t('New Announce URL'),
        '#description' => t('This is the URL to use for the torrents being uploaded.')
      );
    
      $form['cron'] = array(
        '#type' => 'fieldset',
        '#title' => t('CRON Jobs'),
        '#description' => t('Controls what CRON jobs run within this module.'),
        '#collapsible' => TRUE,
        '#collapsed' => FALSE,
      );
      $form['cron']['scrape_cron'] = array(
        '#type' => 'radios',
        '#title' => t('Scrape other trackers'),
        '#options' => array(
          t('Do not scrape'),
          t('Scrape for statistics'),
        ),
        '#description' => t('Use CURL to load the statistics for torrents that are not tracked locally.'),
      );
      $form['cron']['scrape_cron_interval'] = array(
        '#type' => 'textfield',
        '#title' => t('Scrape Interval'),
        '#description' => t('If enabled, how often the tracker should scrape other trackers.'),
      );
      $form['cron']['advanced'] = array(
        '#type' => 'fieldset',
        '#title' => t('Advanced'),
        '#description' => t('Advanced settings for the cron run.  Note that altering these may make the cron run take longer to run.  Only alter these if you are certain that your php_execute_time is large enough to handle it.'),
        '#collapsible' => TRUE,
        '#collapsed' => TRUE,
      );
      $form['cron']['advanced']['scrape_cron_count'] = array(
        '#type' => 'textfield',
        '#title' => t('Scrapes per cron'),
        '#description' => t('How many trackers to scrape per cron run.'),
      );
      $form['cron']['advanced']['scrape_cron_timeout'] = array(
        '#type' => 'textfield',
        '#title' => t('Scrape timeout'),
        '#description' => t('How many seconds to wait for a tracker to be scraped.  Make sure that "Scrapes per cron" multiplied by "Scrape timeout" is less than your php_execute_time.'),
      );
      return system_settings_form(setting_prepare_settings_form('bt_torrent', $form));
    }
    
    liquixis’s picture

    This is very big thread to read it all.
    Could someone point me to where I can read a documentation for solution from this thread?

    Berdir’s picture

    If variable is set to it's default value, it still must be stored in DB, just for case of default value change (for example in next versions of module).

    Short answer: no.

    Reasons:
    a) A module developer doesn't know if a admin explicitly selected the default value or if he simply didn't changed it (and maybe doesn't care). If you change 1 option in a admin settings screen which has 20 options and settings, you are probably not interested in the other 19.
    b) If the type of a variable changes (for example, string -> array), the old variable needs to be converted, we can't just change it.
    c) That's the way it is currently handled too.

    Also see for example comments #175 and #181 for more discussion about this.

    liquixis’s picture

    It's clear.
    Are there any docs about all this?
    If there are no docs, where I can download basic functions to review them?
    Thanks.

    dww’s picture

    @overall: We do not document that which doesn't yet exist. This issue (and the comments in the attached patches) is the documentation about the proposed solution here.

    plach’s picture

    Alan D.’s picture

    ++ hook
    Get idea!

    ++ no defaults

    In regards to #175, #181, #219, #221.

    Not saving the defaults in the database is a nice idea, but significant side-effects do happen when the module maintainer decides to change their default preferences. A possible work-around would be to create a new update function that takes the old and new defaults into account. Module maintainers should call it if the changes to a default value are significant. Normally, the logic is too simple to warrant a function, but it make the issue more obvious to module maintainers when it is listed in the API. The following will fail on complex variables, it is too late to read the entire thread in detail, so I'm not sure if a better comparison operation is required.

    <?php
    function variable_update_default_value($name, $old_value, $new_value) {
      if (variable_get($name, $old_value) === $old_value) {
        variable_set($name, $old_value);
      }
    }
    ?>
    
    Xano’s picture

    When updating modules we can make the update script remove the old defaults and save the new ones automatically, which would be a +1 for DX. Something similar should be done anyway, since newer versions may introduce new variables or no longer use older ones.

    adrian’s picture

    I am going to take a stab at this patch.

    I suggested something similar many years ago : http://lists.drupal.org/pipermail/development/2005-November/011764.html

    I have also _just_ written a very similar system for Drush (namely the context system), so I am going to create a simplified port of what I did for that system to D7.

    I also maintained a similar patch for 3 major Drupal versions :
    #25745: Administrator configurable defaults to variables.

    How it works, is that instead of putting all the variables into one bucket (namely the global $conf array), the variables are stored in different realms.

    So the 3 realms I would have here are :

    'defaults' : these are populated by the hook_default_variables
    'database' : these are what the current variable system is like
    'runtime' : settings that are loaded at runtime, or act as overrides.

    We might end up needing more than these, but I am going to stick to them for now.

    The definition of variable_get and variable_set are modified slightly.

    function variable_get($name, $default = null, $realm = null) {}
    
    function variable_set($name, $value, $realm = 'database') {}
    

    If the realm is not specified for variable_get, it will retrieve the first value that is set in the highest priority queue (runtime then db then defaults).
    The default parameter also becomes optional from this, so variable_get('variable') will become valid.
    I found this a _necessity_ since I had very complicated defaults that needed to be set on every occurrence of the drush_get_option function, it got so bad i ended up defining my defaults as constants!
    I have yet to determine whether the default definition in the variable_get function should have a higher priority than the defaults realm, but we can discuss this.

    Variable_set will save the setting to the database by default, but it will also allow you to set the defaults or the variables for runtime. This will be achieved by using a drupal_static instead of a global.

    A runtime realm is a requirement because it is a necessary component for many other issues with the variable system, and will allow us to finally solve issues such as #155381: Minimum support for multilingual variables

    Essentially the runtime layer will allow modules to override variables as necessary. A good example of this would be the context module, or the aforementioned locale, which needs to override variables based on the currently active localization settings, for each page load.

    adrian’s picture

    Keep in mind, i am keeping the implementation simple for now.

    What we are actually in need of is namespaced variables, but I would like to get the basic patch in first, and then expand the system with more 'realms'

    Each module could get it's own realm potentially, but I don't believe we should 'boil the ocean' on the first phase.

    Damien Tournoud’s picture

    Just to clarify #227: we already have right now a "runtime" realm, it is called $GLOBALS['conf']. Its "interface" is:

    • variable_get($name, $default): get a variable from the runtime realm
    • $GLOBALS['conf'][$name] = $value: set a variable to the runtime real

    ... so it makes total sense to keep this and to at last make a proper interface out of this.

    dww’s picture

    I'm still opposed to being able to specify a default value at all during variable_get(). The fact you had to use constants for it is exactly the point: if it's a constant, you should define it in your hook, not have to lug around a constant for every variable_get() call site. Otherwise, the talk of realms seems fine to me, assuming it helps in some way (presumably performance).

    The $realm paramater only makes sense in variable_set() to be 'runtime' or 'database'. You can't set a hook-defined default this way. What should happen if someone invokes variable_set() with an invalid $realm? How can we make this behavior clear to developers?

    It's not immediately obvious why you'd ever want to specify the realm during variable_get() -- seems like you should just always get the "active" value of the variable, with a clearly documented ordering of realm precedence. Can you explain when you'd ever need to get the value from a specific realm? I guess it'd be nice to ask "what's the hook provided default?", compare that to "what's stored in the DB?", and if they're the same, remove the one from the DB. That's basically the operation I was talking about in the original post. ;) So, hrm, ok... I guess we could keep that. Still seems a bit yucky. I'd almost rather that was a completely separate function, variable_get_from_realm($variable, $realm), to avoid any potential for stupidity or confusion with the usual case of variable_get(), which, IMHO, should only take a single argument.

    adrian’s picture

    Ah, but it isn't really that you see.

    by setting the variable in $conf, it's impossible to see what the variable is in the database and it's impossible to set it to anything else than what is in runtime.

    With this mechanism you can still access the database saved variable using variable_get("variable", null, "database").

    The issue with $conf is that it stores everything in one bucket, and thus makes modules like Strong arm and the patches i mentioned necessary.

    dww’s picture

    re #231: Right, that's what I wrote to myself in #230. ;) However, I still think this "get a variable from a specific realm" is an edge case, and 99% of call sites should never use it. Therefore, I'd support making it a separate you-must-know-you-really-want-this function, so that in general, developers don't worry about (and get wrong and introduce bugs) the $realm when calling variable_get().

    adrian’s picture

    I agree.

    i hate the default variable in the function call too.

    The realm parameter becomes useful when moving variables between realms.

    variable_set('variable', variable_get('variable', 'runtime'));

    It's something I found i used a lot.

    I agree however we can make the realm access a separate function.

    adrian’s picture

    well. the 'edge case' you are talking about here is almost everywhere in forms.

    when you are editing the form, the runtime value should not be used, but the database value.

    if the runtime value is used, it means you can never set the value in the database.

    Jose Reyero’s picture

    This seems to be moving again, good :-)

    I think the 'realm' idea is ok but it just addresses part of the problem. One of the issues we are always running into and the main reason this is taking so long is that we have many different types of variables that need specific handling. There are numbers, there are strings, there are arrays....

    I.e. to get a textual variable, and for the defaults hook to work, we will always need the language to be passed on, just in case the default needs to be localized.

    Another issue is that variable functions are used in so many places that any small change to this system will take huge patches and hours of work. I can tell you because of the patches I've done, above in the thread... :-(

    So I'm thinking we should take some intermediate step here to be able to move on with this variables headache: We need modules to provide variables meta information so we can later figure out how to handle them for each case.

    mymodule_variables(
       $variables['myvariable'] = array(
          'name' => 'This is my text variable',
          'type' => 'text',
          'description' => 'Blah, blah, blah',
          'default' => 'Hey, this is a text so we know it must be localized',
      );
      return $variables;
    );
    

    Then, guess what, we can produce most settings forms on the fly, thus any subsequent patch will be much, much smaller...

      drupal_get_settings_form('mymodule'); // That's it, full form with variable names, descriptions.
      // And we can save some 'realms' here as this api function will know it needs to check current values from database.
    

    Think also of module install, uninstall, hook_variables_alter(), etc, etc... This approach has worked well for other parts of Drupal, and will open up unlimited possibilities for variable handling, like having different storage per variable type, etc, etc... Actually I think texts should be stored differently, which would move a lot of crap out of our variables table and serialized arrays, btw, #517044: Improve text handling, texts in text files (editable, overridable, translatable, etc...)

    This should be a big but simple patch, just I'm afraid no one would produce it unless we can get some sign ons for the idea beforehand.

    Also the patch by itself would do nothing but really, this is like a big monster moving so we need first to clean up the path for it to move on.

    adrian’s picture

    FileSize
    5.63 KB

    Here's an uncommented and largely untested version of the patch verifying the approach.

    Berdir’s picture

    Interesting idea.

    However, there are a few things that make this complicated and these things are mostly missing in the proof of concept patch...

    - The bootstrap phase is complicated, drupal relies on variable_get() extensivly before the database values are loaded and/or the modules/registry/hooks are available, that means that some variable defaults need to be hardcoded somewhere. Have a look at the variable_default() function in my last patch to see how many checks are needed...

    - There are also other issues, for example the installation and loops.... Simple loop example: hook_variable_info() -> t() -> variable_get() that relies on a default value...

    - The variable system does have a quite large impact on performance, with the approach in my patches, I was not able to get it as near as fast as without, especially when page caching is enabled. I have no idea how the impact of your patch is, but there is no caching and things like that, which make it even more complex....

    adrian’s picture

    - The bootstrap phase is complicated, drupal relies on variable_get() extensivly before the database values are loaded and/or the modules/registry/hooks are available, that means that some variable defaults need to be hardcoded somewhere. Have a look at the variable_default() function in my last patch to see how many checks are needed...

    I don't believe there's anything we can do about that. We can only provide defaults once enough of drupal is loaded to be able to run the defaults hook.

    - There are also other issues, for example the installation and loops.... Simple loop example: hook_variable_info() -> t() -> variable_get() that relies on a default value...

    Not sure what you mean by this.

    - The variable system does have a quite large impact on performance, with the approach in my patches, I was not able to get it as near as fast as without, especially when page caching is enabled. I have no idea how the impact of your patch is, but there is no caching and things like that, which make it even more complex....

    This can be easily cached, and in fact we can make it so that the realms are collapsed in a single visible namespace.

    This could be made so changing the variable in the running site is a bit more complex, but fetching it is the same. It's an extra array_merge per realm every load.

    Berdir’s picture

    I don't believe there's anything we can do about that.

    Neither do I. I just wanted to point out that it is something that is not yet solved in your patch.

    Not sure what you mean by this.

    Ok, I'm trying it again: Some default values are more complex, for example, they rely on node_get_types(), which itself relies on t(). And that does rely on variable_get() aka variable default values. That means that the defaults are used before they can be available.

    catch’s picture

    Variables dependent on node_get_types() are a bit wrong though - we have the node_type table for holding settings for node types, it's just that a lot of patches have added more settings as variables rather than node type properties as time has gone on. Would removing support for default for variable variables help with this at all?

    dww’s picture

    @catch: I believe Berdir is speaking for contrib, not core, and obviously, contrib shouldn't be modifying the {node_type} table... For core, I totally agree -- those things should *not* be settings in the variable system. But for contrib, there needs to be a reasonable solution. Each contrib maintaining its own {foo_node_type} table seems wrong, and clearly, so does modifying core's {node_type} table. The variable system seems like the most sane thing here.

    That said, why can't all per-node-type settings have the same default for any given setting? That's how the code presumably works already. E.g. og_content_type_usage_[node_type] -- shouldn't that always default to 'group_post_standard' regardless of [node_type]? That's how og uses it, anyway. So, perhaps all we need is a way to specify a wildcard or prefix or token or something in the hook that says "the default for any variable that looks like this should be that".

    Berdir’s picture

    Actually, I *do* speak about core to.. blogapi.module, for example: http://api.drupal.org/api/function/blogapi_admin_settings/7

    As you can see, the issue aren't "node type specific" but "for which node types should this be enabled" kind of settings.

    Crell’s picture

    If so many modules need to have per-type settings, and those should not be in the variable table for whatever reason, doesn't that seem like a good argument for a more extensible node type configuration storage mechanism? eg, a "node_type_options" table that is keyed on node type and a variable name, and maps to a value (preferably unserialized if possible). That could simply be saved off of a node_type_save() operation and a $type->options array.

    Maybe a bit off topic for this issue, but if node type-specific options are an issue here (and I know they're an issue for modules in general) maybe the solution is "well don't use this system for those" in the first place.

    dww’s picture

    @Berdir: that's effectively the same thing. A setting that holds an array of node types which should have a feature enabled is equivalent to a bool setting for each node type.

    @Crell: Agreed. Let's just break it here and force the hand of building a better system in a new issue. ;)

    adrian’s picture

    One of the things I want to get to in a later patch, is the ability for different realms, identified by keys.

    realms :
    user / uid
    node_type / type
    locale / locale

    and have the different realms be loaded as applicable.

    I don't think we need that for a first patch tho, but it's definitely the next point.

    sun’s picture

    Related to the last 3-4 follow-ups: #347988: Move $user->data into own table (node type data has been partially discussed there, too)

    catch’s picture

    Version: 7.x-dev » 8.x-dev

    It's a shame, but I think we have to move this to Drupal 8 since it's a far reaching API change, doesn't block anything else, and we don't have a viable patch at all.

    chx’s picture

    Version: 8.x-dev » 7.x-dev
    Category: feature » bug

    We went to far with realm and all. This is a MUST -- defaults are nice but that's not the important part: we currently have a good bunch of developer-only variables which desperately need description. Stuffing them into default.settings.php often feels inadequate when the intent is that some module will change them. Also it's not on api.drupal.org that way. I will quickly come with a small and easy patch and strongly say again this is a must for documentation purposes. My hook will contain default and description and that's all. Limit how far the patch goes or it does not go anywhere.

    chx’s picture

    The battle plan is to

    1. Only before writing the variable cache call the hook. So every time there is a variable cache hit during variable_init, during that request we have the necessary result in $conf without consulting anything else.
    2. Create a hook_boot and a normal variable cache to keep the hook_boot cache quick and efficient. If all goes well then, for cached pages we actually get a speed benefit which only gets bigger as you have more variables which now do not need unserialized.
    3. For dynamic variables, add a 'key' or 'base' in place of the default so that we run variable_get('node_options_' . $node->type, 'node_options') and description and defaults are stored under the key node_options.
    Crell’s picture

    I'd think we'd want a key/base in addition to default, not instead of. node_options_$foo should always default to 1/TRUE, regardless of what node type $foo is. Otherwise it sounds good, if we can still get it in.

    kbahey’s picture

    A centralized default is essential. Right now, the default is repeated in every variable_get() as the second argument, and this is wrong because it violates DRY and introduces the possibility of inconsistencies (one section in the code sets it to X, and another to Y).

    If we have one place where a variable is defaulted, then it is an improvement.

    Dries’s picture

    - I think we should get rid of the realm.

    - I'd love to see some examples of how we'd deal with dynamic variables (e.g. a content type setting where the name is not known ahead of time).

    David Strauss’s picture

    I'd love to see some examples of how we'd deal with dynamic variables

    I think these should all be converted to variables with static keys and arrays as values. It's pretty much impossible to keep the variables table clean when modules are creating ones on the fly.

    Dries’s picture

    I'll want to talk with webchick about it, but if she's comfortable making an exception, I'm happy making one too.

    webchick’s picture

    I guess we can make an exception, but -- call me a pessimist -- I don't really see how an issue that's been around since 2007 is going to get resolved in 2 weeks. :P We also have many other more pressing issues to deal with, like the fact that so far none of the big issues we explicitly marked as exceptions have hit a committable state. :\

    There is also nothing in replies #248+ that is any different than the current situation we've been dealing with since whenever the variables table was introduced. It's definitely worth fixing the DX situation of not having documentation of the possible variables (perhaps could be solved by define('variable_xxx', $default_value) in MODULE.api.php) as well as the annoyance of having to specify the default in every. single. variable_get(). call. But these were obviously not release blockers for Drupal 4.7, 5, and 6, so I'm not sure why they'd be release blockers for Drupal 7.

    catch’s picture

    Version: 7.x-dev » 8.x-dev

    Shame.

    JacobSingh’s picture

    double shame. All the monkeys know your name.

    Subscribing.

    aaron’s picture

    we're successfully using a version of this for the media module (and i have d6 versions in other places, because it's so handy). i still need to read this issue and find its status, but it would definitely be a boon all around. subscribe.

    RobLoach’s picture

    fgm’s picture

    @Rob loach: Variable is not related with this discussion (but comes with a nice security hole, luckily it has not been ported beyond 4.6).

    fgm’s picture

    Two additional pieces of information should likely be present in these variable declarations:

    • a "grouping" information: most variables can be configured on an automatic settings form, as suggested by several in this thread, but a given settings form:
      • can spread over several pages, grouped as tabs for a given package (module set)
      • is typically split in several fieldsets, which have to be present in the declaration in some form if the settings form is to be generated
    • a "context" information: some settings are defined on the unloved block configuration screens, not on normal settings forms, so in order to automate both block config forms and module settings forms, these two kinds of variables bust be differentiated

    The "context" part for a settings variable should likely be a default value of ("settings", $tab, $group), while a block variable would have something like ('block', $delta, $group), making a nice homogeneous two-level variable context, with block configuration forms not being tabbed anyway.

    Of course, this needs to be related with Blocks TNG.

    Xano’s picture

    It is a bad idea to link an API to forms so tightly (unless the API is meant for forms, which is not the case in this issue). Also, if we do this we'd have to define form element types, possible options, validation, etc. Last, but not least: system variables do not always need to be set by users.

    fgm’s picture

    I /think/ I see your point: any dependency on the form system should be a no-go, to keep a pure data-based API.

    However, in this case, the extra information provided in the variable info is just that: "extra", which the form system can use for a slicker UI, but that can just as well be missing, and in that case lead to default settings forms. This does not introduce any dependency on the UI in the variable flow.

    Bojhan’s picture

    Priority: Critical » Major
    Jose Reyero’s picture

    Just fyi, the idea of some hook_variable_info() is being worked out in contrib for D7, see http://drupal.org/project/variable

    Some feedback and help would be welcomed for that module.

    JimmyAx’s picture

    Subscribe.

    I haven't used the variable module as mentioned in #265, but I would love something like that in core for 8.x.

    JimmyAx’s picture

    Category: bug » feature

    Isn't this now a feature request?

    Lars Toomre’s picture

    I opened a new issue #1124198: DX: Finally implement revised Variable API in core to try to get some further discussion going about getting this need DX enhancement for the Variable API implemented in Drupal 8.

    wojtha’s picture

    I'm using something like this in my custom modules... Subscribe and +1 to get this into 8.x

    pounard’s picture

    Should this issue be linked with this initiative? Seems duplicated effort if you don't tie together.

    Gábor Hojtsy’s picture

    Yes, it is definitely duplicated effort, or at least that initiative needs to take into account this issue :)

    Berdir’s picture

    This issue is imho pretty much dead, I doubt much will happen here. With the current variable system, this just doesn't work (as in, is too slow). I think there are plans to replace the variable system with something else, which might solve the problems outlined here from the beginning, we'll see.

    Crell’s picture

    Issue tags: +Configuration system

    Tagging for the initiative. (As opposed to rolling for initiative.)

    q0rban’s picture

    subscribe

    bvanmeurs’s picture

    I've found a situation which makes the problem with variables even more clear.

    Theoretically, it is strange that a variable that has been set has one value, but a variable that has not been set can have different default values in different places. This causes a lot of developers headaches.

    For example we had a strange problem today. We use CKFinder, and suddenly on one particular website the pictures were uploaded to the Drupal root folder instead of /sites/{site-name}/files. We had to go through the source code of the CKFinder module to finally figure out the cause of this problem. Not inituitively, it was caused by simply opening the file path system settings and saving it (hence: not even modifying it!): /admin/config/media/file-system. Because when you save a system_settings_form, the variables will all be set, and this is the default value (''). This works well as long as all defaults everywhere in the variable_get function are set to the same default value, but CKFinder did use another default for the variable 'file_private_path'.

    When you use CKFinder, the file_private_path variable is used to identify the location to store the pictures and files uploaded in CKFinder. In the CKFinder module, if the file_private_path is not set, it defaults to 'conf_path() . '/files'' (see modules/ckeditor/includes/ckeditor.lib.inc). However, the default for file_private_path in the core is simply '' (or FALSE). So after saving the variable we the CKFinder used the empty string as file_private_path and files were stored at the Drupal root. We solved the problem by deleting the variable record from the database (ouch!). Good luck finding out the problem to the average Drupal user!

    I also posted a bug to the CKEditor/CKFinder module, because in theory all occurrences of variable_get('file_private_path', ...) should have the same default value. CKFinder should get the variable, then test if it is not false, and if so use it's default, like this:

    $fpp = variable_get('file_private_path', FALSE);
    if (!$fpp) $fpp = conf_path() . '/files';
    

    But of course the underlying reason for a lot of problems like these is the fact that you CAN specify different default values when this really leads to undesired behavior, and never to desired behavior (at least not until after opening and saving the configuration form associated with it). I am in favor of specifying all variables and their defaults using a hook. variable_get could drop parameter 2 (though it can still be optional for backwards compatibility) and the default specified in the hook would be used.

    hook_variable sounds like a good idea!

    Gábor Hojtsy’s picture

    I think the best solution for this in terms of Drupal 7 is in http://drupal.org/project/variable for Drupal 8, it is unlikely that we'll see the variable_* system as we know it, people in http://groups.drupal.org/build-systems-change-management/cmi are hard at work to replace that entirely, so I'm sure this feature request is valuable as input but not as code anymore.

    catch’s picture

    Status: Needs work » Postponed

    Yeah I think it's time to mark this postponed on the work in: http://groups.drupal.org/build-systems-change-management/cmi

    NBZ4live’s picture

    Hello,

    it would be a good thing to have a global solution in the future.

    For D7 && D6 I made a patch to add multilanguage capabilities to _user_mail_text().

    If there is a admin setting set it checkes if the needed language is english. If the language is not english it uses t() to translate the admin setting.

    So you can use the admin settings page of the user module, but the defalts you set must be in english and will be translated to the needed language.

    Best Regards,
    Sergey

    PS: THis is my first patch commit, so I am open for critique if I did something wrong=)

    meba’s picture

    @NBZ4Live - this patch doesn't belong into this issue. I believe the thing you need can be done using i18n_variables

    heyrocker’s picture

    Issue tags: -Configuration system

    Given that the variable system is going away in D8, I'm marking this as won't fix. If someone feels that's not appropriate, feel free to reopen.

    heyrocker’s picture

    Status: Postponed » Closed (won't fix)