After getting CCK to update, I tried to view some of the updated content. I got this error when trying to edit the content:

Fatal error: Call to undefined function _widget() in D:\net\BlackTea_6.0\sites\all\modules\contrib\cck\content.module on line 344

Looking at the code, I changed line content.module:341 to read:

  $function = $field['widget']['module'] .'_widget';

This removed the error, displaying the edit page. However, none of the fields have the data in them (related error, or a new one?). Reviewing the database, the data appears to be there, it's just not making it into the form.

Any ideas?

Comments

fractile81’s picture

fractile81 commented
Title: Broken After Update » Where to populate 'module' in content_node_field?
Priority: Critical » Normal

I sifted through the code and it looks like it was a problem with my incremental update of the CCK and related modules.

I was enabling the CCK module, running update.php, enabling all other CCK module, running update.php again. This method DOES NOT WORK because update 6000 relies on the field_info hook for populating the 'module' column in content_node_field.

To get around this, enable all relevant CCK fields and then run update.php.

So, could enabled CCK-related modules be setup to claim their respective fields when they're enabled? I'm just trying to think through this because I'm certain there are others out there that would update things incrementally rather than in bulk and break their CCK install. Some kind of mechanism to let modules re-claim their fields independent of an update might also work.

karens’s picture

karens commented

Yes, this is one of the many things that can bite you when trying to write CCK updates. I see two ways to fix this. One is to have a function in the content module that will do the update and require every module to invoke it. My fear there is that not all modules will get that code in.

Another possibility is to alter this a bit so we check if all the fields in the table exist in content_fields(). If not we fix the ones we can and report a failure rather than success, with a message that all the field modules need to be installed before this update can run to encourage the admin to get them all installed. Then it keeps reporting failure until all the modules are there to update the table. The fear with this approach is that if there are one or two field modules that don't get ported to D6 so they can be installed, they will keep the table from being updated.

The advantage of the second approach over the first is that you get warned right away that you need to get the field modules installed.

Getting this table updated with module names is critically important to the way CCK works in D6, so we must find a route that will get that table right as quickly as possible.

Not sure at this point which is the better way to go. yched, what's your vote?

karens’s picture

karens commented

Another idea, we can do an update in the content module for whatever fields are already installed, then use hook_notify to update other field modules as they are added. This is probably the best approach of all.

fractile81’s picture

fractile81 commented

FWIW, I have a CCK install with Content Taxonomy that I've upgraded to D6. Content Taxonomy doesn't have a D6 version yet, and thus won't work. However,

  1. CCK fails gracefully - It's nice that CCK just ignores these fields that have no "module" setting in the fields table. Because I'm so close to the code on my site, I understand what's going on and am not too worried. For people that don't have a similar level of familiarity, I can see an error message stating a specific field (or so) will not function properly until the X module is installed.
  2. The old data is untouched - As a result of part 1, I can casually wait for Content Taxonomy to be updated, while that data just sits there and waits. Now, I haven't tested re-saving a Content Type with a missing module, but I'm afraid that would wipe data.

Your first suggestion would be nice, but I agree that people might just miss a callable function at update time entirely. The second suggestion does not sound like the way to go. Something like that would keep me from upgrading to D6 until all of my CCK modules were updated. If one never is, I can see that leaving a bad taste in people's mouth. Your third suggestion sounds like the best approach simply because it's automated and, given the right documentation, can be easily anticipated by a developer.

Just my two cents.

yched’s picture

yched commented

Right, if it is feasible, the hook_notify way would probably be best. The way I understand this hook, its role is precisely to let content.module act on field module status change, so this should be the best tool here ? I like the "don't trust field modules too much" motto.

As fractile81 suggested, we do need to check what happens to those 'in limbo' fields if you modify your content type (change name, add fields, whatever...) before the all field modules are enabled.

Also, not sure this comes into play here : if the admin enables content.module and some field modules at the same time, the updates for a few field modules might run before the ones for content.module, depending on alphabetic order. Maybe at some point we might need to specify that content.module should be enabled / updated first...

Sigh... CCK upgrade paths are an indescribable pain. Drupal module system is not adapted to this sort of module dependency. This is probably another reason why CCK needs to get into core.

fractile81’s picture

fractile81 commented

Here's a little work based on what KarenS said in #3:

/**
 * Modules notify Content module when uninstalled, disabled, etc.
 *
 * @param string $op
 *   the module operation: uninstall, install, enable, disable
 * @param string $module
 *   the name of the affected module.
 * @TODO
 *   figure out exactly what needs to be done by content module when
 *   field modules are installed, uninstalled, enabled or disabled.
 */
