Converting comments between core and nodecomment looses file attachments. Fails converting back to core.

@merlinofchaos
Your suggestion helps - the conversion goes OK with disabled comment_upload.module. It is not a very robust solution though. In addition, conversion process looses all files attached to the comments, which is very destructive.

I will use this issue to track my progress on it, since I have few ideas on how to make it work.

First, I dug a little bit into the code, and made progress on plugging the error without disabling comment_upload.module. I figured that comment_upload.module stores files info as arrays, while upload.module stores files info as objects. I fixed that within the conversion process with two small pieces of code. (No patch yet - this code is not complete. just for illustration)

in function nodecomment_convert_batch_process() / file nodecomment.convert.inc add this:

	  // Make comment_upload.module happy - convert each file entry to array instead of object.
      if (isset($comment->files) && count($comment->files)) {
        $new_files = array();
        foreach ($comment->files as $key => $file) {
          $new_files[$key] = (array)$file;
        }
        $comment->files = $new_files;
      }

before this:

      // Insert the new comment.
      $edit = (array) $comment;
      db_query("INSERT INTO {comments} (nid, pid, uid, subject, comment, format, hostname, timestamp, status, thread, name, mail, homepage) VALUES (%d, %d, %d, '%s', '%s', %d, '%s', %d, %d, '%s', '%s', '%s', '%s')", $edit['nid'], $edit['pid'], $edit['uid'], $edit['subject'], $edit['comment'], $edit['format'], $edit['hostname'], $edit['timestamp'], $edit['status'], $edit['thread'], $edit['name'], $edit['mail'], $edit['homepage']);

and add this:

	  // Make upload.module happy - convert each file entry to object instead of array.
      if (isset($node->files) && count($node->files)) {
        $new_files = array();
        foreach ($node->files as $key => $file) {
          $new_files[$key] = (object)$file;
        }
        $node->files = $new_files;
      }

before this:

      node_save($node);

Next, I will need to figure out how to deal with:

      node_invoke($node, 'delete');
      node_invoke_nodeapi($node, 'delete');

and

        comment_invoke_comment($item, 'delete');

so that the files are not erased but transferred to new comment format.

But that's for a later session...

I rolled a patch against latest -dev from CVS.

It works with the attached patch: file attachments are carried over from core comments/comment_upload.module to nodecomments/upload.module and back.

Please review for committing to 6.x.-2.x-dev.

nag nag

Any chance of this being reviewed?

nag nag. Happy New Year!

"Problem is I use neither upload nor comment upload so it will take some effort to setup test environment"
I feel your lack of enthusiasm, and appreciate the time you will spend with this.

My reasons for critical:

1. comment_upload is "alpha" since 1 year ago, with almost 5000 reported users (http://drupal.org/project/usage/comment_upload)... I don't think it is an alpha - looks like very stable module. The only critical open bugs are PHP5.3 and Views2 integration (both with patches in review). There are many "released" modules out there with tons of critical bugs (check out pages full of views critical issues: http://drupal.org/project/issues/search/views?text=&assigned=&submitted=...).

2. Looses data without any warning. It is critical to everyone affected. Even with backups. If I remember correctly, the code in nodecomment conversion diligently wiped out comment file uploads. So it did care of contrib metadata, just in a wrong way.

3. One of very few reasons to use nodecomment module is to be able to attach files and filefields to comments. Given that nodecomment is new, the people trying it most likely had some solution in place, like... comment_upload. If you think of it, these 5000 users have a lot of data on their websites that would be lost during conversion and would require a lot of manual labor to convert. Either warn them of it LOUDLY in the documentation and on the conversion form, or commit the fix.

4. <700 users of nodecomment in a year? It would grow fast if this issue is fixed. For one, I was not able to use it before the fix. I had a live site that I wanted to move to advanced_forum, and nodecomment is (still) a requirement.

My strong opinion is that nodecomment is the right way to go conceptually, and core comment is a mistake. So I support development and use of nodecomment. It just needs few touches to get going and grow users support behind it.