Schema not available in hook_install()

You mention a workaround in hook_enable, but if I need to do a drupal_write_record in hook_install (not possible by now as schema is not available), is it wrong if I use a db_query in drupal 6 or a db_insert in drupal 7?

I was tracking this one down for some time and tried several different ways to manually reset the schema cache and enabled modules list. I even tried implementing hook_modules_installed() on the module as a way to get around a workaround through hook_enabled(). In the end, I just used a db_insert(), but I'm a HUGE +1 to have this fixed. It seems to me that drupal_install_schema() should mean just that... the schema has been installed and is available for any drupal_write_record() after that.

My hard drive is litterred with half attempts at this same patch. I commend you on your skill and tenacity. This wasn't easy.

If you have enabled several modules at once with drush, you very well might have felt the pain of this bug. We need to fix it.

I reviewed the code and it looks good. The test is icing on the cake.

+++ includes/module.inc	15 Feb 2010 22:38:23 -0000
@@ -336,63 +336,73 @@ function module_enable($module_list, $en
     if ($existing->status == 0) {
+      // Load the module's code.
+      drupal_load('module', $module);
       module_load_install($module);

Question about this.

Let's say my module has in its hook_requirements() in mymodule.install that in order to use it, you need the external foodybar PHP library.

At the top of my .module file is the code require_once('/FoodyBar.baz.php');

Won't this code lead to a fatal error?

Powered by Dreditor.

back to rtbc

#7: schema-hook-install-620298-7.patch queued for re-testing.

all green

Great job, David. The changes to install.inc and module.inc make sense and seem to stream-line the installation process some more. It is quite grokkable actually. While I think we're really close, I still have some feedback:

+++ includes/module.inc	25 Feb 2010 05:05:27 -0000
@@ -336,63 +336,73 @@ function module_enable($module_list, $en
+      // Check if node_access table needs rebuilding.
+      // We check for the existence of node_access_needs_rebuild() since
+      // at install time, module_enable() could be called while node.module
+      // is not enabled yet.
+      if (function_exists('node_access_needs_rebuild') && !node_access_needs_rebuild() && module_hook($module, 'node_grants')) {
+        node_access_needs_rebuild(TRUE);
+      }

I wonder if this could (and should) be moved to node module using the new _modules_installed() or _modules_enabled() hook? It would further clean-up and streamline the installer code. If not, maybe we can clarify the code comment to explain why we can't? The current code comment makes sense but at the same time, it doesn't. :P

+++ modules/shortcut/shortcut.install	25 Feb 2010 05:05:28 -0000
@@ -7,14 +7,9 @@
 /**
- * Implements hook_enable().
+ * Implements hook_install().
  */
-function shortcut_enable() {
-  if (shortcut_set_load(SHORTCUT_DEFAULT_SET_NAME)) {
-    // Quit out; this module has already been installed before.
-    return;
-  }
-
+function shortcut_install() {

I like this clean-up! Nice catch.

+++ modules/simpletest/tests/module.test	25 Feb 2010 05:05:28 -0000
@@ -126,13 +126,44 @@ class ModuleUnitTest extends DrupalWebTe
+   * Test that calls to drupal_write_record() work during module installation.

It would be good to explain why this test is useful. The help text lacks some context to help people understand the bigger picture. This seems to suggest that 'drupal_write_record()' is special but reading the patch it is not clear why.

Anyway, I think we should be able to commit this really soon.

Looks good now. Committed to CVS HEAD. Thanks.