Refactor install.php to allow Drupal to be installed from the command line

subscribe. thanks for your work here, david.

Sub, this is awesome.

Awesome work David

@David - currently we specifically block simpletest from using install.php for security reasons. However, it would seem to make sense to get most of the code out of install.php and into a .inc file or some such (e.g. the way xmlrpc.php has nearly no code in it).

We are using hacked together methods in aegir because we need to support d6 too.

we support making d7 easier to install from the command line.

Regarding tasks in .info files, I need to re-evaluate moving them back.

I believe that for most developers (except those developing exceptionally complex install profiles), they would be better served by having them in a simple format.

I am working on several issues regarding install profiles, and one of the issues I am working on is to make install profiles have access to all the drupal hooks, which would allow them to be extended via hook_info_alter.
see: #509404: Fix some conceptual problems with install profiles and make them actually usable

The other side effect of this would be that we could work towards making it possible for modules to add tasks during install.

I am aiming to make install profiles more like modules than they are now, and therefor I want to reduce the amount of additional hooks that are exclusively used in install profiles.

Adrian, while I appreciate the install profile status update and the work you're doing (rock on!), I'm not sure how some of these comments are relevant to the patch. While install profiles can be improved so that they become more powerful and module-aware, there is always the initial Drupal install (e.g. installing the database, essential configuration, etc). Long story short, I'm not entirely sure what you're suggesting relative to this patch. Care to elaborate or to write a review?

Dries : David commented on this related issue of mine (http://drupal.org/node/509392#comment-1830106)

This patch reverts some of the changes from there. I should have linked back to the previous issue.

I simply stated my concern in that the patch was adding back a hook that I removed to make install profiles easier to write, and also made the implementation of the hook more complex. This goes against my goal of making install profiles more like modules, as I would like install profiles to be using the standard Drupal API as much as possible without inventing their own special case hooks that might be served just as well by existing hooks.

I fully intend on writing a proper review of this patch, and I've been in contact with David regarding the approach and possible difficulties as well.

Ah, thanks for clarifying -- that helps. I don't know what you guys discussed in private, but I'm sure David (or you) will follow up. It sounds like it might have been an oversight/timing issue instead of an intentional re-introduction. I'm with you on making install profiles more like modules though! :) Looking forward to the follow-ups.

After discussion with David, I have come across an idea that I believe would simplify the process of automating Drupal installations, without being affected by the edge cases that install profiles can present :
#525594: Installation should consist of 2 phases instead of one

This mechanism would also be a preferred solution when building a hosted service such as acquia gardens or an aegir based solution, as it dramatically reduces the install profile specific information that is needed to be able to successfully provision a site.

To followup in regards to PIFR.

Since PIFR runs in 6.x and will test an externally installed Drupal 7 site it needs to be able to install it without changing any code. I assume that is in the words are you wrote.

The current script is as follows.

  protected function install() {
    // Make a copy of the settings file to ensure there is no permission issue.
    $default_settings = $this->CHECKOUT_DIRECTORY . '/sites/default/default.settings.php';
    $settings = $this->CHECKOUT_DIRECTORY . '/sites/default/settings.php';
    copy($default_settings, $settings);

    require_once drupal_get_path('module', 'pifr_client') . '/review/browser.inc';
    $this->setPath();

    // Install Drupal.
    $b = new PIFRBrowser();

    // Step: Select an installation profile.
    // Step: Choose language.
    $b->drupalGet('install.php', array('query' => 'profile=default&locale=en'));

    // Step: Database configuration.
    $db_info = $this->getDatabaseInformation();
    $edit = array();
    if ($b->xpath('//input[@name="driver"]')) {
      $edit['driver'] = PIFR_CLIENT_DB;
    }
    $edit['database'] = $db_info['name'];
    $edit['username'] = $db_info['username'];
    $edit['password'] = $db_info['password'];
    $edit['host'] = $db_info['host'];
    $edit['db_prefix'] = '';
    $b->drupalPost(NULL, $edit, t('Save and continue'));

    // Step: Site configuration.
    $edit = array();
    $edit['site_name'] = 'checkout';
    $edit['site_mail'] = 'admin@example.com';
    $edit['account[name]'] = 'admin';
    $edit['account[mail]'] = 'admin@example.com';
    $edit['account[pass][pass1]'] = $this->adminPassword = $b->randomName(12);
    $edit['account[pass][pass2]'] = $this->adminPassword;
    $edit['update_status_module[1]'] = FALSE;
    $b->drupalPost(NULL, $edit, t('Save and continue'));

    // Step: Finished.
    $b->assertText(t('Drupal installation complete'));

    $this->setPath(TRUE);

    // Make sure that site installed correctly.
    if (($b->results['#exception'] + $b->results['#fail']) != 0) {
      $this->setError();
    }
  }

It would appear from reading that the re-factor supports what is needed, except as of now it require code modification to fill in the settings array. I assume is in the works to make something from command line that PIFR can just execute with parameters.

Overall, this appears to be exactly what is needed!

David , while I still haven't had time to review the patch, my only concern with the current approach (and it's really a nitpick), is that this refactoring doesn't really turn it into a command line script but turns it into an API.

