All fields of a node type are lost on module disable

-  $is_existing = (bool) db_query_range('SELECT 1 FROM {node_type} WHERE type = :type', 0, 1, array(':type' => $existing_type))->fetchField();
+  $is_existing = (bool) db_query_range('SELECT COUNT(*) FROM {node_type} WHERE type = :type', 0, 1, array(':type' => $existing_type))->fetchField();

why SELECT COUNT(*) instead of SELECT 1 here?

Also there isn't a new index on disabled.

just curious: how would one find a bug like this?

@dmitrig01: all the nodes from module-defined node types lose their bodies during the D6 to D7 upgrade :)

@dmitrig01 - try upgrading a site that has some custom node types, or actually, I think it may just be node types that are provided by a module, that you have altered. Disable all the contrib modules (including the one that was providing that node type) as recommended, then try the upgrade.

Is there a D6 issue for this?

Rerolled: moved the module uninstall where it belongs (in node_modules_uninstall()) and fixed the upgrade path.

This will fail because MySQL schema.inc implementation is currently unable to create a NON NULL field.

+      'defining_module' => array(
+        'description' => 'The module defining this node type.',
+        'type' => 'varchar',
+        'length' => 255,
+        'not null' => TRUE,
+      ),

Please make this just 'module' - this'd be consistent with other things in core which record module ownership.

