Created from #1775842: [meta] Convert all variables to state and/or config systems.

This looks like a good candidate for the state system as it is an internal flag of sorts.

Other references to this variable are http://drupal.org/node/1178246#comment-5123444 and http://drupal.org/node/1178246#comment-5123444, where Crell mentions:

"The better way of doing it would likely be "whatever the CMI folks come up with when they figure this out". :-) Heyrocker said to go ahead with this approach for the time being and when CMI figures out how to handle super-early configuration we can move it over to that, along with a half-dozen other things."

So, is the state system what we should use for this super-early configuration? Or do we need another approach altogether? Either way, the reference to variable_get() needs to be removed from comments as well.

The Autoloader doesn't exist in D7 so we don't need to worry about d7->d8 migrations.

CommentFileSizeAuthor
#15 autoloader_mode_global-1814086-15.patch2.87 KBsamhassell
#12 autoloader_mode_global-1814086-12.patch2.95 KBsamhassell
#9 autoloader_mode_global-1814086-6.patch2.97 KBfgm
#7 autoloader_mode_global-1814086-6.patch2.97 KBsamhassell
#4 autoloader_mode_global-1814086-4.patch2.97 KBsamhassell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samhassell’s picture

samhassell CreditAttribution: samhassell commented
Issue summary: View changes

added created from reference.

samhassell’s picture

samhassell CreditAttribution: samhassell commented
Issue summary: View changes

added comment about upgrades.

samhassell’s picture

samhassell CreditAttribution: samhassell commented
Status: Active » Needs review

config.inc gets loaded in _drupal_bootstrap_configuration directly after the call to drupal_classloader(), which contains the autoloader_mode get. This means we can't use config().

autoloader_mode isn't a good fit for state either, as it is a setting that should be sent between dev environments.

Setting this to needs review as I need some guidance on how to proceed before I start writing patch. Sorry if that's the wrong status :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Convert autoloader_mode in drupal_classloader to the state system » Convert autoloader_mode in drupal_classloader to its own global

This should be in neither state nor config at the moment, it'd need to move to its own global in settings.php similar to $databases. Eventually we might want to replace those with some other mechanism, but that should be enough to remove the variable_get().

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Active
samhassell’s picture

samhassell CreditAttribution: samhassell commented
FileSize
autoloader_mode_global-1814086-4.patch2.97 KB

Okay I removed autoloader_mode and added a global in settings.php, $class_loader. The new name seems more in line with the terminology used in the comments. $class_loader gets declared in drupal_settings_initialize() along with the other settings.php variables.

Needs some more perspective on comments as well.

samhassell’s picture

samhassell CreditAttribution: samhassell commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, autoloader_mode_global-1814086-4.patch, failed testing.

samhassell’s picture

samhassell CreditAttribution: samhassell commented
Status: Needs work » Needs review
FileSize
autoloader_mode_global-1814086-6.patch2.97 KB

fixed php error.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

Looks generally ok, we're still looking for a pattern and often still use $conf in settings.php but access it directly instead of through variable_get(). So that would be a possiblity as well that wouldn't require as much changes.

Also, we should check if it's set and initialize to default if not, then we don't need the block in settings.php, because e.g. sites that are upgrading from 7.x won't have that.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review
FileSize
autoloader_mode_global-1814086-6.patch2.97 KB

Changed the docs a bit, fixed Symfony name.

Regarding the check for the variable, it needn't be initialized to default: because there is a global $class_loader; before its first use, it will always be defined, possibly to NULL, even without being defined in settings.php.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

Ah, I didn't know about the part with global $class_loader.

What about having "# $class_loader = 'apc';" in there? There are quite a few lines in there with a similar pattern, then users just need to comment it out to enable apc?

+++ b/sites/default/default.settings.phpundefined
@@ -247,6 +247,19 @@
+ * as it does not break when code is moved on the file system. However, as it

"The APC classloader provides better performance and is recommended for production sites." instead of the last sentence?

+++ b/sites/default/default.settings.phpundefined
@@ -247,6 +247,19 @@
+ * Use 'apc' to use the APC class loader.
+ * 'default' will use the Symphony Universal Class Loader.
+ * @todo: Better docs.