If it was a command line script, i would be able to do :

php install.php sitename.com default --locale=en --db_user=user --email=username@place.com

From my perspective (aegir and drush), I would be served fine by the API-fication, but if this issue is about installing Drupal from the command line it feels unfinished unless you can actually install drupal from the command line without having to write additional code.

If this is solely about creating an API for install.php, I think that unless you can include install.php from within a command line script,
this patch in itself lacks usefulness.

I also don't notice a way to install a site other than sites/default with this patch, but i might have missed that in reading the code.
I will take the time to review this patch properly soon.

Tested the patch, works great for me.

should probably add this to install.php:

if (realpath($_SERVER['SCRIPT_FILENAME']) == __FILE__) {
install_drupal();
}

This will allow a user to include install.php while not actually running it.

So the example above can run as a standalone script.

Ideally, the constants should either do a defined() check or be inside a block somewhere as well so they can be safely included.

I personally feel that an API is a good first step, and is worth committing. If we build a CLI app later, great, we still want an API. But also, I don't think a command line app is really useful. There would be so many damn switches, it would start to look like ffmpeg. I'd personally prefer a settings file I can pass in, and perhaps some overrides at a first iteration.

What I would change (although I think we should commit what we've got ASAP), is the code design. If it is an API, it should behave like one IMO, which means more than just one function, some validation, etc.

$di = new DrupalInstaller();
$di->setDomain();
try {
$di->setDatabaseDetails();
} catch DrupalInstaller_NoDBException() {
$di->createDatabase();
$di->setDatabaseDetails();
}

Anyway, that's not important for now. RTBC IMO.

Best,
Jacob

Oh, one other nitpiky thing... the settings array has pass1 and pass2. That's just kinda silly.

-J

I've read through the patch and I agree 100% with the approach, it's a lot cleaner and a lot simpler. I actually ended up writing my own wrapper around hook_profile_tasks, which ended up working very similarly.

I haven't done a functional test of the code yet however, and I'm still a bit unclear about handling pages that return plain text.

If the change that jacob suggests is implemented (ie: switch to stop the script from running when included), i support it to be RTBC

This makes one very simple change to dbapi btw, which makes it a lot simpler to provide clean log information.

If we could get another review I'd RTBC it. moshe? dries?

I have reviewed the patch quickly but I feel I am not in a position to confirm that it is proper as I do not know well enough the internals of the D7 install process. I do however fully support the inclusion of this patch into core as it will make things much easier for us (Aegir/Drush). The code does seem pretty sound. (Plus I want to subscribe to the issue. :)

The documentation of this patch is fantastic. It's a really big change diff-wise, but the code that's left over is very easy to read and follow IMO. I also really like the move to exception-based error handling rather than compiling a bunch of #markup code.

+define('INSTALL_SKIP_TASK', 1);
+define ('INSTALL_RUN_TASK_IF_REACHED', 2);
+define('INSTALL_RUN_TASK_IF_NOT_COMPLETED', 3);

Let's namespace these so they appear grouped together on api.drupal.org. INSTALL_TASK_XXX. Also, minor, but there is an extra space between define() on #2. The PHPDoc should also be improved to indicate to developers when they might need to use these constants.

+ * @param $settings
+ *   An optional array of installation settings. Leave this empty for a normal,
+ *   interactive, browser-based installation intended to occur over multiple
+ *   page requests. Alternatively, if this is passed in, the installer will
+ *   attempt to use it to perform the installation in a single page request
+ *   (optimized for the command line) and not send any output intended for the
+ *   web browser.

If "this" is passed in? What exactly is "this" that I pass in to get this to work from the command line? Is there another function that explains this? If so, let's add a reference to it.

install_state_defaults()

Congratulations on the most well-documented function in all of Drupal! :D That's great!

+    // An array of forms to be programmatically submitted during the
+    // installation.
+    'forms' => array(),

Some more info here? Is this an array of form callbacks, or an array of $form_state['values'] arrays or..?

+    // This becomes TRUE only at the end of the installation process; certain
+    // theme functions may have access to it then.
+    'installation_finished' => FALSE,

Could you define what it means to be at the end of the installation process? Is this after all tasks have completed, or all tasks plus import of languages (I saw a distinction elsewhere), or..?

+    // An array of parameters for the installation, pre-populated by the URL
+    // or by the settings passed in to install_drupal().
+    'parameters' => array()

Can I get a for example? locale and profile? if it's always URL parameters, could we maybe make this url_parameters rather than just parameters?

+    // Installation tasks can set this to TRUE to force the page request to
+    // end (even if there is no themable output), in the case of an interactive
+    // installation.
+    'stop_page_request' => FALSE,

Why?

+    // Installation tasks can set this to TRUE to indicate that the task should
+    // be run again, even if it normally wouldn't be.
+    'task_not_complete' => FALSE,

Why?

+      'run' => INSTALL_RUN_TASK_IF_REACHED,

This seems to be in there an awful lot. Should that be the default value?

-  install_tasks($profile, $task);
+    $install_state['database_tables_exist'] = TRUE;

Indentation off here.

+    if (isset($install_state['parameters']['profile']) && isset($install_state['parameters']['locale'])) {
+    $filename = 'profiles/' . $install_state['parameters']['profile'] . '/translations/' . $install_state['parameters']['locale'] . '.po';
     if (file_exists(DRUPAL_ROOT . '/' . $filename)) {
       require_once DRUPAL_ROOT . '/includes/locale.inc';
       $file = (object) array('filepath' => $filename);
       _locale_import_read_po('mem-store', $file);
       $locale_strings = _locale_import_one_string('mem-report');
     }
+    }

Indentation looks off here.

+  return st('We were unable to find any installer profiles. Installer profiles tell us what modules to enable and what schema to install in the database. A profile is necessary to continue with the installation process.');

I know you didn't write this string, but can we please change those to "installation profiles" for consistency with everything else?

-  print theme('install_page', $output);

I only see - signs next to all calls to theme('install_page'). Did we lose the ability to theme the installer as part of this patch?

+if (realpath($_SERVER['SCRIPT_FILENAME']) == __FILE__) {

Had I not read one of the replies in this thread, I'd have no idea what that was doing. Please add a comment to explain.

+function drupal_override_server_variables($variables = array()) {

This function is a flaming wtf to people who don't spend their lives monkeying about with command-line PHP. Can you add some more information to the PHPDoc as to why this function is needed?

Also, run-tests.sh and drupal.sh in /scripts both seem to have parts very similar to this. Can we replace those with calls to this function instead? And is there stuff from those scripts that needs to be replicated here?

+function cache_get_multiple(array &$cids, $bin = 'cache') {
+  return FALSE;
+}
+
 function cache_set($cid, $data, $table = 'cache', $expire = CACHE_PERMANENT, $headers = NULL) {
   return;
 }

I have no idea why that is here. I see that there's already a function with no PHPDoc explaining below it, so probably not for this patch to change, but why are you returning FALSE instead of nothing like the one below?

+ * In general, each task function you define should return content to be
+ * displayed to the end user, if you want the page request to end. You should
+ * also set custom page titles within the task function, if necessary.
+ * Alternatively, if you want to pass control to the next task without ending
+ * the page request, do not return any output.

Overall, these comment improvements are fantastic! However, since realistically this is the only spot in all of core where we describe the installation process, a bit more overview of how it works here would be good, as I'm still left with questions. How do I know if I want a page request to end? How do I know if setting custom page titles is necessary?

+ * Remember that a user installing Drupal interactively will be able to reload
+ * the pages multiple times, so you might want to use variable_set() and
+ * variable_get() to remember your data and control further processing. It is
+ * important to remove any temporary variables using variable_del() before your
+ * last task has completed and control is handed back to the installer.

Again, this is a little wishy-washy. Why might or might not I want to use variable_set/get?

+ *     - 'display'
+ *       A boolean which can be set to force the task to either display or not
+ *       display to the end user. This can be used to provide finer-grained
+ *       control over whether or not the task will display.

Well, I can guess that from the name. ;) What does that actually mean, though? display_name hides a task from the list, this hides it from... the... screen? Then why bother having it in the first place? What's the use case?

+ *     - 'type'
+ *       A string representing the type of task. If this is left unset or set
+ *       to 'normal', the task will be treated as a regular callback function,
+ *       which does its processing and optionally returns HTML output. If this
+ *       is set to 'batch', the task function should return a batch API
+ *       definition suitable for batch_set(), and the installer will run the
+ *       task via batch processing. Finally, if this is set to 'form', the task
+ *       function should return a standard form API definition (and separately
+ *       define validation and submit handlers, as appropriate), and the
+ *       installer will appropriately direct the user through the form
+ *       submission process.

Could we itemize the list of possible values as a sub-list so it's a bit easier to scan? See hook_menu() for an example.

+ *     - 'run'
+ *       A constant representing the manner in which the task will be run. If
+ *       this is left unset or set to INSTALL_RUN_TASK_IF_NOT_COMPLETED, the
+ *       task will run once during the installation of the profile, until the
+ *       installer marks it complete. If this is set to INSTALL_SKIP_TASK, the
+ *       task will not run during the current installation page request.
+ *       Finally, if this is set to INSTALL_RUN_TASK_IF_REACHED, the task will
+ *       run on each installation page request that reaches it.

Same here. And can you help me again with the "why"?

+ * @see install_state_defaults()

We should probably also add a @see batch_set() to explain the desired format of a batch array.

+  // This is an example of a variable that your profile might set and unset
+  // throughout the tasks below, to control which tasks actually need to be
+  // processed.
+  $myprofile_needs_batch_processing = variable_get('myprofile_needs_batch_processing', FALSE);

I was confused reading this (figured it out a few lines later). How about adding something like, "Here, we define a variable to allow tasks to trigger a batch process if they are about to start a processor-intensive task."

There are no extra
+    // steps required for your profile to act as a multi-step installation
+    // wizard; you can simply define as many tasks of type 'form' as you wish
+    // to execute.

YAY!!! :D Just a question. Does it make sense to generalize this out in another issue to something modules can use as well? I don't know anyone but Jeff Eaton who can explain how multistep forms work currently. :P

+    // This is an example of a task that will not be displayed in the list that
+    // the user sees. To implement this task, your profile would define a
+    // function named myprofile_final_site_setup(), in which additional,
+    // automated site setup operations would be performed. Since this is the
+    // last task defined by your profile, you might also use this function to
+    // call variable_del('myprofile_needs_batch_processing') and clean up the
+    // variable that was used above. You might choose to return no output from
+    // this function, if you want the user to pass to the final Drupal
+    // installation tasks uninterrupted, or you could return themed output that
+    // the user will see (for example, a confirmation page explaining that your
+    // profile's tasks are complete, with a link to reload the current page and
+    // therefore pass on to the final Drupal installation tasks when the user
+    // is ready to do so).

This chunk has a lot more of these sort of wishy-washy "might" recommendations. Each time you use that word it leaves me with the question "Well why might I NOT want to do that?" It seems like at least calling variable_del() is a *should* and not a *might*. Maybe rephrase the second part as "if" rather than "might": "If you want the user to pass to the Drupal installation tasks uninterrupted, return no output from this function. Or, if you wish to..."

In a follow-up issue, we should really create a new install profile that really configures your site for a blog or a knowledgebase or something. Once plugin manager is in core, is it legit for core profiles to download contrib modules? Good times.

@moshe weitzman

Sadly, #395480: Plugin Manager in Core: Part 4 (installation profiles) has not started yet. I hope we can get in, but the process has been rather slow given how straight forward most of the engineering is. Hopefully it will get more streamlined as people realize the deadline is fast approaching. If you would care to donate any precious time to the effort, #395474: Plugin Manager in Core: Part 2 (integration with update status) is pending fixes to make it work in Windows, and something from catch to prioritize minor over major versions.

Sorry to take this off topic.

Best,
Jacob

Nice patch. I gave it a whirl, and it's all working smoothly for me.

However, I think that if we're going to support command-line installation, then we should provide at least the bare minimum to allow it to be used without any supporting third-party script (e.g. Drush, PIFR). Of course, anything advanced should be left to those third-party scripts. But the current advice given to testers of this patch — i.e. "hack the bottom of install.php and replace install_drupal() with (settings) code" — is IMHO a pretty clear sign that we need to do more out-of-the-box.

Therefore, I've extended this patch with the following features:

1. Support for loading install settings from a file. The contents of the file should be the $settings array needed to configure the installer, nothing more. This file defaults to sites/default/settings.install.php. The default can be overridden, by passing in a filepath as a command-line argument, e.g. calling php install.php sites/site2/settings.install.php. This should ease people's worries about having to add a gazillion command-line options to install.php: all I'm suggesting is that we add one (and an optional one, at that).
2. Added a sites/default/default.settings.install.php file.
3. Feedback gets printed to the command-line for success or for errors. If all goes well, it simply prints Drupal installed successfully. If any exception is thrown by the installer, it is caught and printed out (HTML is stripped, newlines are inserted, entities/symbols are decoded). If the settings.install.php file isn't found, or doesn't contain a $settings array, an error and some useful help is printed.

(PS: the install_is_cli() function is borrowed from Drush's drush_verify_cli() function.)

With these enhancements, it will be possible to download Drupal, to create a settings.install.php file, and to run:

php install.php

And that's it.

I'm anticipating that many people will consider this to be feature bloat and to not belong in the core installer. To those of you in that camp, I have to disagree: it's currently not even possible to test this patch without an additional hack to install.php. This enhancement at least makes it possible to test a command-line install in 5 seconds. However, I can understand if you insist that this be split into a separate follow-up patch.

@Jaza: Nice stuff. I actually did something similar and decided not to add it because I thought it a separate issue.

You can in fact test this patch without hacking install.php.

See the comments above, I added a little condition to check if the script is being run directly or included. This means you can write your own install script which just looks like this:

include_once 'install.php';

$settings = array()...

install_drupal();

Best,
J

@David_Rothstein: sounds good to me. I'm happy to work with yourself and @JacobSingh on this in a follow-up patch.

I also can't understand why my patch gives a 'failed to install HEAD' error. But since I have no idea how PIFR works, I assume that something I did is tripping up the way PIFR triggers the installer. Will leave that problem for another day.

I gave your latest patch a whirl, and it's still working great. +1 from me.

Instead of creating a new settings file, can't we re-use the existing settings.php?

As a developer, I'd love to be able to do something like: "./install.php --truncate" or something to start with a clean site. Pretty destructive, i know so maybe I should leave that as a custom script. ;-)

Will do a review shortly, if webchick doesn't beat me to it.

I agree with Dries having a "truncate" type functionality would be really nice, especially when working on Drupal HEAD and you mess something up or CVS update requires a re-install.

For provision / aegir, it would actually be better for us if it just used the settings.php we created instead of creating the config file itself.

so the process for installing a site is a) creating the settings.php file, b) running the script.

I wish there would also be a command line update.php... to remove bad fun - updating 500 sites every few weeks...

@hass - there already is, check out drupal.org/project/drush

we should definitely tackle making update.php includable next though

Once this goes in...either roll a patch for PIFR...or just make sure I am notified and I'll get it in as quickly as possible.

Committed this to CVS HEAD. Coordinated with DamZ so that he disabled the test bot for now. Thanks all!

MAINTENANCE_MODE is a bootstrap-time constant. It belongs into the entry point (for example install.php).

I would personally move maint mode into a function and actually let it get unset if the caller knows what it is doing.

I have reviewed the patch and it looks fine to me. I have attached a new version of the patch that removes the whitespace changes to enhance readability. My patch obviously shouldn't be tested or considered for inclusion in HEAD, I just include it so that people can see more easily what are the non-whitespace changes (because there are a few, because of word wrapping) in the patch.

I think this should go in, in short.

Committed #47 to CVS HEAD. Maybe we should create a new patch for the follow-up work?

What I was referring to in #42 was #533676: Take advantage of refactored installation system

Cross post.

Re: #48 @adrian, the issue for update.php is #233091: Move code from update.php into includes/update.inc.

Otherwise, is this complete? I feel a lot of functions still sit in install.php and while the patch committed makes it possible to include install.php, it's still a sub-standard/hackish API. We should move the functions to install.inc.

Indeed, the end of install.php currently lists:

// TODO: This if() statement allows Drupal to be installed interactively when
// install.php is visited in a web browser, while simultaneously allowing the
// file to be included by command line scripts so that it can be used as an
// API. It should be removed after the API functions in this file have been
// moved out to a separate, reusable location.
if (realpath($_SERVER['SCRIPT_FILENAME']) == __FILE__) {
  // Start the installer.
  install_drupal();
}

Quite a hack... Ideally, install.php would be only a few lines long and would handle forms and things like that... Should a new issue be opened for this or should we reopen this one?

It's not very clear by looking at file names what does installer.inc and what install.inc. More logical names perhaps in future bikeshedding followup?

Like kika, I agree that installer.inc is a confusing name.

I don't think installer.inc is confusing. It's the main installer UI. Install.inc should be a separate file that contains the Drupal Install API, and it should be clearly labeled as such.

I absolutely love seeing install.php reduced to 2 lines and support this patch. I haven't tested it so I'm not marking it as RTBC, but I strongly suggest kicking this in so we unblock the other installer work (like #525594: Installation should consist of 2 phases instead of one).

Right, so we have a small issue here:

Since this was committed, I haven't been able to install core. I've talked to webchick and ksenzee in IRC and they said to create a new issue (#546390: Fix CLI detection in install.php), but I thought I'd bring it up here since the problem area was in this patch.

My problem is that, from a browser, realpath($_SERVER['SCRIPT_FILENAME']) is returning FALSE, so the installer does not run. IMHO, install.php needs to be another two-liner file with all the install functions moved to the includes dir fairly quickly.

Yes, I think we can defer naming changes. Even kika suggests it ... My .02 are for install_ui or install_wizard

My bad, I thought what I put in was the best method... Not sure why it isn't working, but anyway:

From: http://www.codediesel.com/php/quick-way-to-determine-if-php-is-running-a...

 
function isCli() {
 
     if(php_sapi_name() == 'cli' && empty($_SERVER['REMOTE_ADDR'])) {
          return true;
     } else {
          return false;
     }
}
 

Might be better.

Instead of using wizard, I'd prefer to use something that we can use consistently for other .inc files. Maybe we should use MVC naming patterns? install.view.inc, install.controller.inc, install.model.inc ?

I'm confused, what is the problem here?

If you determine that the script is being called from the browser, isn't that good enough to distinguish between an include and a hit from a browser directly?

Just reporting my results in trying to use this by SimpleTest and PIFR.

PIFR:
Need a way to call this externally, without having to be inside the Drupal 7 codebase. Looks like there might be work to make CLI tool, not sure. I could hack and insert a file into the D7 checkout that purely executes the installation code, but that seems a bit hackish, no more so then what it currently does I suppose.

SimpleTest:
The code needs to be separated into two parts: 1) wizard that syncs with forms and does all the validation of input and settings directory, etc; 2) API code the does what it is told regardless of conditions.

The reasoning behind this is that SimpleTest needs to use the install code to generate a new DB environment, but doesn't want to mess with the settings.php files and such. You can see what I had to do in order to get the code to work in #488182: Shared test runner between webrunner and script and perfectly clean test environment. A hack that I was hoping would be fixed by the time I finished patch.

I haven't read all the comments, so I'm not sure if this stuff is in the works on already in the patch, but it seems that these items make sense to be address so that this code can be utilized (especially the SimpleTest portion as that is necessary for other implementations such as UTS and makes sense in general. We separate API from forms and things work better.).

Lets keep things moving here.

I'm not sure why we need to keep this moving. As far as I understand, it doesn't block anything. It is merely a cosmetic patch, and one that I don't necessarily agree with in its current shape.

It isn't cosmetic. drush can't easily use this as is. install.php *forces* a DRUPAL_ROOT value, for example.

Any chance for a re-roll, David?

Also, good luck installing Drupal from the command line without this patch, no matter which installer (Aegir, Drush, Acquia hosting). SimpleTest part can easily be a follow-up

If this is abstracted properly. Right now the installer does way to much validation, such as checking for existing db (without reguard to prefix) and for settings.php file. It needs to be split into two aparts.
1) installer.php with validation and such
2) installer.inc pure API which does what its told.

