Currently, using comment_save() to update an existing comment requires the argument timestamp; if it's not provided, then the error msg returns "Missing required arguments".

However, in the code, we already have the section to handle the comment creation timestamp:

        if (empty($edit['timestamp'])) {
          $edit['timestamp'] = time();
        }

We can simplely move this section above, so updating proccess no longer "require" the timestamp, if it's missing, then use the current just like creating a new comment.

patch is coming later.

CommentFileSizeAuthor
#5 drupal-882722-D6.patch1.56 KBskyredwang
#3 drupal-882722_1.patch1.56 KBskyredwang
#1 drupal-882722.patch1.51 KBskyredwang

Comments

skyredwang’s picture

skyredwang
Location Shanghai
commented
Status: Active » Needs review
StatusFileSize
new drupal-882722.patch1.51 KB

Status: Needs review » Needs work

The last submitted patch, drupal-882722.patch, failed testing.

skyredwang’s picture

skyredwang
Location Shanghai
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-882722_1.patch1.56 KB

re-submitted the patch with corrected path

Status: Needs review » Needs work

The last submitted patch, drupal-882722_1.patch, failed testing.

skyredwang’s picture

skyredwang
Location Shanghai
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-882722-D6.patch1.56 KB

rename the file so the bot will ignore simpletesting this D6 patch.

thedavidmeister’s picture

thedavidmeister commented
Version: 6.19 » 6.x-dev
Status: Needs review » Closed (won't fix) 
+      
+      // If the timestamp is not provided, then automatically use the current timestamp for either creating a new comment or updating the existing comment
+      if (empty($edit['timestamp'])) {
+        $edit['timestamp'] = time();
+      }
+

There are multiple coding standards violations here.

- Trailing whitespace above and below the code
- Inline comment exceeds 80 characters
- Comment does not finish with a full-stop "." character

Regardless, I disagree that updating a comment should silently reset the "created timestamp" to the current time if the timestamp is not provided. In this situation, throwing an error seems like the correct thing to do as it notifies the developer that something unexpected is happening.

For most cases, modifying the created stamp for a comment when it is modified (which could just be it being published/unpublished by a moderator) is a bug, for the cases where this is intended behaviour, it should be the responsibility of the developer to recognise the situation and explicitly pass in a new timestamp as required.

#2094585: [policy, no patch] Core review bonus for #2025699: Consolidate 'datetime'/'datetime_form' and 'datelist'/'datelist_form' #type and #theme names for consistency.