Problem/Motivation

There are no longer any usages of variable_get/set/del outside of the functions themselves and specific tests.

Proposed resolution

  1. Remove the Variable API/subsystem entirely.
  2. Remove the DRUPAL_BOOTSTRAP_VARIABLES bootstrap phase.

Remaining tasks

Review patch

User interface changes

None

API changes

Removes:

  • DRUPAL_BOOTSTRAP_VARIABLES bootstrap phase
  • variable_initialize()
  • variable_get()
  • variable_set()
  • variable_del()
  • Removes $new_phase parameter of drupal_bootstrap()
  • Removes {variable} table
  • Removes _drupal_bootstrap_variables()

Actual commit message

Issue #2167109 by Berdir, sun, alexpott, ACF, acrollet, adamdicarlo, Albert Volkman, andreiashu, andyceo, andypost, anenkov, aspilicious, barbun, beejeebus, boombatower, cam8001, chriscalip, chx, cosmicdreams, dagmar, damiankloip, dawehner, deviance, disasm, dixon_, dstol, ebrowet, Gábor Hojtsy, heyrocker, Hydra, ianthomas_uk, japicoder, jcisio, jibran, julien, justafish, jvns, KarenS, kbasarab, kim.pepper, larowlan, Lars Toomre, leschekfm, Letharion, LinL, lirantal, Lukas von Blarer, marcingy, Mike Wacker, mrf, mtift, mtunay, n3or, nadavoid, nick_schuch, Niklas Fiekas, ParisLiakos, pcambra, penyaskito, pfrenssen, plopesc, Pol, Rok Žlender, rvilar, swentel, tim.plunkett, tobiasb, tsvenson, typhonius, vasi1186, vijaycs85, wamilton, webchick, webflo, wizonesolutions, xjm, yched, YesCT, znerol: Remove Variable subsystem.

Files: 
CommentFileSizeAuthor
#42 variable-die-2167109-42-interdiff.txt767 bytesBerdir
#42 variable-die-2167109-42.patch46.54 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 63,323 pass(es). View
#38 variable-die-2167109-38-interdiff.txt12.91 KBBerdir
#38 variable-die-2167109-38.patch46.53 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 63,122 pass(es), 0 fail(s), and 3 exception(s). View
#37 36-37-interdiff.txt5.78 KBalexpott
#37 2167109.37.patch49.23 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 63,159 pass(es). View
#36 variable-die-2167109-36-interdiff.txt1.73 KBBerdir
#36 variable-die-2167109-36.patch55.36 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#32 2167109.32.patch57.09 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 63,231 pass(es), 2 fail(s), and 0 exception(s). View
#32 26-32-interdiff.txt1.32 KBalexpott
#26 2167109.25.patch57.54 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 63,262 pass(es), 2 fail(s), and 0 exception(s). View

Comments

aspilicious’s picture

Why are we opening a new critical task... Can't we do it in the meta? There was a patch in that issue already provided.

sun’s picture

Priority: Critical » Normal
FileSize
3.56 KB

The issue priority is irrelevant. A meta issue is not supposed to perform functional changes and the actual functional change might require more discussion.

Note that the bootstrap phase is removed separately in #2167105: Remove DRUPAL_BOOTSTRAP_VARIABLES — perhaps we're even able to proceed with that issue independently of the blockers.

xjm’s picture

Priority: Normal » Critical
Issue tags: +7.x-2.0 beta blocker

A meta issue is not supposed to perform functional changes and the actual functional change might require more discussion.

I'm not sure that this actually reflects current practice, but I'm entirely on board with marking #1775842: [meta] Convert all variables to state and/or config systems fixed when the blockers sun lists are fixed, and then handling the actual removal in this new issue. And this is critical, and a beta blocker. Thanks @sun.

xjm’s picture

Tag fail.

ianthomas_uk’s picture

We also need to remove the variable table itself, and @berdir pointed out that "there's also a VariableTest that you can remove and SystemTestController::variable_get() including the route that points to it"

There are still some direct uses of the variable table in update-related functions, but these can be ignored because these functions are now only useful as a reference for the migrate initiative.

There may also be places looking directly into the global $conf variable for items that used to be variables. The $conf variable is still used by CMI though - see #1881582: Change configuration overrides to use $config instead of $conf

sun’s picture

As long as we're able to cleanly separate the two parts of (1) the subsystem and (2) the subsystem's usage, it makes sense to distill, identify, and discuss the necessary changes independently of each other. We can merge the issues later, but it helps to have a clear picture first.

Thanks for the major pointer of missing the {variable} table! :)

To wit: One of the primary reasons to keep these functional changes separate:

  // NOTE: {variable} needs to be created before all other tables, as
  // some database drivers, e.g. Oracle and DB2, will require variable_get()
  // and variable_set() for overcoming some database specific limitations.
  $schema['variable'] = array(

Thankfully, the comment does not explain why and does not point (@see) to anything else. ;-)