Otherwise SimpleTest cannot use this. Even if command line installer exists, if it checks the stuff above then it won't work for SimpleTest.

Yay! I love whitespace clean-ups. ;) Committed #75 to HEAD.

This will need a re-roll.

Assuming the copy-paste was done properly (which it looks like from a quick review, not byte per byte), this patch looks fine and "compiles" (ie. I can still install Drupal with it applied).

Update: I have reviewed the actual diff between the original install.php and the new insta_update.inc and it looks fine to me. Here's the resulting diff, which *should not* be applied and is provided here simply for peer review:

--- install.php	2009-09-01 05:24:06.000000000 -0400
+++ includes/install_drupal.inc	2009-09-01 05:23:13.000000000 -0400
@@ -1,17 +1,11 @@
 <?php
-// $Id: install.php,v 1.202 2009/08/25 21:53:46 dries Exp $
+// $Id$
 
 /**
- * Root directory of Drupal installation.
+ * @file
+ * Defines an API for installing Drupal, either interactively or via a command
+ * line script.
  */
-define('DRUPAL_ROOT', getcwd());
-
-require_once DRUPAL_ROOT . '/includes/install.inc';
-
-/**
- * Global flag to indicate that site is in installation mode.
- */
-define('MAINTENANCE_MODE', 'install');
 
 /**
  * Global flag to indicate that a task should not be run during the current
@@ -240,6 +234,7 @@
   require_once DRUPAL_ROOT . '/modules/system/system.install';
   require_once DRUPAL_ROOT . '/includes/common.inc';
   require_once DRUPAL_ROOT . '/includes/file.inc';
+  require_once DRUPAL_ROOT . '/includes/install.inc';
   require_once DRUPAL_ROOT . '/includes/path.inc';
 
   // Set up $language, so t() caller functions will still work.
@@ -1708,13 +1703,3 @@
   // Record when this install ran.
   variable_set('install_time', $_SERVER['REQUEST_TIME']);
 }
-
-// TODO: This if() statement allows Drupal to be installed interactively when
-// install.php is visited in a web browser, while simultaneously allowing the
-// file to be included by command line scripts so that it can be used as an
-// API. It should be removed after the API functions in this file have been
-// moved out to a separate, reusable location.
-if (php_sapi_name() != 'cli' && !empty($_SERVER['REMOTE_ADDR'])) {
-  // Start the installer.
-  install_drupal();
-}

This is the final phase of refactoring of install.php, and the fulfillment of the following TODO:

-// TODO: This if() statement allows Drupal to be installed interactively when
-// install.php is visited in a web browser, while simultaneously allowing the
-// file to be included by command line scripts so that it can be used as an
-// API. It should be removed after the API functions in this file have been
-// moved out to a separate, reusable location.

As such, since it's an API change of some sort (well, the functions move from a different file) I think it should go in before the freeze (today?) so that we know where to find the install functions in the long term.

Also note that similar work was done on update.php (#233091: Move code from update.php into includes/update.inc) so it would just make sense to finish the work here and keep install.php small (like update.php).

Now to specifically adress this:

I'm not sure why we need to keep this moving. As far as I understand, it doesn't block anything. It is merely a cosmetic patch, and one that I don't necessarily agree with in its current shape.

@Dries - could you expand on what you don't agree with in the current patch?

Thanks,

Right - this is just moving code around to match update.php. Lets please commit or elaborate on objections.

Something has broken here recently I think, still tracking it down.

As a side note, when investigating, I saw this:

// The user agent header is used to pass a database prefix in the request when
// running tests. However, for security reasons, it is imperative that no
// installation be permitted using such a prefix.
if (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE) {
header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
exit;
}
I don't see how this protects anything. I can easily change my user agent to simpletest, and since it is in Drupal core, it's not like it is a secret or anything.

-J

Ah... I just up'd to snow leopard which uses php 5.3.0, and command line install doesn't work from there.

I don't know exactly what the issue is, but the output is something about PDO not getting what it is looking for in terms of a driver name.

If someone is interested / has a quick idea:

Warning: PDO::__construct(): [2002] No such file or directory (trying to connect via unix:///tmp/mysql.sock) in /Users/jacob/work/gardens-trunk/docroot/includes/database/database.inc on line 324

Warning: Parameter 2 to install_settings_form() expected to be a reference, value given in /Users/jacob/work/gardens-trunk/docroot/includes/form.inc on line 474

Notice: Undefined index: _database in /Users/jacob/work/gardens-trunk/docroot/install.php on line 925

Warning: array_shift() expects parameter 1 to be array, null given in /Users/jacob/work/gardens-trunk/docroot/includes/form.inc on line 1468

Notice: Undefined index: settings_file in /Users/jacob/work/gardens-trunk/docroot/install.php on line 926

Notice: Undefined index: driver in /Users/jacob/work/gardens-trunk/docroot/install.php on line 949

Notice: Undefined index: driver in /Users/jacob/work/gardens-trunk/docroot/install.php on line 951

Fatal error: Uncaught exception 'Exception' with message 'In your <em></em> file you have configured Vineyard to use a <em></em> server, however your PHP installation currently does not support this database type.' in /Users/jacob/work/gardens-trunk/docroot/install.php:416

The reason for the check being so simple in that location is bootstraps does a proper check...so we just want to know that it is a simpletest user agent at that point.

This again seems to point out that we need a proper separation...the install.php which has UI and that check...and an API that can be called from CLI script and UI.

Okay, here is a new one. This is a regression in PHP 5.2:

#524728: Refactor install.php to allow Drupal to be installed from the command line (committed 2 days ago).

Tries to access a cache before it is created.

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'dev7_drupal.cache' doesn't exist in /Users/jacob/work/github/drupal/includes/database/database.inc on line 1735

Any ideas how to quickly fix this?

sorry, wrong link:

#557542: Cache module_implements()

@catch and I looked at this a bit in IRC, and here is the gist:

During system_requirements check in install_drupal, module_implements gets called. That tries to hit the cache. Normally, we would use the FakeCache

#576096: Port cache-install.inc to the new cache system

However, it is made to unset itself after the DB is setup (when doing a GUI install) and start using the DB cache.

When installing from the GUI, it happens over multiple page loads, so this works.

From the CLI, it don't. The cache_default_cache variable gets unset before we have set up the DB, so it fails for us. By commenting out the line to unset the var it works fine.

here is the code for reference:

install.php:277

if ($install_state['settings_verified']) {
    // Since we have a database connection, we use the normal cache system.
    // This is important, as the installer calls into the Drupal system for
    // the clean URL checks, so we should maintain the cache properly.
    unset($conf['cache_default_class']);

    // Initialize the database system. Note that the connection
    // won't be initialized until it is actually requested.
    require_once DRUPAL_ROOT . '/includes/database/database.inc';

    // Verify the last completed task in the database, if there is one.
    $task = install_verify_completed_task();
  }

@catch found this in CVS:
#151280: After install, old cached variables are used instead of those chosen using installer

Where the comment was added about "the cache properly"

Gabor, et all: Is this still required?

Sub.

subscribing

subscribe

+1, we absolutely want to do this.

Re-rolled from scratch against HEAD.

Missed the line that includes install.inc.

oh I like install.core.inc! - that makes a clear difference from install.inc and install.core.inc.

It's not clear to me exactly what Dries's previous concerns were with this patch, nor why it's required for Drush (drush installcore works fine for me without it). But, since it was ready before code freeze and in any case is just moving some code around, and since it brings install.php inline with similar changes we made with update.php, I'm ok with committing this.

I'll leave it RTBC for another 48 hours for feedback.

+++ includes/install.core.inc
@@ -0,0 +1,1728 @@
+    include_once DRUPAL_ROOT . '/' . $profile->uri;
+····
+    $details = install_profile_info($profile->name);

The last patch contains trailing whitespaces in several locations.

This review is powered by Dreditor with whitespace detection enabled.

Last Post "2 weeks 3 days", what is needed here? Commit?

#110: install.core_.524728.110.patch queued for re-testing.

(Edit: Oooooh. I like the new way this feature works!)

#110: install.core_.524728.110.patch queued for re-testing.

I assume the test failure were unrelated with this patch, as they were about some minor javascript generation problem while all other tests were succeeding, requeuing.

We are still waiting :)

Re-post latest patch to awaken bot.

Wait for green before commit (WFGBC)

Let's do this. It's been ready since forever, it makes this consistent with what we did for update.php, and frankly I'm getting sick and tired of this haunting me in my RTBC queue. ;P

And though the issue status doesn't reflect it atm, qa.drupal.org shows this gets the green light.

Committed to HEAD. Dries, if you strongly disagree, feel free to roll back.

Is this issue summary the only place where this is documented? It's 4 years later but I don't see anything about this in INSTALL.txt or at https://drupal.org/documentation/install/run-script