When an admin edits a comment, the "authored on" date field gets its initial value from the "changed" field, but on submission saves its value to the "created" field.

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfb’s picture

Status: Active » Needs review
FileSize
1.34 KB
mfb’s picture

FileSize
1.33 KB

Changed the logic around because $comment->created doesn't always exist.

Status: Needs review » Needs work

The last submitted patch, comment-date.patch, failed testing.

mfb’s picture

Status: Needs work » Needs review
FileSize
2.28 KB

Failed because the entered date string is now getting processed and translated to the local time zone. Tweaked test to expect the date to be formatted in the local time zone.

retester2010’s picture

#4: comment-date.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, comment-date.patch, failed testing.

salvis’s picture

Title: Comment authored on date issues » Comment timestamp lost when edited by administrator
Status: Needs work » Needs review
FileSize
1.72 KB

This bothers me, too. As administrator I want to be able to edit a comment without changing the timestamp that appears in the GUI, just like editing nodes. As the OP explained, D7 initializes the Authored on textfield with the changed timestamp rather than the created one (!) and then stores it back into the created field.

The attached patch fixes this: it initializes the Authored on textfield with the created timestamp, so that the administrator can save the comment without changing the timestamp that appears on the comment view.

From the OP:

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

This is correct behavior, IMO. My patch does not change this.

Status: Needs review » Needs work

The last submitted patch, comment-date.714958.7.patch, failed testing.

salvis’s picture

FileSize
842 bytes

Just get the right value...

mfb’s picture

Status: Needs work » Needs review

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

This is correct behavior, IMO. My patch does not change this.

What makes this correct? Doesn't $comment->changed exist so that $comment->created can store the time the comment was created while $comment->changed can store the time the comment was last updated?

I don't think I agree, but setting to needs review so test bot will give it a whirl.

salvis’s picture

Non-admins should not be able to change their comments without leaving a (visible-for-all) trace that they did so. This is how d.o here works: if you go back and edit your #4 above (for the sake of the example), then it'll get today's timestamp and you cannot claim that "you've said it all back in February"...

This, to me, is more important than knowing when you wrote the original version of #4.

Thanks for setting the status!

mfb’s picture

FileSize
866 bytes

fine :p although that sounds like a theme issue to me -- the site should display "Updated: $comment->changed" for non-admin comments..

I couldn't help but notice a bunch of micro-optimizations: 1) get rid of extra parentheses, 2) use isset(), and 3) use date() rather than format_date() because this is a numeric date format that can't be translated (and there's no need to apply a time zone because Drupal now sets the PHP default time zone to the user's time zone during bootstrap).

Status: Needs review » Needs work

The last submitted patch, 714958-comment-date.patch, failed testing.

salvis’s picture

1. It's a good habit to always enclose the ternary operator in parentheses, because its precedence is not well-established. Different programming languages handle it differently — those parentheses are free and they ensure that there are no surprises.

2. !empty() is not exactly the same as isset(). I wouldn't trade one for the other without having really really studied the implications. I haven't found out where $comment->date could come from — the safe thing to do is to preserve what's there and tested.

3. The testbot disagrees.

mfb’s picture

Status: Needs work » Needs review
FileSize
886 bytes

hmm couldn't reproduce the test failure so not sure what the problem was. Let's roll this back to your patch from #9

mfb’s picture

salvis’s picture

Status: Needs review » Closed (duplicate)

Shoot, we've been there seven weeks ago, but we should have marked this 'critical'...

Fixed in #1005004: Editing a comment destroys its creation date...