Editing a comment destroys its creation date

Here's the test that shows the problem.

Here's the patch that fixes the problem. It doesn't include the test patch above, so they both need to be applied.

The fix has two parts, because the test fails for two reasons.

First part. Testing for $comment->date is not correct here, as that's the name of the form field, not the database field.

-    $date = (!empty($comment->date) ? $comment->date : format_date($comment->changed, 'custom', 'Y-m-d H:i O'));
+    $date = (!empty($comment->created) ? format_date($comment->created, 'custom', 'Y-m-d H:i O') : format_date($comment->changed, 'custom', 'Y-m-d H:i O'));

Second part. Saving the comment is timezone-sensitive, making the test fail if you're in any timezone other than +0300. These changes force the system to be UTC for this test, so that the field doesn't change unexpectedly:

+    variable_set('date_default_timezone', 'UTC');
-    $edit['date'] = '2008-03-02 17:23 +0300';
-    $expected_date = format_date(strtotime($edit['date']));
+    $edit['date'] = '2008-03-02 17:23 +0000';
+    $expected_date = format_date(strtotime($edit['date']), 'medium', '', 'UTC');

Cool, testbot actually ran the test in #1. Here's a patch that combines #1 and #2, should still be green...

Here's where the bug came from; it's not present in d6: #537062: Remove unneeded comment_edit() page callback

Thought about this and realized that this bug causes a type of data loss. If we add one more "save" step to the test then the bad date would be written to the database.

Setting this to major as it's a regression from d6, and it does cause data corruption (wrong date written to database) 100% of the time a comment is edited.

Thanks for looking at this. I agree that it makes sense to do:

  $expected_form_date = format_date(strtotime($edit['date']), 'custom', 'Y-m-d H:i O');

instead of messing with the timezone. On my machine that results in the field looking like "2008-03-02 06:23 -0800" rather than the "2008-03-02 17:23 +0300" that was input, but they are equivalent. With my way, the input value of "2008-03-02 06:23 -0000" is exactly what you see if you look at the loaded form, i.e. there's no timezone conversion. But, the system needs to take care of timezones anyway, and this change is simpler.

    $date = (!empty($comment->date) ? $comment->date : format_date($comment->created, 'custom', 'Y-m-d H:i O'));

That makes more sense too. I don't know if $comment->date is ever really non-empty here, but yeah there's no point in pre-filling the field with $comment->changed in any case.

I've beefed up the tests to check the saved/loaded comment data against what it was originally created with. But I'm confused because the subject test should pass but it fails, and the date test should fail but it passes. Is this because comment_load($comment->id) isn't really loading the comment from the database?

The attached "fix" patch is good (same as above) but that subject test will still fail as we're not trying to fix that..

+    // Check that the comment data is correct after submitting the comment edit form with a small change.
+    $edit['subject'] = $edit['subject'] . ' edited';
+    $this->drupalPost('comment/' . $comment->id . '/edit', $edit, t('Save'));
+    $comment_loaded = comment_load($comment->id);
+    $this->assertEqual($comment_loaded->subject, $edit['subject'] . ' edited', t('Subject loaded.'));
+    $this->assertEqual($comment_loaded->comment_body[$langcode][0]['value'], $edit['comment_body[' . $langcode . '][0][value]'], t('Comment body loaded.'));
+    $this->assertEqual($comment_loaded->name, $edit['name'], t('Name loaded.'));
+    $this->assertEqual($comment_loaded->created, $raw_date, t('Date loaded.'));

Doh, I see the trouble with $edit['subject'] = $edit['subject'] . ' edited';. Silly mistake.

But I'm still trying to understand why the final date test is passing in the tests-only patch; when I go through the sequence manually the date does change, which means the test should fail, since we just saved the comment while it was displaying the wrong date in the edit form.

In other words if the first test below fails, then we save the comment, then the second test below should fail as well, but it doesn't.

+    $this->assertFieldByName('date', $expected_form_date, t('Date field displayed.'));
[...]
+    $this->drupalPost('comment/' . $comment->id . '/edit', $edit, t('Save'));
[...]
+    $this->assertEqual($comment_loaded->created, $raw_date, t('Date loaded.'));

@bfroehle thanks, I just realized that -- $edit['date'] wasn't getting picked up from the form like I assumed. I'd like the test to demonstrate that saving the form in the mis-generated state results in bad data in the database. Does that make sense?

Is there a command for reading the value of the form field that's actually displayed? E.g. I want to add a step like "$edit['date'] = DrupalGetFormField('date')" before saving the form.

$this->xpath() is what I was looking for..

Here's the revised test. It includes a section that uses $this->xpath() to retrieve the form fields from the page and then saves the comment using those values. So, the test-only patch will fail twice: once for displaying the wrong date after loading the comment, and once for saving that date to the database.

The fix causes the form to display the correct date, which causes the the correct date to get into the database when the form is saved.

Same thing but with the comment updated to reflect the renamed function (testCommentEditPreviewSave()).