WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 6 weeks ago

Last modified 6 weeks ago

#48772 closed enhancement (fixed)

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 commit
Focuses: Cc:

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

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

Download all attachments as: .zip

Change History (32)

@donmhico
4 months ago

@donmhico
4 months ago

Added unit test.

#1 @donmhico
4 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
4 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
4 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.


4 months ago

#5 @peterwilsoncc
4 months 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
3 months 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.


3 months ago

#8 @Hinjiriyo
3 months 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.


3 months ago

@donmhico
3 months ago

#10 @donmhico
3 months 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
3 months ago

Minor refresh to fix path applying, semver and CS.

#11 follow-up: @garrett-eclipse
3 months 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
3 months ago

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

#12 @donmhico
3 months ago

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

#13 in reply to: ↑ 11 @Hinjiriyo
3 months 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
3 months 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

#15 @imath
2 months ago

  • Keywords commit added; needs-testing removed

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.

#16 @donmhico
2 months ago

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.

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


2 months ago

#18 @SergeyBiryukov
2 months ago

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

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.

#19 follow-up: @SergeyBiryukov
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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, would be nice to clean it up.

Last edited 2 months ago by SergeyBiryukov (previous) (diff)

#20 @Hinjiriyo
2 months ago

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.

#21 in reply to: ↑ 19 ; follow-up: @peterwilsoncc
8 weeks ago

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

#22 follow-up: @garrett-eclipse
7 weeks ago

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

#23 in reply to: ↑ 21 @SergeyBiryukov
6 weeks ago

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 )

#24 in reply to: ↑ 22 ; follow-up: @SergeyBiryukov
6 weeks ago

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

#25 @SergeyBiryukov
6 weeks ago

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

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.

#26 in reply to: ↑ 24 @garrett-eclipse
6 weeks ago

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.

Note: See TracTickets for help on using tickets.