install_tasks() has this section of code:
// Bootstrap newly installed Drupal, while preserving existing messages.
$messages = isset($_SESSION['messages']) ? $_SESSION['messages'] : '';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
$_SESSION['messages'] = $messages;
the problem with this is, any messages that exist in $_SESSION from the bootstrap are clobbered. when you use something like the batch API to do the profile installation (which we need for the Drupal.org Testing profile since it's so huge) this becomes a problem, because the end of the batch does a drupal_goto() back to install.php, and any messages you set at the end of the batch end up in $_SESSION in the next bootstrap.
attached patch makes that section of code a bit more intelligent so that profile generated messages can appear on the finished page. i checked the default profile w/ this patch, and no extra garbage is output on the finished screen, and the Drupal.org Testing profile is now able to output it's messages.
the other thing is, i'm not really sure why we need to preserve messages prior to a bootstrap -- in my testing i didn't see any cases where there were existing messages at that phase. so i'd suggest either ripping out that code, or applying this patch or something like it.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | install_messages_fix-D6.patch | 811 bytes | hunmonk |
| #5 | installer-messages-409410-5.patch | 768 bytes | David_Rothstein |
| #4 | install_messages_fix.patch | 911 bytes | hunmonk |
| #1 | install_messages_fix-D6.patch | 1.03 KB | hunmonk |
| #1 | install_messages_fix.patch | 819 bytes | hunmonk |
Comments
Comment #1
hunmonk commentedthis is still a bug in the 7.x installer. attached are patches which fix the issue in 6.x and 7.x.
Comment #2
MichaelCole commented#1: install_messages_fix.patch queued for re-testing.
Comment #4
hunmonk commentedthe code just moved to an include file. re-rolling for that.
Comment #5
David_Rothstein commentedA patch that just rips out the code sounds like a great idea to me :) Maybe we should try that instead? - I have attached one for consideration.
I can't think of any situation where a drupal_bootstrap() would result in messages getting removed from $_SESSION, so I'm wondering if the code that's in there now is trying to protect against a problem that doesn't exist anymore. It dates back from Drupal 5, and the installer has been seriously rewritten a couple times since then... In fact, I did a little digging in CVS and it appears to literally date back from the very first incarnation of install.php :) (http://drupal.org/node/68926#comment-417148)
I think we should try removing it, and go back to #4 only if that doesn't work for some reason.
Comment #6
David_Rothstein commentedLooked a little more closely to verify that this is OK, and I think it is. The {sessions} table is created by system.module, and the drupal_bootstrap_full() only ever happens in the installer after system.module is installed, so we should have sessions set up and working correctly by the time this code ever gets run, and therefore messages are preserved in the database like normal.
I can see why it might have been a problem if the {sessions} table hadn't been created yet - looks like that was what the current code that is in the installer now was aiming at fixing.
Comment #7
hunmonk commentedi gave this new approach a thorough workover. it works perfectly on both drupal 6 and drupal 7 default install profiles, displaying the same messages both before and after the patch.
it also works successfully on the drupal 6 version of the drupalorg_testing install profile, properly displaying all the messages that profile generates.
attached is the drupal 6 version -- let's kill this dead code. :)
Comment #8
webchickQuestion. Should we just remove the install_bootstrap_full() function then since it seems to be useless now? Or I guess add a @todo to remove it in D8?
Comment #9
hunmonk commentedi think we still need the full bootstrap itself, no? where do you propose we move it, to install_system_module()? given the other tasks on either side of it, i kind of like having it as a separate task.
Comment #10
webchickUh. I have NO idea how I managed to read that as every line in install_bootstrap_full() getting removed. I think I'm officially too tired to review patches.
So... committed to HEAD! ;) I actually think this makes a lot of sense, and if it turns out we need that code, we'll want to add it back in with some comments.
Marking down to 6.x for consideration.
Comment #11
hunmonk commented