Whenever a comment is edited, the "created" timestamp still gets changed in two ways:

a) admin edit
While #1005004: Editing a comment destroys its creation date makes sure it isn't completely overwritten, it still gets rounded down to the full minute.
This is because the "Authored on" field in the Admin section only takes H:i (hh:mm), and this value gets saved to the record, whether it has been changed or not. This won't be a big thing in most cases, but we never know - it's simply not correct.
Probably the "Authored on" field should take H:i:s (hh:mm:ss), which seems the cleanest way and is consistent to the node edit form.

b) user edit
The second part of #714958: Comment timestamp lost when edited by administrator wasn't fixed either:

When a non-admin edits a comment, the "created" date is always set to now.

For non-admins the "Administration" section stays hidden. However, the "Authored on" field is fed with an empty string, which upon saving is automatically replaced by now().
Instead, the created date should be given, if existing (it obviously doesn't exist for new, unsaved comments).

Comments

Pancho’s picture

Issue summary:View changes

Expanded issue to cover both cases

Pancho’s picture

Status:Needs work» Needs review
StatusFileSize
new1.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1374090.patch. See the log in the details link for more information.
[ View ]

Here's a patch that should fix both parts of the bug.

Status:Active» Needs work

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

Pancho’s picture

StatusFileSize
new1.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1374090-3.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Sorry, malformatted for the testbot. This one should work, I hope.

Status:Needs review» Needs work

The last submitted patch, 1374090-3.patch, failed testing.

Pancho’s picture

Status:Needs work» Needs review
StatusFileSize
new1.13 KB
FAILED: [[SimpleTest]]: [MySQL] 37,326 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Again malformed. Will it work this time?

Status:Needs review» Needs work

The last submitted patch, 1374090-5.patch, failed testing.

droplet’s picture

Version:7.x-dev» 8.x-dev
Status:Needs work» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new1.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment_date.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

try this, bump up to D8 as well.

Pancho’s picture

THX for fixing the patch and sorry for all the clutter. I'm going to test it as soon as I find some time.

xanderol’s picture

I tried this patch on drupal 7.15 which still has the bug and it seems to solve the problem.

droplet’s picture

Issue tags:-needs backport to D7

#7: comment_date.patch queued for re-testing.

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

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

droplet’s picture

Issue tags:+Novice

tagging Novice, anyone willing to reroll a patch, thanks.

jfhovinne’s picture

Status:Needs work» Needs review
StatusFileSize
new2.14 KB
PASSED: [[SimpleTest]]: [MySQL] 41,251 pass(es).
[ View ]

Patch for d8.

cachee’s picture

The following test was performed:

1. Downloaded the latest copy of D8 and built a site
2. Added content and initial comments
3. Added a comment then updated the comment - Saw the error of the created date changed
Comment update before the patch
4. Applied the patch
5. Updated a previous comment - Saw that the created date did not change
comment after the patch
6. It appears that the user's comment update does not change the create date of the comments.

Note: I was not able to recreate the Admin error to verify that the patch corrected that issue.

ofauravi’s picture

Status:Reviewed & tested by the community» Needs review

Ok, patch works!

rodrigoaguilera’s picture

Status:Needs review» Reviewed & tested by the community

Works without patch for me too

ofauravi’s picture

Status:Needs review» Reviewed & tested by the community

#17, with admin user always works, the patch applied to the non-admin user.

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests
+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -64,7 +64,6 @@ class CommentFormController extends EntityFormController {
-      $date = (!empty($comment->date) ? $comment->date : format_date($comment->created, 'custom', 'Y-m-d H:i O'));

@@ -74,7 +73,11 @@ class CommentFormController extends EntityFormController {
+      $date = (!empty($comment->date) ? $comment->date : format_date($comment->created, 'custom', 'Y-m-d H:i:s O'));

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentPreviewTest.php
@@ -85,7 +85,7 @@ class CommentPreviewTest extends CommentTestBase {
-    $expected_form_date = format_date($raw_date, 'custom', 'Y-m-d H:i O');
+    $expected_form_date = format_date($raw_date, 'custom', 'Y-m-d H:i:s O');

Hm. Can you explain why you're changing the date format? I think seconds granularity seems a little over the top to me. :) I'd rather retain the old version, unless there's a compelling reason not to do that.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -74,7 +73,11 @@ class CommentFormController extends EntityFormController {
+    if(!empty($comment->cid)) {

(nitpick) There should be a space between "if" and the "(" - See Coding standards

Also, let's make sure we add an automated test for this behaviour so we don't accidentally break it again.

droplet’s picture

@#19,

DB saving a UNIX timestamp and front UI using human-readable format:

Authored on: 2012-10-07 00:57 +0800

Every SECOND will cause a timestamp changes.

To reviewer:
Please wait 2 or 3 mins between each comment editing, or check the DB directly. It's a bug for all user-levels

marthinal’s picture

Status:Needs work» Needs review
StatusFileSize
new2.98 KB
PASSED: [[SimpleTest]]: [MySQL] 42,084 pass(es).
[ View ]

Reviewing the patch I verify that it works as expected. So when a user edits his comment for example, the date and time is the same as in the date of comment creation. At the same time I see that when we edit the comment, we have a new comment.

If a comment is edited, we never know that there's another new comment or one comment modified.

I add a patch with the previous line(patch) and also some code to verify that we have a new comment when a new one is edited. If we have a new one ... and we edit this comment ... only 1 new comment is shown, not 2.

If you think this is an interesting feature, I could open a new issue or maybe change the title and fix this in the same issue...

Also I could add the test.

marthinal’s picture

Assigned:Unassigned» marthinal
Status:Needs review» Needs work

About this issue test I think the problem was that we test using admin user and we need to try using web user.

So:

1) I will check if there's another feature request for d8 like added in the previous patch.

2) If not I'll create another issue with the feature.

3) I'll check the actual test and modify to test correctly with admin and web user.

marthinal’s picture

Status:Needs work» Needs review
StatusFileSize
new2.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-comment_date-1374090-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch + test

wryz’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests, +Novice, +needs backport to D7

The last submitted patch, d8-comment_date-1374090-23.patch, failed testing.

brenda003’s picture

Status:Needs work» Needs review
StatusFileSize
new2.94 KB
FAILED: [[SimpleTest]]: [MySQL] 52,213 pass(es), 7 fail(s), and 494 exception(s).
[ View ]

Patch rerolled.

Status:Needs review» Needs work

The last submitted patch, comment-creation_date-1374090-26.patch, failed testing.

brenda003’s picture

Status:Needs work» Needs review
StatusFileSize
new2.94 KB
FAILED: [[SimpleTest]]: [MySQL] 52,592 pass(es), 7 fail(s), and 493 exception(s).
[ View ]

Patch rerolled.

Status:Needs review» Needs work

The last submitted patch, comment-creation_date-1374090-28.patch, failed testing.

droplet’s picture

Test case on #7. doesn't it enough ?

droplet’s picture

Assigned:marthinal» Unassigned

@marthinal, are you still work on it ? :)

G.I.Joe’s picture

Assigned:Unassigned» G.I.Joe

If you don't mind I will take over this issue.

G.I.Joe’s picture

Status:Needs work» Needs review
StatusFileSize
new1.64 KB
PASSED: [[SimpleTest]]: [MySQL] 58,582 pass(es).
[ View ]

It's was not possible to reroll the previous patch.
So, I created a new patch.

pfrenssen’s picture

Status:Needs review» Needs work

The patch contains some weird characters and I'm unable to apply it:

$ patch -p1 < comment-creation_date-1374090-33.patch
patch: **** Only garbage was found in the patch input.
$ git apply comment-creation_date-1374090-33.patch
fatal: unrecognized input

Strange that the test bot didn't complain. Can you perhaps reroll the patch?

G.I.Joe’s picture

Status:Needs work» Needs review
StatusFileSize
new1.68 KB
PASSED: [[SimpleTest]]: [MySQL] 59,396 pass(es).
[ View ]

It's was not possible to reroll the previous patch
So, I created a new patch.

G.I.Joe’s picture

StatusFileSize
new1.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,915 pass(es).
[ View ]

The patch contained color characters.

Please, try following patch.

droplet’s picture

Status:Needs review» Needs work

All few reroll, all tests have been gone.

pfrenssen’s picture

Indeed, there were some tests in the patch from #28 and these have been lost in the reroll.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.phpundefined
@@ -110,7 +110,7 @@ public function form(array $form, array &$form_state) {
-      $date = (!empty($comment->date) ? $comment->date : DrupalDateTime::createFromTimestamp($comment->created->value));
+      $date = (!empty($comment->date) ? $comment->date : (isset($comment->created->value) ? DrupalDateTime::createFromTimestamp($comment->created->value) : ''));

Nested ternaries are hard to read, maybe you can split it in two lines?
Also, as a minor remark, it is not needed to wrap the entire line in parentheses. This was present in the original code but it can be cleaned up now.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.phpundefined
@@ -125,7 +125,7 @@ public function form(array $form, array &$form_state) {
-      $date = '';
+      $date = (!empty($comment->date) ? $comment->date : (isset($comment->created->value) ? DrupalDateTime::createFromTimestamp($comment->created->value) : ''));
     }

Same remark as above: split nested ternaries, and remove unneeded parentheses.

stenou’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

I was looking into the missing tests from patch #28 and comparing it to the current drupal code. I may be mistaken but it looks like some tests were already added to make sure the created date is the same as before it was edited.

// Check that the saved comment is still correct.
$comment_loaded = comment_load($comment->id(), TRUE);
$this->assertEqual($comment_loaded->subject->value, $edit['subject'], 'Subject loaded.');
$this->assertEqual($comment_loaded->comment_body->value, $edit['comment_body[0][value]'], 'Comment body loaded.');
$this->assertEqual($comment_loaded->name->value, $edit['name'], 'Name loaded.');
$this->assertEqual($comment_loaded->created->value, $raw_date, 'Date loaded.');

droplet’s picture

I'd say #7 test / first part of #28 test is good enough for this change.

droplet’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.37 KB
PASSED: [[SimpleTest]]: [MySQL] 58,735 pass(es).
[ View ]

OK. D8 changed the tests, now it has already caught seconds:

    $expected_form_date = $date->format('Y-m-d');
    $expected_form_time = $date->format('H:i:s');
G.I.Joe’s picture

Status:Needs review» Reviewed & tested by the community

Well done, Paljas.
I reviewed the code and fix solved issue.

droplet’s picture

StatusFileSize
new3.06 KB
PASSED: [[SimpleTest]]: [MySQL] 58,767 pass(es).
[ View ]
new1.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,412 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Sorry. I just read this thread again. We need a test case for non-admin users.

pfrenssen’s picture

Issue tags:-Needs tests
  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
    @@ -125,7 +124,11 @@ public function form(array $form, array &$form_state) {
    +      $date = (!empty($comment->date) ? $comment->date : DrupalDateTime::createFromTimestamp($comment->created->value));

    It is not needed to wrap a ternary operator in parentheses.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentPreviewTest.php
    @@ -164,6 +164,15 @@ function testCommentEditPreviewSave() {
    +    // Check that the date and time of the comment are correct when edited by non-admin users.

    Comment exceeds 80 characters.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentPreviewTest.php
    @@ -164,6 +164,15 @@ function testCommentEditPreviewSave() {
    +    $this->drupalLogout();
    +    $user_edit = array();
    +    $expected_created_time = $comment_loaded->created->value;
    +    $this->drupalLogin($web_user);
    +    $this->drupalPostForm('comment/' . $comment->id() . '/edit', $user_edit, t('Save'));

    I would put the $this->drupalLogout() and $this->drupalLogin() next to eachother.

    The $user_edit array is only used in one line so it can be replaced with an inline array() in $this->drupalPostForm().

  4. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentPreviewTest.php
    @@ -164,6 +164,15 @@ function testCommentEditPreviewSave() {
    +    $comment_loaded = comment_load($comment->id(), TRUE);
    +    $this->assertEqual($comment_loaded->created->value, $expected_created_time, 'Expected date and time for comment edited.');

    I would use $comment rather than $comment_loaded.

droplet’s picture

Status:Needs work» Needs review
StatusFileSize
new3.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,996 pass(es).
[ View ]

Although I think only #2 need to fix, just make extra more changes in this patch to fix everyone needs. :)

#4,
$comment is the original comment object.
$comment_loaded is a new comment object.

We also keep it same to previous usage:

    // Check that the saved comment is still correct.
    $comment_loaded = comment_load($comment->id(), TRUE);

Thanks.

pfrenssen’s picture

Status:Needs review» Reviewed & tested by the community

Makes sense. It's looking good now, thanks!

webchick’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Awesome work, on both the find and the fix!

Committed and pushed to 8.x. Thanks!

Moving to 7.x backport.

webchick’s picture

Issue summary:View changes

minor stylefix

andypost’s picture

tvn’s picture

Lynxas’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new12 KB
PASSED: [[SimpleTest]]: [MySQL] 40,717 pass(es).
[ View ]

Back port from Drupal 8 to Drupal 7.

I had to change the original code that was into the Drupal 8 patch.

Lynxas’s picture

Status:Needs review» Needs work

wrong patch and still working on the issue

droplet’s picture

Assigned:G.I.Joe» Unassigned
mgifford’s picture

@Lynxas can you re-roll d7-comment_creation_date-1374090-50.patch it seems to be filled with a bunch of garbage characters.

droplet’s picture

Status:Needs work» Needs review
StatusFileSize
new3.19 KB
FAILED: [[SimpleTest]]: [MySQL] 39,777 pass(es), 1,448 fail(s), and 844 exception(s).
[ View ]

let me backport my D8 patch.

Status:Needs review» Needs work

The last submitted patch, 54: 1374090-comment-creation-date.patch, failed testing.

droplet’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 54: 1374090-comment-creation-date.patch, failed testing.

droplet’s picture

Status:Needs work» Needs review
StatusFileSize
new3.19 KB
PASSED: [[SimpleTest]]: [MySQL] 40,919 pass(es).
[ View ]
mpv’s picture

I will be testing this patch now.

mpv’s picture

Status:Needs review» Reviewed & tested by the community

The patch for D7 is working for me.

dcam’s picture

Marked #2258527: Comment edit destroys created date as a duplicate issue.

lokapujya’s picture

Just to clarify, Does the D7 patch only fix b.) in the issue summary?

droplet’s picture

Fix all problems.

rob.barnett’s picture

The patch works for me, however, when a comment first gets created the changed field is populated with the unix timestamp. This does not always match up to the created field that gets populated. In my Drupal instances it is often a second off. Is populating the changed field when a comment first gets created by design? I'd rather it be left empty and only get populated when a comment is edited since it is changing only then.

droplet’s picture

@hurley,

nice caught, here's the patch: #2288865: Same comment creation & changed date

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 58: 1374090-comment-creation-date.patch, failed testing.

droplet’s picture

Status:Needs work» Needs review
andypost’s picture

Status:Needs review» Reviewed & tested by the community

back to rtbc

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 58: 1374090-comment-creation-date.patch, failed testing.

Status:Needs work» Needs review
dcam’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 58: 1374090-comment-creation-date.patch, failed testing.

Status:Needs work» Needs review
dcam’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 58: 1374090-comment-creation-date.patch, failed testing.

Status:Needs work» Needs review
dcam’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 58: 1374090-comment-creation-date.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 58: 1374090-comment-creation-date.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 58: 1374090-comment-creation-date.patch, failed testing.

lokapujya’s picture

Status:Needs work» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 58: 1374090-comment-creation-date.patch, failed testing.

Status:Needs work» Needs review
dcam’s picture

Status:Needs review» Reviewed & tested by the community
David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs review

This patch introduces a rather large behavior change which doesn't seem to have been discussed: Non-admin users get the ability to edit their old comments without the date being updated, and therefore there is no visible indicator to everyone else of when the new version was actually written.

See @salvis's comment at #714958-12: Comment timestamp lost when edited by administrator (in the issue linked to from the summary here) for why that might be a bad idea...

This definitely needs more discussion for Drupal 7, and perhaps should be bumped back for more discussion in Drupal 8 also?

andypost’s picture

David, your concerns are valid!

I think we need separate issue for d8 - #2227503: Apply formatters and widgets to Comment base fields
1) changed field already set to NOW in field preSave() so code needs clean-up
2) the change of created field (Authored on) is the same as we have for node, so consistent
but only admins should be allowed to do that

So let's leave this for d7 only, with node's behavior
1) new comment get's created, change to NOW
2) any comment edit changes it's "changed" to NOW
3) admin is allowed to change "created" via "Authored on" field

