This continues http://drupal.org/node/48732 because it's long and the HTML is broken. I began to hammer Jeremy's awesome work based on Steven's comment. drupal_install_profile is IMO broken ATM as I am working on removing the longish $requirement arrays. I reworked drupal_shorthand_to_bytes, drupal_verify_version_php, drupal_verify_install_file and system_requirements_bootstrap. I removed Apache checking, it's very configuration dependent and not everyone runs Apache so it's no core stuff. More to come.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeremy’s picture

Some disadvantages to linear requirement checking:

  • We stop after the first error, thus the user will put effort into the problem, fix it, and retry only to get stopped at the next error. I personally find this frustrating, and would much rather see all the problems at the very beginning. By queuing up all the checks and performing them at once, we get to see everything that is wrong. Perhaps when doing checks linearly you could set a token when there's an error, and then at the end of the requirements check the token and return FALSE.
  • The inclusion of install.apache.inc was intentional, even though not everyone uses apache. The checks in that file are only run if apache is being used. The goal was eventually for someone to contrib an install.iis.inc, install.lighhtpd.inc, etc... The webserver is an important aspect of a Drupal website, and the installer should be able to verify that it meets the minimum requirements.

That said, I'm looking forward to seeing your patch.

Steven’s picture

Not resolving duplicate requirements does not mean you need to stop at the first error. The two are unrelated, and both systems can work either way.

Steven’s picture

PS: One thing that I mentioned in the previous issues...

There are several places in admin where we perform checks about the current setup. The way these run-time requirements are implemented is not ideal... most are form_set_error() statements on admin/settings, which prevent you from making any (unrelated) changes when an error occurs. And some are pure status indicators which don't really belong (e.g. cron and unicode).

Reusability says that these checks and the install requirements should share code.

I would suggest adding an admin/status (or whatever we want to call it) where you can get an overview of all errors as well as recommendations. Something like the cron status would only show up if there was a problem "Note: cron.php has not been run. Are you sure it's set up right?" or "PHP mbstring support is not enabled. This means you don't get full support for international character handling. Consider installing it."
etc etc.

As an encore, we could in fact check these errors on /admin/settings or /admin too, but only display a compact note such as "Errors and warnings have been found with your current setup. Please see the status page." when necessary.

The status page is also an ideal place to display a summary of Drupal site information (apache/php/drupal/sql version, modules enabled, theme, ...) that people can copy/paste when asking for help.

chx’s picture

FileSize
105.99 KB

After checking with Amazon, I simply ripped the whole requirements stuff. We can add it back later. For now, let's concentrate on other things, it's still quite a mouthful. (Looking back, as I have already converted pre and post for system I might as well add back in the next round. We shall see.)

This is mysql specific at the moment, but next spin will give it back the pgsql parts, too. There are still parts that need love, they are not form API'fied and such.

I have not worked past the INSTALL_BOOTSTRAP_SETTINGS part. You are warned.

chx’s picture

Status: Needs work » Needs review
FileSize
108.32 KB

Tada! We are at needs review phase again. More coolness might follow tough (ie I will add back the pre and post requirements hook but without the whole requirements support stuff).

Dries’s picture

Great work, chx! A step in the right direction, but the code needs a bit of love.

For the MySQL install, we might not actually have to duplicate all the default inserts:

+      db_query("INSERT INTO {system} (filename, name, type, description, status, throttle, bootstrap, schema_version) VALUES ('modules/block.module', 'block', 'module', '', 1, 0, 0, 0);");

Looks like it might be fairly easy to get rid of quite a bit of duplication.

The following looks like an ugly hack:

+    global $install;
+    $install = TRUE;
+    include_once './includes/install.inc';
+    if (!_profile_installed()) {
+      _install_goto('install.php');
+    }

If it is not (unlikely?), please add code comment describing what is happening and why.

You added some contact.module changes (2 + 2) that don't belong in this patch.

Please add a code comment describing this line:

+  $string = preg_replace('/<!--.*-->/U', '', $string);

These defines are so-so:

+define('INSTALL_BOOTSTRAP_ENVIRONMENT', 0);
+define('INSTALL_BOOTSTRAP_PROFILES', 1);
+define('INSTALL_BOOTSTRAP_SETTINGS', 2);
+define('INSTALL_BOOTSTRAP_DATABASE', 3);
+define('INSTALL_PROFILES', 4);
+define('INSTALL_CONFIGURE', 5);

