DX: Use hook_variables to declare variables and defaults

This is a SLICK idea. Huge kudos!

I vote the last idea. Elegant.

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.

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:

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.

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

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.

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:

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 :))

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.

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.

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!

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*

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:

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...

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:

Attachment	Size
variables_new_api_00.patch	13.89 KB

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.)

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.

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...

Attachment	Size
variable_get.txt	41.57 KB

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.

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.

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(/

Attachment	Size
variables_new_api_01.patch	153.79 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.

Attachment	Size
variables_new_api_02.patch	155.68 KB

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.

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...

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

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.

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 :-(

Attachment	Size
variables_new_api_03.patch	164.16 KB

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.

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:

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

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:

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.

Attachment	Size
variables_new_api_04.patch	169.57 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.

Attachment	Size
variables_new_api_05.patch	186.16 KB

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

1. These names are poorly chosen:

They're not descriptive.

2. The coding style needs work.

3. The code comments need work.

4.

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:

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.

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.

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?

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

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

@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.

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.

Attachment	Size
variables_new_api_06.patch	171.67 KB

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.

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

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....

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.

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}

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

subscribe

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

subscribe

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. :)

Attachment	Size
variable_defaults.patch	265.49 KB

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

Attachment	Size
variable_defaults_02.patch	265.13 KB

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.

Re-rolled as per #50.

Er. Try again?

Attachment	Size
variable_defaults_03.patch	265.43 KB

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

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.

Attachment	Size
variable_defaults_05.patch	274.84 KB

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.

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.

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.

@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.

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!

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" is misspelled, and that comment isn't all that helpful. Don't you mean something like:

???

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?

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

?? 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:

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:

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:

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

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():

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. ;)

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 ;).