function content_notify($op, $module) {
  switch ($op) {
    case 'install':
      content_clear_type_cache();
      break;
    case 'uninstall':
      module_load_include('inc', 'content', 'content_crud');
      content_module_delete($module);
      content_clear_type_cache(TRUE);
      break;
    case 'enable':
      content_associate_fields($module);
      content_clear_type_cache();
      break;
    case 'disable':
      content_clear_type_cache(TRUE);
      break;
  }
}

/**
 * Allows a module to claim which fields it controls.
 *
 * @param string $module
 *   The name of the module to update on.
 */
function content_associate_fields($module) {
  $module_widgets = module_invoke($module, 'widget_info');
  if ($module_widgets) {
    foreach ($module_widgets as $name => $widget_info) {
      db_query("UPDATE {". content_instance_tablename() ."} SET widget_module = '%s' WHERE widget_type = '%s' AND module = '%s'", $module, $name, '');
    }
  }
}

I created the content_associate_fields(); function with some minor tweaks made to the content_update_6000(); code that does this same thing. The most notable change is that the UPDATE-query now only updates fields that don't already have a module set. Then, I added this function to the content_nofity(); function (which I can't seem to get to work in my install) under the 'enable' path. Since I haven't been able to get the content_notify(); function called, I haven't been able to test this out yet.

But I do agree, if the content_notify(); function can be called properly, this approach should work great.

karens’s picture

karens commented
Assigned: Unassigned » karens

OK, good start but we need to update both the field and widget tables with module names.

As yched says, another issue is to be sure this will work no matter what order the modules are enabled, so we have to run through the logic of what will happen if a field module gets enabled in the same pass as the content module, but before it; in the same pass as the content module but after it; or at a later time. I am pretty sure the combination of this change and the content module update will take care of this, but we need to double-check.

I like the idea of a message at the time the content module update runs to remind admins to enable other field modules. We can't tell them exactly *which* modules to update because we don't know what module created the field since we don't have that info stored in the database (this is the very problem this update is attempting to fix). And at the time the content module is enabled, the others may be already lined up ready to go and there's no way to know which ones are already set up to be enabled and which are not. So I guess it would be just a general message saying you should be sure to enable all modules that have ever created any fields, or maybe a list of all fields that are still missing module names.

As to changes made while the field module is disabled, yech! That could get really messy. Not sure what to do with that. Ideas welcome.

I'll try to work this today.

moshe weitzman’s picture

moshe weitzman commented

@Karen - I'm pretty sure that http://drupal.org/node/194010 is assuring that modules will are enabled in an order such that their dependencies come first.

fractile81’s picture

fractile81 commented

What if a check was done after this field/widget module update was done to see what fields in content_node_field didn't have a module assigned? Then just print out the field name + associated "type" (I realize that might be less than desirable, but it would at least give the admin an idea as to what needed to be updated) in an error log of some sort (an alert, watchdog, w/e). If that's all we have available, why not use it?

Couldn't this same checking if there's an associated module be used on schema saves? If there's a field that doesn't have module information, don't do anything to it. I fully admit to not understanding all of CCK's schema and database workings, but where/how would it need to be more complex than this? Couldn't the same be applied to the widget_module field, too?

karens’s picture

karens commented

@moshe, that will ensure they're *installed* in the right order, but what if you have modules that were installed but are currently disabled (as you would often do during an upgrade), then you later come back and enable them. They might not update in the expected order then. I'm not sure about that, just pointing out the kind of possibility we have to keep thinking about.

Anyway, this is the first chance I've had to test the upgrade process using a site with lots of different field modules, several of which are not ported yet, so I've got a semi-complicated port to test. In doing this lots of other things popped out, including several core update bugs that I had to stop and report and make patches for, and now I see several other issues in the CCK update process that need cleaning up, so I'm working through that on my way to getting a fix for the original issue.

karens’s picture

karens commented

I got some of the fixes committed so they don't get lost. I still see more things in the upgrade path that need work, and still need to investigate whether hook_content_notify is firing when it should.

karens’s picture

karens commented

The more I look at this the more I think we need to add another column to the fields tables, an 'active' column. We can check/uncheck that column when field modules are enabled and disabled, then alter our queries to include or omit inactive fields from being included in the fields array. I'm working on an upgrade for a site that has a number of fields that aren't ported yet but have data, so I need a graceful way to mark them inactive and keep them out of lists. At the same time we need to be sure we don't wipe their values out during updates, so I think (still looking into this) that we need to include inactive fields in the schema and as hidden values in edits, but omit them from all other lists.

yched, any thoughts?

moshe weitzman’s picture

moshe weitzman commented

I have no idea if it would help, but maybe you could store a 'content schema_version' so that when the module gets reenabled, you know what state it is in. Your idea of passing along the values of inactive fields is a bit ugly but we could stomach it if we have to. We could segregate those to an element like $node->fields['inactive'] or somesuch so they would not conflict with the active fields and would not confuse developers/themers too badly.

yched’s picture

yched commented

