DRUPAL_BOOTSTRAP_VARIABLES needlessly loads all bootstrap modules

So we do a module_load_all($bootstrap = TRUE) in addition we run drupal_load('module', $module) inside bootstrap_invoke_all('boot'). Maybe we should reduce that down even more?

@Davereid - thats a possiible (minor) optimization, but isn't strictly related to this patch. Anyone available to fix these test failures. Subscribe.

The tests are not testing any units, but entire page loads, so they don't really help much in terms of tracking down where exactly the problem is. They are all related to a session not being destroyed when being viewed anonymously. Basically, when the dsm() is shown, the session should be getting cleared so that next page load it is empty. This isn't happening.

Any ideas?

Relevant code in sessions.test

    // Start a new session by setting a message.
    $this->drupalGet('session-test/set-message');
    $this->assertSessionCookie(TRUE);
    $this->assertTrue($this->drupalGetHeader('Set-Cookie'), t('New session was started.'));

    // Display the message, during the same request the session is destroyed
    // and the session cookie is unset.
    $this->drupalGet('');
    $this->assertSessionCookie(FALSE);
    $this->assertSessionEmpty(FALSE);
    $this->assertFalse($this->drupalGetHeader('X-Drupal-Cache'), t('Caching was bypassed.'));
    $this->assertText(t('This is a dummy message.'), t('Message was displayed.'));
    $this->assertTrue(preg_match('/SESS\w+=deleted/', $this->drupalGetHeader('Set-Cookie')), t('Session cookie was deleted.'));

    // Verify that session was destroyed.
    $this->drupalGet('');
    $this->assertSessionCookie(FALSE);
    $this->assertSessionEmpty(TRUE);
    $this->assertNoText(t('This is a dummy message.'), t('Message was not cached.'));
    $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT', t('Page was cached.'));
    $this->assertFalse($this->drupalGetHeader('Set-Cookie'), t('New session was not started.'));

Interestingly, the message does not show the second time, but the session also has something in it... what is has in it, we'll never know :)

heh, awesome. Okay in session-test.module:

/**
 * Implement hook_boot().
 */
function session_test_boot() {
  header('X-Session-Empty: ' . intval(empty($_SESSION)));
}

I'm guessing since we don't call HOOK_BOOT anymore in BOOTSTRAP_SESSION, this header never gets sent so "simple" test never finds out that the fake session is empty. poo.

Where do we fix this test? Do we fix the test? The test is also giving false positives thinking the session is full when it isn't.
I tried moving the header into hook_init, but that didn't seem to work. Don't know the internals of the testing stuff to say why.

1) Not sure whether the module system should be bootstrapped after the session only.

2) Clearly, we don't need to bootstrap the module system for variables only.

3) Whatever further optimization we do, it's easier to modify when bootstrapping the module system in its own bootstrap phase.

4) module_load_all(TRUE) and bootstrap_invoke_all('boot') are superfluous and the latter should be used during bootstrap.

5) By doing so, we can also vastly simplify module_load_all(), because it's no longer called with two different purposes.

Make even more sense of module_load_all().

Of course, that was a bit "blind" ;)

Strange. There seems to be some more magic going on here.

Even with this patch, my local Apache completely freezes when trying to run any tests. -- although the bootstrap behavior should be identical...

This seems like a nice clean-up to me, but it would be good to get sign off from dww or Jacob.

+++ includes/module.inc	11 Nov 2009 06:37:46 -0000
@@ -9,27 +9,24 @@
 /**
  * Load all the modules that have been enabled in the system table.
  * 
- * @param $bootstrap
- *   Whether to load only the reduced set of modules loaded in "bootstrap mode"
- *   for cached pages. See bootstrap.inc.
+ * @param $load
+ *   Whether to load all modules. Defaults to TRUE. Can be set to FALSE to only
+ *   return whether all modules have been loaded already.
  * @return
- *   If $bootstrap is NULL, return a boolean indicating whether all modules
- *   have been loaded.
+ *   A boolean indicating whether all modules were loaded.
  */
-function module_load_all($bootstrap = FALSE) {
+function module_load_all($load = TRUE) {
...
+    foreach (module_list(TRUE) as $module) {
       drupal_load('module', $module);

I find this comment terribly confusing. If the function is called module_load_all and I can pass $load == FALSE into it, it's kinda non-nonsensical isn't it?

This function really includes the module file right? So maybe the function should be called module_include_all?

Also, I think I'd make the $load variable $refresh. Although it is also a bit confusing, it at least is consistent with the name in module_list().

Otherwise, this looks like it would solve the problem. Nice one!

-J

This review is powered by Dreditor.

The patch makes sense, but the API changes introduced here do not :)

+++ includes/bootstrap.inc	11 Nov 2009 06:56:20 -0000
@@ -829,12 +834,17 @@ function drupal_page_is_cacheable($allow
-function bootstrap_invoke_all($hook) {
+function bootstrap_invoke_all($hook = NULL) {
   foreach (module_list(TRUE, TRUE) as $module) {
     drupal_load('module', $module);
-    module_invoke($module, $hook);
+    if (isset($hook)) {
+      module_invoke($module, $hook);
+    }
   }
 }

Erm. No. Let's add a bootstrap_load_all() or something.

Is there really a reason *not to* run the hook_boot() in DRUPAL_BOOTSTRAP_MODULE?

+++ includes/module.inc	11 Nov 2009 06:37:46 -0000
@@ -9,27 +9,24 @@
+function module_load_all($load = TRUE) {
   static $has_run = FALSE;
 
-  if (isset($bootstrap)) {
-    foreach (module_list(TRUE, $bootstrap) as $module) {
+  if ($load) {
+    foreach (module_list(TRUE) as $module) {
       drupal_load('module', $module);
     }
-    // $has_run will be TRUE if $bootstrap is FALSE.
-    $has_run = !$bootstrap;
+    $has_run = TRUE;
   }
   return $has_run;
 }

Erm. No.

We need a module_loaded() or something like this.

This review is powered by Dreditor.

It is a bit confusing to add a phase called DRUPAL_BOOTSTRAP_MODULE. Could we rename to DRUPAL_BOOTSTRAP_BOOT_MODULES. This distinguishes the phase from the part of FULL where we load all enabled modules.

This patch does not move the loading of boot modules, which was the initial point of this issue. IMO it is a critical regression. We need to move this new phase after SESSION phase just like in D6. It sounds like this breaks some session tests. We need to fix those.

This patch is still commit-worthy though.

Not critical. Please read and understand Priority levels of Issues, thanks.

Subscribe.

#12: drupal.bootstrap-module-system.12.patch queued for re-testing.

This proposal has been replaced by DRUPAL_BOOTSTRAP_CODE in D8, which has since been moved into the new bootstrap logic in DrupalKernel.

D7 bootstrap is very unlikely to be changed at this point.

Heads up that if you put this
$conf['page_cache_invoke_hooks'] = FALSE;
in your settings.php file the sessions test fails.