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.
Comments
Comment #0.0
PanchoExpanded issue to cover both cases
Comment #1
PanchoHere's a patch that should fix both parts of the bug.
Comment #3
PanchoSorry, malformatted for the testbot. This one should work, I hope.
Comment #5
PanchoAgain malformed. Will it work this time?
Comment #7
droplet CreditAttribution: droplet commentedtry this, bump up to D8 as well.
Comment #9
PanchoTHX for fixing the patch and sorry for all the clutter. I'm going to test it as soon as I find some time.
Comment #10
xanderol CreditAttribution: xanderol commentedI tried this patch on drupal 7.15 which still has the bug and it seems to solve the problem.
Comment #11
droplet CreditAttribution: droplet commented#7: comment_date.patch queued for re-testing.
Comment #13
droplet CreditAttribution: droplet commentedtagging Novice, anyone willing to reroll a patch, thanks.
Comment #14
jfhovinne CreditAttribution: jfhovinne commentedPatch for d8.
Comment #15
cachee CreditAttribution: cachee commentedThe 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
4. Applied the patch
5. Updated a previous comment - Saw that the created date did not change
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.
Comment #16
ofauravi CreditAttribution: ofauravi commentedOk, patch works!
Comment #17
rodrigoaguileraWorks without patch for me too
Comment #18
ofauravi CreditAttribution: ofauravi commented#17, with admin user always works, the patch applied to the non-admin user.
Comment #19
webchickHm. 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.
(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.
Comment #20
droplet CreditAttribution: droplet commented@#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
Comment #21
marthinal CreditAttribution: marthinal commentedReviewing 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.
Comment #22
marthinal CreditAttribution: marthinal commentedAbout 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.
Comment #23
marthinal CreditAttribution: marthinal commentedPatch + test
Comment #24
wryz CreditAttribution: wryz commented#23: d8-comment_date-1374090-23.patch queued for re-testing.
Comment #26
brenda003Patch rerolled.
Comment #28
brenda003Patch rerolled.
Comment #30
droplet CreditAttribution: droplet commentedTest case on #7. doesn't it enough ?
Comment #31
droplet CreditAttribution: droplet commented@marthinal, are you still work on it ? :)
Comment #32
G.I.Joe CreditAttribution: G.I.Joe commentedIf you don't mind I will take over this issue.
Comment #33
G.I.Joe CreditAttribution: G.I.Joe commentedIt's was not possible to reroll the previous patch.
So, I created a new patch.
Comment #34
pfrenssenThe patch contains some weird characters and I'm unable to apply it:
Strange that the test bot didn't complain. Can you perhaps reroll the patch?
Comment #35
G.I.Joe CreditAttribution: G.I.Joe commentedIt's was not possible to reroll the previous patch
So, I created a new patch.
Comment #36
G.I.Joe CreditAttribution: G.I.Joe commentedThe patch contained color characters.
Please, try following patch.
Comment #37
droplet CreditAttribution: droplet commentedAll few reroll, all tests have been gone.
Comment #38
pfrenssenIndeed, there were some tests in the patch from #28 and these have been lost in the reroll.
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.
Same remark as above: split nested ternaries, and remove unneeded parentheses.
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.Comment #40
droplet CreditAttribution: droplet commentedI'd say #7 test / first part of #28 test is good enough for this change.Comment #41
droplet CreditAttribution: droplet commentedOK. D8 changed the tests, now it has already caught seconds:
Comment #42
G.I.Joe CreditAttribution: G.I.Joe commentedWell done, Paljas.I reviewed the code and fix solved issue.
Comment #43
droplet CreditAttribution: droplet commentedSorry. I just read this thread again. We need a test case for non-admin users.
Comment #44
pfrenssenIt is not needed to wrap a ternary operator in parentheses.
Comment exceeds 80 characters.
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().
I would use $comment rather than $comment_loaded.
Comment #45
droplet CreditAttribution: droplet commentedAlthough 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:
Thanks.
Comment #46
pfrenssenMakes sense. It's looking good now, thanks!
Comment #47
webchickAwesome work, on both the find and the fix!
Committed and pushed to 8.x. Thanks!
Moving to 7.x backport.
Comment #47.0
webchickminor stylefix
Comment #48
andypostClosed #1005004: Editing a comment destroys its creation date in favor of this one
Comment #49
tvn CreditAttribution: tvn commentedComment #50
asirjacques CreditAttribution: asirjacques commentedBack port from Drupal 8 to Drupal 7.
I had to change the original code that was into the Drupal 8 patch.
Comment #51
asirjacques CreditAttribution: asirjacques commentedwrong patch and still working on the issue
Comment #52
droplet CreditAttribution: droplet commentedComment #53
mgifford@Lynxas can you re-roll d7-comment_creation_date-1374090-50.patch it seems to be filled with a bunch of garbage characters.
Comment #54
droplet CreditAttribution: droplet commentedlet me backport my D8 patch.
Comment #56
droplet CreditAttribution: droplet commented54: 1374090-comment-creation-date.patch queued for re-testing.
Comment #58
droplet CreditAttribution: droplet commentedComment #59
mpv CreditAttribution: mpv commentedI will be testing this patch now.
Comment #60
mpv CreditAttribution: mpv commentedThe patch for D7 is working for me.
Comment #61
dcam CreditAttribution: dcam commentedMarked #2258527: Comment edit destroys created date as a duplicate issue.
Comment #62
lokapujyaJust to clarify, Does the D7 patch only fix b.) in the issue summary?
Comment #63
droplet CreditAttribution: droplet commentedFix all problems.
Comment #64
rob.barnett CreditAttribution: rob.barnett commentedThe 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.
Comment #65
droplet CreditAttribution: droplet commented@hurley,
nice caught, here's the patch: #2288865: Same comment creation & changed date
Comment #67
droplet CreditAttribution: droplet commented58: 1374090-comment-creation-date.patch queued for re-testing.
Comment #68
andypostback to rtbc
Comment #71
dcam CreditAttribution: dcam commentedComment #74
dcam CreditAttribution: dcam commentedComment #77
dcam CreditAttribution: dcam commentedComment #83
lokapujyaComment #86
dcam CreditAttribution: dcam commentedComment #87
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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?
Comment #88
andypostDavid, 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-up2) 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
Comment #89
rob.barnett CreditAttribution: rob.barnett commentedForgive me if I'm misunderstanding how this stands currently with D7. I noticed the following change in 7.27.
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.
Comment #90
le72Sorry
Comment #91
le72Comment #92
opdaviesPatch in #58 still applies cleanly to 7.x-dev and prevents the created date from changing.
Comment #93
opdaviesComment #95
mgifford@opdavies - no problem with the RTBC, I just wanted the bots to run on it again as it's been over a year.
Comment #96
opdavies@mgifford - No problem, although if it's RTBC, it'll be retested automatically as per https://qa.drupal.org/node/228. :)
Comment #97
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedNope, 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.
Comment #98
dqd@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:(Any better text ideas are welcome)
Comment #99
lokapujyaOK, so create a a new new issue?
Comment #100
lmeurs CreditAttribution: lmeurs commentedThe 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 thechanged
date. The problem seemed to be that thechanged
date is based onREQUEST_TIME
, but thecreated
date not. We updated the patch from #58 to fix this, see the interdiff.Comment #101
dqd@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.
Comment #104
stefan.r CreditAttribution: stefan.r commentedComment #105
izmeez CreditAttribution: izmeez commentedI 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.
Comment #106
TD44 CreditAttribution: TD44 commentedIf 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
Comment #109
dqdThanks 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.
Comment #110
TD44 CreditAttribution: TD44 commentedOfc we need patching for D7.
Thanks bro
Comment #111
matt_c CreditAttribution: matt_c commentedThis 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.
Comment #112
drummRe-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.
To me, this trade-off is a no-brainer. The comment created time should be when the comment was created.
Comment #113
izmeez CreditAttribution: izmeez commentedAdded 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.
Comment #114
mcdruidSame as patch #100 but with a test-only version to verify the behaviour change.
Comment #116
mcdruidThis 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.
Comment #118
drummYes, this has been running since April 13 without any known issues, https://bitbucket.org/drupalorg-infrastructure/drupal.org/commits/295b07...
Comment #119
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC + 1, approved for Merge! Thanks all!
Comment #121
mcdruidAdded the CR.
Thanks everyone, it's great to finally get this fixed!