beta 3 breaker: Installer translation and other cleanup

Subscribing. Will test tomorrow.

I patched and started to install. After choosing Czech language, "Database configuration" appears immediatelly. So I wonder:
- Was the import so fast I even hadn't a chance to notice it?
- Did it even work as expected?

OK, I finally found a bit of time. (This is *big* for testing, took me quite a while to set up a reasonable install profile, translations etc.)

Basically, the patch works fine for me. Applied with just a tiny offset, everything works, nothing broken. I read through the initial explanation here, and through the code, and it all seems reasonable to me (within the limits of my limited experience).

The functionality: I see that the points A. and B. are probably valid, but I didn't have any trouble with that, given that I didn't enable modules that late. JavaScript didn't throw any errors too, which I don't understand though (there's no 'files' directory). I played quite a bit with adding translations into my testing .po files (strategic sample-strings of different nature, not a full translation), and didn't find anything untranslatable past the language selection page, so this seems good too. The strings for final pages are sprawled between installer and regular translation files, but given the re-using of some regular Drupal strings, this is correct IMO.

My proposals: (And forgive me for being a bit broader, I understand this issue as a bit of general install.php cleanup)

-- While going through the code, I noticed quite inaccurate (previously existing) code comment for st() function, so correcting in the attached patch.

-- The default install profile had quite a bit of inaccurate comments inside. Especially, it mentioned the exact order of the tasks, and explicitly required $task to be set to 'locale-import' at the end of profile tasks - neither is true anymore with this patch.
I corrected the listed order, but I feel really uneasy to have hardcoded 'locale-import' (needs to be changed to 'finished' now) as the end-of-custom-tasks flag. This is a new API as far as I can see, which should be a bit more abstracting - otherwise any change in the tasks order in future might break install profiles or else introduce fake task names or the like.
So I added a placeholder (invisible to user) task 'profile-finished', which seems to me a reasonable solution. Install profiles should advance to this one, which then only just fire next task (whichever it may be in future versions). I know we're in freeze, but I hope this doesn't apply to the new installer profiles so strictly. No one seem to use it yet.

-- I was figuring out how to set up a (testing) multi-step install profile my first time - and got stuck/confused again in the same comment block: There's very little about the return HTML from hook_profile_tasks(), perhaps a bit misleading too. In fact, once we move to any custom task, the return HTML is no more optional. It's truly required, and must contain a link/form-action with correct URL back to the page, otherwise it'll end with just a white screen and no way to continue.
So I reworded that description a bit, although I doubt this is final version. For one thing, I'm really unsure about the suggestion to variable_set('install_task', 'new_task') - seems to me like some cruft. install.php itself doesn't do forms that way, and once the HTML is returned, install.php will set the variable for us anyway. But I can't be so sure, so I left that bit untouched for now.

-- The above means, that every single profile with custom pages needs the URL (like www.example.com/somedirectory/install.php?profile=default&locale=en ) for providing a link to proceed. So I propose to give it as a second parameter to hook_profile_tasks(), to avoid having the global variables and path building hardcoded repeatedly in profiles (possibly buggy, too).

-- I also noticed that, unless drupal_set_title() used in the profile, first custom page shows the title "Configure site" inherited from previous step. Although it might be fitting in some cases, it's a leak from previous step, so I cleared it for consistency. Profiles may/should set own titles for new pages, not relying on leaks.

-- If we're pursuing maximum of the installer being translated, I propose to add a possibility to bypass the language selection entirely, from the install profile. The "Choose language" page is always English, and in some cases makes little sense. Look at the projects for 5.x install profiles: Hebrew, French, German, Japanese, Russian (I found even Czech on Drupal.cz site) - half of the available profiles are language-focused. If I choose German profile for instance, why would I choose to install in English?
While having only just one profile bypasses the profile selection page, language can't do the same. There's always English plus the other one, never just one language, and no way to tell in the code whether the profile is language-oriented.
So I propose to have a setting possible in the profile, to inject language selection and so skip the UI page. If I have a specialized Drupal package with Czech installation profile and translations, it'll start in Czech straight away, without annoying English selections. If I have a multilingual / config focused profile, It'll start with the selection, just like now.
I just added a new possible array key to hook_profile_details() for this. It was somehow possible already via hacking global variables at the begin of profile file, but my version is probably cleaner, and also have a check for validity (so if the pre-selected language is malformed or missing .po file, language selection re-appears showing which other options are left).

Another question is how all the files (install profile, translations) will get into place, but that's another issue (packaging policy - I had quite a difficulty to merge downloaded Czech translations into Drupal release, the 'unpack tarball into Drupal root' suggestion didn't work: It created a subdirectory 'archive_name-FILES', and I had to merge translations to all module's directories manually. So if local Drupal communities choose to maintain already-merged tarballs, big +1 from me. And - my point here - then it might contain only single profile (default removed), and so start installation in the local language straight away. This works for me with the patch below and reasonably prepared package.)

Attaching a patch with all the above included (and t() debug code gone, sorry). Feel free to choose how much is able to go into 6.x and into this issue.

A few thoughts to: (A) - JavaScript

The JavaScript localizer (as well as JS preprocessor, luckily disabled by default) gets most probably called via theme_maintenance_page() and drupal_get_js(), which I discovered in update.php issue: http://drupal.org/node/179143 - My patch in there introduces a global flag usable for external scripts to suppress dangerous hook invokes and the like.
I wonder whether that flag is also suitable for install.php: Once we have modules installed, do we *need* their hook_init(), hook_exit(), and JS preprocessing to fire, or do we rather *fear* it? We probably don't want the JS stuff, but I'm a bit unsure about the rest. (Already discovered cases are Statistics module logging page visits and blogapi module injecting RSD link, not to mention nearly all modules adding their CSS, so I'm inclined to say that we don't want these hooks to fire.)

As for the files directory, my opinion is that we should distribute it just like sites/all, i.e. included into the tarball and containing a simple README.txt inside, to explain what to do with it. I believe most sites are going to have their files there anyway, so requiring the user to create directory manually as soon as the first error message pops up is not too nice. Sure, he'll still need to make it writable, but that's no different from the config file (consistency really helps to avoid confusion here, I think).

Another question is whether the files directory should be better placed somewhere inside sites tree, but that's definitely out of scope for this issue. It's discussed at http://drupal.org/node/98824 and related. But simply just having the (default, for multisite setups just installer-temporary) files directory inside tarball for now might be helpful for us here.

OK, what about this one:

- Added files directory. Includes a README.txt file with suggestions.

- Changed the check in system.install, to work on install too, resulting in files directory being required by the installer. (The patch seems big here, but it's mostly just indentation change. However, there's a separate description for install case, to make the wording a bit sensible, and to avoid one url() call.)

- Modified the requirements error page in install.php, to show another title and exclude version info if necessary, to accomplish sensibly looking output in the 'files' directory case.

So now, Drupal shows *no* errors on files directory, the status report is nicely green on fresh install. (Apart from cron never run, discussed elsewhere, I believe - and settings.php unprotected (sites/default writable), which is probably unsolvable, given that Apache doesn't own the directory.)

What I don't understand though, is why the sites/default writable requirement doesn't show on the "verify requirements" page too. It shows on database settings page.

Patch applies cleanly, good work all!

I was hoping to give it a look today, but my time went elsewhere, so just quick comments:

- I would rather add the second import only if needed, not from the start (to be discussed)

- I would add it with other title, probably "Import additional translations", otherwise the two identical phases are giving an impression of a nasty bug occuring (definitely, as far as I can tell)

- Tasks order is mentioned in default.profile comments, needs update (can wait until we're done with substantial changes)

- My own little TODO: I need to try a multistep install profile with a form. I suspect that there's no need to variable_set() from profile function, in which case that comment chunk should go.

- The page title "Configure site" is only set if the profile function is called as a fall-through in the same page request as previous phase, i.e. only the first time (not on page reloads!). So the title should be either removed (as I suggested previously), or set explicitly and always, or required to be set from the profile. Otherwise it'll disappear if the user hits reload.

- I'm not entirely sure if I like the idea of two different callbacks for batch ending, with installer phases hardcoded inside locale.inc (not too obvious place to look, if patching install.php in future). Can we rather have a single callback, and variable_set() the installer phase to be used there? We're going to remove these temporary variables anyway...

Attaching a new patch (also removing offset).

- Titles for the two imports changed to: 'Set up translations' and 'Finish translations'. Feel free to further change, I'm still unsure...

- The title "Configure site" now set on two different places, one explicitly for the form-page, other explicitly as default for profile-tasks. The initially existing one is now moved to a place only executed if really going to print that page, so no more "leaks".

- The comments in default.profile further changed:
* List of installer tasks updated, and expanded to show ALL the tasks. From install.php perspective, the initial tasks are entirely different, but if a profile developer should see some list, then a full list. From this perspective, the initial tasks are no different than 'configure' - already executed ones simply.
* The bit about displaying forms and using variable_set() completely rewritten. The originally suggested way with variable_set(), avoiding drupal_get_form(), and pasting hackish code from install.php is complicated and entirely not necessary. I tried it, and unless I'm missing something, drupal_get_form() works pretty fine here, we only just need to avoid redirecting; it results in much shorter and cleaner code.
variable_set() is still useful for remembering other data between custom tasks, though.
* Also included some pitfalls I encountered, specifically pages re-loading reminder, and removing temporary variables.

- I'm unsure whether this allows for some simplification in install.php itself or not, but I let it alone for now.

Attaching also my testing profile file (tweaked file extension as usual) as a proof of concept - enforces Czech language (won't work unless some cs.po supplied), and adds three tasks - two simple forms, and a page showing entered values. Perhaps the forms bit might be turned into an example snippet for some documentation page, too. (?)

- Nothing done about variable_set() the batch ending phase, yet...

Along the way, I tested a bit more, and I can confirm that the translations are imported correctly, including the additional part (i.e. update module).

The example profile *definitely* needs *a lot of* polish, currently it's a combination of old version default.profile, Czech profile for 5.x, quick hacks, and no code comments updated. I didn't mean it to go anywhere in this form, except some testing and your quick look for reference. ;)

I hope to look into the rest this weekend. Generally I agree with all the above, but time is pressing on me :-/

(My personal schedule always changes...) Now attaching a new patch, with the following changes:

- No page title pre-set for custom tasks (per #19)
- "Setup translations" title (per #19)
- documentation fixes in default.profile ("should" and the title, per #19)

- Temporary variable for locale batches (already processed files) renamed, because I believe the convention requires us to have "install" as the first part here, rather than "locale". This is installer temporary variable.

- Batch ending callbacks sorted out. Finally, I decided not to do variable_set() (as I suggested earlier). I just moved these callback functions to install.php, because they are only used there, and contain installer-specific data (names of installer tasks). This is also a bit of cleanup of locale.inc (although it's not much of code, really), and consistency of having all the occurences of installer task names in the same file.

I would also like to draw attention to one theoretical security problem, which I discovered along the way and fixed with this patch:

With the previous patches, the function locale_batch_by_language() had '_locale_batch_installer_initial_finished' as default callback, with that function really present in runtime code. So, if anyone (contrib) wanted to use the function, for whatever reason without ending callback, it will work as expected (seemingly), and it may be unnoticed that a grave security hole was opened as a side effect: The default installer callback actually used, setting the installer task variable silently back to "configure". Then, any anonymous user will be able to access install.php, and set up a new UID1 user through a nice simple UI, overwriting the previously existing UID1, and so gaining full control over the site, kicking the true owner out at the same time. (I tested, this really worked!)

Now, locale_batch_by_language() have no default callback, and all installer variable_set() are inside install.php only, so this problem no more exists.

The installer is tempting for cleanup nearly everywhere I look...

Attaching a new patch:

- I really don't see, why the Site configuration form was done in such a strange way. drupal_get_form() works pretty fine there, as well as inside custom profile-tasks. I even see, that drupal_get_form() is already successfully used for all three pre-database forms! (profile, language, database settings)
Because the existing code inside install.php was A LOT of a FAPI hack (I had really a difficulty to understand how it works), I tried to implement the simple way from my testing profile here. And it seems to work pretty fine; only thing I can't test (without extensive Apache config research at least) is clean URL's check (no clean URL's on my system, entirely), but I think it should work too, as two other JS things on the same form are OK.
* This bit definitely needs some review. The patch file, unfortunately, mixed lines a bit, so better see the change in final un-/patched install.php.

- Quite a few code comments improvements, including:
* Added missing (minimal) comments to several functions. There were functions separated only by a single blank line from each other, not very nice for quick scanning the file visually.
* Fixed a mention of "profile-custom" install task, which doesn't exist. Some cruft, probably.
* Removed a comment on install_configure_form(), about adding the installer task to $_GET data - really a nonsense, as far as I see, probably another cruft.

Now, I'm done with all the things I wanted to check and do in the installer, except that I don't know how the 'files' directory issue is playing together with this. Do we still need to translate JS inline? My install seems to do no js translation/parsing on/after install, still, although 'files' is there OK.

I wondered, where that hackish FAPI code came from, in the first place... Did some searching:

It came from http://drupal.org/node/141637 along with the initial commit of the 'Configure site' form, and it's history is unclear even there:
- Initially, it was (a bit more simple) in the patch, inside default.profile
- In #2, the author removed it in favor of drupal_get_form() just like we're doing here, stating that his previous problems were elsewhere.
- It stayed so until #16, first the old way commented out, then removed entirely. Seems no-one mentioned any problems on the following 8 patches.
- Then, #28 moved the form into install.php (BTW - along with the nonsense comment I'm removing now; seems like this patch was not that much carefully reviewed back then), re-introducing the hackish "form breaking". The author only said Because of the way FAPI is structured, there is a small bit of extra work to do multi-step forms without using $_SESSION glue., although I see no really multi-step form here, and it seems no-one ever mentioned this bit ever since. Also note, that this was the *old* FAPI back then.
- Shortly after commit, http://drupal.org/node/138706 updated the code to new FAPI3 ( #88 ) without much of a notice. It's only now, that it got so complicated and hackish, and it seems that no one re-considered whether it's still necessary, unless I'm missing something.

Conclusion: I'm unable to find any sensible reasoning or previous attention given to that bit of code, and given that drupal_get_form() works fine for me, I assume it was really just cruft.

Hopefully better title...

More ideas came overnight...

- Further changed the submit-detection on site configuration form. In the previous patch I used a new temporary variable, being afraid to touch the existing data variables (site name, mail...). The reason was that I saw the form being pre-populated from these variables, which meant there might be some values already in them, breaking my submit-detection.

But after more research it turned out, that there are NEVER pre-populated values, that code (setting defaults from variables on the form) is just another cruft from early phases of http://drupal.org/node/141637 , when the installer was intended to be $_GET driven. The only use case of pre-populated values now is an attack after 'install_task' variable blown away somehow (see #21), revealing the email to attacker additionally (and yes, it worked this way when I tested...) We don't want this.

So I removed the variable-based defaults, used the data variables for submit detection (simplification), and added a check for pre-existing values, which is an indicator of installer re-run and therefore of possible attack. With this change, installer no more allows for UID1 resetting, even if the task variable got blown somehow (after pointless profile/language selections, it'll show "already installed" page now). Now, you'll need to blow THREE variables to open that security hole, which is unlikely to happen due to just a bug somewhere.

- I came across an interesting hook_form_alter() feature allowing the profile to alter the site configuration form. This is quite nice, but I have cleanups there too:
* The call was incompatible to standard hook_form_alter() specification. Although this is a special case, called outside FAPI, for one special form only, and so there's no $form_state yet, I think it's still correct for consistency to provide the parameter. Empty array() added.
* This should be also shown in the default profile somehow IMO. A nice use case here is the default site name in the form, which may be well profile-driven; so I moved the default 'Drupal' site name to the default profile, demonstrating the use of hook_form_alter() at the same time.

Attaching the new patch, and digging further (JS translations are not working for me, mysteriously, yet)

Ah, now I see... The JS translations were not saved only just because my testing .po's didn't have any translations for JS strings yet (5.x files, just tweaked a bit). Silly mistake.

Now it works, although I'm unsure about st()ing the clean URL strings, given that t() should contain a translation anyway (double translations for the same).

So I guess I'm done here (again ;) and hopefully it's true now)

Fair enough. The trouble is, that I discover things along the way, too related - but I already thought about splitting this patch. Anyway, I already stopped here. Going to open a new issue soon for some less related things :)

One very very small followup patch that I just noticed while reading this patch:

The patch introduced two steps labeled "Setup database" and "Setup translations". Or maybe these were already there, I didn't really go back and look.

Anyway, technically, that should be "Set up database" or "Set up translations", or, alternatively, "Database setup" and "Translations setup."

The former looks kinda' weird and the latter is not parallel with the other options.

So the attached patch changes these to "Configure database", and "Configure translations".

The example installer profile now polished with extra code comments, and submitted to http://drupal.org/node/196928 for future use in documentation.