It is not clear why some have _BOOTSTRAP_ in their name. What is the difference between INSTALL_BOOTSTRAP_PROFILES and INSTALL_PROFILES? It would be nice if that distincation was clear based on the names of these defines. Maybe SELECT_PROFILES and INSTALL_PROFILES or something? Each of these phases are 'actions', we might as well use verbs to describe them.

The following looks like dead code. It is not used anywhere ...

+/**
+ * Check if there are any errors at this point in the installation process.
+ *
+ * @return
+ *  An array of errors, if any.
+ */
+function _install_errors() {
+  $messages = drupal_set_message();
+  return $messages['error'];
+}

Is this function really required?

+/**
+ * Test for the existence of $profile in settings.php.
+ *
+ * @return
+ *  TRUE/FALSE if $profile exists.
+ */
+function _profile_installed() {
+  global $installed_profile;
+
+  if ($installed_profile) {
+    return $installed_profile;
+  }
+  else {
+    return FALSE;
+  }
+}

Isn't that just a long and complicated way to write $_GLOBALS['installed']?

I don't like the way you are using _install_goto(). Like, you call a small helper function to figure out the name of the install profile, and that small helper function actually triggers a goto? Even in BASIC that was frowned up on, let alone in modern programming languages. Ugly, ugly code! Bad, bad control flow.

+function _install_profile_name() {
+  global $profile;
+  static $name = NULL;
+
+  if (!isset($name)) {
+    $profile = $profile ? $profile : $_GET['profile'];
+    if (!$profile && !($profile = _profile_installed())) {
+      _install_goto('install.php');
+    }
+    require_once "./profiles/$profile.profile";
+    // load profile details
+    $function = $profile .'_profile_details';
+    if (function_exists($function)) {
+      $details = $function();
+    }
+    $name = isset($details['name']) ? $details['name'] : 'Drupal';
+  }
+
+  return $name;
+}

This is a recurring pattern that could probably be cleaned up by setting a global variable:

+  global $profile;
+  if (!$profile) {
+    $profile = $_GET['profile'];
+  }

Is it 'installer profiles' or 'install profiles'? We're mixing both.

Please use the database abstraction layer:

db_query("INSERT INTO {system} (filename, name, type, description, status, throttle, bootstrap, schema_version) VALUES('". $module->filename ."', '". $module->name ."', 'module', '', 1, 0, 0, 0)");

Do we really need the changes made to theme_status_messages()?

Steven’s picture

Maybe we should drop the name "install profile" and go for "distribution". People might get confused with profile.module later on.

And the contact/filter.module changes are leaked in from another issue/patch.

Dries’s picture

Personally, I think 'profile' is a better name. The word 'distribution' implies that it is provided by others (but maybe I'm mistaken).

webchick’s picture

re:

The following looks like an ugly hack:

+    global $install;
+    $install = TRUE;
+    include_once './includes/install.inc';
+    if (!_profile_installed()) {
+      _install_goto('install.php');
+    }

If it is not (unlikely?), please add code comment describing what is happening and why.

The point of that code is to not present the user initially with a big scary error that says their database connection isn't working, and instead direct them immediately to the installation page. Most of the cutting edge installers I reviewed (Joomla!, Gallery, etc.) do this as a basic user-friendliness thing.

halkeye’s picture

I'd like to point out that install.php is not included in the latest patch.
I'm not sure if we are commenting on word choice yet or not,

# Failed to automatically fix permissions of file ./sites/dev.sfuarc.com/settings.php, insufficient privileges.
# File ./sites/dev.sfuarc.com/settings.php is not writable.

I think those two lines are out of order, first it should be telling the user it can't write to the file. infact I don't think the chmod message should be there at all, i was very confused by what it ment at first glance.

Actually, now that I installed it it makes sense, but I think the unable to adjust permissions (automaticly fix is scary) should be more fatal or not mentioned at all...

Lots of installers tend to use the config file to check if something is installed.. So by default it'd check sites/default/settings.php if it exists, its an install (from ANY PAGE) (also, if someone makes the directory dev.sfuarc.com that'd be used instead), that way the unable to create the file is a bigger problem..

Amazon’s picture

