DB drivers need to be able to change the configure database form during install

I generally support this idea, however we need to make sure not to introduce a usability regression.

Uncollapsing the advanced fieldset IMHO is a usability regression. Our aim is to make install as easy as possible and to reduce the clutter not add more to it.
Also,

Note that every RDBMS has its own limit about the maximum length of constraint names, being conservative table prefix shouldn't be longer than 4cc.

is tech talk we definitly need to avoid.

It should be enough for the moment if we keep the fieldset collapsed as-is, but mention both aspects in the field description. Also, I prefer "dru_" as the default prefix, but that's definitely a matter of taste. How about this:

In some cases a table prefix such as _dru is necessary. This includes sharing the same database with other applications, or compatibility with certain database types. A length of four characters should usually not be exceeded.

I'm aware of "database types" being not quite exact. However "database management systems" is too complicated and "database systems" not more correct.

Minor correction: placeholder no more needed.

I think this caters to a wider problem: there should be a way for database drivers to alter this form. It's already necessary for SQLite and then your DB drivers can take advantage and set dru_ as a default there.

I think this patch only should introduce the form_alter method to DatabaseInstaller and then we can work from there.Another thing that'd be good to patch here is that I do not know why the DatabaseInstaller class is in install.inc, I definitely would move that to includes/database/install.inc. Move, add a method and be done.

Here's my very rough initial stab at introducing a form_alter function to the database installer classes that I'm pretty sure will not install. To do this, it's going to require that the database type and database options screens be split into two different pages.

OMG I have this working and installing now! I'm rather proud of getting this to work with install.php...what a PITA! Some parts of the code are a little hackish, but I have attached screenshots to demonstrate the new install process and the difference between the MySQL configure screen and the SQLite configure screen.

EDIT: I forgot to expand the 'Advanced options' in the sqlite install screen, but there's only the table prefix field there (no host or port fields). Ignore the special format thingys around the username and password fields. It's my LastPass extension at work.

I don't think the PIFR will ever like this code since it messes with the install process.

<?
+ $form['basic_options']['database']['#description'] = st('Enter a path to a sqlite file. The database will be automatically created for you.');
?>
These are unclear and SQLite needs to be capitalized properly

Good grief how did I never see this issue? Linked here from #431844: SQLite database need to be installed in db folder

Putting a hook implementation inside a class is ugly and nasty and calling it form_alter breaks the coding standard for methods. I would actually go further and just say that the form after the DB radio buttons themselves are entirely generated by the driver with an optionsForm() method. Views already uses that exact approach.

And I agree with chx in #3 that the DB installer class should move to the DB directory. That would even simplify some of the DB autoload code I was working on.

Added a link here from #793200: Document all database settings in default.settings.php as well.

I agree that this should be something similar to what views does with an optionsForm method. Crell, is something like what's in this patch what you're thinking of?

It's a very rough implementation; currently it doesn't submit because of hidden required form elements and doesn't provide any database specific fields. But if this is the right direction I can develop it further.

Would anyone be opposed to me increasing the priority on this? I've been working with Drupal for 5 years and I can't figure out how to install D7 with SQLite. If I can't, a nooB definitely won't be able to.

@duellj I think your approach is good, but I'm not Crell.

I'd be +1 for an getForm() or getFormOptions() method.

+ $form['settings'][$key]['#title'] = $driver['name'] . st(' Settings');
should be
+ $form['settings'][$key]['#title'] = st('@driver_name Settings', array('@driver_name' => $driver['name']));

And perhaps to get around the required hidden form fields we could have the chosen driver manually check for required fields. The default implementation could check that all non-advanced fields are required.

Here's an updated patch that actually makes it through the install process :). It adds a getFormOptions() method (thanks Dave Reid) that adds driver specific database options. It bypasses the problems in #12 by passing off driver specific form field validation to the appropriate DatabaseTasks object validateFormOptions() method. The only form fields that the base DatabaseTasks getFormOptions() method provides are database name and table prefix. DatabaseTasks_sqlite::getFormOptions() should probably also be added to change the wording for $form['database'] to be more SQLite specific.

This isn't ideal IMO because there's no visual indication on the database config page of what additional fields are required. Not sure how else to do it, though; it'd be great if states supported conditionally required elements.

