Make WordPress Core

Opened 2 years ago

Closed 4 months ago

Last modified 3 months ago

#52322 closed enhancement (fixed)

Add comment / comment id param to get_comment_time

Reported by: spacedmonkey's profile spacedmonkey Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: normal
Severity: normal Version: 1.5
Component: Comments Keywords: good-first-bug has-patch commit add-to-field-guide
Focuses: Cc:

Description

Bring get_comment_time inline with get_comment_date and add a comment / comment id param to pass comment id into the function.

Attachments (3)

52322.diff (1.0 KB) - added by travisaxton 2 years ago.
52322.2.diff (1.5 KB) - added by travisaxton 2 years ago.
Version 2.0
52322.3.diff (2.6 KB) - added by h4l9k 6 months ago.

Download all attachments as: .zip

Change History (23)

@travisaxton
2 years ago

#1 @travisaxton
2 years ago

  • Keywords has-patch added; needs-patch removed

I've submitted a possible solution for the 'get_comment_time' function. This is my first submission for WordPress Core so I apologize if I have made any mistakes with the Trac workflow.

#2 follow-up: @SergeyBiryukov
2 years ago

Thanks for the patch! Some notes:

  • The new parameter should be added at the end. Adding it after $format matches get_comment_date(), but is not feasible in practice, as it would break backward compatibility.
  • The DocBlock should be aligned per the documentation standards and would also need a @since note.
  • comment_time() would probably need a similar change for consistency.

#3 @travisaxton
2 years ago

Thanks for the guidance and the feedback!

I updated 'get_comment_time' to reflect the changes you mentioned. Still trying to figure out the best way to change 'comment_time' for consistency. Attaching an updated patch file to make sure I am on the right track.

Thanks very much for your patience in helping me navigate the coding standards.

@travisaxton
2 years ago

Version 2.0

This ticket was mentioned in PR #2646 on WordPress/wordpress-develop by DSGND.


13 months ago
#4

Check if function get_comment_id is null before executing it.

Trac ticket: https://core.trac.wordpress.org/ticket/52322

#5 @D SIGNED
13 months ago

Hi,

I've just submitted a #PR2646 to check if get_comment_id is not null to avoid an error when executing.

(It's my first contribution)

Last edited 13 months ago by D SIGNED (previous) (diff)

#6 @audrasjb
7 months ago

  • Keywords needs-refresh changes-requested added
  • Owner set to audrasjb
  • Status changed from new to reviewing

Thanks for the PR @d-signed! I added two feedbacks in your PR :)

#7 @audrasjb
7 months ago

  • Milestone changed from Awaiting Review to 6.2
  • Version set to 1.5

Moving for 6.2 consideration.

#8 in reply to: ↑ 2 @rudlinkon
7 months ago

Replying to SergeyBiryukov:

Thanks for the patch! Some notes:

  • The new parameter should be added at the end. Adding it after $format matches get_comment_date(), but is not feasible in practice, as it would break backward compatibility.
  • The DocBlock should be aligned per the documentation standards and would also need a @since note.
  • comment_time() would probably need a similar change for consistency.

Yes, I agree with @SergeyBiryukov that we need a similar change in comment_time() for consistency. Also, we can add the same extra parameter $comment_ID on the get_comment_time filter so that developer can get a direct comment id instead of getting it from the $comment object.

@h4l9k
6 months ago

#9 @h4l9k
6 months ago

  • Keywords needs-refresh changes-requested removed

I submitted a new patch for this 52322.3, refreshing the one from @travisaxton (new comparison against trunk) and with the additions mentioned by @rudlinkon and @SergeyBiryukov. I added $comment_ID to comment_time() for consistency and to the filter get_comment_time as well, and @since notes in both comment_time() and get_comment_time().

This is my first contribution.

Last edited 6 months ago by h4l9k (previous) (diff)

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


4 months ago

#11 @mukesh27
4 months ago

  • Keywords changes-requested added

This ticket was discussed in the recent bug scrub.

Thanks @spacedmonkey for the ticket!

@d-signed There are a few reviews that need to be changed.

Props to @costdev

#12 @spacedmonkey
4 months ago

There needs to be a check to see if the comment exists.

 $comment = get_comment( $comment_ID );

A check like this

$comment = get_comment( $comment_id );

        if ( ! $comment ) {
           return '';
        }

We may even need to do this

if ( !$comment || 'trash' === get_post_status( $comment->comment_post_ID ) ) {

This ticket was mentioned in Slack in #core by costdev. View the logs.


4 months ago

#14 @costdev
4 months ago

  • Keywords commit added; changes-requested removed

This ticket was discussed during the bug scrub. This looks ready for commit consideration. Adding the appropriate keyword. JB agrees that the suggested docs change on the PR can be made during commit.

Additional props: @audrasjb

@audrasjb commented on PR #2646:


4 months ago
#15

I added one commit to the PR to fix a WPCS issue

This ticket was mentioned in Slack in #core by costdev. View the logs.


4 months ago

#17 @audrasjb
4 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 55284:

Comments: Allow to pass $comment_ID parameter to get_comment_time().

Props spacedmonkey, travisaxton, SergeyBiryukov, d-signed, audrasjb, rudlinkon, h4l9k, mukesh27, costdev.
Fixes #52322.

#19 @SergeyBiryukov
4 months ago

In 55287:

Comments: Allow to pass $comment_ID parameter to comment_time().

This brings consistency with:

  • get_comment_time()
  • get_comment_date()
  • comment_date()

Includes:

  • Correcting the @since tag for get_comment_time().
  • Synchronizing parameter description between get_comment_date() and get_comment_time().

Follow-up to [55284].

See #52322.

#20 @milana_cap
3 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.