Install system: schema versions not being set [#76209]

The installer does some strange voodoo to install modules before the full drupal bootstrap is possible. Some of that voodoo resulted in the SCHEMA_VERSION never being set for installed modules. This meant that update.php NEVER worked, and also hinted at other problems.

Fixing this the right way is not trivial. This patch fixes it. How?

drupal_get_filename() now has an optional final parameter: $use_database. It defaults to true, the current behavior. If FALSE is passed in, it will skip the DB lookup for a given file and walk the usual list of modules/, etc looking for the file in question. This is useful in the special case where we're verifying the existence of the critical system.module and system.install file before bootstrapping.
install.inc now defines two general-use functions: drupal_install_module() and drupal_uninstall_module(). These are now used by both the install system AND admin/module. One function, one central location.
Populating the system table with entries for each module, and updating it when modules moved, was previously done when loading the admin/modules page. Now, it's handled by module.inc in the modules_rebuild_cache() function. This allows both the install system and system.module to keep things tidy without duplicating any code.
Two more functions have been added to module.inc: module_enable() and module_disable(). admin/modules now uses those functions rather than updating the tables directly.

There's a lot of functionality that's consolidated and smoothed over. Really, we should do the same thing with blocks and themes as well -- centralize the rehashing functions and so on -- but in the case of modules and the install system, it's a showstopping bug rather than an aesthetic preference.

Comment	File	Size	Author
#20	install.api_1.patch	17.23 KB	eaton
#20
#19	install.api_0.patch	17.54 KB	Steven
#19
#18	install.api.patch	17.52 KB	Steven
#18
#15	install_system_7.patch	14.91 KB	eaton
#15
#11	install_system_6.patch	15.29 KB	eaton
#11
#10	install_system_5.patch	15.42 KB	eaton
#10
#9	install_system_4.patch	15.83 KB	eaton
#9
#7	install_system_3.patch	16.33 KB	eaton
#7
#6	install_system_2.patch	15.77 KB	eaton
#6
#4	install_system_1.patch	15.77 KB	eaton
#4
#2	install_system_0.patch	14.42 KB	eaton
#2
	install_system.patch	14.42 KB	eaton

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

eaton CreditAttribution: eaton commented 29 July 2006 at 22:19

For those willing to help test this patch, here's an important check:

Using the unpatched install system, set up a drupal site. If you have phpmyadmin or some other such tool, check the contents of the system table. Note that the schema version for EVERY module is 0.

With the patch, it should be 187, or something thereabouts. Other modules installed and enabled should have a schema version of 0. And modules not automatically installed by the profile should have a schema version of -1.

What to hammer on:

Enable, disable, re-enable various modules. Ones with and without .install files. Make sure the tables get created when they're installed, and no attempts are made to re-create them the second time around. See if moving a module from modules/foo to sites/default/modules/foo breaks anything. Try a few third party modules.

All these cases have been tested, but enough critical bits were touched that it demands everyone taking a poke at it.

Comment #2

eaton CreditAttribution: eaton commented 29 July 2006 at 22:26

File	Size
install_system_0.patch	14.42 KB

...Aaaaand, naturally I posted the version with a typo.

Use this version, not the previous one. (on line 90, $module should be 'system'...)

Comment #3

eaton CreditAttribution: eaton commented 29 July 2006 at 22:31

...And then yet one more fix. The 'bootstrap' flag was always being set to false whenever the module cache was rebuilt.

This raises another important point -- we need a way to centralize the list of modules that should have their bootstrap flag set to true. That used to be handled by the database files, but seems to have been left out of the install system.

Comment #4

eaton CreditAttribution: eaton commented 29 July 2006 at 23:03

File	Size
install_system_1.patch	15.77 KB

False alarm -- bootstrap marking of modules works with this patch, I'd just left out one chunk of system_modules() when moving it over to module_rebuild_cache().

Comment #5

asimmonds CreditAttribution: asimmonds commented 30 July 2006 at 02:18

In drupal_install_module():

I think

$versions = drupal_get_schema_versions($module);

should be moved after the include_once / module_invoke, as drupal_get_schema_versions scans the current function list and the new module's .install needs to be loaded.

Related to this issue, there is a bit of install code in user.module that would be unnecessary with this patch.

Index: modules/user/user.module
===================================================================
RCS file: /cvs/drupal/drupal/modules/user/user.module,v
retrieving revision 1.642
diff -u -2 -Ffunction -r1.642 user.module
--- modules/user/user.module    29 Jul 2006 17:56:41 -0000      1.642
+++ modules/user/user.module    30 Jul 2006 02:06:59 -0000
@@ -1199,8 +1199,4 @@ function user_register_submit($form_id,
     user_authenticate($account->name, trim($pass));

-    // Set the installed schema version of the system module to the most recent version.
-    include_once './includes/install.inc';
-    drupal_set_installed_schema_version('system', max(drupal_get_schema_versions('system')));
-
     return 'user/1/edit';
   }

Comment #6

eaton CreditAttribution: eaton commented 30 July 2006 at 15:23

File	Size
install_system_2.patch	15.77 KB

You're right; that line is unecessary in *almost every case*, since including install.inc in most scenerios pre-includes all .install files, but it's possible during the initial install process to load without them. Attached is a version with those lines swapped around.

I'm scanning through the rest of core to find other locations where we manually update schema versions, as that would be unecessary after this patch.

Comment #7

eaton CreditAttribution: eaton commented 30 July 2006 at 16:17

File	Size
install_system_3.patch	16.33 KB

Errr, ignore that patch. It included the DB url from my settings.php file. Obviously, that won't work on anyone else's setup. :-)

