WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#7956 closed defect (bug) (fixed)

get_comment_link() should be paged comments aware

Reported by: Viper007Bond Owned by:
Milestone: 2.7 Priority: high
Severity: normal Version: 2.7
Component: Comments Keywords:
Focuses: Cc:

Description

get_comment_link() needs to be made page-aware.

wp_list_comments() should also be made to use get_comment_link() rather than a relative URL anchor to ensure the comment permalink is correct.

I'm working on this, but figuring out what page a comment is on is a bitch. Suggestions welcome.

Attachments (10)

7956.patch (3.7 KB) - added by Viper007Bond 5 years ago.
Not finished, see comments
7956.2.patch (3.8 KB) - added by Viper007Bond 5 years ago.
Work when threading is disabled too
7956.3.patch (3.9 KB) - added by Viper007Bond 5 years ago.
If paging is disabled, don't bother with the "gotocom" stuff
7956.4.patch (4.5 KB) - added by Viper007Bond 5 years ago.
Ditch all of the "gotocom" crap and just go directly to the comment's page
7956.5.patch (4.4 KB) - added by Viper007Bond 5 years ago.
Don't add "nofollow" to comment link as it's just a link to another page now
7956_while_fix.patch (1.1 KB) - added by Viper007Bond 5 years ago.
Fix while() loops + don't return int if any parent is deleted/unapproved
7956_while_fix.2.patch (2.2 KB) - added by Viper007Bond 5 years ago.
Properly use get_comments() so it doesn't return the whole DB table + phpdocs
7956_while_fix.2.r9378.patch (2.4 KB) - added by Viper007Bond 5 years ago.
7956.6.patch (4.6 KB) - added by Viper007Bond 5 years ago.
7956.7.patch (4.6 KB) - added by Viper007Bond 5 years ago.
Properly intval the $page, props azaozz

Download all attachments as: .zip

Change History (34)

comment:1 Viper007Bond5 years ago

Actually, I'm going to leave this to a code ninja master. I can't handle this when threading is enabled.

My idea is we need a Walker function that will walk through all elements looking for the specified element and when it's found, it returns the current page number.

function get_comment_link($comment = null) {
	$comment = get_comment($comment);

	if ( get_option('page_comments') ) {
		/* do the stuff here */
	} else {
		return get_permalink( $comment->comment_post_ID ) . '#comment-' . $comment->comment_ID;
	}
}

comment:2 Viper007Bond5 years ago