+      'disabled' => array(
+        'description' => 'A boolean indicating whether the node type is disabled.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+        'size' => 'tiny'),

This should be 'status' - to make it consistent with modules, themes, nodes themselves etc. If not status for some reason, then at least it should be 'enabled' to avoid the double negative in queries (':disabled' => 0)

+function _node_types_build($rebuild = FALSE) {

No documentation for the new parameter.

+ * Test node type customizations persist.

persistence.

survive

survives

-    if (module_hook($module->name, 'uninstall') || module_hook($module->name, 'schema')) {
+    $node_type_count = db_query("SELECT COUNT(*) FROM {node_type} WHERE defining_module = :module", array(':module' => $module->name))->fetchField();
+    if (module_hook($module->name, 'uninstall') || module_hook($module->name, 'schema') || $node_type_count) {

System module has to know about node module now? I think we need to allow modules to stop themselves or others to be disabled/uninstalled, but not sure this is the issue to do this, and not keen on the special casing.

I applied the patch in #8, but still get errors like the following:

Failed: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'disabled' in 'where clause': SELECT nt.* FROM {node_type} nt WHERE (disabled = :db_condition_placeholder_0) ORDER BY nt.type ASC; Array ( [:db_condition_placeholder_0] => 0 ) in _node_types_build() (line 701 of /home/tom/workspace/d7/modules/node/node.module).

Is the upgrade patch not working?

Here's my initial review. Note that I have tried to remain as ignorant as possible as to what's been going on in this issue, so that I can give it a totally fresh set of eyes. Apologies if any of this was already mentioned above; I'll read the issue once I'm done here.

+++ modules/node/node.install	2010-08-27 11:49:45 +0000
@@ -294,6 +294,12 @@ function node_schema() {
+      'defining_module' => array(
+        'description' => 'The module defining this node type.',

Regardless of what has been in Drupal 6, 5, 4, 3, or 1, in 1942, or whether the answer is 42, we call this column "module" everywhere else, and so should we do here.

+++ modules/node/node.install	2010-08-27 11:49:45 +0000
@@ -344,6 +350,13 @@ function node_schema() {
+      'disabled' => array(
+        'description' => 'A boolean indicating whether the node type is disabled.',

Please rename to {node_type}.status.

+++ modules/node/node.test	2010-08-27 13:19:41 +0000
@@ -1141,6 +1141,76 @@ class NodeTypeTestCase extends DrupalWeb
+    $this->drupalPost('/admin/modules', $poll_enable, t('Save configuration'));
...
+    $this->drupalPost('/admin/modules', $poll_disable, t('Save configuration'));

Bogus leading slash.

Powered by Dreditor.

ok, chx clarified once again that we cannot change ->disabled, as that is used by Node type API already, just not stored yet. Reading the patch, I'm not sure whether there is an actual difference in behavior of the property, aside from persistent storage.

Likewise, he does not want to change ->defining_module into ->module, because we just moved from ->module to ->base since D6. I don't see a real problem with that, but ok.

This mainly leaves us with the last minor issue of #13.

P.S.: I also wondered why this entire node type info is not cached... looks like low-hanging performance fruit to me. (separate issue)

+++ modules/node/node.install	2010-08-27 11:49:45 +0000
@@ -424,9 +437,25 @@ function node_update_dependencies() {
 function node_update_7000() {
+  db_add_field('node_type', 'defining_module', array(

Shouldn't we also try to populate this new column with the old 'module' column's values?

We can compare all existing module names and check whether the old module column value contains a module name as prefix. That should probably work for 99% of all cases, if not even 100%.

FYI: Also created #898360: Cache node types

Powered by Dreditor.

+++ includes/common.inc	2010-08-27 11:47:03 +0000
@@ -6309,8 +6309,8 @@ function drupal_flush_all_caches() {
-  menu_rebuild();
   node_types_rebuild();
+  menu_rebuild();

I think these two lines got switched and flipped and moved around a gazillion of times during D7 already, and it's really time we start to document the technical reason for their order when changing them.

+++ modules/node/node.module	2010-08-27 13:18:55 +0000
@@ -464,16 +464,7 @@ function node_type_get_name($node) {
 function node_types_rebuild() {
-  // Reset and load updated node types.
-  drupal_static_reset('_node_types_build');
-  foreach (node_type_get_types() as $type => $info) {
-    if (!empty($info->is_new)) {
-      node_type_save($info);
-    }
-    if (!empty($info->disabled)) {
-      node_type_delete($info->type);
-    }
-  }
+  _node_types_build(TRUE);
 }

@@ -676,42 +672,67 @@ function node_type_update_nodes($old_typ
+  if ($rebuild) {
+    foreach ($_node_types->types as $type => $type_object) {
+      if (!empty($type_object->is_new) || !empty($type_object->disabled_changed)) {
+        node_type_save($type_object);
+      }
+    }
   }

Working further on #898360: Cache node types, I'm not sure whether this change is a good idea. We have to differentiate between

- "totally rebuilding stored node type information" and

- "refreshing static/db caches"

In other words: Only node_types_rebuild() should contain code that changes stored node type information.

It looks like that is as easy as moving the conditional $rebuild code "back" into node_types_rebuild().

+++ modules/node/node.module	2010-08-27 13:18:55 +0000
@@ -2288,6 +2313,12 @@ function node_form_block_custom_block_de
 function node_modules_uninstalled($modules) {
+  // Remove the node types defined by the modules.
+  $node_types = db_query("SELECT * FROM {node_type} WHERE defining_module IN (:modules)", array(':modules' => $modules));
+  foreach ($node_types as $type_object) {
+    node_type_delete($type_object);
+  }

wow. Do we really want to delete node types of uninstalled modules? For instance, that likely means that, after upgrading with Image module from D6 to D7, weird things could happen upon uninstalling the D7 Image module.

+++ modules/system/system.admin.inc	2010-08-27 11:47:03 +0000
@@ -1294,7 +1294,8 @@ function system_modules_uninstall($form,
-    if (module_hook($module->name, 'uninstall') || module_hook($module->name, 'schema')) {
+    $node_type_count = db_query("SELECT COUNT(*) FROM {node_type} WHERE defining_module = :module", array(':module' => $module->name))->fetchField();
+    if (module_hook($module->name, 'uninstall') || module_hook($module->name, 'schema') || $node_type_count) {

It looks like we don't use the actual count of node types, so we should use SELECT 1 instead of COUNT(*), no?

Aside from that, it's pretty ugly to have node-specific code in here... looks like a regular hook scenario to me, i.e., various modules using API of one module, so that module can be responsible for indicating that there's data to uninstall. Sounds pretty easy, but I guess you want to defer that to D8...

Powered by Dreditor.

Since the upgrade path issue is handled elsewhere, I'm downgrading this to major - behaviour has been around for a very long time, and it's 'configuration data loss' which is easily fixable/replaceable, rather than actual user content that disappears here.

This is actually worse. All the field data of a node type are lost on module disable.

This is because node_types_rebuild() calls node_type_delete() that calls field_attach_delete_bundle() that... deletes the field data.

The field instance move in forum_enable() is because that instance will stay there, and enabled, despite the bundle being disabled. It's sucky that this will take memory and processing despite not being used anywhere, but for today it's better than it disappearing off the face of the earth. In D8 it might be necessary to add a field_attach_disable_bundle() or something, but not now, and the node module changes would still be necessary even if we had that.

hook_node_types() is being invoked twice, but the results of the first invocation are never used.

$_node_types->types[$type_db]->disabled_changed = $disabled != $type_object->disabled;
makes me sad but it's not really worse than what's already there. Which is saying something.

The tests could do with line breaks before the comments.

The test tests the contents of the node type table, but doesn't test the persistence of field instances, which is what set this back to critical, but that can be a followup.

Cross-posted. Looks RTBC to me, needs the test bot run (or tests run locally and verified here).

Re-uploading in attempt to jump the test bot queue, no credit please.

+  $modules = db_select('system', 's')
+    ->fields('s', array('name'))
+    ->condition('type', 'module')
+    ->fetchCol();
+  db_update('node_type')
+    ->expression('defining_module', 'module')
+    ->condition('module', $modules);

This last condition needs an explict 'IN' operator... and we are missing an ->execute() here.

Fixed.

Also missing parenthesis after node_menu() in a code comment.

Re-roll of catch's patch from #32, removing errant semi-colon, to keep things moving.

Properly fixed this damn query.

How did this issue possibly go seven hours without being RTBC.

+++ modules/node/node.install
@@ -294,6 +294,12 @@ function node_schema() {
+      'defining_module' => array(

Instead of 'defining_module' can we use 'module'? That is more consistent with the rest of core.

Other than that, this patch looks good. Love the tests -- very clear.

Ok. So this looks like a pretty huge patch, but it really comes down to three changes:

1. Adding two new columns to the node_type table: "defining_module" (The module defining this node type) and "disabled" (A boolean indicating whether the node type is disabled.)

2. Removes this from node_types_rebuild():

-    if (!empty($info->disabled)) {
-      node_type_delete($info->type);
-    }

which previously would destroy node types of disabled modules. This was not consistent with my D6 testing, which keeps node types around in between module enable/disable (or at least their title, body, and taxonomy associations).

3. Tests to demonstrate the new workflow.

The end result is that modules that defined node types will stick around even after uninstalling, unless the module explicitly calls node_type_delete() in hook_uninstall(). I think this is acceptable; we did automate some of the tedious hook_install() / uninstall() tasks like schema stuff, but we're talking here about data destruction which IMO needs to be a very opt-in process.

I found a couple of minor nits which chx is re-rolling for. We needlessly changed the argument in node_type_delete() and need to revert it, and are missing PHPDoc on the testX function. I also earlier asked him to make defining_module default to 'node' so that those creating nodes programmatically don't have to make an API change, which was already taken care of.

Looks like latest patch has all of these changes. Now we wait...

Please also include #38 in the upcoming re-roll. Thanks. :)

@Dries, did you read #9 and the followups from that review? I asked the same question, chx's answer was satisfactory for me as to why we can't do that.

I agree with Dries, and this was one thing that's been nagging at me (other than 'disabled' => 'status' which I understand why we can't do without an API change). Yeah, I've read chx's arguments and tbh they don't totally make sense to me. The reason we renamed "module" to "base" in D7 was because that column was not, in fact, the defining module. But this one is. The schema descriptions seem clear to me.

chx is concerned that people coming from D6 will be confused about the change, but "The Drop Is Always Moving." We don't call page.tpl.php page-template-not-to-be-confused-with-the-page-module-which-actually-you-now-make-in-the-ui-and-add-fields-with-cck-which-btw-is-now-in-core-as-of-drupal-7.tpl.php :P Drupal 7's code and DB schema should be written for Drupal 7. Not for Drupal 6.

So let's get that one last change in there and then get this sucker in. Yay!

I disagree, but here is a reroll for #43.

Scratch #44. I should remember to save all the files in my editor *before* rolling the patch :)

Seems last blocker now, rest of last minute ones just committed.

Damien Tournoud's:

+      'module' => array(
+        'description' => 'The module defining this node type.',

Holding breath...

Tested this on the #934042: The Drupal.org test, and found out that I broke the update path query here.

Here is a fixed version.

Is the last foreach trivial or should the code be done within the prior foreach?

+  if ($rebuild) {
+    foreach ($_node_types->types as $type => $type_object) {
+      if (!empty($type_object->is_new) || !empty($type_object->disabled_changed)) {
+        node_type_save($type_object);
+      }
+    }

I ran this version on a smaller version of drupal.org (with only a few 10,000 of nodes, users and comments), and it worked perfectly. #yay

Well, that sounds like about the most ringing RTBC endorsement I can think of.

Committed this, the last Drupal 7 beta blocking patch, to HEAD! :D :D :D

I know I don't belong here, but wanted to say THANKS GUYS! :D

Today is D-Day... Congratulations all!

Now, let's go build wonderful websites with Drupal 7 beta, and do a lot of real-life testing. :)

+++ modules/node/node.install
@@ -438,16 +451,40 @@ function _update_7000_node_get_types() {
+  $modules = db_select('system', 's')
+    ->fields('s', array('name'))
+    ->condition('type', 'module');
   db_update('node_type')
-    ->fields(array('module' => 'node_content'))
-    ->condition('module', 'node')
+    ->expression('module', 'base')
+    ->condition('base', $modules, 'IN')
     ->execute();

Crazy! I didn't know that this is possible. Still learning something new about DBTNG every day, thanks! :)

Powered by Dreditor.

Just updated from dev of oct 5th to the beta1
and after update.php finds nothing to update I get error message similar to #11

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'disabled' in 'where clause': SELECT nt.* FROM {node_type} nt WHERE (disabled = :db_condition_placeholder_0) ORDER BY nt.type ASC; Array ( [:db_condition_placeholder_0] => 0 )  in _node_types_build() (line 698 of G:\testd7\modules\node\node.module).

I was wondering if this is OK passing from a dev version to a beta or is this a real BUG ??

@GiorgosK: this is by design. We do not support alpha-to-beta upgrades. It should be easy enough to add this column manually and populate it.

We don't support upgrades from D7 to D7 until beta 1, so this is not technically a bug (though I agree it's weird). See http://drupal.org/project/head2head which should get an update for that at some point. Else, if you add the columns manually that works too.

This should help for the upgrade:

ALTER TABLE node_type ADD module varchar(255) NOT NULL DEFAULT '';
ALTER TABLE node_type ADD disabled tinyint(4) NOT NULL DEFAULT 0;
UPDATE node_type SET module = base WHERE base IN (SELECT name FROM system WHERE type = 'module');
UPDATE node_type SET module = 'node' WHERE base = 'node_content';

Great
thanks a lot @Damien and @webchick

(hopefully last) head2head issue here #934548: Update for #895014.

This needs to be backported to Drupal 6. Currently there is no way for the Drupal 7 upgrade path to know about node types from modules that were disabled.

Quoting chx from #1:

Right now we recommend disabling contrib in Drupal N before upgrading to Drupal N+1 so this issue needs a fix in Drupal 5 and 6 and they need a release before we can release Drupal 7 hence the super urgent marker.

Coming here from #1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors.

subscribing

Just spotted this one while working on the image D6 upgrade path.

Do we need to backport the extra field on the node type table, or just prevent node type deletion in node_types_rebuild()?

Subscribing.

+++ modules/node/node.install
@@ -438,16 +451,40 @@ function _update_7000_node_get_types() {
 function node_update_7000() {
+  // Rename the module column to base.
+  db_change_field('node_type', 'module', 'base', array('type' => 'varchar', 'length' => 255, 'not null' => TRUE));

How is this going to work with the backport? If these db changes need to be made on D6 too, where should they go so that an eventual upgrade to D7 doesn't give errors?

1 days to next Drupal core point release.

We'd need to db_field_exists() and similar checks in Drupal 7 if this gets into Drupal 6 I think so the upgrade works either way.

I think this is semi-related to what I'm seeing:

1. Moved a module that defines a node type
2. Run drush updb -y;
3. module_rebuild_cache finds the moved module, but module never drupal_load'ed
4. cc all is called on the same bootstrap as ^, node_type_rebuild goes and deletes the node type defined by that module, info about that node type lost.

tl;dr: it's not just disabling where this crops up.

edit: I now run drush rr (registry rebuild, from registry_rebuild project) before updb to avoid issues like this.

I got the same thing trying to go from D6 to D7, but two points against me for:
a) Not being on the most recent D6 first
b) Not deleting all the D6 code from my sites

That said, I just did:
alter table PREFIX_node_type add column disabled tinyint(4) default 0;

and moved on with life...

But that's just me...

Tagging this with 'commonslove' so that it can get more attention as we work on the Commons D6=>D7 upgrade process.

Have I missed something??

There is a patch to be ported from pre June 2012 that is needed for a successful upgrade from D6 to D7?? In other words, an upgrade to D7 from D6 is officially not recommended? How does a critical bug get left alone for so long? 2 years!

The upgrade path was fixed in Drupal 7 for this issue in a different way, although it took more than 6 months after release for the patch to be committed. See #1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors for the gory details. This patch should have blocked the Drupal 7 release altogether, but for various reasons that was entirely overlooked and Drupal 7 released anyway without it.

It hasn't been fixed in Drupal 6 because no one has worked on it, but it's not strictly necessary for a successful Drupal 6-7 upgrade any more.