The attached patch removes that chunk and includes modifications to user.module to remove the forced schema change.

All modules -- system, core, contrib -- will have their schemas set correctly.

Comment #8

eaton CreditAttribution: eaton commented 30 July 2006 at 20:43

A brief discussion with Dries on #drupal led to some questions about the necessity of the patch, and whether it makes things more complicated. It's true that this ONE problem (schema versions not being set properly) could be sidestepped with less than a dozen lines of code.

However, a lot of mucking with the installer and system.module, I believe the extensive duplication of code between several Drupal subsystems would lead to continued problems and successive 'fix-up' patches. I'll try to explain the rationale for each of the changes:

Currently, the installer uses a set of custom functions and directory-scanning routines to validate the existence modules and install files before it proceeds. This leads to unecessary duplication of code between the normal drupal_get_filename() function, and (at present) results in the installer not properly recognizing modules in sites/all/modules.
Adding the $use_database parameter to drupal_get_filename() lets us use it before the DB bootstrap, reducing duplicated code and ensuring that we find modules and install files consistently. It also allows us to eliminate the unecessary drupal_find_modules() function, which was only uesful during initial install.
Code to manage the installation of modules is currently duplicated between the install system and system.module's admin/module page. That means inconsistency and sneaky, difficult-to-track-down problems. Making install.inc's drupal_install_module() usable in all situations reduces duplication and ensures consistency.
Right now, populating the system table with the list of currently *present* modules, regardless of their install state, is handled by custom code in system.module's admin/module page as well. Moving that code to module_rebuild_cache() in module.inc centralizes it, and also eliminates the need for duplicate code in the initial install system.
Several helper functions have been added during the creation of the patch, in particular module_enable() and module_disable() in the module.inc file, and drupal_uninstall_module() in install.inc. These two serve to centralize code that was previously scattered. And while the drupal_uninstall_module() function is not explicitly necessary right now, it rounds out the full course of API functions for module management. It's a good thing.

While this patch *file* is relatively large, it's worth noting that the net increase in code is only a few dozen lines, all contained in the two helper functions for module.inc and some sanity-checking for install.inc. Most of the work is pure reshuffling to centralize functionality.

