Clean up installation of required modules [#728820]

Currently, the db_create_table() function looks like this:

function db_create_table($name, $table) {
  if (!db_table_exists($name)) {
    return Database::getConnection()->schema()->createTable($name, $table);
  }
}

This is a problem because we are skipping the operation whenever a database table with the same name already exists, but we have no idea that it's actually the same table (i.e., with the same schema that was requested) - so this could lead to silent failures in certain situations. Plus, due to #582948: Improve safety of schema manipulation we are now supposed to be throwing a DatabaseSchemaObjectExistsException in these situations, and in fact the underlying createTable() function does this; but it will never bubble up since that function never even gets called.

Fixing this is a bit tricky since just removing that if() statement breaks the installer - in fact, that appears to be why it was added in the first place (see http://drupal.org/node/306151#comment-1777998). However, I think I have a patch that does this, with the happy side effect that it cleans up the installation of required core modules a bit (by getting node/user/etc specific code out of the system module) so that we do not have to hardcode creating their tables within the system module but can rather use the normal module install process to do it.

Note that this is technically a (small) API change to db_create_table(); however, as far as I know it is basically just restoring the Drupal 6 behavior of that function.

Comment	File	Size	Author
#4	install-required-modules-cleanup-728820-4.patch	7.42 KB	David_Rothstein
#4
#1	fix-db-create-table-728820-1.patch	8.13 KB	David_Rothstein
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

David_Rothstein CreditAttribution: David_Rothstein commented 1 March 2010 at 03:31

Status:

Active

» Needs review

File	Size
fix-db-create-table-728820-1.patch	8.13 KB

Here is the patch. Everything seems to work fine for me with it, but let's see what the testbot thinks.

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 1 March 2010 at 03:58

I love this patch. Contains even more clean-ups than in my patch for #375397: Make Node module optional

Comment #3

catch

he/him

English

CreditAttribution: catch commented 1 March 2010 at 04:05

Status:

Needs review

» Needs work

This is almost the same patch i posted at #686196: Meta issue: Install failures on various hosting environments.

Two complaints:

+  $module = 'system';
+  drupal_install_schema($module);
+  $versions = drupal_get_schema_versions($module);
+  $version = $versions ? max($versions) : SCHEMA_INSTALLED;
+  drupal_set_installed_schema_version($module, $version);

No reason to put system in a variable here.

Please remove this hunk:

 
Index: includes/database/database.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/database/database.inc,v
retrieving revision 1.97
diff -u -p -r1.97 database.inc
--- includes/database/database.inc	15 Feb 2010 22:12:27 -0000	1.97
+++ includes/database/database.inc	1 Mar 2010 03:07:57 -0000
@@ -2402,9 +2402,7 @@ function db_next_id($existing_id = 0) {
  *   A Schema API table definition array.
  */
 function db_create_table($name, $table) {
-  if (!db_table_exists($name)) {
-    return Database::getConnection()->schema()->createTable($name, $table);
-  }
+  return Database::getConnection()->schema()->createTable($name, $table);
 }

Has nothing to do with this, should be dealt with at #582948: Improve safety of schema manipulation.

Otherwise RTBC.

Comment #4

David_Rothstein CreditAttribution: David_Rothstein commented 1 March 2010 at 04:39

Status:

Needs work

» Needs review

File	Size
install-required-modules-cleanup-728820-4.patch	7.42 KB

Sorry, @catch, you're right - I hadn't been following that issue too closely but looking back on it now it does seem like it's similar to the patch you started working on there. (Core committers: Please give @catch credit on any commit here!)

Anyway, I removed the $module = 'system' stuff and removed the db_create_table() part as well, although please realize that fixing db_create_table() was actually the initial motivation that got me working on this - the rest was just required to make it work :) However, we can certainly save it for a followup patch instead. Especially since the above patch looks like it got a couple serious testbot failures, and the one I tried locally with this new patch worked fine - so maybe it was related somehow. Let's see.

Comment #5

catch

he/him

English

CreditAttribution: catch commented 1 March 2010 at 05:04

Title:

Fix db_create_table() and clean up installation of required modules

» Clean up installation of required modules

Oh I missed you'd posted this today, i thought mine was the duplicate patch :p. No one tested mine on that issue that I could see, so makes sense to split it out anyway, makes sense in itself.

This is RTBC pending the test bot.