However, my greps for this and the other issue did not yield any results in database drivers. So my only hope is that this comment is outdated and obsolete, but I'm not sure... :-/

Damien Tournoud’s picture

Yes, database drivers do not and cannot depend on the variable table, feel free to nuke this. At one point PostgreSQL depended on the schema, but we nuked that a long time ago.

sun’s picture

Issue summary: View changes
FileSize
18.3 KB

Merging #2167105: Remove DRUPAL_BOOTSTRAP_VARIABLES into this issue.

Attached patch removes all remnants of the variable system, including the bootstrap phase.

Leaving issue status postponed until the blockers are resolved.

Berdir’s picture

There are also a number of global $conf of which most can be removed now, and we have #1881582: Change configuration overrides to use $config instead of $conf for the remaining ones.

sun’s picture

Indeed, regex-grepping for \bconf\b yields a lot more instances.

Attached patch (1) removes obsolete instances and (2) changes remaining to $GLOBAL['conf'] or adds a @todo.

interdiff wasn't possible, failed to reconstruct file.

andypost’s picture

Suppose statistics.php could skip page cache #2094335: statistics.php needs a lower bootstrap level

Berdir’s picture

Status: Postponed » Needs review
FileSize
37.61 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/update.php. View

Re-roll.

I was able to bootstrap with this now, so I think it's time to let the testbot have a go with this :) Language negotiation tests will obviously still fail, and there's a weird node access test variable_set() left-over, but the issue for the first part is already RTBC and I want to review and RTBC #2175739: Actually test node_access_test_secret_catalan and clean up deprecated code in NodeAccessLanguageTest asap.

Status: Needs review » Needs work

The last submitted patch, 12: variable-die-2167109-12.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
37.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch variable-die-2167109-14.patch. Unable to apply patch. See the log in the details link for more information. View
278 bytes

Uhm, no idea where that was coming from.

sun’s picture

+++ b/core/includes/bootstrap.inc
@@ -1986,7 +1856,6 @@ function _drupal_bootstrap_page_cache() {
-    drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES, FALSE);

To my knowledge, that was and is the one and only use-case for the second $new_phase argument and (slightly weird) recursion logic of drupal_bootstrap().

→ Would be great to remove that here as well?

That said, another removal of that same call got lost in the re-roll: LanguageNegotiator::getConfiguration()

catch’s picture

Just committed the negotiation CMI conversion so this ought to pass now.

alexpott’s picture

Status: Needs review » Needs work

The last submitted patch, 14: variable-die-2167109-14.patch, failed testing.

alexpott’s picture

Berdir’s picture

FileSize
38.84 KB
FAILED: [[SimpleTest]]: [MySQL] 61,161 pass(es), 59 fail(s), and 57 exception(s). View
2.18 KB

Re-roll, Removed $new_phase, the second call has been removed already.

Fixed a notice in WebTestBase, that stuff there might need some more work...

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: variable-die-2167109-20.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
55.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch variable-die-2167109-23.patch. Unable to apply patch. See the log in the details link for more information. View
16.97 KB

Cleaning up DUBT tests.

webchick’s picture

Status: Needs review » Needs work

The last submitted patch, 23: variable-die-2167109-23.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
57.54 KB
FAILED: [[SimpleTest]]: [MySQL] 63,262 pass(es), 2 fail(s), and 0 exception(s). View

Reroll since core/lib/Drupal/Core/EventSubscriber/ConfigGlobalOverrideSubscriber.php no longer exists and a couple of minor comment edits since the phrase "Drupal variables" doesn't really make any sense.

alexpott’s picture

Issue summary: View changes

Improving issue summary and adding a suggested commit message.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/includes/bootstrap.inc
@@ -139,19 +139,14 @@
+const DRUPAL_BOOTSTRAP_FULL = 4;

And then there were 4!

Other than hook_update_N, upgrade tests, Migrate code, and core/scripts/generate-d6-content.sh, all variable code is gone.
Most of the additions to this patch are working around the setting of the request, or adding @todos.

I don't see why this isn't RTBC, so marking as such.

Berdir’s picture

There might still be a few test fails that need to be looked into, if you look at the test fails in #20, there is for example MigrateSimpletestConfigsTest that fails and that does look suspicios...

heyrocker’s picture

I've been out of things for a while, but what an amazing accomplishment! Three years of work finally coming to fruition. Can't thank everyone who has helped out on CMI enough.

webchick’s picture

While we're waiting on testbot, would someone be able to take on crafting an appropriate commit message that at least attempts to credit all of the various people involved? :)

alexpott’s picture

FileSize
1.32 KB
57.09 KB
FAILED: [[SimpleTest]]: [MySQL] 63,231 pass(es), 2 fail(s), and 0 exception(s). View

We still need to initialise global $conf in drupal_settings_initialize() since this is where settings.php is included.

ianthomas_uk’s picture

