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.

Files: 
CommentFileSizeAuthor
#15 autoloader_mode_global-1814086-15.patch2.87 KBsamhassell
PASSED: [[SimpleTest]]: [MySQL] 49,289 pass(es).
[ View ]
#12 autoloader_mode_global-1814086-12.patch2.95 KBsamhassell
PASSED: [[SimpleTest]]: [MySQL] 47,857 pass(es).
[ View ]
#9 autoloader_mode_global-1814086-6.patch2.97 KBfgm
PASSED: [[SimpleTest]]: [MySQL] 46,916 pass(es).
[ View ]
#7 autoloader_mode_global-1814086-6.patch2.97 KBsamhassell
PASSED: [[SimpleTest]]: [MySQL] 42,815 pass(es).
[ View ]
#4 autoloader_mode_global-1814086-4.patch2.97 KBsamhassell
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/bootstrap.inc.
[ View ]

Comments

samhassell’s picture

Issue summary:View changes

added created from reference.

samhassell’s picture

Issue summary:View changes

added comment about upgrades.

samhassell’s picture

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

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

Status:Needs review» Active
samhassell’s picture

StatusFileSize
new2.97 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/bootstrap.inc.
[ View ]

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

Status:Active» Needs review

Status:Needs review» Needs work

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

samhassell’s picture

Status:Needs work» Needs review
StatusFileSize
new2.97 KB
PASSED: [[SimpleTest]]: [MySQL] 42,815 pass(es).
[ View ]

fixed php error.

Berdir’s picture

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

Status:Needs work» Needs review
StatusFileSize
new2.97 KB
PASSED: [[SimpleTest]]: [MySQL] 46,916 pass(es).
[ View ]

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

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

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

samhassell’s picture

Status:Needs work» Needs review
StatusFileSize
new2.95 KB
PASSED: [[SimpleTest]]: [MySQL] 47,857 pass(es).
[ View ]

- 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

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

It would surprise me if this still passes. Lets retest

aspilicious’s picture

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

Status:Needs work» Needs review
StatusFileSize
new2.87 KB
PASSED: [[SimpleTest]]: [MySQL] 49,289 pass(es).
[ View ]

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:

<?php
/**
 * 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

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

samhassell’s picture

Status:Needs work» Needs review
aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

Looks good

catch’s picture

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

Issue summary:View changes

i think i a word