The installer is a big step forward, but as was noted by drumm, it really needed a more robust set of underlying APIs for module management as a foundation. We coded around that when making the installer, and problems have crept in. Rather than patch around those problems, we should put in the underlying management functions and ues them throughout the entire system.

That's my case. ;)

Comment #9

eaton CreditAttribution: eaton commented 30 July 2006 at 20:47

File	Size
install_system_4.patch	15.83 KB

...And finally, one small revision. A helper function from http://drupal.org/node/76104 crept into the diff, making this patch larger than it needed to be.

That's no longer there.

Comment #10

eaton CreditAttribution: eaton commented 30 July 2006 at 20:51

File	Size
install_system_5.patch	15.42 KB

After reviewing comments from #drupal, changed $use_database to the more intuitive $check_db. Obviously, a minor change but one that helps keep things more straightforward.

Comment #11

eaton CreditAttribution: eaton commented 31 July 2006 at 03:31

File	Size
install_system_6.patch	15.29 KB

As per drumm's review on #drupal, the following changes have been made:

Style (true/false/null now capitalized)
Trimming: drupal_uninstall_module() is no longer included. Nothing uses it (yet) so it's an enhancement, not a fix. Save it for another patch.

Comment #12

eaton CreditAttribution: eaton commented 1 August 2006 at 23:47

Title:

Install system: schema versions not being set!

» Install system: schema versions not being set

The recent patch to install.inc (http://drupal.org/cvs?commit=37312) broke installation of modules, as system.module no longer auto-loads the .install files. By centralizing the installation functions, this patch fixes that, too. ;-)

Comment #13

beginner CreditAttribution: beginner commented 2 August 2006 at 15:39

I wanted to test, but I couldn't apply the patch because all lines end with ^M.
Result: all hunks fail:

patching file includes/bootstrap.inc
Hunk #1 FAILED at 192.
Hunk #2 FAILED at 212.
etc.

Is it possible to get a standard linux/unix patch file? Thanks.

Comment #14

beginner CreditAttribution: beginner commented 2 August 2006 at 15:44

Well, maybe it IS a standard patch file.
Maybe the problem is at my end. It's (only) the third time in many months, and scores of patches that a patch completely fail to apply.
??

Comment #15

eaton CreditAttribution: eaton commented 2 August 2006 at 15:58

File	Size
install_system_7.patch	14.91 KB

I've attached a re-rolled version of it. The only difference that I can see is that it no longer applies with offsets..?

Comment #16

beginner CreditAttribution: beginner commented 2 August 2006 at 16:49

This patch applied flawlessly. I confirm that patch 6 had ^M at the end of the lines. I made a diff between the two patches, and they were 100% different! Did you by any chance create the patches using different methods?

A quick review before going to bed.
module.install now works. :)
schema_version is updated as expected.

What this patch doesn't solve, is a mysterious (to me) but critical issue also related to install:
http://drupal.org/node/74992 .

I may do some more testing tomorrow, but given that module install in cvs is completely broken, this patch can only improve the situation, so if you want to commit it while I'm sleeping, I'd say +1 :)

Comment #17

eaton CreditAttribution: eaton commented 2 August 2006 at 17:13

Beginner, no, but on this patch I did convert DOS->UNIX line endings before uploading it. Strange, both patches applied fine for me using 'patch' from the unix command line.

I think the problem in http://drupal.org/node/74992 is a different one than this patch attempts to solve. This is more about consolidating the install/enable functions to avoid inconsistencies, while that issue looks to be something very specific to the DB install process.

Anyone willing to RTBC this?

Comment #18

Steven CreditAttribution: Steven commented 2 August 2006 at 23:52

File	Size
install.api.patch	17.52 KB

This patch is really good. After some IRC discussion, some minor changes:

We decided to move all installation hooks, including hook_enable() / hook_disable() in the module's .install file. These hooks are not needed for normal operation, after all. The install.inc API is available during all hooks executed from a .install file.
The profile verification (locating all the modules) was split from the actual installation.

Tested installation of Drupal and modules locally. Appears to work fine. Will commit after someone confirms.