When a module is disabled then the record in the node type table is deleted losing title label, body label, description etc customizations. 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.

I have a Drupal 6 patch in the making already. There is no upgrade function because we need to fix this in D6 anyways.

Files: 
CommentFileSizeAuthor
#47 895014-node-type.patch15.01 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 25,936 pass(es).
[ View ]
#45 895014-node-type.patch14.94 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 25,919 pass(es).
[ View ]
#44 895014-node-type.patch15.01 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#39 895014.patch14.23 KBchx
PASSED: [[SimpleTest]]: [MySQL] 25,771 pass(es).
[ View ]
#36 895014.patch15.31 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 25,724 pass(es).
[ View ]
#34 last_beta_blocker_5.patch15.9 KBbabbage
FAILED: [[SimpleTest]]: [MySQL] 24,255 pass(es), 170 fail(s), and 121 exception(es).
[ View ]
#32 last_beta_blocker.patch15.9 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in node.install.
[ View ]
#31 last_beta_blocker.patch15.9 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in node.install.
[ View ]
#29 last_beta_blocker_2.patch14.85 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 24,258 pass(es), 170 fail(s), and 121 exception(es).
[ View ]
#26 last_beta_blocker.patch14.85 KBchx
FAILED: [[SimpleTest]]: [MySQL] 24,261 pass(es), 170 fail(s), and 121 exception(es).
[ View ]
#25 last_beta_blocker.patch15.69 KBchx
FAILED: [[SimpleTest]]: [MySQL] 24,257 pass(es), 170 fail(s), and 121 exception(es).
[ View ]
#24 last_beta_blocker.patch15.86 KBchx
FAILED: [[SimpleTest]]: [MySQL] 24,257 pass(es), 170 fail(s), and 121 exception(es).
[ View ]
#23 last_beta_blocker.patch15.85 KBchx
FAILED: [[SimpleTest]]: [MySQL] 24,252 pass(es), 172 fail(s), and 121 exception(es).
[ View ]
#18 node_type_delete.patch16.9 KBchx
PASSED: [[SimpleTest]]: [MySQL] 23,388 pass(es).
[ View ]
#12 node_type_delete.patch16.9 KBchx
PASSED: [[SimpleTest]]: [MySQL] 23,387 pass(es).
[ View ]
#8 895014-node-type-delete.patch15.04 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] 22,714 pass(es), 164 fail(s), and 141 exception(es).
[ View ]
node_type_delete.patch14.27 KBchx
FAILED: [[SimpleTest]]: [MySQL] 22,693 pass(es), 164 fail(s), and 148 exception(es).
[ View ]

Comments

catch’s picture

-  $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.

Status:Needs review» Needs work

The last submitted patch, node_type_delete.patch, failed testing.

dmitrig01’s picture

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

Damien Tournoud’s picture

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

mrfelton’s picture

@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.

catch’s picture

Issue tags:+D7 upgrade path
mrfelton’s picture

Issue tags:-D7 upgrade path

Is there a D6 issue for this?

Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new15.04 KB
FAILED: [[SimpleTest]]: [MySQL] 22,714 pass(es), 164 fail(s), and 141 exception(es).
[ View ]

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.

catch’s picture

Status:Needs review» Needs work
+      '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.

chx’s picture

I am working on this. Note that the orphaned nodes losing their bodies is NOT this issue. This is one way to end up with an orphaned node but #891028: Orphan node types lose bodies on upgrade is the issue you want for that.

This issue is about a data loss as the original issue says. Try what the test does: enable poll, change the description, disable poll, reenable poll poof customization is gone.

mrfelton’s picture

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?

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new16.9 KB
PASSED: [[SimpleTest]]: [MySQL] 23,387 pass(es).
[ View ]
  1. Please make this just 'module' - this'd be consistent with other things in core which record module ownership. <= we are not doing that because 'base' was 'module' in D6 and we do not want to confuse the living hell out of everyone.
  2. Disabled should be 'status' -- agreed. In Drupal 8 it should be. In Drupal 7 we already expose ->disabled via all node type API see _node_types_build() doxygen.
  3. documentation for the new parameter.- added.
  4. Minor grammar fixes done.
  5. Damien already moved that but note that system.admin.inc already needed to know about it.
sun’s picture

Status:Needs review» Needs work

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.

chx’s picture

Assigned:chx» Unassigned

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.

I am not doing that. But, I have unassigned myself from the issue so another poor sucker gets the chance to pour a few days into fixing and backporting of this.

Please rename to {node_type}.status.

No. ->disabled is API exposed as I said already and having a property and database column named different and having the opposite meanings is a no-go.

chx’s picture

Title:[SUPER URGENT] node type customization is lost on module disable» node type customization is lost on module disable

if i go on i will won't fix the goddamned issue too. where is my unsubscribe link?

chx’s picture

Well. The patch, I think, already changes the API as described in this comment:

*   Both of these arrays will include new types that have been defined by
*   hook_node_info() implementations but not yet saved in the {node_type}
*   table. These are indicated in the type object by $type->is_new being set
*   to the value 1. These arrays will also include obsolete types: types that
*   were previously defined by modules that have now been disabled, or for
*   whatever reason are no longer being defined in hook_node_info()
*   implementations, but are still in the database. These are indicated in the
*   type object by $type->disabled being set to TRUE.

