{system} records of installed modules are removed

Interesting. So this might actually explain #896672: error in activating locale module on a D6->D7 conversion site.

So, at the minimum we need something like this.

Damien, your patch won't explain #1

Hmmm,

This is REALLY strange and must go back a ways. Looking at the system table causing problems in #896672: error in activating locale module on a D6->D7 conversion site the db is a d5->d6->d7 upgrade.

The system table in the d5 db has the schema_version for locales as -1, the d5->d6 updated db carries the schema_version of -1 forward (never enabled in d6, just in d5 and disabled in d5), so this explains why I'm getting the exception on d6->d7

In this case the patch would not work unless the {locales_source} table was deleted as well. That might be a little tricker through because it might have revelant data the user wishes to use, in this case there isn't any cause I elected to not translate to other languages early in the d5 sites creation and disabled it but never removed it as it was a core module.

At one time during D5, when a module was disabled was schema_version set to -1 to tell the system it was disabled? If that is the case then as d5 sites update this will crop up again. [edit] Especially if they did like I did where d5 is a production site, run that db through single latest d6 update then d6 to d7 [end edit]

Interesting. It does seem like a large oversight to delete {system} entries and lose track of possibly installed schemas.

One thing that I'm not so clear on, is whether we are supposed to delete our D6 modules before update.php or just disable them. I was under the impression that it was the latter, from reading UPGRADE.txt:

 	5. Disable all custom and contributed modules. This includes any modules that
 	are not listed under 'Core - required' or 'Core - optional' on
 	http://www.example.com/?q=admin/build/modules (replace www.example.com with
 	your installation's domain name and path).
 	
 	6. Remove all old files and directories from the Drupal installation directory.
 	
 	7. Unpack the new files and directories into the Drupal installation directory.
 	
 	8. Copy your backed up "files" and "sites" directories to the Drupal
 	installation directory. If other system files such as .htaccess or
 	robots.txt were customized, re-create the modifications in the new
 	versions of the files using the backups taken in step #1.
 	
 	9. Verify the new configuration file to make sure it has correct information.
 	
 	10. Run update.php

8. seems to indicate that "sites" will contain all our old D6 modules by the time we run update.php (Does that need revising?) After #895140: _system_rebuild_module_data() and _system_rebuild_theme_data() will choose 6.x versions of modules and themes over 7.x versions was fixed it doesn't seem necessarily to remove them completely, and for what it's worth, it avoids {system} entries being deleted, but the bug should definitely be fixed either way.

@chx, can you explain #3?

I got this #fail with scenario b) (which I thought was the "only" way to do it).

I think this is happening for every upgrade.

Well the fix is correct. We might need more / something else to fix documentation and #896672: error in activating locale module on a D6->D7 conversion site (if that's not a corrupted db in the first place) but the fix here is correct.

+++ modules/system/system.module
@@ -2184,9 +2184,11 @@ function system_update_files_database(&$files, $type) {
-    // Delete all missing files from the system table
+    // Delete all missing files from the system table, but only if the plugin
+    // has never been installed.

"plugin" is not a Drupal term. Let's use "module" instead, since schema_version only applies to modules.

Agreed that the code fix is correct, though.

Powered by Dreditor.

Committed to CVS HEAD. Thanks.

Two things:

1. We should make that minor string correction carlos mentions in #9.
2. We should add a test condition for this in our upgrade tests.

Additionally,

4. We should use SCHEMA_UNINSTALLED and not get in the habit of hard-coding constant values. That might mean loading install.inc though.

Still a valid issue for the tests. Leaving as bug report, because the suggestion in #9 is still not fixed.

1. We should make that minor string correction carlos mentions in #9.
2. We should add a test condition for this in our upgrade tests.
3. Since we no longer remove {system} records for installed modules, all modules have to be able to be uninstalled. Otherwise, they will exist forever.
4. We should use SCHEMA_UNINSTALLED and not get in the habit of hard-coding constant values. That might mean loading install.inc though.

I think it might be overkill for #4 to load install.inc just for that constant.

As of today, disabling D6 contrib before upgrade does not appear to remove their entries from {system}.
The upgraded D7 database end-up with (possibly) a lot of outdated entry which belong to D6 only.

There is no {system} in D8.