I'm setting this to review to see what testbot says.

I just discovered #limit_validation_errors which might be able to help with our "don't enforce required fields" problem.

http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....
http://api.drupal.org/api/function/form_set_error/7

duellj, are you going follow up here?

Crell, yes, I can work the patch some more. I was curious as to wether this fell into the realm of API change, since it adds new methods to the DatabaseTasks classes, and therefore wouldn't be considered for D7.

dalin, I did play around with #limit_validation_errors, but couldn't quite get them to work the way the form needs. As far as I can tell, #limit_validation_errors can't be used in conjunction with #states. When the state switches between the database drivers, it doesn't change which fields #limit_validation_errors ignores. Am I missing something else here? This is why the ability for #states to handle required fields would have been great if it made it in.

What about using #ajax to load only the appropriate driver settings form, then we could keep the #required for the settings for elements (though we could still keep validateFormOptions() for more complex driver specific validation). We'd have to make sure that it degrades gracefully. Anyone see any problems with this?

I played a bit trying to get AJAX to work on the database settings form. After having to jump through a few hoops I think I hit a bit of a wall. The AJAX framework makes calls to q=system/ajax which naturally doesn't exist yet, that URL returns the first install page (select a profile). In order to make AJAX work I think we'd need to setup a special .php file that would handle AJAX requests only for install :(

I'm attaching the bit of a patch that I managed to squeak out just in case someone wants to pick it up again, but it's totally not functional.

I'll play around a bit more with non-AJAX approaches, but this is looking harder and harder to solve.

I was actually able to figure out to work #limit_validation_errors with states (basically conditionally setting limit_validation_errors on the form build), so required elements are mostly working. I say mostly because for some reason nested form values aren't being validated properly (for example, the host field isn't being validated). Might be a bug, because from my understanding if you set a limit_validation_errors element, it should validate everything within it. I'll have to explore that more (or if anyone has any insight why that might not be working).

Also, install_settings_form_submit() needs to be updated to include the correct values in the database settings. Currently, all of the allowed values are hard coded, but they should be defined by the driver. Would the best way to use element_children() on $driver->getFormOptions()? Or should we have the driver define a list of all available settings?

Thanks dalin for trying to figure out the AJAX for the settings form, it looks like we're not going to need it now though. I think it'd be too much trouble to try and get it working, and when limit_validation_errors works correctly we won't need it.

I doubt testbot will allow this to go through, but let's see what it says.

In theory I like what's in #23. But, is it too late to get this into D 7.0 release? I'm inclined to think so.

There is a repercussion of this change in that Installation Profiles can modify this page. This change alters the form layout. Once this goes in install profiles that change this page will not work properly. So, it's not a backwards compatible change.

It's also an API change, because it changes the default screen to not include a username and password, which other databases may have depended upon. I'm tentatively marking this as 8.x, but feel free to change this back.

I can completely see your point about 8.x. However as things currently stand mere mortals are not able to install Drupal via SQLite without some foreknowledge of what special information your are supposed to put into the DB fields (that does not resemble the field labels or descriptions).

So on the one hand we have an increased barrier to entry for the layman, versus the potential to break some installation profiles (and that should be quite rare). IMO the increased accessibility is more important.

I'm moving this to the D7 critical queue for discussion (based on the theory that if we don't get it fixed before RC1, we probably can't get it fixed at all - see http://drupal.org/drupal-7.0-beta3). That will give it the visibility it needs for a wider group of people to decide what to do about it.

I'm inclined to say we should fix this - the number of install profiles that modify this form is probably either zero or close to zero (and I don't think it's that hard for them to adjust to this change, right?)

However, as per @dmitrig01's comment in #27 I think we should do this in a way that minimizes the API change. Instead of only putting the absolute minimum shared options in the parent class implementation, why not put all of the common ones (including username, password, etc)? That way, MySQL and Postgres (and any other drivers in contrib that are happy with the standard set of form options) don't have to override anything; they will continue to work exactly like they did before. Only SQLite will need to override it. That makes a bit more sense to me (especially given that we are trying to minimize API changes at this point in the cycle, but even probably if we weren't): We'd go with the principle of starting with sensible defaults and then putting the burden on the most unusual database drivers to override them, not the other way around :)

@David_Rothstein (#29):
Good point about the default options in the base DatabaseTasks class. It makes more sense to remove options for SQLite, since MySQL and Postgres share most of their options (hopefully once we get this in we can also get in #793200: Document all database settings in default.settings.php). I'll throw up a new patch with those changes.

@mfer (#26):
Can you provide examples of how installation profiles have modified the form? I agree that it doesn't seem like it would be that common.

It's technically not an accessibility question but a usability one. That said, I think this falls into the "Drupal works without it but it's going to be quite painful if we don't fix it" category, so leaving as critical per webchick's announcement since there does seem to be some momentum here.

If we can fix this stabily quickly, let's do. If not, we can downgrade it later.

This patch basically works with MySQL. For SQLite there are some problems (server doesn't appear to be able to write data to the DB after tables are created). But I don't *think* it has anything to do with this patch.

To reduce the WTF factor, the latest patch includes a default for the name and location of the SQLite file. In doing so I discovered that where that file lives has some significant usability impacts because the server needs write access to the file, and the directory that contains it (I don't know much about SQLite, I assume that it's writing temp or cache files). Some options of where to put that would be:

1) sites/default/
2) sites/default/files/
3) sites/default/database/

#1 doesn't appear to be an option because it would require changing all the instructions about "installation complete, remove write permissions from sites/default ..."

#2 just feels icky

#3 seems like a good idea. But would require reworking the sqlite installation procedure to attempt to create the directory.

thoughts?

nm. I see that this decision has already been made in INSTALL.sqlite.txt. The latest patch uses that suggested default, and pulls additional help text from there.

Not sure if PIFR is complaining because we broke installation, or because we broke some of its assumptions about install.

Still some issues with SQLite. I needed to manually refresh the page a couple times to get it to go through with installation. No idea why. I think this is a pre-existing issue.

This is going to need a patch against pifr/pift to change the installation code most likely, along with simultaneous commits/deployment to save the test bot being broken. Marking CNW since unless I have this wrong there's no way for the bot to pass.

Created issue over at PIFR for the issue raised in #37: #972956: Update Drupal 7 installer to reflect installer form changes

Fixed a typo and made all fields required that should be required.

#limit_validation_errors is no longer working, so required fields are no longer being validated. It looks like the patch that landed here #953914: #limit_validation_errors fails is parents array contains numeric indexes broke our form.

#39: 346494-39-cross-db-install-D7.patch queued for re-testing.

@webchick patch in #39 won't pass testing until #972956: Update Drupal 7 installer to reflect installer form changes gets committed and deployed.

But the last committed patch in #953914: #limit_validation_errors fails is parents array contains numeric indexes did sort out the issue where required fields were not actually being required.

Still needs review.

I can't comment the actual implementation...
...except that it works!

I have one comment regarding the sqlite option form. With the patch in #39 if you select SQLite, you get an empty settings form, where all options (file location and table prefix) are inside the "advanced options" fieldset. (See first screenshot)
That is not only against our UI guidelines, but it also hides away a very important setting (the file location), which if at all possible any user should change from the default value (to something outside the webroot). The description already makes that crystal clear, except that not many people will see it if it is hidden in that collapsible fieldset.

Attached patch makes no other change, but moves the 'file location' field up on level onto the top-level form. (See second screenshot).

....and the patch.

I guess I was trying to make it too simple ;)

If/when someone re-rolls this it looks like my brain was still in D6 string concatenation mode:
$default_database = conf_path(FALSE, TRUE) .'/files/.ht.sqlite';
should be
$default_database = conf_path(FALSE, TRUE) . '/files/.ht.sqlite';

I'll reroll when we get another review on #46 vs. #39.

This is one of very few issues in the critical queue that truly is both really, really bad (it creates total and absolute WTFs for both database driver authors and user) and can't be fixed after RC1.

Really hope we can get the testbot errors sorted. :\

Attached patch fixes issue in #49 and removes a comma from the SQLite file description. Leaving this as CNW because we know the testbot will break, but it's actually at review stage.

@webchick dalin has made some great progress over at #972956: Update Drupal 7 installer to reflect installer form changes, making it so PIFR can handle both pre and post commit states from this patch. As soon as we can get that tested, I think we should be good to go (with proper review, of course).

subscribe

This issue needs review but is soft-postponed on #972956: Update Drupal 7 installer to reflect installer form changes to make the test bots compatible with this change.

@chx and I were discussing this in IRC the other day and I think we both felt that although this is important, it probably shouldn't really be considered critical? The INSTALL.sqlite.txt file already has good instructions, and with them you can install using SQLite just fine.

Either way, it's an important issue, and the patch overall looks good. Here is a first review:

1. As shown in the attached screenshots, this patch makes visual changes to a JavaScript-based installation. It really shouldn't (and the fieldsets-within-a-fieldset look incorrect). I'd suggest getting rid of the fieldsets, and having the "MySQL, MariaDB, or equivalent Settings" text display as hidden, so that for a JavaScript installation we aren't changing the UI at all. (Perhaps use the "js-hide" CSS class for this. The alternative would be '#title_display' => 'invisible' but I think even screen reader users do not need these labels, if they are using JavaScript.)
2. The patch needs to change INSTALL.sqlite.txt because some of the instructions in it are outdated as a result of the changes here.

3. @@ -246,7 +247,7 @@ function drupal_detect_database_types() 
        $class = 'DatabaseTasks_' . $driver;
        $installer = new $class();
        if ($installer->installable()) {
   -      $databases[$driver] = $installer->name();
   +      $databases[$driver] = $installer;
   

   I don't think we should do that API change at this stage. How about a separate function for returning the full driver class? drupal_detect_database_types() could use that function internally, of course, so there wouldn't have to be any code duplication.

4. +  public function validateFormOptions($database) {
   ....
   +    // Verify the table prefix.
   +    if (!empty($database['prefix']) && is_string($database['prefix']) && !preg_match('/^[A-Za-z0-9_.]+$/', $database['prefix'])) {
   +      form_set_error($database['driver'] . '][advanced_options][db_prefix', st('The database table prefix you have entered, %prefix, is invalid. The table prefix can only contain alphanumeric characters, periods, or underscores.', array('%prefix' => $database['prefix'])));
   +    }
   

   This function gets called by install_database_errors(), which is used outside of the form API in at least one case. So using form_set_error() won't work here.

   Instead it needs to return some kind of array of errors, like install_database_errors() already uses. Given that, a better name for the method might be something like validateDatabaseSettings().

   Alternatively, I guess we could leave it as is and switch it to being called from inside install_settings_form_validate() only. The kind of validation it is doing is essentially form-API-related only... since there is already separate validation to make sure the database connection itself can be made.

5. +  public function getFormOptions() {
   +    global $database;
   ....
   +  public function validateFormOptions($database) {
   +    global $databases;
   

   There is a bug in the first one, since there is no global $database variable. Why not just make them consistent though (and pass $database in to each, no global variables needed)? In fact, it looks to me like the global variable isn't even being used in the second one.

Those were the more serious issues. Here are some less important ones:

6. +    $form['settings'][$key]['#title'] = st('@driver_name Settings', array('@driver_name' => $driver->name()));
   

   "Settings" should not be capitalized.

7. +        ':input[name=driver]' => array('value' => $key)
   ....
   +    '#limit_validation_errors' => array(
   +      array('driver'),
   +      array(isset($form_state['input']['driver']) ? $form_state['input']['driver'] : current($drivers_keys))
   

   It's better code style to always put commas at the end of each array element (even if it's the last one in the list).

8.  function install_settings_form_validate($form, &$form_state) {
   +  global $settings_file;
   

   It didn't look to me like this was ever used?

9. -  form_set_value($form['_database'], $form_state['values'], $form_state);
   

   If you're removing this and replacing it with $form_state['storage'], that seems like a good change (although I'm not totally sure it's necessary as part of this patch)?

   In any case, it looks like there are some places that you left behind that still refer to $form['_database']; this patch would need to remove all of them.

10.    // During installation, the profile information is stored in the global
       // installation state (it might not be saved anywhere yet).
    -  if (drupal_installation_attempted()) {
    -    global $install_state;
    +  if (drupal_installation_attempted() && isset($install_state['profile_info']['distribution_name'])) {
         return $install_state['profile_info']['distribution_name'];
    

    I don't understand why this change is needed. When do we wind up in a situation where there is no profile selected?

11. +   * @param $database
    +   *   An array of submitted driver specifc configuration options.
    

    Typo here ("specifc").

Needs work based on David's review.

(I agree that we should get rid of the nested fieldsets. Yuk.)

Thanks, David_Rothstein, for the great review! Attached patch addresses the issues in #57:

1. Agreed, the nested fieldsets are ugly. Wasn't sure of a way around them with states until I ran across the undocumented 'container' form element. It sounds like js-hide would be the best option for the driver settings titles; added them in as prefixes (is that the best way?).

2. Changed text to reflect settings page.

3. It makes sense not to change the api if we don't need to. Added a drupal_get_database_types() function that's called by drupal_detect_database_types(). drupal_detect_database_types() now has the same return.

4. Changed validateFormOptions() to validateDatabaseSettings() that returns an $errors array.

5. Removed the global variables from both methods.

6. Fixed.

7. Fixed.

8. Removed.

9. Removed other references to $form['_database'].

10. Not sure why this change is in here either. It was added by dalin in #35. dalin, can you speak to why this was added?

11. Fixed typo.

@duellj looks like you forgot the -u flag on cvs diff.

oops, needs work.

Re #58 and #59 changing form_set_value() to $form_state['storage']: I changed it simply because for some reason form_set_value() wasn't working for me at the time. If it works as is, great.

AFAICT the failing test has nothing to do with this patch.

Here's a re-roll, which:
- includes the INSTALL.sqlite.txt changes which the last patch was missing.
- Some docbook tweaks (blank line before @return, etc.)
- reverted $form_state['storage'] to form_set_value().

One thing I don't like is that DatabaseTasks->validateDatabaseSettings() is tightly coupled to the installation form. It must have knowledge of how the install form is structured:

  public function validateDatabaseSettings($database) {
    $errors = array();

    // Verify the table prefix.
    if (!empty($database['prefix']) && is_string($database['prefix']) && !preg_match('/^[A-Za-z0-9_.]+$/', $database['prefix'])) {
      $errors[$database['driver'] . '][advanced_options][db_prefix'] = st('The database table prefix you have entered, %prefix, is invalid. The table prefix can only contain alphanumeric characters, periods, or underscores.', array('%prefix' => $database['prefix']));
    }

    // Verify the database port.
    if (!empty($database['port']) && !is_numeric($database['port'])) {
      $errors[$database['driver'] . '][advanced_options][port'] =  st('Database port must be a number.');
    }

    return $errors;
  }

Though I'm not sure of a way around this. The calling function can't assume that each field is under the "advanced_options" fieldset because some database driver might override the method and try to validate a field that is outside that fieldset. Is there a function that we can use in install_database_errors() that takes a $form array and a field name as arguments, and returns the full path of the field? Or should we just insert a simple iterator here for that purpose?

@dalin:

Thanks, realized I forgot to diff against the install txt file. You missed one change in the file:

19c19
< On the "Database configuration" form in the "Database name" field, you must
---
> On the "Database configuration" form in the "Database file" field, you must

Also, just ran into a big bug. Try putting invalid database info in the settings form. The form error for invalid settings no longer appears and the form is allowed to submit.

+++ includes/install.inc	22 Nov 2010 03:48:11 -0000
@@ -190,10 +190,11 @@ function drupal_set_installed_schema_ver
 function drupal_install_profile_distribution_name() {
+  global $install_state;
+
   // During installation, the profile information is stored in the global
   // installation state (it might not be saved anywhere yet).
   if (drupal_installation_attempted()) {
-    global $install_state;

Also, you probably don't need to make this change anymore.

Yes sorry that last patch wasn't adequately tested. This one cleans up the errors and includes the suggestions from #66 and #67.

Still running into the problem from #67:

Also, just ran into a big bug. Try putting invalid database info in the settings form. The form error for invalid settings no longer appears and the form is allowed to submit.

@duellj which form validation are you referring to? "Required fields", "port must be an integer", and "prefix must be alphanumeric" validators are all working for me.

@dalin it's not the form validation, it's the database connection validation. Try entering in an incorrect username/password for your database. You should get back a nice long error message about why the database couldn't be connected. Instead, the form submits and then promptly fails, since it can't connect to the database.

The reason this is critical is that it does not simply change UI but it might invalidate every install tutorial we will have. That's not good.

Actually, this doesn't change the UI very much, currently only when selecting the SQLite driver. MySQL and postgreSQL installations are exactly the same.

The basic installation instructions (http://drupal.org/documentation/install/basic) still work fine:

Choose the type of database you created (e.g. MySQL or SQLite). Enter the name of the database you created, the username, and password. Save and continue.

Arguably those weren't very clear for SQLite in the first place.

Hrrrm. So if you enter some bogus connection data the form validator is calling db_run_tasks() and getting a connection error as expected. However form_set_error() is not properly registering the error. Hence the installation starts and when the system module tries to install tables we get a general exception in an non-useful error message. But I ran out of time before I could figure out why form_set_error isn't working.

Ah, the problem was that because these are general errors we can't attach them to a specific form field, so we we're using numeric keys. But because we are now using #limit_validation_errors those numeric keys fall outside of the selected driver. The fix was just to prepend the numeric key with the driver name.

Why is testbot ignoring my last patch? This one might even be good-to-go.

Re-posting the patch to see if it will jump-start testbot.

Tested latest patch an it works great.
The form looks much better now without the nested fieldset and it switches around quite nicely.
Without JS the form looks a bit weird because of the unpromiment titles of the DB-specific settings, but it's usable, and the trade-off for making it look just that much better for JS-users is worth it, IMO.
I also verified that it doesn't validate if you enter something wrong, ie a non-writable dir for the SQLite form. I also checked that it does install if you enter the correct info :)

Two screenshots showing how the form looks without JS.

Changing the name of the patch per DamZ's suggestion.

I swear, testbot hates me. Now it's just implicitly ignoring the patch instead of explicitly. Trying again.

#79 and #80 saw this patch tested, #81 and #82 saw it passed by the bot. There is nothing left to do. Thanks to everyone who made this possible, this is a very important patch.

Excellent work, folks!

Summary, for the UX folks:

Initial database screen before the patch: http://img.skitch.com/20101126-96q4b17xgmh34qi1qa8p438j.render.png
Initial database screen after the patch: http://img.skitch.com/20101126-ecfnjqcakm3m6ug74ebuhn5ysx.render.png

(Basically, they look identical, other than username/password are now marked "required")

Database screen after SQLite selection before the patch: http://img.skitch.com/20101126-96q4b17xgmh34qi1qa8p438j.render.png with SQLite selected instead of MySQL :P
Database screen after SQLite selection after the patch: http://img.skitch.com/20101126-fbmcu97h966tnj6mkdpeuer9rb.render.png

(No more asking for "username" and "password" fields that are totally irrelevant to SQLite, and replacing them with a single "Database file" field which is.)

Now some minor nits and then this should be good to go:

1. The password field shouldn't be required, in at least PostgreSQL, but I think MySQL too. If you go back into CVS annotate far enough, you'll find some issue where we bickered about this a lot, but came to the conclusion that it was a legitimate way to set up a database (if you authenticate via SSH keys, for example). And many of the *AMP installs don't have a pw on root apparently (have fun with those nightmares!).

2.

   // TODO: remove when PIFR will be updated to use 'db_prefix' instead of
   // 'prefix' in the database settings form.
-  $form_state['values']['prefix'] = $form_state['values']['db_prefix'];
+  $database['prefix'] = $database['db_prefix'];

Er. Why are we going into RC with TODO code for PIFR? Can that just be removed?

3.

+    $form['database']['#description'] = st('The path and name of the file that your @drupal data will be stored in. @drupal must always have write permissions for both the file and the directory where the database file resides. It is strongly suggested that you choose a path that is outside of the webroot, yet ensure that the directory is writeable by the web server.', array('@drupal' => drupal_install_profile_distribution_name()));

That's quite verbose, and breaks our existing UI text patterns. I'd push it to later, but we don't have a later. So can we shorten that up a little to something like:

"Absolute path to file where @drupal data will be stored. Must be writable by the web server, and should exist outside of web root."

(That's still a bit technical, but if you choose SQLite here, you're probably a geek.)

4. Do we need a validateDatabaseSettings in includes/database/sqlite/install.inc to check that the directory is writable?

5. Just a question about this:

+    unset($form['username'], $form['password'], $form['advanced_options']['host'], $form['advanced_options']['port']);

Are username, password, host, and port common enough options across major database systems that they belong in abstract class DatabaseTasks? Or should one or some of them be in DatabaseTasks_mysql/pgsql? Just wondering how much unsetting other database drivers are going to have to do.

Btw, this regrettably seems to break drush si, though I guess that's to be expected.

WD php: Exception: Database name field is required.                  [error]
Database username field is required.
Database password field is required.
Database name field is required.
Database username field is required.
Database password field is required.
In order for Drupal to work, and to continue with the installation
process, you must resolve all issues reported below. For more help
with configuring your database server, see the installation handbook.
If you are unsure what any of this means you should probably contact
your hosting provider.Failed to CREATE a test table on your database
server with the command CREATE TABLE drupal_install_test (id int
NULL). The server reports the following message: SQLSTATE[3D000]:
Invalid catalog name: 1046 No database selected.Are you sure the
configured username has the necessary permissions to create tables in
the database? in install_run_task() (line 419 of
/Users/webchick/Sites/core/includes/install.core.inc).
WD php: Warning: Cannot modify header information - headers already  [warning]
sent by (output started at
/Users/webchick/drush/includes/drush.inc:980) in
drupal_send_headers() (line 1042 of
/Users/webchick/Sites/core/includes/bootstrap.inc).

We'll need a follow-up issue in the Drush queue after this gets committed (or maybe before, as a heads-up).

This gets a +1 from me, architecturally. Awesome work, dalin!

Re webchick in #84:

1) You're right, rerolled with that.

2) That todo is not being added by this patch so is beside the point.

3) Switched to the shorter string.

4 and 5) I just did a quick skim of the Oracle and SQL Server drivers and they don't seem to do anything funny with the form, so it's probably just SQLite that needs an alternate form. I am therefore fine with SQLite being where the custom logic lives.

I also agree that we should be checking for directory writability. This patch does so.

Needs testers at this point? I could give this patch a try tonight.

I reviewed this patch and it looks great to me. I haven't tested the patch, so happy to wait for some test results from cosmicdreams. Or maybe someone else can do some testing.

I'm not too worried about the help text being too technical. Making a file/directory writable is well-documented. Before they get to the database specific help text, they need to understand what database to select. That choice might be harder than you'd expect. If making a directory writable is too difficult, then selecting the database might also be too difficult. Not exactly sure where this paragraph is going -- anyway, I think the help text is acceptable. :-)

Excellent, I'm done testing the other two patches I had slated for tonight so i'll give this a good test. I'll read through this thread for a bit to find what I should test.

The changes in the latest patch look good. I did not even know Drupal required the SplFileInfo stuff to be available in PHP, but apparently we already do - that's pretty cool, and we should use it more often :)

I rerolled to fix a few small issues (none of which affect functionality, so no worries if you already started testing the other version, @cosmicdreams - and yes, more testing would be awesome, since this is a big patch with a lot of different places where things could break)

Issues fixed (more details can be seen in the attached interdiff file):

1. One stray reference to "Database name" was still in INSTALL.sqlite.txt.
2. Added a few words to make the new SQLite help text into a complete sentence :)
3. Removed use of "please" in the error message (to follow UI guidelines). Maybe I removed too much, but I just took out the whole second sentence, since it didn't seem to add anything over the first.

Also, not really a commit blocker, but I am wondering why the SQLite validateDatabaseSettings() method has to duplicate all that code for verifying the database prefix. Can't it just call parent::validateDatabaseSettings() and merge in the results from that?

Pre-test;
* Dropped all of my d7 tables so that I could go through the install process again.
* moved my settings.php to settings.0.php so that I could copy default.settings.php to settings.php. This allowed me to see the proper installation prompts.

Test 1: Before the patch
* started installation by going to /install.php
* Chose standard installation
* Chose english as my language
* Saw notification that my settings.php wasn't writeable
* fixed that, proceeded with installation, got to our prompt
* chose the MySQL option, continued installation
* also checked the SQL lite option, no difference from the MySQL option.
* installation worked great

Test 2: After the patch
* dropped tables in database
* moved settings.php to settings.1.php and copied default.settings.php to settings.php
* started installation again
* chose standard installation
* chose english as my language
* got to same file permission issue, fixed it, and moved on to database configuration
* only had MySQL and SQLite types. Both UI's look as expected.
* Mysql installation worked great!

* removed settings.php and copied default.settings.php to settings.php so that I could test the SQLite installation
* received error message informing me that, "The database version 3.3.6 is less than the minimum required version 3.3.7." I guess that refers to my version of SQLite. Any way that can be explicit about which database is of version 3.3.6? Or is that impossible?

more after I update SQLite...

David et all: I tested with 346494-db-config-form.patch

David: Because it's not duplicating all of the code from the parent class. :-) The parent has two stanzas, one for prefix and one for ports. SQLite has no concept of ports, just prefixes, so I copied that one stanza and called it a day.

Oops, that interdiff was supposed to be a .txt file :(

Crell: Yes, but the code already skips trying to validate the port if it doesn't exist. That's why the previous patches worked OK also (where the validation was inherited from the parent implicitly).

Looks like I can't update SQLite since i'm running CentOS 5.2 ( I think). So I think I've testing everything I can.

+1 for RTBC

Oh. See, that's the dark parts of FAPI I don't grok. :-) We can probably just call parent:: then. You want to reroll? (I'm currently trying to rescue a dying server, sorry. :-( )

OK, I rerolled the patch:

• Switched to calling parent::validateDatabaseSettings() at the beginning of the SQLite validation.
• Based on comments above that the form looks really ugly when JavaScript is turned off, switched to having the driver section headers be in <h2> rather than <div>. This makes the form slightly less ugly, and also at least it's now semantically correct. (A screenshot is attached - this only affects people with JavaScript turned off.)

I also have an up-to-date version of SQLite, so I did some testing of that which @cosmicdreams wasn't able to do. I tested pretty heavily and wasn't able to find any regressions, except for one issue:

• Database prefixes were not working correctly (for either SQLite or other drivers) anymore; they were not getting written to settings.php with the right key. That's because the @todo to work around the PIFR issue wasn't making it into the new form state storage, and the old 'db_prefix' was no longer getting properly filtered out. That is fixed in the attached patch also.

The patch, the interdiff (as a .txt file this time!), and the above-mentioned screenshot are all attached. I'm feeling pretty good about this one...

Rerolled patch to remove port validation in SQLite validateDatabaseSettings() and call parent::validateDatabaseSettings().

When I tried the latest patch, I got Fatal error: Class 'SelectQuery' not found in /includes/database/sqlite/query.inc on line 17.

The test bot is still running so let's see if it is reproducible.

It looks to me like #99 and #100 were cross-posted. Since they contain the same change, but #99 also contains the additional fixes I noted above, I'm reuploading that one.

I can reproduce the fatal error from #101, but I can reproduce it without this patch also :(

The testbot probably wouldn't catch that, I don't think (unless there is a testbot machine that uses SQLite?)

Looks like that fatal error was caused by the recent commit at #851136: Make the database autoloading more robust - so I reopened that issue and moved it to critical.

other issue has been fixed

Is the approach of putting all options on one page accessible? I think it's used in authorize.php, but that doesn't necessarily mean it's correct.

@dmitrig01 early on in this thread it was decided that adding an additional step to the installation procedure was not an option (at least for D7). We needed to make the current form work.

Thanks to all who put in time on this over the weekend.

I tested the latest patch and it all looks great. If I'm not mistaken all concerns have been addressed.

RTBC!

Awesome work, folks! :D

Committed #102 to HEAD. Great work finishing this up in time.

A patch for Drush is here: #983914: Drush site install command no longer works for Drupal 7

Re the question about accessibility above, this is just using the new D7 #states API to conditionally reveal form elements, so if there are accessibility issues with that (I don't specifically know of any) it seems like something which needs to be addressed more generally.

dalin, you rock!

Now that this seems mostly complete could i ask someone to look at #793200: Document all database settings in default.settings.php (the people here would probably know how to fix it real quick :D )
until that gets fixed Drupal7 is completely useless on ZendServer CE unless they have rights to change the ZendServer Settings.