It would be helpful if the Drupal core maintainers could lay out a list of features they want to be included as well as comment on code quality. If we could determine a clear roadmap for which features will get in with this patch then we could rally support. For example, we have agreed to remove requirements for now, and re-submit as a broader API later. Should we remove the installation profiles and just focus on the Drupal bootstrap install?

What level of usability features do you want included in this initial patch? Do you want automatic redirection to install.php? Do you want theme capabilities in the installation profile so that we can add javascript collapsible fieldsets for error and status messaging? We know through our studies this capability is very important for users.

Once you layout a list of what you want and will accept it will be much easier for contributors to participate to get this done. If you want to comment on code quality it would help to continuosly refer to code quality and style guidelines that can be reviewed before patches are submitted.

Thanks for the reviews.

Kieran

drumm’s picture

Status: Needs review » Needs work

"Should we remove the installation profiles and just focus on the Drupal bootstrap install?"

My understanding is that right now the install profile wizards are the output of new configure hooks in alphabetical order by module name. Install profiles should provide a cohesive configuration experience which can not be done by making a custom form. However, the module-by-module config is good for enabling modules later via admin/modules and is a good fallback for quickly making new profiles.

The main issue is that the installer is not a normal bootstrap loading of Drupal. I think it is best to get out of the installer as soon as possible so we know that all the features of Drupal's API will work as expected. This also logically separates the patch work more and allows you to put that in a later patch.

"What level of usability features do you want included in this initial patch? Do you want automatic redirection to install.php?"

Looks like it is done right, testing on DB errors to avoid unnecessary checks on every page, except for it only being in the mysqli database engine apparently.

"Do you want theme capabilities in the installation profile so that we can add javascript collapsible fieldsets for error and status messaging? We know through our studies this capability is very important for users."

The collapsing or the information is important? Either way, collapsible fieldsets are for fields, not status and error messages.

Dries’s picture

I don't mind leaving the profile stuff in -- but if you think it is easier without the profile stuff, that is fine to.

Do you want automatic redirection to install.php?

Up to you. The current implementation needs work, or at the very least, good code comments that explain the ugly code.

Do you want theme capabilities in the installation profile so that we can add javascript collapsible fieldsets for error and status messaging?

How does this affect the code? Where in the code does this happen?

Once you layout a list of what you want and will accept it will be much easier for contributors to participate to get this done. If you want to comment on code quality it would help to continuosly refer to code quality and style guidelines that can be reviewed before patches are submitted.

Thanks. I reviewed the code a couple comments above. Some of the changes are trivial. I'll try to do another review as time permits.

Jeremy’s picture

The main issue is that the installer is not a normal bootstrap loading of Drupal. I think it is best to get out of the installer as soon as possible so we know that all the features of Drupal's API will work as expected.

The installer bootstrap was written _because_ we can't bootstrap Drupal. If you look in _drupal_install() at the big switch, each case with an INSTALL_BOOTSTRAP in the name is a required step just to get to where we can connect to the database. Technically at INSTALL_PROFILES though, we should be able to do a full Drupal bootstrap. And I agree it is important that we do, as profiles will want to be able to use all standard Drupal functions. The problem is, at that point even though we can connect to the database there are no tables created so a full bootstrap would fail.

One idea would be to force a core installation before performing a profile installation. This core installation would be a special case just to get the core tables created to get Drupal up and running, and at which point we'd then be able to fully bootstrap Drupal.

"My understanding is that right now the install profile wizards are the output of new configure hooks in alphabetical order by module name."

Yes, at this time it's that simple. The goal is to expand this to allow profiles to define one or more screens, and to control the order of the screens displayed. (module.install files would also be able to define screens). But simple first seemed like a good place to start.

chx’s picture

FileSize
35.6 KB

I simplified this and that, esp. drupal_install(). I tried to find a valid use case for this bootstrap-like behaviour but failed to catch one. Leaving an install in an uncofigured state is surely not desired and if we need to always run until the next step, what's the point of invidual steps? Removed dead & crufty code, too.

Totally untested though.

moshe weitzman’s picture

hmm. any progress here? lets keep moving forward.

chx’s picture

I am on the patch now. Occasionly even I have a life besides Drupal, but worry not, it won't happen again for a long time (ie. I failed my entrace exam to the university last Saturday).

chx’s picture

Besides that, noone reviewed the last version...

merlinofchaos’s picture

Subscribing. I'll test this one hopefully tomorrow. Unless you've got a new version.