[Fri 01:13:53] <Viper007Bond> or maybe get all top level, page those (as that's the only thing that gets paged), and then walk upwards from the given comment to find the top level parent and what page that top level parent is on

comment:3 ryan5 years ago

  • Priority changed from normal to high

comment:4 ryan5 years ago

Maybe forget paging when linking to a comment, just link as usual. When handling a comment permalink, show the comment in context of all comments if paging is off. If paging is on show just that comment. This would probably using an actual query arg rather than a fragment, though.

comment:5 ryan5 years ago

Of course, I'd rather go with your idea if it can be made to work. Avoiding page references in comment permalinks would be nice too. Have it figure out the page based on the comment id.

comment:6 Viper007Bond5 years ago

Oh, I was thinking of including the page number in the URL.

I like the idea of a query var that redirects to the proper page though.

Viper007Bond5 years ago

Not finished, see comments

Viper007Bond5 years ago

Work when threading is disabled too

comment:7 Viper007Bond5 years ago

My patch (7956.2.patch) makes get_comment_link() return the post permalink with gotocom=123 tagged onto the end of it.

canonical.php needs to be updated to redirect to /post-name/comment-page-456/#comment-123.

The "456" page number can be determined by passing the gotocom value to the new function this patch introduces, get_page_of_comment(). It will calculate what page a comment will appear on and yes, it is threaded aware.

Viper007Bond5 years ago

If paging is disabled, don't bother with the "gotocom" stuff

comment:8 follow-up: ryan5 years ago

Maybe get_page_of_comment() can setup $wp_query->comments in the same way comments_template() does that way comments_template() can avoid doing a second query if it is already set.

Viper007Bond5 years ago

Ditch all of the "gotocom" crap and just go directly to the comment's page

comment:9 in reply to: ↑ 8 Viper007Bond5 years ago

Replying to ryan:

Maybe get_page_of_comment() can setup $wp_query->comments in the same way comments_template() does that way comments_template() can avoid doing a second query if it is already set.

get_comment_link() and get_page_of_comment() can be used outside of the comments.php template. For example, a "Latest Comments" widget. The comment ID being passed to either of those functions may or may not belong to the post/page that's being displayed.

Viper007Bond5 years ago

Don't add "nofollow" to comment link as it's just a link to another page now

comment:10 ryan5 years ago

(In [9367]) Make get_comment_link() paging aware. Props Viper007Bond. see #7956

comment:11 ryan5 years ago

This ise dying in get_page_of_comment(). Maybe an infinite loop?

comment:12 Viper007Bond5 years ago

Must be the while. That's designed to keep getting the parent comment until it finds the one that has a parent value of 0, i.e. the top level parent.

I'm not sure why it's getting stuck in a loop through for you. It works fine here.

Is there a case where a top level comment's $comment->comment_parent will not be 0?

Viper007Bond5 years ago

Fix while() loops + don't return int if any parent is deleted/unapproved

comment:13 ryan5 years ago

Still getting a loop that exhausts memory.

Comments for the post are

* Top-level
** Second
*** Third
**** Fourth
* Top

comment:14 ryan5 years ago

(In [9377]) get_page_of_comment() fixes from Viper007Bond. see #7956

Viper007Bond5 years ago

Properly use get_comments() so it doesn't return the whole DB table + phpdocs

comment:15 ryan5 years ago

(In [9379]) get_page_of_comment() fixes from Viper007Bond. see #7956

comment:16 ryan5 years ago

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

comment:17 Viper007Bond5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

get_comment_link() is currently using using a heavy MySQL query every single time it's called (due to a call to get_comments() inside of get_page_of_comment()).

Incoming patch (which were all Mark's ideas) ditches the usage of get_comments() inside of get_page_of_comment() and instead just does a simple, lightweight query to find the number of older top-level comments. That's all that's needed as you can then divide that count by the comments per page to get what page that comment is on.

However this patch also allows you to skip even that query by passing a $page value to get_comment_link() if it's known (i.e. if you're in the regular comment loop and not in the sidebar). That way we can recycle the value and don't have to calculate the page for every single get_comment_link() call.

Oh, and the patch also updates the Recent Comments widget to use get_comment_link() (I used it for testing) and fixes a "bug" that resulted in get_comments() returning only the 4 columns that the widget queried for (due to caching).

Viper007Bond5 years ago

Viper007Bond5 years ago

Properly intval the $page, props azaozz

comment:18 ryan5 years ago

That doesn't count orphaned comments as top level though.

comment:19 ryan5 years ago

Right now orphans are stuck at the end of the comment list rather than in their chrono order. Getting the page for them could be tricky, but at least at the end their not screwing with the page number of other comments by not being counted in the query. Orphans should be rare so maybe we should forget about them and call this fix good enough.

comment:20 markjaquith5 years ago

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

(In [9522]) Massive get_comment_link() performance improvements for posts with a lot of comments. props Viper007Bond. fixes #7956

comment:21 Viper007Bond5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

get_comment_link() is now queryless for inside of the default comment loop due to the above changeset.

However I'm still considering how to further optimize it for outside of the comment loop (i.e. recent comments widget) or for themes that use a callback parameter for wp_list_comments() but no $page parameter when they call get_comment_link().

Now that get_comments() caches, it could potentially be better than that changeset which still results in one query per call always. With get_comments(), you'd only query if you hadn't queried for that comment's parent post before.

Example of "Latest 10 Comments":

Current as of [9522]: 10 queries
With get_comments(): Between 1 (all comments on the same post) and 10 (all comments on different posts) queries

Mark pointed out that get_comments() is a heavy query and that one query is not always better than more than one query (get_page_of_comment()'s current query is fairly light).

I guess we'll need to do some trials.

comment:22 Viper007Bond5 years ago

Stupid Trac.

Example of "Latest 10 Comments":

  • Current as of [9522]: 10 queries
  • With get_comments(): Between 1 (all comments on the same post) and 10 (all comments on different posts) queries

comment:23 ryan5 years ago

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

comment:24 Viper007Bond5 years ago

RE Above stuff: it's fine as is, can be taken a further look at for future versions. 2.7.0 needs to get out the door.

Note: See TracTickets for help on using tickets.