Symfony. Maybe write this in a single sentence?

The docs look ok to me apart from that.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented

Oops, looks like I re-uploaded the original patch. Will redo.

samhassell’s picture

samhassell CreditAttribution: samhassell commented
Status: Needs work » Needs review
FileSize
autoloader_mode_global-1814086-12.patch2.95 KB

- reworked the settings.php $class_loader comment incorporating Berdir's last line change, also fixing misspelling of Symfony. (You've no idea how often I'm doing that :P)
- following Berdir's advice set $class_loader to 'apc' and commented it out.
- removed the @todo in settings.php

// @todo Use a cleaner way than a settings.php variable to switch class loaders.

Maybe we should get rid of this todo from bootstrap.inc? Settings.php variable seems logical for setting this.

@fgm: add your changes to this one?

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

It would surprise me if this still passes. Lets retest

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Needs work 
+++ b/core/includes/bootstrap.incundefined
@@ -725,7 +725,7 @@ function drupal_settings_initialize() {
+  global $databases, $cookie_domain, $conf, $installed_profile, $update_free_access, $class_loader, $db_url, $db_prefix, $drupal_hash_salt, $is_https, $base_secure_url, $base_insecure_url, $config_directories;

Why do we need this global here? (at this spot in code)

+++ b/core/includes/bootstrap.incundefined
@@ -3094,11 +3093,13 @@ function drupal_classloader() {
+    // @todo Use a cleaner way than a settings.php variable to switch class loaders.

This comment surpasses te 80 chars line. I'm not sure we need the todo. A settings.php setting looks fine to me for this case.

samhassell’s picture

samhassell CreditAttribution: samhassell commented
Status: Needs work » Needs review
FileSize
autoloader_mode_global-1814086-15.patch2.87 KB

Why do we need this global here? (at this spot in code)

This line in drupal_settings_initialise() is used to declare the variables in settings.php as global variables. As all the other settings.php variables are declared global here, it makes sense to follow the pattern and declare $class_loader in the same manner.
I don't see a better alternative for this - it could be declared global in settings.php, but that has not been done in any other case.

*edit* Here's the relevant code for context:

/**
 * Sets the base URL, cookie domain, and session name from configuration.
 */
function drupal_settings_initialize() {
  global $base_url, $base_path, $base_root, $script_path;

  // Export these settings.php variables to the global namespace.
  global $databases, $cookie_domain, $conf, $installed_profile, $update_free_access, $class_loader, $db_url, $db_prefix, $drupal_hash_salt, $is_https, $base_secure_url, $base_insecure_url, $config_directories;
  $conf = array();

  // Make conf_path() available as local variable in settings.php.
  $conf_path = conf_path();
  if (is_readable(DRUPAL_ROOT . '/' . $conf_path . '/settings.php')) {
    include_once DRUPAL_ROOT . '/' . $conf_path . '/settings.php';
  }
This comment surpasses te 80 chars line. I'm not sure we need the todo. A settings.php setting looks fine to me for this case.

I agree. Removed the comment in the attached patch (rolled against latest 8.x)

Status: Needs review » Needs work

The last submitted patch, autoloader_mode_global-1814086-15.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

#15: autoloader_mode_global-1814086-15.patch queued for re-testing.

Test failure was a database deadlock.

Status: Needs review » Needs work

The last submitted patch, autoloader_mode_global-1814086-15.patch, failed testing.

samhassell’s picture

samhassell CreditAttribution: samhassell commented
Status: Needs work » Needs review

#15: autoloader_mode_global-1814086-15.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, autoloader_mode_global-1814086-15.patch, failed testing.

samhassell’s picture

samhassell CreditAttribution: samhassell commented
Status: Needs work » Needs review

#15: autoloader_mode_global-1814086-15.patch queued for re-testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

Looks good

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed this one to 8.x. This also reminded me of #1833516: Add a new top-level global for settings.php - move things out of $conf so I added a follow-up patch for this one there.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

i think i a word