When you attempt to execute an install profile which refers to a module that wasn't found, you get a whole slew of errors:

Warning: array_diff() [function.array-diff]: Argument #1 is not an array in /Applications/MAMP/htdocs/head/includes/install.inc on line 306

Warning: session_start() [function.session-start]: Cannot send session cache limiter - headers already sent (output started at /Applications/MAMP/htdocs/head/includes/install.inc:306) in /Applications/MAMP/htdocs/head/includes/bootstrap.inc on line 717

Warning: Cannot modify header information - headers already sent by (output started at /Applications/MAMP/htdocs/head/includes/install.inc:306) in /Applications/MAMP/htdocs/head/includes/bootstrap.inc on line 395

Warning: Cannot modify header information - headers already sent by (output started at /Applications/MAMP/htdocs/head/includes/install.inc:306) in /Applications/MAMP/htdocs/head/includes/bootstrap.inc on line 396

Warning: Cannot modify header information - headers already sent by (output started at /Applications/MAMP/htdocs/head/includes/install.inc:306) in /Applications/MAMP/htdocs/head/includes/bootstrap.inc on line 397

Warning: Cannot modify header information - headers already sent by (output started at /Applications/MAMP/htdocs/head/includes/install.inc:306) in /Applications/MAMP/htdocs/head/includes/bootstrap.inc on line 398

Warning: Cannot modify header information - headers already sent by (output started at /Applications/MAMP/htdocs/head/includes/install.inc:306) in /Applications/MAMP/htdocs/head/includes/bootstrap.inc on line 399

The installer should check for the existence of all modules in the list prior to executing any install hooks, and inform the user of which modules are missing so they can remedy the problem.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Weird. I see that there's a drupal_verify_profile function that already should be doing this but isn't getting called for some reason.

webchick’s picture

Ok. So it is getting called, we're just not doing anything about it:

  // Perform actual installation defined in the profile.

  # NOTE: If one or more modules are missing, $modules will be FALSE. However,
  # we don't check for whether or not drupal_verify_profile returned anything, we just
  # happily pass it on and then finish the installation.
  $modules = drupal_verify_profile($profile, $install_locale);
  drupal_install_profile($profile, $modules);

  // Warn about settings.php permissions risk
  $settings_file = './'. conf_path() .'/settings.php';
  if (!drupal_verify_install_file($settings_file, FILE_EXIST|FILE_READABLE|FILE_NOT_WRITABLE)) {
    drupal_set_message(st('All necessary changes to %file have been made, so you should now remove write permissions to this file. Failure to remove write permissions to this file is a security risk.', array('%file' => $settings_file)), 'error');
  }

  // Show end page.
  install_complete($profile);

I tried throwing the rest of the function in an if ($modules) thing but I'm not sure what to do in the "else" case. Any ideas?

webchick’s picture

Priority: Normal » Critical
Status: Active » Needs review
FileSize
1.39 KB

Ok, maybe something like this.

I'm marking this critical because this seems like a major bug in the install system. The installer should not continue if required modules are not found, and the end user should definitely never be exposed to huge scary errors like that.

Rok Žlender’s picture

Patch works as promised. If module is not present installer outputs which module and exits without installing drupal and wihtout any "scary" messages.

I am wondering why are there so many install_*_error functions couldnt we join them all in one instal_error($title, $body) function. The only reason I can think off are comments. Now every error function is clearly commented if there will be only one maybe it would lose clarity. Otherwise they all have exact same structure.

I have a patch for this ready but didnt want to upload it if this is a bad idea.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I am marking this RTBC and Rok, please submit another issue, looks promising.

drumm’s picture

Status: Reviewed & tested by the community » Needs work

<ul><li>One or more required modules is missing.</li><li>Please check the error messages and try again.</li></ul>

THis doesn't make much sense as a list.

webchick’s picture

Status: Needs work » Needs review
FileSize
1.39 KB

Sure, makes sense.

This patch places the message in paragraph tags, and also makes "try again" a link back to the same page; it's not intuitive that you would refresh the page to try again.

Dries’s picture

"One or more required modules is missing." Should be "are"?

Also, are the "error messages" going to provide useful information? What do they look like? (I'm not saying we should change things, just being curious.)

webchick’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.42 KB

For some reason I thought you treated "one or more" as singular, but in Googling there seem to be more instances of using it as plural, so changed it. :)

Also, yes, the error messages (set as 'error' drupal_set_message()s) say:

"The blah module is required but was not found. Please move it into the modules subdirectory."

Tentatively setting this back to RTBC again.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Anonymous’s picture

Status: Fixed » Closed (fixed)