WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 37 hours ago

#48772 assigned enhancement

Missing post ID as parameter for get_comments_number_text()

Reported by: Hinjiriyo Owned by: donmhico
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.0
Component: Comments Keywords: has-patch needs-testing
Focuses: Cc:
PR Number:

Description

There is no way to pass the post ID to get_comments_number_text() to use this function outside the loop.

After all, get_comments_number_text() calls get_comments_number(). The latter function can be used outside the loop because it uses the post ID as an optional parameter.

So, what about rectivating the deprecated fourth parameter of comments_number() to pass the post ID, and extending get_comments_number() by a fourth parameter to pass the post ID, for using both functions outside of loops?

Attachments (5)

48772.diff (1.2 KB) - added by donmhico 2 months ago.
48772.2.diff (2.0 KB) - added by donmhico 2 months ago.
Added unit test.
48772.3.diff (3.2 KB) - added by donmhico 39 hours ago.
48772.4.diff (3.4 KB) - added by garrett-eclipse 39 hours ago.
Minor refresh to fix path applying, semver and CS.
48772.5.diff (3.4 KB) - added by garrett-eclipse 39 hours ago.
Oops minor copy-paste issue in my .4 patch fixed here

Download all attachments as: .zip

Change History (19)

@donmhico
2 months ago

@donmhico
2 months ago

Added unit test.

#1 @donmhico
2 months ago

  • Keywords dev-feedback has-patch added

Hello @Hinjiriyo,

Try to test out the patch I submitted above, i'm not sure about reactivating the deprecated param in comments_number() so the changes is just applied to get_comments_number_text() at the moment.

Let's wait for other dev feedback regarding comments_number().

#2 follow-up: @Hinjiriyo
2 months ago

Great! Just a tiny hint:

@param int|WP_Post $post_id

=> I think it is only an int parameter.

#3 in reply to: ↑ 2 @garrett-eclipse
2 months ago

Replying to Hinjiriyo:

Great! Just a tiny hint:

@param int|WP_Post $post_id

=> I think it is only an int parameter.

Reviewing the get_comments_number function which is passed the post_id it support WP_Post;
https://developer.wordpress.org/reference/functions/get_comments_number/

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


7 weeks ago

#5 @peterwilsoncc
7 weeks ago

  • Milestone changed from Awaiting Review to 5.4
  • Owner set to donmhico
  • Status changed from new to assigned
  • Version changed from 5.3 to 4.0

This was discussed during an APAC triage session, @donmhico agreed to look after the ticket so I've assigned it and placed it on the 5.4 milestone.

#6 @donmhico
4 weeks ago

Need some dev-feedback in here. Basically the patch above, adds $post_id param in get_comments_number_text(). Now the concern is comments_number() is basically a wrapper of get_comments_number_text(). So I think it's good to be able to somehow pass the $post_id param in comments_number() as well but it already has 4 params and adding the fifth one does not seem to be the best approach. Any suggestion is highly welcome.

Reference:
https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/comment-template.php#L878-L883

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


4 weeks ago

#8 @Hinjiriyo
4 weeks ago

Since the 4th parameter is flagged as deprecated, why not reactivate it and use it for the post ID?

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


41 hours ago

@donmhico
39 hours ago

#10 @donmhico
39 hours ago

@Hinjiriyo 48772.3.diff adds $post_id as the fifth parameter on comments_number()instead of remove the 4th $deprecated param. This is because looking in a past commit [34118], the $deprecated was preserved and a new param was added instead.

@garrett-eclipse
39 hours ago

Minor refresh to fix path applying, semver and CS.

#11 follow-up: @garrett-eclipse
39 hours ago

  • Keywords needs-testing added

Hi @donmhico thanks for the updated patch.

Testing on trunk I wasn't able to apply it so made a refresh in 48772.4.diff. Also in this I've made two minor tweaks;

  1. Updated @since 5.4 to use proper semver 5.4.0
  2. Updated docblock for get_comments_number_text to have similar indentation as comments_number for CS.

With my refresh tests are still passing here but will leave for a Comments maintainer to sign off.

@garrett-eclipse
39 hours ago

Oops minor copy-paste issue in my .4 patch fixed here

#12 @donmhico
39 hours ago

Thanks you for the new patch @garrett-eclipse :)

#13 in reply to: ↑ 11 @Hinjiriyo
38 hours ago

@garrett-eclipse
Thank you for your work!
I couldn't comprehend your reason for a 5th parameter instead of re-activating the 4th parameter. Did you mean "since adding a new parameter was done in 34118, I'm doing it that way, too"?
I'm sure you did it in the best way.

#14 @garrett-eclipse
37 hours ago

  • Keywords dev-feedback removed

Thanks @Hinjiriyo
I assume @donmhico preserved the deprecated parameter as it appears that is the standard convention for this type of thing. For instance wp_install has a 5th param that's deprecated but also then has a 6th and 7th. By not removing the deprecated param we preserve the call to _deprecated_argument which flags to users they're misusing the function. Adding an addition instead of replacing in this case is primarily for back-compat reason.

  • @donmhico correct me if I'm wrong
Note: See TracTickets for help on using tickets.