Allow modifying forms using the FAPI throughout the entire install process

Thankyou.
I did the same (or similar) (1 line core hack) because I was asked to streamline a kitset site-maker.

I'm aware that many normal features are not available at that stage of the pre-install ... but it's still FAPI!

... although I don't see how preventing the password being pre-set is a vulnerability. If it were an issue, then the DB password actually working on the db-server would be the problem.

Are you just trying to dissuade folk from using it that way?
Me - for my other admin users - I actually convert the password field to plaintext so we can compare it with the one in our internal documentation before proceeding. It makes rapid development prototype sites a matter of clickthrough rather that reconfigure-each-time.
There are issues, but for internal throw-away testing ...
.dan.

Sounds like an edge case - but is a valid concern
... if you've already pre-filled that password yourself earlier. Why not just not do that, instead of adding it then later blanking it?
If the subsite folder has a db connection string pre-set, yes, you'll see that preset again, but if your new subsite folder was a clean template settings.php then it won't..
When I deploy new site folders, the settings.php is a copy of default.settings.php - as usual.
Then my install profile fills it in via FAPI (based on custom arcane tweaks of my own)

Which I guess leaves it open to the same exploit you avoid ... but by choice on our dev intranet.
But I have the choice. My use-case is training, rapid prototyping, and getting non-admins to run through simple site setups quick.
And for me to tear-down and remake install profiles a lot while developing those install profiles.

Copy & Paste is possible, but is between 5 and 20 point & clicks more than just going "Next"
If the UI is doing its job and usable, then after the first run through, documentation should be redundant and forgotten about.

If you feel the need to zero out the password - do it in your (now supported) form_alter. That's what it is there for!

Forcing it out like that line in this patch does would actually change current core behaviour (which right now allows easy rebuilds of sites after you drop the database). I think this is by design.

I support the form_alter enhancements, but not breaking existing functionality.

Subscribing...

that password stuff makes no sense, if the profile wants to prefill the db password then it takes away the password field and uses #type value. get rid of it. the profile form_alter in itself is a good idea.

++ Cool and tidy.

Can't a URL hacker call any function on your site that that ends with _form_alter()? Granted that most files are not included yet but still.

Moshe is right we need to validate the profile.

We need to validate that $_GET['profile'] is a valid profile.

That's good and eventually we want to backport the check IMO.

Coding style plz!
Two inserted lines with spaces
An extra unnecessary space at the end.

+  if (!empty($_GET['profile']) && file_exists(DRUPAL_ROOT . '/profiles/'. $_GET['profile'] . '/'. $_GET['profile'] . '.profile')) {

Wrong string concatenation.

+  

Trailing white-space; blank lines should be blank.

+  // Allow the profile to alter this form. $form_state isn't available
+  // here, but to conform to the hook_form_alter() signature, we pass
+  // an empty array.
+  $hook_form_alter = $_GET['profile'] . '_form_alter';
+  if (function_exists($hook_form_alter)) {

I'm not bold on this one, but we might want to consider to also state that drupal_function_exists() doesn't work at install time.

   return $form;
+
 }

Please remove that trialing blank line.

We try to avoid abbreviations in comments, but anyway, good to go.

Nope

+  if (!empty($_GET['profile']) && file_exists(DRUPAL_ROOT . '/profiles/' . $_GET['profile'] . '/' . $_GET['profile'] . '.profile')) {

I don't know what DRUPAL_ROOT is doing here - it's not defined anywhere at all, and makes my install totally fail. (submitting the first page profile selector just refreshes it, as it thinks I've chosen a non-existent profile)

+  if (!empty($_GET['profile']) && file_exists( 'profiles/' . $_GET['profile'] . '/' . $_GET['profile'] . '.profile')) {

is fine instead.

it needed fuzz. I think testbot doesn't like that.

I feel that this patch slightly conflicts with my goals for #509404: Fix some conceptual problems with install profiles and make them actually usable

specifically regarding this :
#525594: Installation should consist of 2 phases instead of one

the database details form should not be editable. but once i get to the above patch the rest of the install process will run in a full bootstrap, so modules and install profiles will be able to create their own wizard forms and modify any of the existing forms.

I disagree with the database form not being alterable by install profiles, because we have an existing use-case: http://drupal.org/project/demo_profile wants to restore the db_prefix that has been dumped along with some other information.

if you select the install profile before that form, you can set the global db_prefix and just make the form default/override to the global if found.

This serves both our purposes and also works with #524728: Refactor install.php to allow Drupal to be installed from the command line, where you can create the settings.php before you run the install from the command line.

if you make that form alterable, it means that even if you can develop a mechanism to automate every other install profile, you will still end up needing to write demo_profile specific code.

I'd keep the preg_replace() in the profile validation or otherwise null byte poisoning attacks are possible on the profile name. Since it is only used install time, it might not be that critical, but anyway, just whitelisting for the allowed chars still makes sense IMHO. http://www.google.com/search?q=null+byte+poisoning+php

BTW, the database settings absolutely need to be alterable so we can remove the unnecessary username and password fields when SQLite is used, or change the database name maxlength for PostgreSQL and SQLite. I hope adrian's work will not completely block #346494: DB drivers need to be able to change the configure database form during install.