Make variable subsystem pluggable via includes/variable.inc

There are a lot of issues around with the variables system indeed, and while I'm hoping we can get something good into core, this would allow improvements to happen in contrib projects - from working on some of the core patches it is very hard to get something clean that solves all the issues and passes tests, so allowing contrib to take over might be the better option.

If we're going to swap out the variable include file, just swap it out and include the right file early on as part of the base variable initialization. All of the wrapping _variable_set() and such only adds unnecessary complexity and CPU cycles. It is not possible for the variable implementation to change during a single request anyway.

If we did that, would have to be in http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal..., the patch would be shorter if we did that, and no private functions, so it makes sense.

It would mean you couldn't use variable_get() for the file include itself, but checking $GLOBALS['conf'] at that low a level is probably worth it to make the change less intrusive.

While we're hoping to nuke variable_get() completely in D8, the current work on the configuration proposal with file based storage, covers only half of what variable_get() is used for - it's also used for tracking volatile state changes (and other stuff) between requests - for use cases that the cache system is not able to cope with - see #1175054: Add a storage (API) for persistent non-configuration state. The system that replaces that stuff is going to have to deal with some of the multiple performance issues that variable_set() currently has in D8 and below (hopefully not memory so much, but definitely balancing read vs. write performance), so making it easier to experiment with that stuff in contrib may assist with the overall effort of replacing it.

I'd love the variable system to be pluggable, but the method of swapping out PHP files should most likely be replaced with #1497366: Introduce Plugin System to Core and #1497230: Use Dependency Injection to handle object definitions .

Ah, didn't even see the 7.x-dev in there. Okay! Carry on! :-)

It doesn't hurt. It works. Tests passes. It allows to play with alternatives in contrib.

+1

The variables system it's outdated and it would be awesome to be able to write modules around it.

Oh, not necessarily not keen on it. Just wanted to thinker on it some more, and get some feedback from folks like catch, etc. I'll take a look at your project.

Is there a reason this is not going into D8? I am aware that variable_get() is going away eventually, but #1175054: Add a storage (API) for persistent non-configuration state is still far off, and last time I checked (literally) CMI isn't going to be able to nuke variable_get() on its own. So ($better && $has_patch > $perfect && !$has_patch)?!

