D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors

I can't confirm but its like an upgrade path problem. so changed to bug report

This is now in field_data_comment_body.

things to check:

1. SELECT COUNT(*) FROM comments; vs. SELECT COUNT(*) FROM field_data_comment_body
2. Check a specific comment in your D6 comment table, compared to the D7 field table. (entity_id in the D7 field table will be the same as {comments}.cid
3. If you find that the comment body text has made it into the D7 site, but the comments aren't showing up, then check which text format is being used in field_data_comment_body - it may be that the filter is broken due to a missing contrib module and you'll need to go to admin/config/ (something text formats) to sort it out - after doing so + cache clear it may all come back.

#895014: All fields of a node type are lost on module disable is the issue this was originally found in. That issue was happily closed, forgetting that we needed patches for Drupal 5 and 6 - those patches should have blocked the first beta.

I'm bumping this one to critical, but it depends on a Drupal 6 patches for #895014: All fields of a node type are lost on module disable - however we need to have an active, critical issue in the Drupal 7 queue so it doesn't get lost again.

This needs tests.

subscribing.

Similar issue, not sure if it's related or not: #890128: Comment body missing if comment module enabled after a content type module

Thinking about it, the backport doesn't do anything for D6 sites that have already disabled a node-type providing module.

I'm wondering if we can do an additional catch-all in Drupal 7 which would more or less amount to SELECT DISTINCT type FROM {node} and then create disabled node types for any that are missing in the Drupal 7 upgrade path itself with some default settings. This would mean the configuration for those node types will get lost, but the actual data at least in the body field and taxonomy would be preserved.

Let me a sideline remark ... I think, there is something essentially bad when managing node types and fields of disabled modules.

Please, have a quick look at ...
#943588: node_type_delete(): hook_node_type_delete will fail for a disabled node_type. Trying to get property of non-object in hook_node_type_delete()
#943772: field_delete_field() and others fail for inactive fields

Isn't this a duplicate of #895014: All fields of a node type are lost on module disable?

It is, but it's intentional, I really want active Drupal 7 and Drupal 6 issues so this doesn't get lost a second time.

Subscribing.

Here's a rough patch that starts the process of trying to fix this within D7 - I think we need this because any Drupal 6 site that has ever disabled a node type before the backport goes in will still have orphaned node types that don't exist anywhere except for {node}.

This issue is really a continuation of #232327: Deleting node type leaves orphan nodes and similar.

Also note there is absolutely no way we can repair this for sites that have already upgraded to Drupal 7 and lost content, because we drop the node_revision.body column in that update.

I won't have much time to look at this further, next step would be either trying this on a real D6 site, or adding a test case.

Subscribing. Will try to test the patch out once I can return my site to D6.2, as I've already gone to D7 and have a host of issues with image nodes now.

Two patches, test_only.patch should have one fail. The other should pass.

Extra comment for node upgrade was causing duplicate key errors, with that gone the comment upgrade ran fine, seeing what's left.

Test bot seems down?
Needs work?

Yep, it's still broke. The data in the upgrade path dump is what breaks it, need to re-organise things so that it doesn't conflict with any of the other tests.

I may not get to this for a while. If anyone wants to take over, all that really needs doing is scrapping the upgrade data from the patch here, then adding it separately similar to other tests, so it doesn't conflict with them. The actual fix and test changes should be more or less identical once that's done.

Also this is now D7 only after #1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path), there's not much we can do for 7-7 or 7-8 installs that were already upgraded from Drupal 6 (if there is this should be a new issue) - so no need to move this to D8 and backport (as I'm currently doing for other Drupal 7 criticals).

Here's a patch that passes tests locally.

Off-topic - anyone thinking this is the only Drupal 7 critical, 'fraid not, but it's the only one that doesn't need to go into Drupal 8 first.

_update_7000_node_get_types() is in Drupal 8, so patch for that.

Bot!

Patch doesn't apply due to #1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path). Waiting on resolution there.

Marked #1133402: Cannot upgrade site if no node types exist as duplicate since this is similar, and exactly the same code.

Attached untested patch (for D7 only, since it is still impossible to roll a patch for Drupal 8 until #1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) is fixed), ought to do it.

One other scenario that this (looks like it) would fix is the instance where you actually have no node types or nodes. Currently if you attempt to upgrade a Drupal 6 site that has no node types, you receive a PDO exception, as it's trying to use WHERE type NOT IN ();. See #1133402: Cannot upgrade site if no node types exist for more details.

I tested this patch with an orphaned node, and saw the data created properly. This test also was on a database with no node types, so #1133402 was also resolved.

This might fix the issue, but it is still blocked on #1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) since there is no viable D8 patch. Leaving at needs review since there is no work that can currently be done here, and 'postponed' would take it out of the default criticals listing.

This is the patch from #30, rolled against 8.x with #1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) already applied. Once that patch is in, it should apply cleanly.

(edit, thank you git for making this a 2 minute process)

Looks great!

I committed #1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path).

+++ b/modules/node/node.install
@@ -449,5 +449,21 @@ function node_install() {
+    $type_object->type = $type;
+    // Always create a body. Querying node_revisions for a non-empty body
+    // would skip creating body fields for types that have a body but
+    // the nodes of that type so far had empty bodies.
+    $type_object->has_body = 1;

I had to read this 3 times but I'm still not sure I understand this explanation. Can we rephrase this a bit better?

It would also be good to add a sentence explaining what is going on at a slightly higher level.

Powered by Dreditor.

Heh, yeah, that comment actually made it into D7, but also took me about 5 reads before I understood it. Revised patch.

That comment has been in Drupal 7 since September last year, and is unchanged by the patch, the only reason it's here is because we're moving the hunk of code around. But this bug has only been known about for 8 months so it's entirely appropriate to hold it up on an 8 month old comment.

#1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) wasn't actually committed, so I had to re-roll this one again, to depend on that patch.

When I upgraded my site in February, I did a really bad winging it job and messed a lot of things up. I noticed that all the comments were gone on all the forum posts and assumed it was yet another thing I had done wrong in the major fail upgrade. Since I really didn't care about the old forum posts, I just said f-it and deleted all the forum nodes as well. Now I see talk about this on IRC and wonder if I got bit by this and that's why all the comments were gone?

Not much useful there but catch said it would be good to post that I lost data to this. In my case I didn't care so much but I would definitely be upset if I still cared about the forum on that site.

Michelle

Whoops, crossposted.

Michelle

Updated comment via beejeebus.

Patch doesn't apply because it depends on the still uncommitted #1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path). Back to CNR.

#46: still_depends_on_1097100_3_weeks_later.patch queued for re-testing.

#1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) was committed, re-rolled again. I'm not re-rolling this patch another time so hopefully this is it now, otherwise someone else is going to have to take over here.

Looks good!

Committed to 7.x and 8.x. Marking this fixed. Sorry for the delays -- and thanks for all the work.

Commit for 7.x missing from the commit log: http://drupal.org/node/3060/commits

Really committed this time. :)

Thanks a lot for sticking with this, catch. I know it's been very frustrating for you. Don't worry; we'll get D7 into tip-top shape here in the next couple of weeks. :)

Grrr. So I tried (I really did!) to commit this earlier, but it failed testbot for some reason. The error was related to DrupalWebTestCase not being able to be loaded from Block module (I was terribly distracted and forgot to copy it) which makes absolutely no sense, and doesn't happen from the UI. Then I tried to figure out run-tests.sh and that was a disaster. And now I'm about to be traveling all day tomorrow.

So I'll take another look at this this weekend when I have some uninterrupted time in front of the computer (probably late night, since I'll be seeing my family).

In the meantime, I'm re-uploading catch's patch from #46 in the hopes that testbot can either a) like it, or b) give us some extra help in figuring out what the problem is.

#55: stop_bleeding_please.patch queued for re-testing.

Well, at least it's consistent, I guess...

For the record, this is the line the testbot is running:

/usr/bin/php ./scripts/run-tests.sh --concurrency 8 --php /usr/bin/php --url 'http://drupaltestbot659-mysql/checkout' --all 2>&1

I'm trying to replicate some proximity of this on localhost (I don't have concurrency support):