I see what you mean. At 1st sight, I'd say trying more and more to treat disabled fields as if they were enabled, saving their data on node edit form, etc brings us quite far, adding more and more complexity..

Couldn't have an update function that moves the data for these 'in limbo' fields out of the way in temporary tables, that wouldn't be affected by subsequent node edits ? If we need to add cruft to manage these side cases, I'd rather add it to updates than to the code of the module itself ?

Side note : we don't support beta to beta upgrades, so I guess we can still modify the order of things in the 6000 updates series if needed.

Second side note : I'm currently mainly focusing on dnd-enabling the 'manage field' tab. Not urgent per se and rather cosmetic compared to the update issues at hand here, but people will scream if core has dnd and *this* page doesn't, and getting dnd here requires a patch for core tabledrag.js, so now's the time to try to get this in...

karens’s picture

karens commented

First of all, hooray for DND on the manage fields screen. I was hoping you would be up for that, but didn't want to ask :)

But the big question is how to handle these in-limbo fields. Moving the data to temporary tables is also quite messy. Some of them will be multiple value fields and some not, and when we re-enable them we have to move everything back to the right place, and depending on what else has happened in the meantime, the new location might be different than where they originally came from. And if a content type was deleted, we need to delete its temporary data as well as its active data. That seems even more messy to me than just keeping the values there, but hidden.

And this is going to be important. In my test case I have a variety of fields and I figure some will get ported quickly and others more slowly, so I need to preserve everything while gradually enabling more and more of the fields. And I think this will be a real world situation that will be faced by many.

yched’s picture

yched commented

And I think this will be a real world situation that will be faced by many.
I have no doubt about that, sadly :-)

OK still pondering.
Why is it we didn't care about that for 4.7 to 5 upgrade, BTW ?

karens’s picture

karens commented

Why is it we didn't care about that for 4.7 to 5 upgrade, BTW ?

For one thing, it was an easy upgrade for field modules, so they got it done pretty quickly, but this one won't be. Also, CCK in 4.7 was not so widely used, nor did it have so many field modules, but once people found in D5 they could add fields to any content type, it really exploded. I suspect we have a lot of people who never made the 4.7 to 5.x upgrade, they just started using it in D5.

Besides that, I think we just didn't think about it, which is not to say it might not have caused problems.

karens’s picture

karens commented

I've been trying upgrades that have both active and inactive fields and have been working on fixes to problems created when some but not all field modules are enabled on an update. Not only do I get pages and pages of errors, but the inactive fields still show up on the Display Fields and Manage Fields screens, and elsewhere.

Working off an idea floated by Earl Miles in IRC yesterday, I've added an 'active' column to the field tables and altered all the queries so they filter for only active fields. That got rid of numerous errors on the update and also fixed the problem where inactive fields were still showing up on the Manage Fields and Display Fields screens.

I set up the active field to default to zero, then update it when field modules are updated, so, hopefully, inactive fields will just disappear everywhere. That should work to keep them out of the mix until the module is enabled, and seems to work no matter what order the fields are updated in.

We'll need to watch this to be sure it doesn't introduce other problems.

There are still lots up update errors, so this doesn't fix everything, but it's something.

karens’s picture

karens commented
Title: Where to populate 'module' in content_node_field? » Data handling for inactive fields during update process
Component: General » upgrade path

Renaming this to reflect the current state of the issue so I don't keep clicking on it to see what it is :)

And assigning it to our new 'upgrade path' category.

karens’s picture

karens commented
Title: Data handling for inactive fields during update process » Data handling for inactive fields

Renaming, this is generally about how to handle inactive fields without creating havoc.

karens’s picture

karens commented

Marking http://drupal.org/node/204190 as a duplicate. It's not about problems during update but about what happens if you temporarily or accidentally disable a field module, which is another realistic scenario.

yched’s picture

yched commented

- The fix for http://drupal.org/node/204190 introduced a possibility for data from a disabled field to be wiped out, when removing the last active field instance from a content type.
- When creating a new field, we also need to check the name doesn't exist amongst disabled fields - or havoc follows.

Got a fix almost ready for both issues.

Other that that, I've run a few tests upgrading a D5 db with an imagefield (thus inactive in D6), everything works OK, data silently remains, and is not altered no matter what (field CRUD, move other fields' data around, node edit...)

We might want to remind the user about these inactive fields, though - maybe by a warning on the 'manage fields' type, as a forget-me-not. We might also want to list them on the list of fields admin/content/types/fields.

yched’s picture

yched commented

Committed a fix for both issues mentioned in #22.
Leaving open for the informational messages (those inactive fields need to be listed somewhere)

karens’s picture

karens commented
Status: Active » Fixed

I added a couple helper functions, one to create an array like the content_fields() array of only inactive fields, and one to display messages about inactive fields. The inactive fields message will now display on the Manage fields and Display fields screens.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.