Here are the 80 people, in alphabetical order, who were credited on a patch with the commit message "Convert ... configuration system" or "Convert ... CMI". Sorry if I missed anyone.

ACF, acrollet, adamdicarlo, Albert Volkman, alexpott, andreiashu, andyceo, andypost, anenkov, aspilicious, barbun, beejeebus, berdir, boombatower, cam8001, chriscalip, chx, cosmicdreams, dagmar, damiankloip, dawehner, deviance, disasm, dixon_, dstol, ebrowet, Gábor Hojtsy, heyrocker, Hydra, ianthomas_uk, japicoder, jcisio, jibran, julien, justafish, jvns, KarenS, kbasarab, kim.pepper, larowlan, Lars Toomre, leschekfm, Letharion, LinL, lirantal, Lukas von Blarer, marcingy, Mike Wacker, mrf, mtift, mtunay, n3or, nadavoid, nick_schuch, Niklas Fiekas, no_commit_credit, ParisLiakos, pcambra, penyaskito, pfrenssen, plopesc, Pol, Rok Žlender, rvilar, sun, swentel, tim.plunkett, tobiasb, tsvenson, typhonius, vasi1186, vijaycs85, wamilton, webchick, webflo, wizonesolutions, xjm, yched, YesCT, znerol

The last submitted patch, 26: 2167109.25.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 2167109.32.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
55.36 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
1.73 KB

Bad testbot, bad! :)

Messing with our tests, tsss...

Will provide another patch that removes all the $conf/$config related todo's, I think that's unecessary, will just unecessary conflict with patches that are already in that issue and once this is in, it will simply have to get rid of all the $conf left-overs.

alexpott’s picture

FileSize
49.23 KB
PASSED: [[SimpleTest]]: [MySQL] 63,159 pass(es). View
5.78 KB

Here's a patch removing all the @todos

Berdir’s picture

FileSize
46.53 KB
FAILED: [[SimpleTest]]: [MySQL] 63,122 pass(es), 0 fail(s), and 3 exception(s). View
12.91 KB

Here's another patch that does the same and changes/reverts a few more things.

- Revert the changed variable order in drupal_settings_initialize()
- Revert the removal of the similar global $conf in _drupal_load_test_overrides(), I think we should continue to support config overrides like that and will just have to rename it then.
- Revert changes to the update_variable_*() function docblocks, they will be removed in #2168011: Remove all 7.x to 8.x update hooks and disallow updates from the previous major version, which is close to RTBC as well, so let's not mess with that unnecessarily.
- Instead of simply dropping it, converted the $conf removal in DrupalKernelTest to settings, which is actually what the PhpStorageFactory is now using. There was also a second usage of $global conf in there which I updated too
- Instead of simply removing the bogus DatabaseStorageTest $conf change, I replaced it with a setting, to make it explicit that we are using the database backend and aren't just relying on the default.

I think we need one additional follow-up to move container_service_providers to settings instead of using global $conf, we have one for $config and for system.file:path.private.

The last submitted patch, 36: variable-die-2167109-36.patch, failed testing.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 38: variable-die-2167109-38.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
46.54 KB
PASSED: [[SimpleTest]]: [MySQL] 63,323 pass(es). View
767 bytes

Arg, copy & paste fail.

Status: Needs review » Needs work

The last submitted patch, 42: variable-die-2167109-42.patch, failed testing.

The last submitted patch, 42: variable-die-2167109-42.patch, failed testing.

Berdir’s picture

42: variable-die-2167109-42.patch queued for re-testing.

Edit: Random fail.

Berdir’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Reviewed & tested by the community

Regarding all of the variables in settings.php, see also #2160705: settings.php setting names are inconsistent (and undocumented)

Even though I authored major parts of this patch, I'd like to RTBC it. :-)

catch’s picture

Status: Reviewed & tested by the community » Fixed
alexpott’s picture

Title: Remove Variable subsystem » Change notice: Remove Variable subsystem
Priority: Critical » Major
Status: Fixed » Active

Committed e373ebe and pushed to 8.x. Thanks!

A change notice already exists though it needs a lot of work :) https://drupal.org/node/1785368

alexpott’s picture

Issue summary: View changes

Correct change notice: https://drupal.org/node/1500260

Gábor Hojtsy’s picture

I've updated https://drupal.org/node/1500260/revisions/view/6872993/6873033 a tiny bit for changes in core to code style and the typification of config yaml, but not sure what else would be needed to cover this issue. That already mentions variable_get()/set/del are replaced with the config() API. Maybe more examples would be nice, eg. on how to del/remove things. But otherwise what is missing?

Gábor Hojtsy’s picture

Title: Change notice: Remove Variable subsystem » Remove Variable subsystem
Priority: Major » Critical
Status: Active » Fixed

Added change notice for the actual change in https://drupal.org/node/2183531 - this was confirmed to be looking good by @beejeebus and @berdir, so closing this issue. Yayayayay!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.