I've updated a 6.20 site to 7.0 I've seen no errors during the update.

Strangely, a lot of comments are now empty. That is: The title, author and date are correct, but the comment content is not there anymore. It's not a theme issue: I've tested with garland. Additionally I'm receiving this error in dblog

Warning: array_flip() [function.array-flip]: Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->cacheGet() (line 354 of /var/www/zona7/includes/entity.inc).

But the error appear also when I visit a page without comments, it seems to be unrelated.

If I create a new comment, there content is there.

In what table of the database is the content comment? (I've checked the 'comment' table and I found the title of the comments, but not the content)

Please, report if you have seen this behavior or write a comment if you can help me somehow, thanks

Files: 
CommentFileSizeAuthor
#69 stop_bleeding_please_0.patch2.23 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 31,837 pass(es).
[ View ]
#55 stop_bleeding_please.patch4.97 KBwebchick
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#50 last_reroll_i_hope.patch1.27 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 29,435 pass(es).
[ View ]
#46 still_depends_on_1097100_3_weeks_later.patch4.45 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch still_depends_on_1097100_3_weeks_later_0.patch.
[ View ]
#46 stop_bleeding_please_d7.patch4.97 KBcatch
#42 still_depends_on_1097100_3_weeks_later.patch4.45 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch still_depends_on_1097100_3_weeks_later.patch.
[ View ]
#41 fixing_old_comments_while_rome_burns_d7.patch3.84 KBcatch
#41 fixing_old_comments_while_rome_burns-D8.patch1.27 KBcatch
#40 node-types-1017672-40.patch1006 bytesq0rban
PASSED: [[SimpleTest]]: [MySQL] 29,407 pass(es).
[ View ]
#35 node_types_D8.patch1.1 KBcatch
#30 node_types_D7.patch3.67 KBcatch
#27 1017672.patch1.13 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1017672_0.patch.
[ View ]
#26 1017672-D8.patch1.13 KBcatch
#25 1017672.patch4.65 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 31,609 pass(es).
[ View ]
#19 node_upgrade.patch329.36 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 31,561 pass(es), 29 fail(s), and 0 exception(es).
[ View ]
#16 node_upgrade.patch330.11 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 31,529 pass(es), 29 fail(s), and 0 exception(es).
[ View ]
#16 node_upgrade_test_only.patch328.03 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 31,519 pass(es), 30 fail(s), and 0 exception(es).
[ View ]
#14 node_types.patch2.35 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 31,545 pass(es), 25 fail(s), and 50 exception(es).
[ View ]

Comments

droplet’s picture

Category:support» bug
Priority:Normal» Major

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

catch’s picture

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.

federico’s picture

SELECT COUNT( * ) FROM comment = 2791
SELECT COUNT( * ) FROM comments = 2791 (drupal 6 DB)
SELECT COUNT( * ) FROM field_data_comment_body = 380

* I've made the update twice (same D6 database), with the same results.

* I've tried to manually find a pattern in the comments that made into the D7 database vs those that didn't:
- I noticed is that either all comments in a node are updated the right way, or no comment in a node is correctly upgraded.
- All comments in node type 'story' where updated OK, all comments in other node types ('blog' or 'page') weren't

* Update: I've tried to update a second site from D6 to D7, obtaining the same results. (2586 comments in D6 versus 164 comments with content in D7, 'story comments' updated, 'other comments' without field_data_comment_body).

federico’s picture

Title:Some comments empty after update» Disabling blog.module or forum.module before upgrade, causes the loss of comments in blog and forum nodes
Priority:Major» Normal

In my previous attempts, I've disabled all optional modules including "blog" and "forum", I've tried again letting those modules enabled before upgrade, and now I can see the comments after upgrade.

UPGRADE.txt says Disable all modules that are not listed under "Core - required" or "Core - optional". So it was my mistake.

Anyway, I don't know if this behavior is by design or if this is a bug. I think this is a bug because:

  1. There can be some situations where you temporary enable or disable a module, but you want to keep the data for the future. I usually enable the "poll" module for some days, but I want to keep the data for future reference. The same may apply for the comment.module . In my opinion, the comments on blogs and forum should be kept even if blog.module and forum.module are disabled. If I want to remove the data, I uninstall the modules, like UPGRADE.txt says "If you know that you will not re-enable some modules for Drupal 7.x and you no longer need their data, then you can uninstall them under the Uninstall tab after disabling them."
  2. If the comment content is removed (field_data_comment_body), the rest of the comment (title, date,author, etc.) should be also removed.
catch’s picture

Title:Disabling blog.module or forum.module before upgrade, causes the loss of comments in blog and forum nodes» Disabling modules that provide node types in D6 leads to data loss during Drupal 7 upgrade
Component:comment.module» node.module
Priority:Normal» Critical

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

catch’s picture

Issue tags:+Needs tests

This needs tests.

carlos8f’s picture

subscribing.

carlos8f’s picture

catch’s picture

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.

jpsoto’s picture

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

joachim’s picture

catch’s picture

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.

Leeteq’s picture

Subscribing.

catch’s picture

Status:Active» Needs work
StatusFileSize
new2.35 KB
FAILED: [[SimpleTest]]: [MySQL] 31,545 pass(es), 25 fail(s), and 50 exception(es).
[ View ]

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.

drumrwaldo’s picture

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.

catch’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new328.03 KB
FAILED: [[SimpleTest]]: [MySQL] 31,519 pass(es), 30 fail(s), and 0 exception(es).
[ View ]
new330.11 KB
FAILED: [[SimpleTest]]: [MySQL] 31,529 pass(es), 29 fail(s), and 0 exception(es).
[ View ]

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

Status:Needs review» Needs work

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

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new329.36 KB
FAILED: [[SimpleTest]]: [MySQL] 31,561 pass(es), 29 fail(s), and 0 exception(es).
[ View ]

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

aspilicious’s picture

Status:Needs review» Needs work

Test bot seems down?
Needs work?

catch’s picture

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.

catch’s picture

Issue tags:+D7 upgrade path

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.

catch’s picture

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

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new4.65 KB
PASSED: [[SimpleTest]]: [MySQL] 31,609 pass(es).
[ View ]

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.

catch’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7
StatusFileSize
new1.13 KB

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

catch’s picture

StatusFileSize
new1.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1017672_0.patch.
[ View ]

Bot!

Status:Needs review» Needs work

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

catch’s picture

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

catch’s picture

StatusFileSize
new3.67 KB

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.

q0rban’s picture

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.

q0rban’s picture

Status:Needs work» Needs review
q0rban’s picture

Status:Needs review» Reviewed & tested by the community

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.

catch’s picture

Status:Reviewed & tested by the community» Needs review

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.

catch’s picture

StatusFileSize
new1.1 KB

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)

