Error message deleting non CCK content when CCK is enabled

This is related to a number of other deletion issues. I'm working on trying to clean them up and hope to have something to commit soon.

This actually turned into a whole can of worms, but I have a fix ready to commit. The original problem is that a table called node_blog should have been created by the content module as a place to store its fields and it didn't get created. It turns out that the code to create that table got inexplicably left out of the program completely. I can't believe we all missed this, but we did. So I added that code to the create function in content_crud.inc, but found there is another, less obvious problem.

The content module obviously can only create that table if it is installed when the blog table itself is created. If you install the blog module first, then later install CCK, CCK will never realize that node type was added and will not create its table. And a related problem will occur if CCK is uninstalled, even temporarily, for any reason and content types are added or changed while it is uninstalled.

So I added a function called content_types_rebuild() to the content_crud.inc file and used hook_form_alter() to execute that program any time the modules page is submitted and CCK is on the list of enabled modules. That function then checks for all available content types and compares them to the CCK tables, and adds any that may have been missed.

There will be other problems if you uninstall CCK and do something like change a content type name, then reinstall CCK. Even with the rebuild function, CCK will have no way to know which new content matches data it may have stored using the old name. I tried to think of a way to fix that problem, but I'm not sure that's fixable, so I think the best we can do is add a message when you install it that warns the admin that they could corrupt their database if they make changes to content types while CCK is uninstalled and later reinstall CCK. It doesn't look like drupal_set_message works in the .install file, so instead of using hook_uninstall(), I used the same hook_form_alter() on the modules pages to set a message if CCK is not enabled.

We still need to fix other issues related to deleting content info, and I'm getting ready to work on that next, but that is a different issue.

Anyway, I'll be getting changes to fix this issue committed in the next half hour or so. After downloading the fixed version of the code, you'll then want to go to the modules page and submit it so that content_types_rebuild() gets executed.

Committed.

Karen :
Before today's commit, the table for the content type was created in content_alter_db_field(). Meaning, not on content-type creation time, but only when a CCK field was added to the content-type - which possibly makes some sense ? Anyway this explains IMO the "how could we miss something so huge" part.

This happened in a branch :

if (!db_table_exists($new_db_info['table'])) {
  if ($new_field['db_storage'] == CONTENT_DB_STORAGE_PER_CONTENT_TYPE) {
    (code to create the table)
  }
}

The way I see it, content_type_create() was only an empty placeholder JonBob created to lay out a proper CRUD code structure (which he did not finish - yet ?). I'm not exactly sure what he wanted in this, and how this would have affected other db-related code. Plus he added this to 4.7 branch, where things were simpler as content types were managed entirely in content.module.

If we move the creation of the CCK content-type table to the type creation time (which I don't really know right now if we should or should not), we should probably update the rest of the code to match this ? I'm not sure it is currently completely coherent : if the table gets created in content_type_create(), there should be no code to create it in content_alter_db_field().
But content_alter_db_field is not a simple piece of code, and altering it is probably not a pleasant thing to do...

Suggestion : why not keep pre-today's commit behaviour, and fix the bug reported by chrissearle by checking the existence of the tables on node type deletion operations ?
And : wouldn't that maybe simplify the content/module activation / deactivation issues ?

I'm currently short of time to really delve into this - sorry if the above is just me missing something you already took into account :-)

@yched,

Actually I've concluded we should also delete that code in content_alter_db_field() because it is not working right in that location anyway. I found several situations where it failed to create the table when it should. That code was not there in 4.7, it was added in 5.x. The behavior in 4.7 was to *always* create the table when the content type was created, whether or not a field was ever added to it, and none of that code was in content_alter_db_field(), so the changes I'm making to 5.x just get it back in line with the way that 4.7 worked.

And yes, the content_alter_db_field() function is a deep, dark, scary place :-) But I had to dig into to it to figure out the reason why deletions weren't working, and that's when I realized that putting the create table code back into its own function was more in line with the way things worked in 4.7.

I think one reason we need to always create the table is that our content_info() function assumes it exists, even if it doesn't, which is going to come back and bite us whenever we try to use the 'table' value from the array that function creates.

There was already a if db_table_exists in there before dropping the table, but for some reason that didn't work.

I'm open for discussion, but if we leave it up to content_alter_db_field() to create the table, it needs some work done on it to make sure it gets triggered at the right times. And it makes the code much longer and messier, and less comparable with 4.7.

Just to explain a bit more, if you look at the content_alter_db_field() function for 5.x compared to 4.7, the only difference between them is that a whole section was added in 5.x from lines 880 to 926, which is the place where the node_ table is created. In 4.7, the node_ table is created, unconditionally, in the _content_admin_type_edit_submit() function. In 5.x there is no _content_admin_type_edit_submit() function, of course, so I put it in content_crud in the function that is called by hook_node_types() when a node is created, which seemed like the most logical place for it to go.

Then, to see one of the problems with the 5.x code, just use the story and page types that come with 5.x. Create a new field in page and you get a table created called node_page. OK.

Now pull up the content type story and add the field you just created to story, too. No node_ table gets created. If you look down the content_alter_db_field() function to see what happens, you can see that it is a per-field content type, so it creates the per-field table and moves the content from the story type into it. When it gets to the part of the code that would create the node_ table, it skips past it, since that part of the code only triggers for per-content types. And there's more. The first part of that code for per-content types triggers only for per-content types that are multiple values. Multiple values are never stored in per-content type tables, so that code is completely useless and needs to be omitted. The other part of it needs some more if/then logic added so that if this is a per-field type that does not already have a node_table created the node_ table gets created along with the field table, so now the code gets even messier.

It is so much easier to just go back to the way things were done in 4.7 and unconditionally create that table and remove those extra lines from the content_alter_db_field() function.

BTW, I'm not trying to be argumentative nor am I assuming you disagree, I'm just going through this step by step to be sure my thinking seems rational.

@JonBob, yched, dopry - I'm re-opening this just to get feedback on the way this was fixed. I just want to be sure I haven't gone down the wrong path here.

Is this bug actually fixed? I'm getting the same error message today even after doing a cvs update of cck, visiting the admin/content/types page, etc.

@pwolanin : Are you still experiencing this ? There has been some recent commits that might have fixed it.

I'm going to assume this was fixed since we don't have a huge pile of related issues.