rob.barnett’s picture

Forgive me if I'm misunderstanding how this stands currently with D7. I noticed the following change in 7.27.

diff --git a/docroot/modules/comment/comment.module b/docroot/modules/comment/comment.module
index cecea45..3c94200 100644
--- a/docroot/modules/comment/comment.module
+++ b/docroot/modules/comment/comment.module
@@ -1914,6 +1914,7 @@ function comment_form($form, &$form_state, $comment) {
   if ($is_admin) {
     $author = (!$comment->uid && $comment->name ? $comment->name : $comment->registered_name);
     $status = (isset($comment->status) ? $comment->status : COMMENT_NOT_PUBLISHED);
+    $date = (!empty($comment->date) ? $comment->date : format_date($comment->created, 'custom', 'Y-m-d H:i O'));
   }
   else {
     if ($user->uid) {
@@ -1923,11 +1924,7 @@ function comment_form($form, &$form_state, $comment) {
       $author = ($comment->name ? $comment->name : '');
     }
     $status = (user_access('skip comment approval') ? COMMENT_PUBLISHED : COMMENT_NOT_PUBLISHED);
-  }
-
-  $date = '';
-  if ($comment->cid) {
-    $date = !empty($comment->date) ? $comment->date : format_date($comment->created, 'custom', 'Y-m-d H:i:s O');
+    $date = '';
   }

In short, it used to make the the $comment->date a formatted version of $comment->created no matter the user if a comment is getting updated. The new changes only creates a formatted version of the $comment->created if the current user is an admin. Otherwise $comment->date is empty.
In function comment_submit if $comment->date is empty then it becomes 'now' as does $comment->created. So created date gets changed to current date/time if logged in as a non-admin user.

Why would we want to change the created date? Shouldn't that always remain the same. The changed date should always change if a comment is updated.

I agree with David_Rothstein there should be a visible indicator to everyone else of this regardless of whether the currently logged in user is an admin or non-admin.

le72’s picture

Status:Needs review» Fixed

Sorry

le72’s picture

Status:Fixed» Needs review
opdavies’s picture

Patch in #58 still applies cleanly to 7.x-dev and prevents the created date from changing.