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

EDIT (@diqidoq): fixd for D8, but not for D7, please read comments #97/#98 why this issue isn't fixed yet. Any thoughts on this are much appreciated.

CommentFileSizeAuthor
#116 1374090-116.patch3.57 KBmcdruid
#116 1374090-116_test_only.patch2.14 KBmcdruid
#116 interdiff-1374090-113-116.txt462 bytesmcdruid
#114 1374090-113_same_as_100.patch3.58 KBmcdruid
#114 1374090-113_test_only.patch2.17 KBmcdruid
#100 interdiff-1374090-58-100.txt467 byteslmeurs
#100 1374090-comment-creation-date-100.patch3.58 KBlmeurs
#58 1374090-comment-creation-date.patch3.19 KBdroplet
#54 1374090-comment-creation-date.patch3.19 KBdroplet
#50 d7-comment_creation_date-1374090-50.patch12 KBasirjacques
#45 d8-comment_creation_date-1374090-45.patch3.1 KBdroplet
#43 tests.patch1.7 KBdroplet
#43 d8-comment_creation_date-1374090-43.patch3.06 KBdroplet
#41 d8-comment_creation_date-1374090-41.patch1.37 KBdroplet
#36 d8-comment_creation_date-1374090-36.patch1.51 KBG.I.Joe
#35 d8-comment-creation_date-1374090-35.patch1.68 KBG.I.Joe
#33 comment-creation_date-1374090-33.patch1.64 KBG.I.Joe
#28 comment-creation_date-1374090-28.patch2.94 KBbrenda003
#26 comment-creation_date-1374090-26.patch2.94 KBbrenda003
#23 d8-comment_date-1374090-23.patch2.8 KBmarthinal
#21 d8-comment_date_new_comment-1374090-21.patch2.98 KBmarthinal
#14 comment-creation_date-1374090-14.patch2.14 KBjfhovinne
#7 comment_date.patch1.85 KBdroplet
#5 1374090-5.patch1.13 KBPancho
#3 1374090-3.patch1.15 KBPancho
#1 1374090.patch1.13 KBPancho
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

Issue summary: View changes

Expanded issue to cover both cases

Pancho’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

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

FileSize
1.15 KB

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
FileSize
1.13 KB

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
FileSize
1.85 KB

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
FileSize
2.14 KB

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
FileSize
2.98 KB

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
FileSize
2.8 KB

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
FileSize
2.94 KB

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
FileSize
2.94 KB

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
FileSize
1.64 KB

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
FileSize
1.68 KB

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

G.I.Joe’s picture

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.

Anonymous’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
FileSize
1.37 KB

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

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
FileSize
3.1 KB

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

asirjacques’s picture

Status: Patch (to be ported) » Needs review
FileSize
12 KB

Back port from Drupal 8 to Drupal 7.

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

asirjacques’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.

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
FileSize
3.19 KB
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

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.

opdavies’s picture

Status: Needs review » Reviewed & tested by the community

mgifford’s picture

@opdavies - no problem with the RTBC, I just wanted the bots to run on it again as it's been over a year.

opdavies’s picture

@mgifford - No problem, although if it's RTBC, it'll be retested automatically as per https://qa.drupal.org/node/228. :)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Nope, Drupal 7 RTBC patches are no longer retested automatically (only Drupal 8).

So... if everyone who has replied to my comment in #87 agrees with it, why is this back to RTBC? I think we need a spin-off issue to fix that in Drupal 8, and am very wary of introducing that problem in Drupal 7.

dqd’s picture

@97 / @87: I agree with the potential trouble making change to discuss, especially when users find out about the fix surprisingly after updating core and the comment dates have changed back to the $created date without warning. But the main problem of this issue persists: This years old (mis)behavior is not well seated there, nor as an "option" nor as a temporary work-around. It is badly forced, and this by using the wrong place (variable name $created says it all). Nor has it any description or explanation somewhere for why the value of a variable called $created will change its value in any way(?). What definitely renders this behavior to a bug, standalone and no matter what the use case is.

Especially from the site admin's point of view, in the very moment, when (maybe too late) realizing that - while the creation date on admins comment-edits stays untouched like expected from a printed variable named $created and while another printable value and variable called $changed coexists - but on user comment-edits, the value from the $created variable confusingly changes (updates). It wouldn't surprise me when a lot of users didn't even have noticed this behavior @ their Drupal installations at first time. It is definitely unexpected.

While I see the trouble in silently fixing it, fixing this in any possible way is deeply needed. We can't get away with like it is: the $created date value changes while the $changed date variable coexist for this purpose. This has chance to become a #WTF, because if it is wanted to show the date of when a comment of a web user has been updated for many understandable reasons, then why the heck the printed $changed variable shouldn't do this well enough? It does it very perfectly and is made for that. To flip their behavior, a checkbox would have been enough when it is unwanted to show both values.

A suggested solution by watching such scenarios at other cases: Since many themes and core have mostly only $created in the comment.tpl.php by default these days in D7 world and a core update can't make new variables suddenly and magically printed in themes beside $created, nor can we change its behavior unwarned, we rather should think about a backport contrib module for that option, like we often did this for other features backported from D8. Usually the project page explains the backport module and makes it findable by google and leaves the decision by the user if to implement this (correct) behavior or not. The goal should be: leave the $created date alone. That's the behavior what the name of that variable suggests.

