WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 3 months ago

#38268 closed enhancement (fixed)

WP_Comment_Query Pagination

Reported by: wordpresssites Owned by: AdamWills
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7
Component: Comments Keywords: good-first-bug has-patch
Focuses: Cc:

Description

Is it possible to build pagination into WP_Comment_Query so previous & next page of comments can be easily added when listing comments using this function & get_comments?

Something like paged and comments per page parameters?

Attachments (2)

38268.patch (14.1 KB) - added by AdamWills 12 months ago.
First core contribution attempt
38268.2.patch (3.0 KB) - added by AdamWills 12 months ago.
Previous upload wasn't what I was expecting. Trying again…

Download all attachments as: .zip

Change History (13)

#1 follow-up: @rachelbaker
14 months ago

@wordpresssites Howdy! WP_Comment_Query does have the number and offset parameters, and the $max_num_pages property which should give you what you need to build custom pagination functions. Would that fit your use case?

#2 @rachelbaker
14 months ago

  • Keywords reporter-feedback added

#3 in reply to: ↑ 1 @wordpresssites
13 months ago

Yes and have done so however is there any reason this can't be built into WordPress? It's a fair amount of work and understanding to be able to do this yet there's pagination functions built into WordPress for posts and pages as well as single post comments. Why include functions like WP_Comment_Query and get_comments without including a built in page nav function?

Replying to rachelbaker:

@wordpresssites Howdy! WP_Comment_Query does have the number and offset parameters, and the $max_num_pages property which should give you what you need to build custom pagination functions. Would that fit your use case?

#4 follow-up: @boonebgorges
13 months ago

  • Keywords needs-patch good-first-bug added; reporter-feedback removed
  • Milestone changed from Awaiting Review to Future Release

I think we'd probably be happy to consider pagination parameters. As you note, they add convenience for many use cases.

Note that a patch for this feature ought to have tests that demonstrate how the new params work alongside number and offset - one set of params will have to "win" when both are provided.

@AdamWills
12 months ago

First core contribution attempt

@AdamWills
12 months ago

Previous upload wasn't what I was expecting. Trying again...

#5 in reply to: ↑ 4 @AdamWills
12 months ago

In the patch, I used the same logic used in WP_User_Query - a valid (>0) offset value exists, it will override the paged parameter. Unit tests are attached.

Replying to boonebgorges:

I think we'd probably be happy to consider pagination parameters. As you note, they add convenience for many use cases.

Note that a patch for this feature ought to have tests that demonstrate how the new params work alongside number and offset - one set of params will have to "win" when both are provided.

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


12 months ago

#7 follow-up: @swissspidy
12 months ago

  • Keywords has-patch added; needs-patch removed

@AdamWills Thank you for your contribution! I'm assigning this ticket to you to mark this good first bug as claimed.

Also adding the 'has-patch' keyword so people see there's a patch to test.

#8 @swissspidy
12 months ago

  • Owner set to AdamWills
  • Status changed from new to assigned

#9 in reply to: ↑ 7 @AdamWills
4 months ago

Replying to swissspidy:

@AdamWills Thank you for your contribution! I'm assigning this ticket to you to mark this good first bug as claimed.

Also adding the 'has-patch' keyword so people see there's a patch to test.

Not being that familiar with the process - what happens now with this ticket? I think it's been ready to go for months...

#10 @boonebgorges
3 months ago

  • Milestone changed from Future Release to 4.9

@AdamWills Thanks for your patience, and sorry for the delay in reviewing this ticket.

The /src/ patch looks good to me. I'm going to make a slight modification to the documentation, and add a @since annotation, before moving forward.

Thanks so much for including tests. I'm going to modify them a bit:

  • Instead of comparing results of two comment queries to each other, compare the results of a query directly to a list of expected results. (Just in case the same bug affects both queries.)
  • Specify the comment dates and the query params more precisely, to be sure there are no problems with query/orderby indeterminacy in cases where the comments have matching comment_date_gmt.

Thanks for the contribution!

#11 @boonebgorges
3 months ago

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

In 41287:

Introduce paged argument to WP_Comment_Query.

Using paged with number allows developers to request
paginated comment results without having to do a manual offset
calculation.

Props AdamWills.
Fixes #38268.

Note: See TracTickets for help on using tickets.