/Applications/MAMP/bin/php5.3/bin/php ./scripts/run-tests.sh --url 'http://localhost/core' --all

...and with or without the patch applied, the test suite, including the block tests, seems to execute fine. :( It definitely hasn't crapped out at 0 seconds with a fatal error, at any rate.

I'm going to need Damien or Randy to help me debug this. :(

Last test was from client 659, I want to see this fail on another test client.

#55: stop_bleeding_please.patch queued for re-testing.

659 again. Disabled the client. Sending for retest again.

#55: stop_bleeding_please.patch queued for re-testing.

Different test bot, same failure. Re-enabled 659.

That's me out of this issue: I have neither access to debug it on the test bots, nor any intention to put more time into the black hole.

Good luck folks.

Subscribe. I'm open to suggestions as to what I can or should do about this. (Most or all of my comment text is blank on a 3 year old site). Blah. It's a showstopper for me.

+  $node_types = db_query('SELECT * FROM {node_type}')->fetchAllAssoc('type', PDO::FETCH_OBJ);
+
+  // Create default settings for orphan nodes.
+  $all_types = db_query('SELECT DISTINCT type FROM {node}')->fetchCol();
+  $extra_types = array_diff($all_types, array_keys($node_types));

Any upgrade test that works with just drupal-6.bare.database.php gets into trouble at the array_diff().
Edit: I'm reading up now, the above may or me not be nonsense. The one thing on my mind was "If there is no next record, FALSE will be returned".

The ->fetchCol() and ->fetchAllAssoc() methods both return array() if there's no result, so that ought to be OK unless I missed something.

I bit my tongue and bisected this on another issue to try to track down the test failures.

The actual fix passes tests fine. Uploading the tests by themselves or combined with the patch causes the bizarre block.test issue. Since this has passed the test bot in the past, passes locally etc. I don't think it should be delayed any further on test bot weirdness (and if it is, I'm out of this issue again). so I'm uploading #55 minus the tests and marking RTBC again.

Just to add to the ultra-urgency of finally getting this applied - there is more than one reason that comment bodies might not display after upgrade, if I remember correctly we committed a patch for text formats to display empty content if filter configuration is invalid. Additionally if some node types are disabled and some aren't, then you will have the table for the comment body field created, and it'll have some perfectly fine records in it - just not for all of the comments on your site.

So not only does this delete the content during upgrade, and not only does it fail silently during the upgrade process itself: unless you've seen this issue, the only way to diagnose it is to run database queries against your upgraded database and understand the results - so even people who backup and run the upgrade, might not realise they've run into the bug until it's too late, and then they'd either have to start over from scratch, or try to restore from a D6 backup into their D7 site (if they still have one).

That sound you just heard was the scream of 8000 comments going off into the wind.

Opened #1160928: Add tests for node type/comment body upgrade path.

Yay!! Committed to 7.x at long last!!

Thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you!!!

Could we get this released in a 7.3 pretty soon, so contrib modules can tell users to use that to upgrade?

@joachim,
it's already in 7.2