Again. Let's not wait another year, it absolutely makes no sense that we have the two printable dates $created and $changed available for comments while we treat the $created date value under certain undocumented circumstances like the $changed date.

A good start to soften the issue would be to add an additional code comment line in the root/modules/comment/comment.tpl.php line 15-16 with sth. like this to fix the missing info problem:

To make sure that later user comment changes get recognized without $changed variable printed in theme, this value may change when non admin users with the permission to edit own comments edit their comment. (Additionally in case of:) Please consider Comment Advanced Settings module (e.g.) to withdraw this behavior.

(Any better text ideas are welcome)

lokapujya’s picture

OK, so create a a new new issue?

lmeurs’s picture

The patch from #58 does solve the issue in our case: whenever an admin or regular user updates a comment, only the changed date gets updated.

But another problem occurred: for new comments the created date sometimes differed from the changed date. The problem seemed to be that the changed date is based on REQUEST_TIME, but the created date not. We updated the patch from #58 to fix this, see the interdiff.

dqd’s picture

Issue summary: View changes

@lmeurs: did you read the comments #97/#98? Just to be sure that you realize some other behaviour coming in with this patch. David_Rothstein is right, we actually rather need another discussion about how to finally solve this in D7. Sadly this is an old minor issue of an old Drupal core version, which wont get much love no more, I guess. All I can suggest is, to try to soften the issue with my advices on the end of comment #98 and to hope that a fast new idea arises to kick that dirt off the code, since no one will discuss this here for longer than another minute, I think.

  • webchick committed 945efa0 on 8.3.x
    Issue #1374090 by droplet, Pancho, G.I.Joe, marthinal, brenda003,...

  • webchick committed 945efa0 on 8.3.x
    Issue #1374090 by droplet, Pancho, G.I.Joe, marthinal, brenda003,...
stefan.r’s picture

Issue tags: -Novice
izmeez’s picture

I agree with @diqidoq in comment #98 this is incorrect behaviour for Drupal 7 and is deserving of a fix.

Maybe a new issue should be opened to address this.

Maybe, if there is concern about adding the fix to D7 core would it be a suitable item to add to another module similar to the old https://www.drupal.org/project/advanced_comment for D6.

Personally, I think this behaviour warrants a fix in core even if it causes disruption like the missing modules change introduced in D7.50 so that themes that need fixing can be corrected.

TD44’s picture

If a non-admin edit a comment then the comment created date is set to now. It changes the order of comments are displayed.
Why this is not commited to core?
Thousands websites have this issue and no one cares?
W
T
F

  • webchick committed 945efa0 on 8.4.x
    Issue #1374090 by droplet, Pancho, G.I.Joe, marthinal, brenda003,...

  • webchick committed 945efa0 on 8.4.x
    Issue #1374090 by droplet, Pancho, G.I.Joe, marthinal, brenda003,...
dqd’s picture

Thanks to droplet, Pancho, G.I.Joe, marthinal, brenda003, jfhovinne, webchick for D8.

So seeing the #needs-backport-to-D7 tag and looking at the issue settings saying version D7 dev, we can go on with patching for D7.

TD44’s picture

Ofc we need patching for D7.
Thanks bro

matt_c’s picture

This is definitely a bug, just discovered it when I noticed updated comments were moving around in a view. +1 for fixing this in core 7.x.

drumm’s picture

Re-affirming this affects Drupal.org. I’ll be deploying the patch from #100, which looks okay.

Our issue fork & merge request integration expects time to move forward. Comments out of chronological order is not something I ever expected. Losing the original comment created time is not good at all.

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.

To me, this trade-off is a no-brainer. The comment created time should be when the comment was created.

izmeez’s picture

Added reference to the patch in #100 to #3259739: [meta] Priorities for 2022-06-01 release of Drupal 7. The related issue #2288865: Same comment creation & changed date may no longer be relevant considering the interdiff with the patch in comment #100.

mcdruid’s picture

Same as patch #100 but with a test-only version to verify the behaviour change.

The last submitted patch, 114: 1374090-113_test_only.patch, failed testing. View results

mcdruid’s picture

This is pretty minor but the final logout in the test is redundant, so we might as well save a few http requests.

For review: noting that in #112 @drumm confirmed this patch was being deployed to d.o

This will need a CR.

The last submitted patch, 116: 1374090-116_test_only.patch, failed testing. View results

drumm’s picture

For review: noting that in #112 @drumm confirmed this patch was being deployed to d.o

Yes, this has been running since April 13 without any known issues, https://bitbucket.org/drupalorg-infrastructure/drupal.org/commits/295b07...

Fabianx’s picture

Assigned: Unassigned » mcdruid
Status: Needs review » Reviewed & tested by the community

RTBC + 1, approved for Merge! Thanks all!

  • mcdruid committed 0d75dbb on 7.x
    Issue #1374090 by droplet, mcdruid, Pancho, G.I.Joe, marthinal,...
mcdruid’s picture

Assigned: mcdruid » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7, -Pending Drupal 7 commit, -Needs change record

Added the CR.

Thanks everyone, it's great to finally get this fixed!

Status: Fixed » Closed (fixed)

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