Make WordPress Core

Opened 15 years ago

Closed 8 years ago

Last modified 8 years ago

#8071 closed task (blessed) (fixed)

Refrain from querying all the comments on a post when paged

Reported by: markjaquith's profile markjaquith Owned by: boonebgorges's profile boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 2.7
Component: Comments Keywords: dev-feedback has-patch
Focuses: Cc:

Description

If a post has more than (comment_per_page) comments, we should query them specifically, not query all the comments and sort in PHP (doesn't scale well).

Logic:

if threading is off, use a simple limit query

elseif the page has equal or fewer comments than (comments_per_page), query them all

else query (comments_per_page) parent comments, and keep doing queries for their children, up to the threading limit (10).

Attachments (8)

8071.diff (6.4 KB) - added by wonderboymusic 9 years ago.
8071.2.diff (3.6 KB) - added by wonderboymusic 9 years ago.
8071.3.diff (8.1 KB) - added by wonderboymusic 9 years ago.
8071.4.diff (7.9 KB) - added by wonderboymusic 9 years ago.
8071.5.diff (8.3 KB) - added by boonebgorges 9 years ago.
8071.6.diff (28.0 KB) - added by boonebgorges 8 years ago.
8071.7.diff (8.5 KB) - added by boonebgorges 8 years ago.
8071.8.diff (30.3 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (78)

#1 @ryan
15 years ago

  • Component changed from General to Comments
  • Owner anonymous deleted

#2 @markjaquith
15 years ago

  • Milestone changed from 2.7 to 2.8

Still think this is worth testing, to see how much of a performance benefit it yields.

#3 @Denis-de-Bernardy
15 years ago

  • Milestone 2.8 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing as wontfix. As much as I agree this should really be dealt with in SQL, I dug into this one myself recently while redoing my theme's comments template.

Basically, the paging is so in WP that you the walker class grabs all of the comments. You then extract the correct ones based on filters (by type, etc.) and number of comments per page. As the stuff in there can use custom callbacks, there's absolutely no way this can land in SQL.

#4 @scribu
14 years ago

  • Keywords needs-patch added
  • Milestone set to Future Release
  • Resolution wontfix deleted
  • Status changed from closed to reopened

That's not exactly true. The separation is done in wp_list_comments() and only after that it's sent to Walker_Comments.

So, I think it can be moved into SQL, when threading is disabled.

#5 @scribu
14 years ago

It will require some non-trivial refactoring, but it is doable.

#6 @westi
14 years ago

  • Cc westi added

#7 @brianlayman
14 years ago

  • Cc brianlayman added

#8 follow-up: @scribu
14 years ago

For threaded comments, something like the following might or might not be worth implementing:

  1. Get the list of all ids and parent ids
  2. Build the hierarchy tree
  3. Figure out which items are to be displayed now
  4. Get all the info for those items, in one go
  5. Display

#9 @simonwheatley
12 years ago

  • Cc simon@… added

#10 in reply to: ↑ 8 @nacin
12 years ago

Replying to scribu:

For threaded comments, something like the following might or might not be worth implementing:

  1. Get the list of all ids and parent ids
  2. Build the hierarchy tree
  3. Figure out which items are to be displayed now
  4. Get all the info for those items, in one go
  5. Display

This sounds like a great idea.

#11 @wonderboymusic
12 years ago

I just did this (get all comments for post but just id=>parent / cache it) for my implementation of threaded replies in bbPress - once you do that, Walker can figure out hierarchy, but you need to do a Walker before you query paged comments so you can do an IN query for the cpage instead of calling get_comment( ) for every item in the comment loop

I'll attach code if what I did is portable

Doing the walker for the page will also get you a proper pagination matrix, and allow you to display 1-{total number of comments} vs 1-{total number of top level comments per page}

#12 @chriscct7
9 years ago

Any update @wonderboymusic?

#13 @wonderboymusic
9 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to wonderboymusic
  • Status changed from reopened to assigned

@wonderboymusic
9 years ago

#14 @wonderboymusic
9 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

8071.diff makes this work

Create 2 helper functions (these can be renamed, whatever):

  • wp_comments_per_page() - figures out what the default per page value is
  • wp_comment_pages_count( $total, $top_level ) - total can be read from $post->comment_count, $top_level is the count of a queried array of top level comment ids for a post that runs when comments are threaded

Add 2 query vars to WP_Comment_Query:

  • parent__in - for querying for comment_IDs that belong to a set of parents
  • parent__not_in for completeness

Add a new orderby value in WP_Comment_Query: comment__in - when querying by comment__in, need to return them in the same order.

In comment_template():

  • We're going to split the query - add 'fields' => 'ids' to $comment_args
  • If comments are threaded, query for all of the top level comment ids - else, get all of the comment IDs
  • Figure out how many pages of comments there are using wp_comment_pages_count()
  • Save the number of pages on $wp_query as ->comment_pages_count
  • If we determine that there is more than 1 page of comments:
    • save the current page on $wp_query as ->comment_pages_page
    • slice the list of comment IDs based on page and per_page
    • if comments are threaded, use parent__in in wp_threaded_comment_ids() to query for comment children (1 query for comment IDs per depth level)
  • set comment__in to the comment IDs, orderby comment__in, get all of the comments in 1 query

In wp_list_comments():

  • If no comments are passed to the function (means we're in the loop) and $wp_query->comment_pages_page is set, reset $r['page'] to 1, so that the Walker doesn't try to create the offset that we created by slicing the comment_IDs in comments_template()

Any Comments? (Huzzah!)

#15 @wonderboymusic
9 years ago

#28410 was marked as a duplicate.

#16 @wonderboymusic
9 years ago

8071.2.diff are the remaining bits after r34205 and r34212​.

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


9 years ago

#18 @wonderboymusic
9 years ago

8071.3.diff scales a post that has 100,371 approved comments / 400,000 total comments bound to the post ID.

Without the patch, the page doesn't even load, even when paginated.

tl;dr Force comments pagination on posts that have either:

  • 2 times comments_per_page
  • 100 comments (50 is the default, 2 pages)

If your comments are already paginated, and you have an extraordinary # of comments, these queries now scale. If you are not paginating comments, and all of a sudden, you get 100,000 comments, and you're not using an object cache, your page will now load.

Details

  • Split The Query - get IDs first, then query for all fields for all relevant comments using an IN query
  • In WP_Comment_Query, mimic the ->found_rows implementation of WP_Query by using SQL_CALC_FOUND_ROWS and adding a ->set_found_comments() method and ->max_num_pages prop. This allows us to paginate our queries, rather than paginating in Walker after querying every single comment.
  • Set $wp_query->max_num_comments_pages equal to the new $wp_comments->max_num_pages
  • When checking for get_option( 'page_comments' ), also check for $post->comment_count > $paging_threshold - this is a filterable value equal to (See Above)
  • When getting comment IDs for threaded comments, get them one depth level at a time, versus having Walker sort through a gigantic list that causes memory exhaustion. This can be done using the parent__in query var, which is now possible via [34205]
  • After getting all of the comment IDs via the passed query args, query all of the relevant objects at once via IN and then order the results via comment__in, ditching the extra sorting values. This is now possible via [34212].

#19 @wonderboymusic
9 years ago

  • Owner changed from wonderboymusic to boonebgorges

Boone is going to shepherd my work here while I attend to other things

#20 @boonebgorges
9 years ago

Current plan is to move much of the guts of 8073.3.diff to WP_Comment_Query, where it'll benefit all those who venture into WordPress comments, not just those using comments_template().

@boonebgorges
9 years ago

#21 @boonebgorges
9 years ago

8071.5.diff is the first step in generalizing wonderboymusic's patches - it splits the query in WP_Comment_Query. In the process, it normalizes the way that single comments are cached to reduce cache misses in certain cases. Posting here for comment (LOLZ) before I move on to the next stage.

#22 @boonebgorges
9 years ago

In 34310:

Split the comment query.

WP_Comment_Query now fetches comments in two stages: (1) a query to get the
IDs of comments matching the query vars, and (2) a query to populate the
objects corresponding to the matched IDs. The two queries are cached
separately, so that sites with persistent object caches will continue to have
complete cache coverage for normal comment queries.

Splitting the query allows our cache strategy to be more modest and precise, as
full comment data is only stored once per comment. It also makes it possible
to introduce logic for paginated threading, which is necessary to address
certain performance problems.

See #8071.
data is only stored once per comment, instead of along with

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


9 years ago

#24 @DrewAPicture
9 years ago

In 34355:

Docs: Add a description for the documented use of the $wpdb global in _prime_comment_caches().

See #8071. See #32246.

@boonebgorges
8 years ago

#25 @boonebgorges
8 years ago

8071.6.diff takes the spirit of wonderboymusic's patch and moves it deeper into the API. By moving some of the threading+pagination logic into WP_Comment_Query rather than comments_template(), we open up the performance improvements to other developers.

Notes:

  • In order to maintain some separation of powers in WP_Comment_Query, I had to stash the SQL clauses in an object property. That's why you see all the red/green there.
  • I'm defaulting no_found_rows to true. This is in keeping with current behavior. SQL_CALC_FOUND_ROWS can be very expensive, and I don't want to wreak havoc with people using WP_Comment_Query for weird things.
  • New param hierarchical takes three values: false (current and default - ignores comment children), 'flat' (returns a flat array that includes descendants), 'threaded' (returns a tree)
  • To support the tree display, I added some children infrastructure to WP_Comment (add_child(), get_child(), get_children()). None of this threaded/tree stuff is used in core - we use a walker instead - but it seemed to round out the API.
  • I removed the 'found_comments' filter. A filter on the query seems like it's enough. (sometimes it's faster to do SELECT COUNT(*), which I assume is why you put the filter on the SQL)
  • With this patch, the 'page_comments' setting becomes pretty meaningless - we stomp on it whenever we pass a certain threshold. I think it's a good idea to stomp on it, but it makes the UI setting "Break comments into pages..." fairly awkward. At some point - now or in the future - we should remove the checkbox, leaving only the 'comments_per_page' setting.
Last edited 8 years ago by boonebgorges (previous) (diff)

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


8 years ago

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


8 years ago

#28 @boonebgorges
8 years ago

Thinking more about recent patches, I'm going to remove the filter on the SELECT FOUND_ROWS() query altogether. SELECT FOUND_ROWS() itself is not costly - it's SQL_CALC_FOUND_ROWS that introduces overhead. For the time being, if a dev doesn't like the way WP gets a total comment count, it can be disabled with 'no_count_rows=true'. Alternative methods for counting (SELECT COUNT(*) WHERE...) can be achieved independently.

#29 @boonebgorges
8 years ago

In 34544:

Improve pagination internals in WP_Comment_Query.

WP_Comment_Query will now report the total number of comments matching the
query params (comments_found), as well as the total number of pages required
to display these comments (max_num_pages). Because SQL_CALC_FOUND_ROWS
queries can introduce a lot of overhead in some cases, we disable the feature
by default. Pass no_found_rows=false to WP_Comment_Query to enable the
count. (We use the negative parameter name 'no_found_rows' for parity with
WP_Query.)

Props wonderboymusic, boonebgorges.
See #8071.

#30 @boonebgorges
8 years ago

In 34546:

Introduce hierarchical query support to WP_Comment_Query.

Comments can be threaded. Now your query can be threaded too! Bonus: it's
not totally insane.

  • The new $hierarchical parameter for WP_Comment_Query accepts three values:
    • false - Default value, and equivalent to current behavior. No descendants are fetched for matched comments.
    • 'flat' - WP_Comment_Query will fetch the descendant tree for each comment matched by the query paramaters, and append them to the flat array of comments returned. Use this when you have a separate routine for constructing the tree - for example, when passing a list of comments to a Walker object.
    • 'threaded' - WP_Comment_Query will fetch the descendant tree for each comment, and return it in a tree structure located in the children property of the WP_Comment objects.
  • WP_Comment now has a few utility methods for fetching the descendant tree (get_children()), fetching a single direct descendant comment (get_child()), and adding anothing WP_Comment object as a direct descendant (add_child()). Note that add_child() only modifies the comment object - it does not touch the database.

Props boonebgorges, wonderboymusic.
See #8071.

#31 @boonebgorges
8 years ago

In 34549:

Remove debug cruft, introduced in [34546].

Props ocean90.
See #8071.

#32 @boonebgorges
8 years ago

In 34550:

Hierarchical comment query tests should be order-agnostic.

Travis-CI and other test environments can create weird race conditions.

See #8071.

@boonebgorges
8 years ago

#33 follow-up: @boonebgorges
8 years ago

8071.7.diff is what remains to fix this issue.

The way that comment pagination works is complicated and not very intuitive. The 0th page of comments - what you see when reading a post with no 'cpage' query var - contains either the most recent or the oldest #per_page comments, depending on your 'default_comments_page' setting. However, the behavior of non-zero 'cpage' is *not* sensitive to 'default_comments_page'; cpage=2 always contains more recent comments than cpage=3, etc. In other words, when 'default_comments_page=newest', the order of comment pages (if you have 4 pages total) is 0, 3, 2, 1, where if you show the oldest page first, the order is 0, 1, 2, 3. (The magic, such as it is, is in Walker_Comment.)

I don't think this makes a lot of sense, but I also don't know what would break if we changed the behavior. It's very likely that someone out there is building custom pagination links, and is expecting the behavior described above.

In order to introduce pagination that respects the new pagination powers of WP_Comment_Query - which are the key to making comment pagination scale - without breaking backward compatibility for comment pagination URLs, I've put a couple of pretty awful hacks into comment_template(). The hacks would be slightly less ugly if I put them into the Walker or into wp_list_comments(), but both of these could have unexpected consequences, because of the various ways they're used. By isolating everything inside of the already hideous comments_template(), I've greatly limited the possibility of collateral damage.

Primary problem at this point: I discovered that wonderboymusic's technique for forcing 'page_comments' is incomplete. When pretty permalinks are enabled, redirect_canonical() will check to see if get_option( 'page_comments' ) is enabled. If not, it disregards 'comment-page-x' URL chunks, so that comment pagination links get redirected back to the post itself. No query has taken place at this point, so there's no way we can force the threshold for pagination based on comment count. In 8071.7.diff, I removed the get_option( 'page_comments' ) check from redirect_canonical(). But, as I mentioned above, this effectively neuters the 'page_comments' settings. It may be time to kill the setting.

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


8 years ago

#35 @boonebgorges
8 years ago

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

In 34561:

Force comment pagination on single posts.

Previously, the 'page_comments' toggle allowed users to disable comment
pagination. This toggle was only superficial, however. Even with
'page_comments' turned on, comments_template() loaded all of a post's
comments into memory, and passed them to wp_list_comments() and
Walker_Comment, the latter of which produced markup for only the
current page of comments. In other words, it was possible to enable
'page_comments', thereby showing only a subset of a post's comments on a given
page, but all comments continued to be loaded in the background. This technique
scaled poorly. Posts with hundreds or thousands of comments would load slowly,
or not at all, even when the 'comments_per_page' setting was set to a
reasonable number.

Recent changesets have addressed this problem through more efficient tree-
walking, better descendant caching, and more selective queries for top-level
post comments. The current changeset completes the project by addressing the
root issue: that loading a post causes all of its comments to be loaded too.

Here's the breakdown:

  • Comment pagination is now forced. Setting 'page_comments' to false leads to evil things when you have many comments. If you want to avoid pagination, set 'comments_per_page' to something high.
  • The 'page_comments' setting has been expunged from options-discussion.php, and from places in the codebase where it was referenced. For plugins relying on 'page_comments', we now force the value to true with a pre_option filter.
  • comments_template() now queries for an appropriately small number of comments. Usually, this means the comments_per_page value.
  • To preserve the current (odd) behavior for comment pagination links, some unholy hacks have been inserted into comments_template(). The ugliness is insulated in this function for backward compatibility and to minimize collateral damage. A side-effect is that, for certain settings of 'default_comments_page', up to 2x the value of comments_per_page might be fetched at a time.
  • In support of these changes, a $format parameter has been added to WP_Comment::get_children(). This param allows you to request a flattened array of comment children, suitable for feeding into Walker_Comment.
  • WP_Query loops are now informed about total available comment counts and comment pages by the WP_Comment_Query (found_comments, max_num_pages), instead of by Walker_Comment.

Aside from radical performance improvements in the case of a post with many
comments, this changeset fixes a bug that caused the first page of comments to
be partial (found_comments % comments_per_page), rather than the last, as
you'd expect.

Props boonebgorges, wonderboymusic.
Fixes #8071.

#36 @boonebgorges
8 years ago

In 34562:

After [35461], remove 'page_comments' from database.

  • Don't set as part of initial schema.
  • Delete as part of the $unusedoptions routine.

Props ocean90.
See #8071.

#37 @wonderboymusic
8 years ago

In 34569:

Comments: in WP_Comment::get_children(), accept an array so that the values for format, status, hierarchical, and orderby can be passed, instead of just format. The defaults for get_comments() include status = 'all' and orderby = '' - which is no bueno.

For threaded comments, we need comments to be retrieved within bounds, so logged-out users don't see unmoderated comments on the front end, etc.

Updates unit tests.

See #8071.

#38 @boonebgorges
8 years ago

In 34595:

Use correct property name when setting child comments.

This fixes a typo from [34546], uncovered by unrelated changes in [34583].

See #8071, #27571.

#39 @boonebgorges
8 years ago

In 34623:

WP_Comment::get_children() test should be order-agnostic.

See #8071.

#40 @swissspidy
8 years ago

Either this broke comment links (see #34050) or it's a bug on dotorg.

#41 follow-up: @johnbillion
8 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

r34561 has broken the order of comments. The comment_order option is having the opposite of its intended effect.

#42 @johnbillion
8 years ago

Actually it might not have been r34561, but one of the recent changes to comment behaviour.

#43 in reply to: ↑ 41 @boonebgorges
8 years ago

Replying to johnbillion:

r34561 has broken the order of comments. The comment_order option is having the opposite of its intended effect.

Are you able to give more precise instructions on how to reproduce? There are a number of discussion settings that interact in weird (and awful) ways. What are your values for 'comment_order' and 'default_comments_page'?

#44 @johnbillion
8 years ago

As far as I know, I've not altered any discussion settings from the defaults. This is on a vanilla trunk install.

comment_order is asc. default_comments_page is newest.

https://i.imgur.com/JbLpuLC.png

https://i.imgur.com/Xz1N1kO.png

#45 @johnbillion
8 years ago

This is reproducible on a brand new, fresh as daisies, vanilla install. Install WordPress, then leave two comments on the default post (you'll need to moderate the first comment).

#46 @boonebgorges
8 years ago

Thanks for the additional details. This has to do with the legacy logic in comments_template() that separated the logic for multiple-page-threads from single-page-threads. It's no longer relevant, so I'm going to remove it. Some unit tests and a fix coming up.

#47 @boonebgorges
8 years ago

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

In 34669:

Fix comment_order for single page comment threads.

The old comment pagination logic had a separate block for comment threads that
appeared on a single page. After the refactoring in [34561], all comment
pagination logic is unified.

This change ensures that 'comment_order' is respected in all scenarios.

Fixes #8071.

#48 @boonebgorges
8 years ago

In 34713:

Fix logical errors in some comment pagination tests.

The tests included in [34669] contained a couple of problems:

  • Comments were not always created in the expected order, due to the incorrect use of 'comment_date_gmt'.
  • 'asc'/'desc' and 'older'/'newer' were confused.
  • 'default_comments_page=newest' ('last') didn't properly account for the cpage=1 offset.

The code itself powering this pagination was correct; it's only the tests that
were wrong.

See #8071.

#49 in reply to: ↑ 33 @Viper007Bond
8 years ago

Replying to boonebgorges:

The way that comment pagination works is complicated and not very intuitive. The 0th page of comments - what you see when reading a post with no 'cpage' query var - contains either the most recent or the oldest #per_page comments, depending on your 'default_comments_page' setting. However, the behavior of non-zero 'cpage' is *not* sensitive to 'default_comments_page'; cpage=2 always contains more recent comments than cpage=3, etc. In other words, when 'default_comments_page=newest', the order of comment pages (if you have 4 pages total) is 0, 3, 2, 1, where if you show the oldest page first, the order is 0, 1, 2, 3. (The magic, such as it is, is in Walker_Comment.)

I don't think this makes a lot of sense, but I also don't know what would break if we changed the behavior. It's very likely that someone out there is building custom pagination links, and is expecting the behavior described above.

This was my doing when I helped write comment paging back in the day and it was done this way so that comment permalinks are actually permanent.

cpage=2 always contains more recent comments than cpage=3, etc

That's backwards. Page 1 is always the oldest comments, page 2 are slightly newer comments, and so forth.

It needs to be this way otherwise as new comments were added, older comments would be bumped off of one page number and onto the next, rendering permalinks useless. What page a comment is on shouldn't ever change regardless of new comments or what page is displayed by default.

The only exception to this is if you change the number of comments per page. Only way to avoid that is to have true comment permalinks instead of just page numbers with anchor tags.

#50 @boonebgorges
8 years ago

That's backwards. Page 1 is always the oldest comments, page 2 are slightly newer comments, and so forth

Right - I misspoke. Sorry about that. (Staring at oldest/newest, first/last, asc/desc for long enough will get to you.)

it was done this way so that comment permalinks are actually permanent.

Ah, this makes sense. Thanks for chiming in :)

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


8 years ago

#52 @boonebgorges
8 years ago

In 34729:

Simplify pagination logic in comments_template().

[34561] "fixed" the problem of newest-first comments showing fewer than
'per_page' comments on the post permalink when the total number of comments
was not divisible by 'per_page'. See #29462. But this fix caused numerous
other problems. First, comment pages reported by get_page_of_comment()
(which expects comment pages to be filled oldest-first) were no longer correct.
Second, and more seriously, the new logic caused comments to be shifted
between pages, making their permalinks non-permanent.

The current changeset reverts the changed behavior. In order to preserve the
performance improvements introduced in [34561], an additional query must be
performed when 'default_comments_page=newest' and 'cpage=0' (ie, you're viewing
the post permalink). A nice side effect of this revert is that we no longer
need the hacks required to determine proper comment pagination, introduced in
[34561].

See #8071. See #34073.

#53 @boonebgorges
8 years ago

In 34730:

Prevent extra db queries in WP_Comment::get_children().

WP_Comment_Query::fill_descendants() queries for a comment tree in a way that
minimizes database overhead, and places the located descendants with their
proper parents. However, it doesn't touch leaf nodes - comments with no
children - so future calls to get_children() on those comment objects
result in unnecessary database queries. To prevent this, fill_descendants()
now sets a populated_children flag on all located WP_Comment objects.

See #8071.

#54 @smerriman
8 years ago

A lot of sites will currently (by choice) be displaying all comments on the page with the page_comments checkbox unchecked. Some of them may even be using themes which don't support paged comments because they have no need for this.

Will this change result in those sites 'breaking' until the comments per page setting is manually changed to a huge number? If so, should the WP update automatically change this to a 'huge number' if the checkbox was unchecked to prevent this?

#55 @boonebgorges
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@smerriman - Thanks for your comment. I don't have a lot of sympathy for themes that don't have comment pagination built in - this should be a basic feature of any them. But I am sympathetic to sites that will suddenly have the behavior of their sites changed in a radical way. And thinking it over, I see that comment permalinks will be affected fairly broadly by this change, since comment-page-x is part of comment permalinks, and forcing pagination is going to move some comments to secondary pages.

One of the pillars of this ticket is that we must force pagination at some point. Originally, wonderboymusic had suggested that we set a threshold higher than comments_per_page. In working on the patch, I decided that forced pagination made the 'page_comments' setting pointless, so I pulled it out. But in retrospect, the problems described above suggest that we should be much more delicate about forcing pagination. I think I'd like to reintroduce page_comments, and continue to respect it, until we get to a fairly high threshold - maybe 250 or 500 - at which point we'll force pagination.

This ticket was mentioned in Slack in #meta by otto42. View the logs.


8 years ago

#57 @DrewAPicture
8 years ago

@boonebgorges So what's left here? Needs a new patch?

#58 @wonderboymusic
8 years ago

  • Type changed from enhancement to task (blessed)

@boonebgorges
8 years ago

#59 @boonebgorges
8 years ago

  • Keywords has-patch added; needs-patch removed

In an effort to minimize comment "permalink" breakage, 8071.8.diff does the following:

  • Restores 'page_comments'
  • Changes the default value for 'page_comments' from 0 to 1
  • Introduces a couple of new helper functions. wp_get_top_level_comments_count( $post_id ) does what it says. wp_get_comment_pages_count( $post_id ) gets the *canonical* comment-page count for a given post. By "canonical" I mean the count that is actually used when WP how to build comment pages (in comment_template()). This means that it respects threading as well as comments_per_page. It also doesn't require full comment objects. In these ways, it differs from the existing get_comment_pages_count().
  • When page_comments is disabled, and the comment count for a post is above the (filterable) 'comment_paging_threshold', comment pagination is forced, with 'comment_paging_threshold' used as the per-page value.

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


8 years ago

#61 @boonebgorges
8 years ago

After some discussion, and lots of teenage angst, I've decided to revert the 'page_comments' changes. Forcing comment pagination raises lots of UX and backward compatibility issues that need to be addressed. After the revert, I'll move the latest patches to a new ticket, for consideration in a future release.

#62 @boonebgorges
8 years ago

In 35331:

Don't force comment pagination.

[34561] instituted the policy of forcing pagination for comments. This strategy
was intended to avert problems when 'page_comments' is set to 0 - as it is by
default - and the number of comments on a given post rises into the hundreds or
thousands. By forcing pagination in all cases, we ensured that WordPress would
not time out by processing unwieldy numbers of comments on a given pageload.

The strategy proves problematic, however, because comment permalinks are
generated using the page of the comment. Forcing pagination for posts that
were not previously paginated would change the URL of all comments that do not
appear on the default comment page.

This changeset reintroduces the 'page_comments' setting and its corresponding
checkbox on Settings > Discussion. A number of tests, which were written after
[34561], are modified to work now that 'page_comments' will, once again, be
disabled by default.

See #8071.

#63 @boonebgorges
8 years ago

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

[35331] stops forcing comment pagination. While this revert is a shame :( we did actually manage to accomplish some real improvements. Comments are loaded in a much more svelte manner [34310]. And we now only load the comments required by the page, instead of loading them all and letting Walker_Comment do the pagination work; this part of [34561] survived, and it addresses the issue that the current ticket was originally opened for.

I've opened #34389 for further discussion of the performance problems that arise from setting page_comments to 0. I think we can mark the current ticket as fixed.

#64 @boonebgorges
8 years ago

In 35933:

Omit cpage query var in comment link if comment pagination is disabled.

WP 4.4 changed the way comment pagination is calculated. See #8071. In the
context of get_comment_link(), these changes introduced a regression that
causes cpage (or its pretty-permalink correlate comment-page-x) to appear
in comment links when comment pagination is disabled. The current changeset
fixes the regression.

Fixes #34946.

#65 @boonebgorges
8 years ago

In 35934:

Omit cpage query var in comment link if comment pagination is disabled.

WP 4.4 changed the way comment pagination is calculated. See #8071. In the
context of get_comment_link(), these changes introduced a regression that
causes cpage (or its pretty-permalink correlate comment-page-x) to appear
in comment links when comment pagination is disabled. The current changeset
fixes the regression.

Merges [35933] to the 4.4 branch.

Fixes #34946.

#66 @boonebgorges
8 years ago

In 36040:

Respect approval status when determining comment page count in comments_template().

Since 4.4, when fetching the first page of comments and the 'newest' comments
are set to display first, comments_template() must perform arithmetic to
determine which comments to show. See #8071. This arithmetic requires the
total comment count for the current post, which is calculated with a separate
WP_Comment_Query. This secondary comment query did not properly account for
non-approved comment statuses; all unapproved comments should be part of the
comment count for admins, and individual users should have their own
unapproved comments included in the count. As a result, comments_template()
was, in some cases, being fooled into thinking that a post had fewer comments
available for pagination than it actually had, which resulted in empty pages
of comments.

We correct this problem by mirroring 'status' and 'include_unapproved' params
of the main comment query within the secondary query used to calculate
pagination.

Fixes #35068.

#67 @boonebgorges
8 years ago

In 36041:

Respect approval status when determining comment page count in comments_template().

Since 4.4, when fetching the first page of comments and the 'newest' comments
are set to display first, comments_template() must perform arithmetic to
determine which comments to show. See #8071. This arithmetic requires the
total comment count for the current post, which is calculated with a separate
WP_Comment_Query. This secondary comment query did not properly account for
non-approved comment statuses; all unapproved comments should be part of the
comment count for admins, and individual users should have their own
unapproved comments included in the count. As a result, comments_template()
was, in some cases, being fooled into thinking that a post had fewer comments
available for pagination than it actually had, which resulted in empty pages
of comments.

We correct this problem by mirroring 'status' and 'include_unapproved' params
of the main comment query within the secondary query used to calculate pagination.

Merges [36040] to the 4.4 branch.

Fixes #35068.

#68 @boonebgorges
8 years ago

In 36157:

Ensure that non-default pagination values work in wp_list_comments().

Prior to 4.4, it was possible to pass 'page' and 'per_page' values to
wp_list_comments() that do not match the corresponding global query vars.
This ability was lost in 4.4 with the refactor of how comments_template()
queries for comments; when the main comment query started fetching only the
comments that ought to appear on a page, instead of all of a post's comments,
it became impossible for the comment walker to select comments corresponding to
custom pagination parameters. See #8071.

We restore the previous behavior by (a) detecting when a 'page' or 'per_page'
parameter has been passed to wp_list_comments() that does not match the
corresponding query vars (so that the desired comments will not be found in
$wp_query), and if so, then (b) querying for all of the post's comments and
passing them to the comment walker for pagination, as was the case before 4.4.

Props boonebgorges, smerriman.
Fixes #35175.

#69 @boonebgorges
8 years ago

In 36158:

Ensure that non-default pagination values work in wp_list_comments().

Prior to 4.4, it was possible to pass 'page' and 'per_page' values to
wp_list_comments() that do not match the corresponding global query vars.
This ability was lost in 4.4 with the refactor of how comments_template()
queries for comments; when the main comment query started fetching only the
comments that ought to appear on a page, instead of all of a post's comments,
it became impossible for the comment walker to select comments corresponding to
custom pagination parameters. See #8071.

We restore the previous behavior by (a) detecting when a 'page' or 'per_page'
parameter has been passed to wp_list_comments() that does not match the
corresponding query vars (so that the desired comments will not be found in
$wp_query), and if so, then (b) querying for all of the post's comments and
passing them to the comment walker for pagination, as was the case before 4.4.

Merges [36157] to the 4.4 branch.

Props boonebgorges, smerriman.
Fixes #35175.

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


8 years ago

Note: See TracTickets for help on using tickets.