catch’s picture

Title:Disabling modules that provide node types in D6 leads to data loss during Drupal 7 upgrade» D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors
q0rban’s picture

Status:Needs review» Reviewed & tested by the community

Looks great!

Dries’s picture

Dries’s picture

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

q0rban’s picture

Status:Needs work» Needs review
StatusFileSize
new1006 bytes
PASSED: [[SimpleTest]]: [MySQL] 29,407 pass(es).
[ View ]

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

catch’s picture

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.

catch’s picture

StatusFileSize
new4.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch still_depends_on_1097100_3_weeks_later.patch.
[ View ]

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

Status:Needs review» Needs work

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

Michelle’s picture

Status:Needs work» Needs review

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

Michelle’s picture

Status:Needs review» Needs work

Whoops, crossposted.

Michelle

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new4.97 KB
new4.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch still_depends_on_1097100_3_weeks_later_0.patch.
[ View ]

Updated comment via beejeebus.

Status:Needs review» Needs work

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

catch’s picture

Status:Needs work» Needs review

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.

catch’s picture

catch’s picture

StatusFileSize
new1.27 KB
PASSED: [[SimpleTest]]: [MySQL] 29,435 pass(es).
[ View ]

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

q0rban’s picture

Status:Needs review» Reviewed & tested by the community

Looks good!

Dries’s picture

Status:Reviewed & tested by the community» Fixed

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

xmacinfo’s picture

Version:8.x-dev» 7.x-dev
Status:Fixed» Reviewed & tested by the community

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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

webchick’s picture

Status:Fixed» Reviewed & tested by the community
StatusFileSize
new4.97 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

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.

Status:Reviewed & tested by the community» Needs work
Issue tags:-D7 upgrade path, -needs backport to D7

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

catch’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+D7 upgrade path, +needs backport to D7

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

webchick’s picture

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

catch’s picture

Status:Needs work» Needs review
Issue tags:-D7 upgrade path, -needs backport to D7

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.

Status:Needs review» Needs work
Issue tags:+D7 upgrade path, +needs backport to D7

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

catch’s picture

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

catch’s picture

Status:Needs work» Needs review
Issue tags:-D7 upgrade path, -needs backport to D7

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

Status:Needs review» Needs work
Issue tags:+D7 upgrade path, +needs backport to D7

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

catch’s picture

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.

Starminder’s picture

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.

Eric_A’s picture

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

catch’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.23 KB
PASSED: [[SimpleTest]]: [MySQL] 31,837 pass(es).
[ View ]

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

Starminder’s picture

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

catch’s picture

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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

Status:Fixed» Closed (fixed)

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

joachim’s picture

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

droplet’s picture

@joachim,
it's already in 7.2