drewish’s picture

that last patch doesn't apply because of changes to module.inc...

chx’s picture

FileSize
71.19 KB

The whole thing works again. Rescyned system.install with current database. Instead of shipping two mysql database definitions, one is enough if all we want is to slap a DEFAULT CHARACTER SET utf8 after the table definition.

I took care that all files needed are included. More work is needed, but we are getting somewhere. I am glad to see the show of hands for reviews. From now on, I plan to dedicate one hour every day to this patch and I would like to see reviews every day.

chx’s picture

FileSize
71.34 KB

new version

chx’s picture

FileSize
71.14 KB

not my lucky day

drewish’s picture

chx's patch on #23 worked for me on WindowsXP with Apache 2.0.51, PHP 5.1 and MySQL 5.0.18.

The only thing I'd suggest is adding a note to the new site message letting the user know that the list of steps will be their new homepage until they add some content. It was tempted to open all the links up in a bunch of tabs because I thought it'd disappear as soon as I went to setup an admin account.

nedjo’s picture

It's good to see some solid progress here.

Here are a few broad questions that I'm not sure if we've dealt with already. They may be beyond what we can take on in this first installer iteration, but considering them now may be useful anyway.

1. What goes into a base install?

It looks on first glance like system_install() has far too much in it. Rather than doing all of what we currently do in a base installation, we probably want only a bare minimum. For example, we probably don't need to be creating the five aggregator tables; only some sites will want/use them. We have a chance here to reverse our problematic special treatment of core modules by giving core modules .install files.

2. Db inserts

Sooner or later we're going to want to provide methods for doing what we do currently through all these db_query() calls. It's not only a question of repetition (the whole series of INSERT INTO {system} calls Dries pointed to). The more critical issue is that some of the methods require references to existing data. We're going to want to e.g. assign additional permissions to the 'authenticated user' role for the new modules we enable in a profile. We're going to want to create items that require a sequence table value (nodes, etc.). These actions require methods that can update existing records or determine new ids.

For this, we could use a series of methods for creating/saving the various objects/items. E.g., drupal_save_system_item() could save module, theme engine, and theme data. In some cases we have existing methods in core that could be adapted, e.g., menu_save_item().

3. Profile components

We're going to want some profiles to do parts of what's in others, but repeating all the code is awkward and error-prone. Ideally, we would be able to construct a profile as a set of components. That way each component could be made generic enough to serve various profiles.

If we went to this approach, we could turn system_install() into a component called e.g. 'base', that probably would be included with every profile.

chx’s picture

Status: Needs work » Needs review
chx’s picture

I am currently moving out database tables from system.install into various install files. Also, I (again) removed those inserts that inserts modules into the system table, there is no point as the profile install will do this.

chx’s picture

FileSize
71.53 KB

Here is a new version. Tons of .install files. system.install only installs 35 tables instead of 58.

Also I prefixed table names in *.install.

chx’s picture

FileSize
71.53 KB

moved two insert statements to their install files.

chx’s picture

FileSize
37.44 KB

Removed menu.install

chx’s picture

FileSize
71.33 KB

better patch

eaton’s picture

Looking good so far. I just patched a clean HEAD and installed from scratch with no hitches using 18. I know there are some additional changes chx wants to make regarding the menu install stuff -- I'm going to be putting together a few experimental install profiles for various contrib combinations.

chx’s picture

FileSize
72.77 KB

Eaton found that the .install files for the modules we enable are not loaded thus not executed and even fixed it.

Steven pointed out that I could remove _mysql_create_table.

Keep it coming, folks!

chx’s picture

FileSize
72.71 KB

UTF-8 should have been UTF8. corrected.

chx’s picture

FileSize
72.72 KB

Eaton found two more bugs, drupal_install conflicts with drupal.install , renamed to drupal_install_main. And profile.install contained a typo.

chx’s picture

FileSize
69.91 KB

Configure now removed per drumm's advice. Instead (my idea) we have a profile_final_link which points to the page where you can continue.

eaton’s picture

This is very, very, VERY close I think. For those who are interested in setting up a test site for this stuff, and have shell access, I've found the following script helpful. Keep in mind that it is DANGEROUS and will DESTROY ANYTHING that currently exists in the directory in which the script is run.

rm -rf *

