D6 Upgrade path broken when CCK modules are disabled

Please contact me through my contact form - If that's possible, I'd like to take a look at a db dump...

OK, I can reproduce that behavior with the db dump you sent me - including the odd stuff in #3.
Now, I need to find what the hell is happening...

OK, got it - your site was configured so that error messages were not displayed, so this was 'hidden'

Actually comment #3 was the key. Problem is that the 'format' columns of your text fields are set to NOT NULL.
Thus leaving the 'notes' field blank causes a 'Column 'field_notes_format' cannot be null' error and the whole insert fails for the fields that are stored in the same table (that is, all fields except the one that is multiple)

Strange, because text_update_6001() is supposed to take care of this. Your dump shows that you ran it, though.
I'll look into this.

In the case of your specific site, yes, manually changing the 2 *_format columns in your content_type_recipe table to allow NULL values (and default to NULL) should solve the issue without further messing with the updates.

The only case where I could reproduce "D5->D6 upgrade does not update _format columns to allow NULL" is when the CCK updates are run while CCK modules are still disabled.
amitbapat : can you confrim that you uploaded the CCK D6 files and ran the updates *before* actually enabling the modules ?

Karen : problem in such case ("user runs CCK updates while CCK modules are still disabled") is that content_types_install(), which relies on module_list() / module_invoke() returns an empty array, so updates such as text_update_6001() return without actually doing anything.
Sigh. Update.php updating disabled modules is really a massive pain when it comes to CCK. I guess I should have helped weighing against it, dunno where I was back then...

OK, well. We already added some logic to abort field module updates until content_update_6000 has run. It seems we also need to abort each module's updates unless that module *and* content.module are both enabled. To be future-proof and secure future updates as well, I think we need to do that check in *every* update function, not just the first ones in the D5->D6 upgrade path. The enforced behaviour would then be :
- no updates for cck fields run until content.module still has pending updates (using drupal_get_schema_versions() and drupal_get_installed_schema_version())
- no updates for content.module and field modules run unless the modules are enabled.
We can probably provide a helper function in content.install to reduce the code duplication in each update function of each field module...

Then we'll also need to find a way to fix existing CCK RC installs, but first I'd like amitbapat to confirm my hypothesis above, to be sure we're chasing the right rabbit.

Thks for reporting back.
"As suggested in the upgrade guide" : Can you spot a specific page ? The doc I can find only deals with core upgrade, and stays really vague on contrib. I cannot find a place in the docs where it is written you should keep contrib modules disabled before running their updates. I'm a little dubious about this, since updates weren't executed for disabled modules until D6.

Anyway. Whether it's the recommended way or not, people *are* going to hit this, so we need to fix it...

Attached patch does what I proposed in #7. If no-one objects, I'll commit this shortly and update http://drupal.org/node/191796.

Now, we also need to fix CCK installs upgraded from D5 to an RC, that are potentially broken if the updates where run with the modules still disabled.
If I'm not mistaken, problematic functions are the ones that call content_types_install() or content_associate_fields().
That is : optionwidgets_update_6000(), text_update_6000(), text_update_6001().

Simply re-running text_update_6001 should not have evil effects on the installs where it has already run, so this should be as simple as :

function text_update_6002() {
  return text_update_6001();
}

I didn't look closely into the other 2 (the ones that call content_associate_fields() - not sure what would be the effect of having this function ran twice)

This looks like a reasonable approach to me, and the helper function is nice -- makes it easier for other modules to do the same thing.

I think it is safe to run content_associate_fields() more than once, I've done it and seen no problems. It will set active modules to active and sets the module names in the database. If that data is already there, it will just get updated again, which shouldn't hurt anything.

None of this will fix disabled contrib modules that are in the modules folder that don't do this testing -- they will still break things and there is not really much we can do about that except try to get the word out.

I committed the patch in #10

And I added updates to re-run optionwidgets_update_6000(), text_update_6000(), text_update_6001().