+++ b/includes/bootstrap.inc
@@ -740,6 +740,14 @@ function drupal_settings_initialize() {
+  if (isset($conf['variable_inc']) && file_exists(DRUPAL_ROOT . '/' . $conf['variable_inc'])) {

The additional filestat should be removed. We don't do that for any pluggable subsystem.

Overall, however, I'm not sure whether this makes sense. It has the potential of making the upgrade path to D8 more complex.

I totally agree with

my take on this is that sites that use alternative storage will have to migrate their variable data back in to {variable} before doing the upgrade. i don't think the core upgrade code should have to cater for contrib backends.

this.

A contrib backend can also just write variables back to the db too, then it'd be easier to switch back for whatever reason.

Haven't reviewed latest patch here. If this goes in, I will want to bump #1175054: Add a storage (API) for persistent non-configuration state to critical for 8.x, since we'll have a de-facto regression between releases until that's fixed.

What are the chances that someone out there has a script like this:

require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
some_low_level_drupal_function();

Where some_low_level_drupal_function() happens to call variable_get().

That would work OK before this patch (variable_get would fall back on its defaults) but result in a fatal error after it.

Not sure how likely that is, but I thought it was worth bringing up. I've certainly seen (in general) scripts that load some Drupal code to access a convenient API function but don't actually bootstrap Drupal in any way.

@David_Rothstein absolutely nil. If you do not bootstrap to at least DRUPAL_BOOTSTRAP_CONFIGURATION there is no Drupal function you can sensibly call. Aside from that, if you do not go up to DRUPAL_BOOTSTRAP_CONFIGURATION and yet call variable_get directly or indirectly you have bogus code because DRUPAL_BOOTSTRAP_CONFIGURATION includes settings.php which contains variable overrides.

@David_Rothstein - that's possible. but do we really test all our D7 patches against that criteria (must be able to run before DRUPAL_BOOTSTRAP_CONFIGURATION)?

Well, this patch seems like a fairly unique case, since it's physically moving commonly-used functions out of bootstrap.inc. But I do see what you're saying.

I guess we can probably go ahead with it.

But shouldn't we get this committed to Drupal 8 first? I don't see any reason to let the codebases diverge, especially with so little of Drupal 8 using config() currently and the likelihood that something more variable-like will still need to exist (for #1175054: Add a storage (API) for persistent non-configuration state or otherwise).

@David_Rothstein absolutely nil. If you do not bootstrap to at least DRUPAL_BOOTSTRAP_CONFIGURATION there is no Drupal function you can sensibly call.

Well, I do know of one, because I've done it :)

user_password()

It doesn't happen to call variable_get(), but it easily could.

Another example came up in IRC: Turns out filter_xss() winds up calling variable_get() indirectly. And filter_xss(), like user_password(), is also the kind of function you might call, e.g., via a custom script that runs on a Drupal site that doesn't quite exist yet.

I don't think we have to worry too much about this; people doing this kind of thing are living on the edge anyway, and the fix (manually loading variable.inc) is pretty easy. I think it just means we have to announce this change pretty clearly in the release notes so nobody deploying the next release of Drupal 7 gets surprised by it.

As I stated in #16 I agree with committing this to D8 first. If this only goes into D7 we have to open a critical task "Remove variable_get()", which we will probably achieve in D8 anyway, but is a pain because of the thresholds.

Did anyone test whether Drush still works with this patch applied?

Yeah I'd rather have this in D8 and rip it out later than have the code bases diverge, moving back.

@sun, I haven't tested drush with this patch, but I did test #973436: Overzealous locking in variable_initialize(). Any implementation that doesn't pre-load all variables into $conf would break drush vget, however I've posted a patch there at #1607072: Make drush vget compatible with alternative variable implementations which makes it work again.

There are some other parts of the drush variable commands (such as telling you whether a variable was originally set during drush vset) which would additionally break if you completely took things out of the variables table. However I think it's OK to require pluggable backends, at least during D7 to write to the variables table in variable_set() even if they don't use it for normal lookups.

There's a whitespace error in #23, but it still applies.

23: 1193396-23-variable-inc.patch queued for re-testing.

It's pretty close, but needs a reroll.

Reroll of#50.

Patch applies nicely in SimplyTest.me. What's the best way to test this to verify it won't interfere with existing sites?

This should be good to go, the change in t() has absolutely negligible performance implications because the first t() call in a normal Drupal bootstrap will fill in that $custom_strings[$options['langcode']] so basically add 1 if isset($conf['variable_inc') and 1 function_exists call to the whole bootstrap. But drush calls t before settings are loaded so it's necessary.

Drush indeed calls t() before bootstrap, such custom strings will be retrieved several times.
...
e.g. xmlsitemap was using variable_get in hook_drush_command()

So we now have at least two real-life examples of the problem discussed above (the first fixed by a workaround in the patch, the second not fixed here). I feel like we really don't have any idea how many scripts there might be out there that use a low-level function like variable_get() (or something like filter_xss() that calls variable_get()) before formally bootstrapping Drupal...

Reading over this issue, is there any reason we can't do anything more like the original patch (which added private functions), only without the separate include file?

In other words, something like this:

function variable_initialize($conf = array()) {
+  static $alternative_implementation_exists = function_exists('_variable_initialize');
+  if ($alternative_implementation_exists) {
+    return _variable_initialize($conf);
+  }
+
  // NOTE: caching the variables improves performance by 20% when serving
  // cached pages.
  if ($cached = cache_get('variables', 'cache_bootstrap')) {
    $variables = $cached->data;
  }
...

(and similar for the other functions)

It would still only require one line of code in settings.php to load in an alternative implementation - it would just need to do the require_once directly rather than setting $conf['variable_inc'].

So basically, same functionality - but all the API functions remain in bootstrap.inc where they've always been and continue to be available regardless of when someone tries to call them.

I would float the idea to make variable_initialize and variable_set overrideable but not variable_get for performance reasons. You must support GLOBALS['conf'] anyways because occassionally people will directly use it so there isnt really a good use case for overriding variable_get but if you really must you can make GLOBALS['conf'] something ArrayAccess and work from there.