cvs -z9 -d:pserver:anonymous:anonymous@cvs.drupal.org:/cvs/drupal checkout drupal
mv drupal/* drupal/.htaccess .
rmdir drupal

curl http://drupal.org/files/issues/installer1_12.patch > install.patch
patch -p0 < install.patch
rm install.patch

mysqladmin -u USERNAME --password=PASSWORD -f drop DATABASENAME
mysqladmin -u USERNAME --password=PASSWORD create DATABASENAME

mysql -u USERNAME --password=PASSWORD
GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, DROP, INDEX, ALTER, CREATE TEMPORARY TABLES, LOCK TABLES ON DATABASENAME.* TO 'USERNAME'@'localhost' IDENTIFIED BY 'PASSWORD';
exit

Run that puppy -- or paste it into your terminal window -- and you're ready to hit www.yoursite.com/install.php.

eaton’s picture

Also, the url passed to curl should be replaced by the url of the current version of the patch. If you don't have curl, wget works too.

eaton’s picture

FileSize
1 KB

And also, here's a sample profile that enables *every* module that Drupal comes with. It also redirects directly to user/register after the initial setup is complete, rather than hitting the main page. That's a demonstration of the hook_profile_final_link() function, which would allow install profiles to link to a custom 'configuration wizard' module once the initial installation is complete.

The formatting of the link on the 'installation complete' page is messed up (HTML gets escaped) but that's the only hitch I can find right now. Very, very cool.

nedjo’s picture

It's great to see each module getting its own _install file.

Besides the various modules, a profile may wish to do setup of its own, e.g., create a specific role or block. Can we include support for a profile _install function, e.g., default_profile_install()?

The profile_final_link makes sense as a way to include extra configuration as desired.

chx’s picture

nedoja, the profilename_install is there already.

eaton’s picture

nedjo,

There was some intense discussion on IRC about the various configuration, installation, etc hooks that the install system could provide.

My feeling is this:

1) The install system should handle basic installation of drupal, including DB configuration and the installation of necessary DB tables (via module .install files).

2) Configuration of the site itself should be handled by a dedicated (if tiny) module operating inside the drupal environment. Right now, it's handled by node.module special-casing when there are no promoted nodes.

3) A given install profile would include its custom setup module in the modules list, and provide that module's path as its final_link.

As soon as this patch lands, I have a small 'welcome to drupal' module that I want to put together that would replace the welcome screen currently provided by node.module. I think that approach provides a much better model for expansion and customization.

In the meantime, this patch is very close. Test! Test! Test! :-)

eaton’s picture

Er... the corollary to that is that the 'initial configuration' module would handle things like making any necessary custom blocks, etc. That would also (and I say this from experience) make testing custom config generators much easier.

Jeremy’s picture

FileSize
18.41 KB

A simple default installation worked, however the final screen doesn't show a link to my new site. Instead, it shows html tags that should create a link. See the attached screenshot.

drewish’s picture

i spent a little time trying to setup a new site with invalid database login info just to see how helpful the error messages were. my feeling is that they could be better. the form element error indicators don't help the user spot the obvious configuration errors. is there a reason that drupal_test_mysql() uses drupal_set_message() rather than form_set_error() to report it's errors back? while referencing form items in install.mysql.inc would increase the coupling between the two files, the names seem pretty standardized and it would make it possible for the error messages to target the problem sources more specifically.

the second thing is that we really need a confirmation screen between the point where the user enters the database info and the point where it's created. it was a bit unnerving to "accidentally" enter the correct info and see it start grinding away creating tables.

oh, one small thing, can we remove the trailing ?> from install.mysql.inc?

chx’s picture

FileSize
75.22 KB

install.mysqli.inc created.

merlinofchaos’s picture

Upon invoking install.php, it shows a LOT of errors that are unnecessary and will be extremely disorienting for newbies.

The only errors I think it should display are the ones related to permissions on the settings.php; and it shouldn't bother checking the database at all if there is nothing in $_POST.

There should be some text in the database not found that IF it's using a default settings.php (or better: no settings.php exists -- don't ship with one at all) to tell the user congratulations, their webserver is serving pages, now go hit install.php to set up the database.

merlinofchaos’s picture

I also agree with install profile --> distribution, since profile.module is established and there is no need to override the term.

nedjo’s picture

A given install profile would include its custom setup module in the modules list, and provide that module's path as its final_link

Thanks for the clarification Jeff, that approach makes sense. In any case we'll need user feedback for some setup tasks, so a profile_install in itself would be of limited use.

For some of the site setup tasks, e.g., adding a role or enabling a theme, we'll probably want to create new methods (and adapt core to use these instead of the current queries embedded in form submit functions). We can do this as a series of followup issues after this patch is applied.

Steven’s picture

FileSize
73 KB

New patch. Changes:

  • Removed install_verify_db() as this was already being done during the settings.php phase and moved inclusion of database.inc higher up.
  • Replaced hook_profile_final_link() with hook_profile_final() where a profile can inject arbitrary content into the final page. This is much more flexible and clean. Passing pieces of a sentence and a link around is just ugly and arbitrary.
  • Instead of telling people to review "possible warnings or errors", we should check if there were messages and only then tell people about it.
  • Tweaked maintenance.css so that long error messages do not burn the eyes (pastel red background instead of fluorescent red text).
  • Don't show errors when you first visit install.php.
  • Don't show the (useless) database configuration form if settings.php is not writable (this was plain confusing).
  • Cleaned up code: "$query" quotes, compacted database type detection, fixed placeholder usage, ...
  • Added code comments all around.

I also removed a lot of calls to drupal_set_message()... the way the code was written, every single permission / directory / file change would cause at least one message. For example, if settings.php was not writable, you'd get this torrent of messages:

  • Failed to automatically fix permissions of file ./sites/default/settings.php due to insufficient privileges.
  • File ./sites/default/settings.php is not writable.
  • The Drupal installer requires write permissions to ./sites/default/settings.php during the installation process.

This looks silly and is also confusing. Plus, generic "foo is (not) bar" messages tend to be quite unhelpful. So I adopted the principle that none of the verification functions output messages. This allows the caller to output the most helpful message. In the case above, only the last message remains.

There was also another issue (and it's surprising this wasn't noticed earlier) with doing drupal_set_message() calls during the profile install. After the profile installation is finished, an _install_goto() was performed. However, because there was no session yet (the session table was only just created), the messages were lost to /dev/null. I replaced the redirect with a direct call and then had to tweak things to make it all work.

Finally there is really only one thing that I dislike about the patch. The check whether Drupal is installed depends only on the presence of $installed_profile = '...' in settings.php. This means that simply wiping the database is not enough to reinstall. This is a plausable scenario, when someone e.g. keeps the settings.php from an existing install and starts over. The variable is inserted at the very bottom, so it is very hard to notice.

Perhaps a better way would be to have a marker in the database which can be queried easily. After the database connection is set up, the marker can be tested for and then Drupal can intelligently say whether or not installation has already been completed. This would only apply to install.php, so there is no performance concern. Testing for the presence of the system table should do the trick.

eaton’s picture

FileSize
1.16 KB

This is looking good. I've taken it throuhg a couple rounds of installing, with bad and good data both entered, and thrown my 'everything' profile at it. Here's a new version of that module that uses the new 'finalization' hook. I agree about the use of a database table to determine the installation state, though I think it could wait until the core patch is committed.

chx’s picture

Status: Needs review » Reviewed & tested by the community

With Steven's patch and Eaton's test I say, let's continue in several other issues.

chx’s picture

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

I rerolled because install.mysqli.inc was missing.

Steven would like to see logi creorganized so there is a DB - only checking for installed profile. I will work later on this.

chx’s picture

FileSize
76.12 KB

Just another reroll.

Steven’s picture

FileSize
80.05 KB

Included the missing mysqli.inc file /with/ all the other changes.

nedjo’s picture

FileSize
80.14 KB

Some minor code comment cleanup:

* Comments are full sentences with punctuation (periods) at the end (or else a colon).
* Function descriptions begin with a sentence in present tense.
* Two-space indent on line after @param or @return (many had only one).

I manually edited the patch file and didn't want to delete lines, so an additional cleanup that could still be done:

* No blank line between @param and @return sections of function documentation.

Also corrected the password field description (it had username instead of password).

eaton’s picture

FileSize
82.51 KB

nedjo, that patch no longer applies cleanly to HEAD. I've attached a simple version that is JUST like installer_3, save compatability with the new /modules/x/x.module directory structure. I'll look at the differences between your patch and chx's to see where it's choking and try to integrate your code formatting tweaks.

Dries’s picture

  • The install system should be minimal. As soon the database is installed, we should redirect things to index.php and do a full Drupal bootstrap. Configuration wizards and whatnot should then be run with the full Drupal functionality available. The default welcome message could then be replaced by these wizards.
  • Accessing install.php after you installed your Drupal site, gives you all kind of interesting warnings and status messages. Maybe we should print a 'Drupal is already installed' message? Chances are that many people will leave their install.php around, so we should keep that in mind.
  • The following double-defines look like overkill:
    +define('FILE_WRITABLE',       4);
    +define('FILE_WRITEABLE',      4);
    

    and

    +define('FILE_NOT_WRITABLE',   64);
    +define('FILE_NOT_WRITEABLE',  64);
    
  • I don't understand the following code comment:
    +  // The system module (Drupal core) is currently a special case
    +  include_once './database/updates.inc';
    

    What does it have to do with the system.module? Why are we including updates.inc?

  • Are we trying to sneak in the variable_get('module_list') crap? Please remove it unless it is strictly necessary!
    -    if ($bootstrap) {
    -      $result = db_query("SELECT name, filename, throttle, bootstrap FROM {system} WHERE type = 'module' AND status = 1 AND bootstrap = 1 ORDER BY weight ASC, filename ASC");
    +    if ($fixed_list = variable_get('module_list'. ($bootstrap ? '_bootstrap' : ''), NULL)) {
    +      foreach ($fixed_list as $name => $module) {
    +        $throttle = isset($module['throttle']) && $module['throttle'] && (variable_get('throttle_level', 0) > 0);
    +        if (!$throttle) {
    +          drupal_get_filename('module', $name, $module['filename']);
    +          $list[$name] = $name;
    +        }
    +      }
  • The changes to theme_status_messages are hackish imo. It illustrates that something is wrong API-wise.

I gotta run now but I'll try to spend some more time with it later on. Good job. Glad to see this moving forward! :)

chx’s picture

I am not sneaking in anything but I need to make module_list database independent because form API relies on the module subsystem which eventually falls back to module_list. This seemed the best way.

Steven’s picture

FileSize
163.7 KB

The install system should be minimal. As soon the database is installed, we should redirect things to index.php and do a full Drupal bootstrap. Configuration wizards and whatnot should then be run with the full Drupal functionality available. The default welcome message could then be replaced by these wizards.

This is exactly what the last patch does. Unless I'm missing something? The hook_profile_final() hook is not intended to output complicated forms and such (they wouldn't work anyhow), but simply to provide an appropriate final page if the default does not suffice.

I don't understand the following code comment:

+  // The system module (Drupal core) is currently a special case
+  include_once './database/updates.inc';

What does it have to do with the system.module? Why are we including updates.inc?

Install.inc is used both by install.php and update.php. Updates.inc was a surrogate for system.install for historical reasons. Your comment made me realize two things:

  1. A lot of the things that were in install.inc did not belong there. Install.php was nearly empty, while update.php contained a lot of UI logic. So, I made the installer consistent with the update system by moving all the UI logic to install.php and keeping install.inc as a pure API.
  2. database/updates.inc is now useless, because nothing prevents us from doing core updates in the right .install file. So, I moved everything from updates.inc to system.install. I also tweaked updates.inc to ensure system.install's updates are run first. This is necessary to make sure that any updates that were written before this moment will still be run before any updates we add to module.install files in the future.

Other changes:

  • The hack in theme_status_messages() was replaced with a cleaner mechanism in drupal_get_messages().
  • Simplified the $conf['module_list'] hack by using an override argument $fixed_list to module_list().
  • Instead of checking for the hardcoded variable $installed_profile in settings.php, we check if Drupal is installed by trying to connect to the database and checking if the system table is present. This has benefit that settings.php should not be writable during the installation if it already contains a valid $db_url. To do a re-install, you only need to wipe the database.
  • I added a separate "Drupal already installed" screen (image
  • (by eaton) Put a link to install.php on the database.inc error screens to guide admins.
  • When opening up install.php for the first time, don't fill in useless "username" "password" "databasename" values in the settings.php form.
  • When connecting/selecting the database fails, don't tell the user to 'fix database permission errors' (wrong message).

Changing the $installed_profile mechanism required a lot of code juggling so please test thoroughly (though it worked fine on my end).

(Note that the database directory is now entirely obsolete)

Steven’s picture

FileSize
163.74 KB

Missing urlencode() when writing out the $db_url (fixes exotic passwords/usernames).

eaton’s picture

Just ran through a series of test installs.

1) The install still works without a hitch.

2) My everything.profile works, too. Looking good.

3) Removing the useless defaults from the form fields is a good thing.

Anyone else? Dries? chx? Bueller? :-)

chx’s picture

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

install_select_profile now uses drupal_get_form and removed the return NULL at the end of the same, because no return statement is the same. No major changes.

I tested as well and it just works. Daring an RTBC again.

chx’s picture

FileSize
158.78 KB

Minor text changes from Angie. Notable is the fact that she did not found any more to change.

Dries’s picture

FileSize
28.32 KB
  • Whenever we write 'your Drupal installation' or 'Drupal', Drupal is written in italic. Why is that? It looks weird.
  • On my system, there is only one supported database,yet there is a drop-down menu in the form. Can't we hide the form when the user has no choice?
  • When Drupal is installed, and I access install.php once more, it prints "To install to a different database, edit the appropriate settings.php file in the sites folder.". What is the 'appropriate setting.php' and what should I edit? This message isn't particularly helpful, IMO. Do you mean I should manually edit the $db_url variable?
    • I made a couple small textual changes. Uploaded a new patch. It's a partial patch though; it doesn't include the newly created files.

      I'll try to have a better look at the code later today.

eaton’s picture

Dries, it appears that patch left out the install.php file... :-)

eaton’s picture

http://drupal.org/cvs?commit=36226 -- and there was much rejoicing. :-)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Yay! I committed the patch. There is room for improvements and extensions, but I figured there were no showstopping issues. Let's keep working in this!

rstamm’s picture

Status: Fixed » Needs review
FileSize
659 bytes

Protect .profile files from prying eyes.

The patch solves the issue.

rstamm’s picture

FileSize
6.26 KB

Missing CVS Id in some files.

The patch solves the issue.

merlinofchaos’s picture

Flanker, shouldn't those be separate issues? They don't seem to relate to the installer at all.

kuprishuz’s picture

Any way of having one or more of the following, though trivial any thing to improve usability helps:

  1. install.php already does a check to see if there is an existing installation, can we ask the user if they would like to upgrade, in which they are directed to update.php
  2. Possibly store the current version in the db, and the in a file, and automatically redirect to update.php if the two do not match
  3. Allow the user to provide the admin credentials, in which it will allow them to drop all tables, and do a fresh install, in case they decided to take the route of a different install profile, or maybe it was just for testing purposes, this eliminates trips to database admin tools such as phpMyAdmin
webchick’s picture

Please file follow-up bug fixes and feature requests to new issues. This one's already long enough as it is.

rstamm’s picture

Status: Needs review » Fixed

#69 and #70 are separate issues now.

#69 http://drupal.org/node/73590

#70 http://drupal.org/node/73591

webchick’s picture

Thanks, Flanker! :)

Gábor Hojtsy’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
5.64 KB

The MySQLi install files lack the connection parameter at all places where it is required. The attached patch fixes this. Without applying this patch, warnings are printed all over the screen, and it is not possible to install with mysqli. Please apply ASAP.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

dnorman’s picture

Title: Installer for Drupal Core » create database?

To make the installer friendly for non-geeks, it should attempt to create the specified database as well. Forcing noobs to go through phpMyAdmin (or worse) to create the database before running the installer will be a barrier for many. If database creation fails, a prompt to the user telling them what is needed would be good (which is what happens now).

Any ideas? Was database creation left off for a good reason? Security?

The installer works perfectly though! Nice work, everyone! Now, to play around with creating profiles...

webchick’s picture

Title: create database? » Installer for Drupal Core

please add feature requests as separate issues, thanks

dmitrig01’s picture

Related: http://drupal.org/node/73993(improvment)

Anonymous’s picture

Status: Fixed » Closed (fixed)
Gábor Hojtsy’s picture

Version: » x.y.z
Category: task » bug
Status: Closed (fixed) » Reviewed & tested by the community

#76 was not committed, although Dries indicated it is committed. The patch still applies since there was no change in that file since then.

Steven’s picture

Status: Reviewed & tested by the community » Fixed

Committed for real now.

Anonymous’s picture

Status: Fixed » Closed (fixed)