If we (who exactly?) decide that we do not care then yes, disabled can be status. But no, defining_module can not be module that's way too confusing. Especially that noone ever needs that aside from uninstall.

On the other hand if we want to abide by this doxygen then someone needs to check whether my patch breaks this (i suspect yes) and then needs to come up with a solution that does not. Good luck...

sun’s picture

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)

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new16.9 KB
PASSED: [[SimpleTest]]: [MySQL] 23,388 pass(es).
[ View ]

Well that's really one minor issue.

sun’s picture

+++ 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.

sun’s picture

Status:Needs review» Needs work
+++ 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.

catch’s picture

Priority:Critical» Major

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.

Damien Tournoud’s picture

Title:node type customization is lost on module disable» All fields of a node type are lost on module disable
Priority:Major» Critical
Issue tags:+beta blocker

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.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new15.85 KB
FAILED: [[SimpleTest]]: [MySQL] 24,252 pass(es), 172 fail(s), and 121 exception(es).
[ View ]

Added prepopulate.

The reason we moved this functionality under one roof is foreach (node_type_get_types() as $type => $info) in node_types_rebuild() -- if we want to keep that code, we would need to make the node_type_get_types function famility disabled-aware. I prefer not to.

We did not have a specific static cache reset before so I did not add that.

Discussed with webchick and removed node_type_uninstall from uninstall. Sure. Makes the code simpler that's for sure.

chx’s picture

StatusFileSize
new15.86 KB
FAILED: [[SimpleTest]]: [MySQL] 24,257 pass(es), 170 fail(s), and 121 exception(es).
[ View ]

Tests now pass. I only flipped the asserts for the uninstall cases.

chx’s picture

StatusFileSize
new15.69 KB
FAILED: [[SimpleTest]]: [MySQL] 24,257 pass(es), 170 fail(s), and 121 exception(es).
[ View ]

Swapped two queries in node.install in the update so now defining_module is properly prefilled with node when possible.

chx’s picture

StatusFileSize
new14.85 KB
FAILED: [[SimpleTest]]: [MySQL] 24,261 pass(es), 170 fail(s), and 121 exception(es).
[ View ]

I removed a line with $info_array which is not needed any more, reverted standard.install as node type defaults will add that 'node' and added line breaks to the test.

catch’s picture

Status:Needs review» Needs work

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.

catch’s picture

Status:Needs work» Reviewed & tested by the community

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

catch’s picture

StatusFileSize
new14.85 KB
FAILED: [[SimpleTest]]: [MySQL] 24,258 pass(es), 170 fail(s), and 121 exception(es).
[ View ]

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

Damien Tournoud’s picture

Status:Reviewed & tested by the community» Needs work
<?php
$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.

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new15.9 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in node.install.
[ View ]

Fixed.

catch’s picture

StatusFileSize
new15.9 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in node.install.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, last_beta_blocker.patch, failed testing.

babbage’s picture

Status:Needs work» Needs review
StatusFileSize
new15.9 KB
FAILED: [[SimpleTest]]: [MySQL] 24,255 pass(es), 170 fail(s), and 121 exception(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, last_beta_blocker_5.patch, failed testing.

Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new15.31 KB
PASSED: [[SimpleTest]]: [MySQL] 25,724 pass(es).
[ View ]

Properly fixed this damn query.

catch’s picture

Status:Needs review» Reviewed & tested by the community

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

Dries’s picture

+++ 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.

chx’s picture

StatusFileSize
new14.23 KB
PASSED: [[SimpleTest]]: [MySQL] 25,771 pass(es).
[ View ]

Removed the node type delete hunk. Added doxygen to the test case.

webchick’s picture

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():

<?php
-    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...

Dries’s picture

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

catch’s picture

@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.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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!

Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new15.01 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Damien Tournoud’s picture

StatusFileSize
new14.94 KB
PASSED: [[SimpleTest]]: [MySQL] 25,919 pass(es).
[ View ]

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

juan_g’s picture

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...

Damien Tournoud’s picture

StatusFileSize
new15.01 KB
PASSED: [[SimpleTest]]: [MySQL] 25,936 pass(es).
[ View ]

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.

mradcliffe’s picture

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);
+      }
+    }
Damien Tournoud’s picture

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

webchick’s picture

Status:Needs review» Fixed

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

Ildar Samit’s picture

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

juan_g’s picture

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. :)

sun’s picture

+++ 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.

GiorgosK’s picture

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 ??

Damien Tournoud’s picture

@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.

webchick’s picture

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.

Damien Tournoud’s picture

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';
GiorgosK’s picture

Great
thanks a lot @Damien and @webchick

catch’s picture

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

Status:Fixed» Closed (fixed)

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

catch’s picture

Version:7.x-dev» 6.x-dev
Status:Closed (fixed)» Patch (to be ported)
Issue tags:-beta blocker

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.

carlos8f’s picture

subscribing

joachim’s picture

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()?

Leeteq’s picture

Subscribing.

joachim’s picture

+++ 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.

catch’s picture

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.

hefox’s picture

Issue tags:-commonslove

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.

RichardLynch’s picture

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...

ezra-g’s picture

Issue tags:+commonslove

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

dynamicdan’s picture

Issue tags:+commonslove

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!

catch’s picture

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.