Missing post ID as parameter for get_comments_number_text()

[Hinjiriyo]

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?

[donmhico]

Added unit test.

[donmhico]

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().

[Hinjiriyo]

Great! Just a tiny hint:

@param int|WP_Post $post_id

=> I think it is only an int parameter.

[garrett-eclipse]

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/

[peterwilsoncc]

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.

[donmhico]

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

[Hinjiriyo]

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

[donmhico]

@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]

Minor refresh to fix path applying, semver and CS.

[garrett-eclipse]

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]

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

[donmhico]

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

[Hinjiriyo]

@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.

[garrett-eclipse]

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

[imath]

Hi @garrett-eclipse & @donmhico

Just tested the 48772.5.diff patch (manually and using the phpunit test). I confirm it works as expected.

Thanks a lot to everyone for your contributions on this ticket.

[donmhico]

Thanks @imath and thank you @garrett-eclipse for your explanation. @Hinjiriyo we can't just change function parameters, especially to publicly exposed functions, in WP. I decided to follow [34118] because I believe there's a "good" reason why it was done like that and like @garrett-eclipse explained, it's most probably for backward-compat and to make sure that this change won't break anything.

[SergeyBiryukov]

In 47276:

Comments: Add a $post_id parameter to get_comments_number_text() and comments_number().

This allow for using these functions outside of the loop.

Props donmhico, garrett-eclipse, Hinjiriyo, imath.
Fixes #48772.

[SergeyBiryukov]

Having second thoughts on this.

The existing fourth parameter of comments_number() was previously the number of comments, deprecated in [5101].

While keeping it as is and adding a fifth parameter the safest option, I believe it should also be safe to just un-deprecate and repurpose the fourth parameter. Given that it's been deprecated for the last 13 years, it's highly unlikely that someone is still using it.

A search for ​plugins or ​themes passing a numeric fourth parameter to comments_number() shows zero results, only some false positives.

This is also different from [34118] / #33654. That was a serious issue for a pluggable function that depended on plugins being updated. This is minor in comparison, and comments_number() is not pluggable. Worst case here, a wrong comments number, should be an easy correction.

Proposing 48772.6.diff​ to address this.

[Hinjiriyo]

Thank you for your research. The results are interesting.

The search covers the free plugins in the repository and cannot take into account the many paid versions in online shops or plugins in other directories like github. So it cannot be excluded that the fourth parameter is used after all.

However, the long time it has been considered deprecated suggests that the parameter is not used. I therefore agree with your proposal.

[peterwilsoncc]

Replying to SergeyBiryukov:

The existing fourth parameter of comments_number() was previously the number of comments, deprecated in [5101].

While keeping it as is and adding a fifth parameter the safest option, I believe it should also be safe to just un-deprecate and repurpose the fourth parameter. Given that it's been deprecated for the last 13 years, it's highly unlikely that someone is still using it.

Is it worth switching to an arguments array? Four parameters is workable but approaching an unwieldy number, switching to an array could be handy going forward.

Important: this isn't a passive aggressive change request disguised as a comment, it's really a question. :)

[garrett-eclipse]

Thanks @SergeyBiryukov for the history. I tested 48772.6.diff and prefer it rather than having to pass an empty string as fourth param just to be able to supply the fifth $post_id param. It applies cleanly and unit tests look good as well.

That's great news it was deprecated 13 years ago in 1.3.0. In that case I would agree to drop it now and hopefully get that changed before 5.4 launches or it'll be harder to go backwards.

Dropping down to 4 attributes also helps avoid the need to switch to a $attr type of approach as proposed by @peterwilsoncc. I did look through the uses and implementation of the function and am unsure of any additional params that'd be desired. That being said I can't predict the future so if that's a safer way to move forward it makes sense.

One last remark, I was wondering if we should utilize the fourth param to pass the $id to the instance of comments_number found here;
​https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/comment-template.php#L1618
*Note: If not provided will get a $post_id in get_comments_number using get_post

[SergeyBiryukov]

Replying to peterwilsoncc:

Is it worth switching to an arguments array? Four parameters is workable but approaching an unwieldy number, switching to an array could be handy going forward.

Thanks for bringing this up, I think that's a good option to consider if the function ever gets more than five arguments. For now, I would just like to make function signatures for comments_number() and get_comments_number_text() consistent:

function comments_number( $zero = false, $one = false, $more = false, $post_id = 0 )
function get_comments_number_text( $zero = false, $one = false, $more = false, $post_id = 0 )

[SergeyBiryukov]

Replying to garrett-eclipse:

One last remark, I was wondering if we should utilize the fourth param to pass the $id to the instance of comments_number found here;
​https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/comment-template.php#L1618
*Note: If not provided will get a $post_id in get_comments_number using get_post

That could be done, however doesn't seem to make a difference, as comments_popup_link() can only be used in the loop, and $id would be the same value that get_comments_number() retrieves via get_post().

[SergeyBiryukov]

In 47366:

Comments: Restore the fourth parameter of comments_number() as $post_id, for consistency with get_comments_number_text().

The parameter was previously used as the number of comments, marked as deprecated in [5101].

Given that it's been deprecated for the last 13 years, it should be safe to undeprecate and repurpose it for a cleaner function signature, instead of adding a fifth parameter.

Follow-up to [47276].

Fixes #48772.

[garrett-eclipse]

Replying to SergeyBiryukov:

Replying to garrett-eclipse:

One last remark, I was wondering if we should utilize the fourth param to pass the $id to the instance of comments_number found here;
​https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/comment-template.php#L1618
*Note: If not provided will get a $post_id in get_comments_number using get_post

That could be done, however doesn't seem to make a difference, as comments_popup_link() can only be used in the loop, and $id would be the same value that get_comments_number() retrieves via get_post().

Thanks for confirming @SergeyBiryukov I found a similar result so wholeheartedly agree it's unneeded there.