Make WordPress Core

Opened 15 years ago

Closed 14 years ago

#8841 closed defect (bug) (fixed)

Threaded comments display out of order if conditions are met.

Reported by: clifgriffin's profile clifgriffin Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.7
Component: Comments Keywords:
Focuses: Cc:

Description (last modified by ryan)

In Wordpress 2.7, threaded comments have a built in limitation that determines how many levels of comment nesting are allowed. This prevents users from replying to messages that are at the nested limit.

The admin interface has no such limitation. Admins are able to reply to messages that are at the nested level limit.

When this happens, comments begin appearing (for all intents and purposes) out of order.

This can also be caused by changing the nested level to a smaller number after comments exist.

Examples of this problem are linked to in this discussion:
http://wordpress.org/support/topic/227413?replies=19

Change History (25)

#1 @ryan
15 years ago

  • Milestone changed from 2.7.1 to 2.8

I think we'll have to change the comment walker to ignore the depth limit when listing comments. Only the reply links should check the depth.

#2 @GregMulhauser
15 years ago

Actually, it's somewhat worse than this -- the new comments code in 2.7 has serious problems handling what I'll refer to as 'orphaned' comments, those where either 1) the parent comment has been deleted or 2) the orphaned comment was entered at a deeper depth than the depth currently specified in the discussion settings. (The admin situation described by clifgriffin falls under 2.)

When comments are orphaned, not only will WP display them in the wrong order (unless threading is switched off completely -- which incidentally is different than setting depth to 1, in terms of its impact on this problem), but when a user attempts to reply to a comment that has been orphaned via parental deletion, WP also then redirects to a broken URL when the user completes their entry, due to further problems in the function which returns the page number of a comment. (Specifically, we get a URL ending in the form "comment-page-/#123", where "123" is the comment ID. Note the lack of any page number.)

I realise this is long on symptoms and short on diagnosis -- and there are multiple problems going on here, which probably merits multiple detailed bug reports on exactly which functions are fouling things up and why. But I hope this adds something just to let folks know there's more than one problem afoot when it comes to comment walking...

#3 @ryan
15 years ago

  • Description modified (diff)

#9003 fixes reparenting of comments after the parent is deleted. It is still possible to get an "orphan" if a comment that has children is marked as spam or put into pending.

#4 @Denis-de-Bernardy
15 years ago

  • Keywords 2nd-opinion added; comments threading removed

I haven't looked into the actual code, but the parent comment should be enforced regardless of the max depth, due to the fact that themes may allow for, or enforce, different maximum depths.

what should happen, I think, is that the walker should enforce the max_depth, and treat all comments beyond a certain real_depth as being of max_depth -- with the two variables being counted for separately.

#5 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added

#6 @Denis-de-Bernardy
15 years ago

  • Milestone changed from 2.8 to Future Release

moving to future, pending patch

#7 @Denis-de-Bernardy
15 years ago

  • Milestone changed from Future Release to 2.9

#8 @Denis-de-Bernardy
15 years ago

  • Keywords 2nd-opinion removed

#9 @aaroncampbell
14 years ago

This also happens if the comment is a reply to a pingback (or other special comment type) that is not included in the query. When I stopped displaying pingbacks on my page I ended up with a bunch of these.

#10 @aaroncampbell
14 years ago

  • Cc aaroncampbell added

#11 @hailin
14 years ago

sorry I wasn't aware of this discussion earlier.

  1. "change the comment walker to ignore the depth limit when listing comments" we can just call the walker with max_depth = big number (such as 9999), but don't include "reply" link on comments beyond the max_depth.


  1. "the walker should enforce the max_depth, and treat all comments beyond a certain real_depth as being of max_depth -- with the two variables being counted for separately"

This may work well, I think. Say if users first set levels to 10, then changed it to 4. So for all comments between level 5 and 10, we just put them on the same level as 4. Not sure if there is any side effect.

#12 @mtdewvirus
14 years ago

  • Cc mtdewvirus@… added

#13 @nacin
14 years ago

  • Milestone changed from 2.9 to 3.0

#14 follow-up: @voyagerfan5761
14 years ago

  • Cc WordPress@… added

I don't know if my issue is the same, but I'm using WP 2.8.6 and having a comment-ordering issue with one of my posts. There are only a few comments, but the whole shebang was imported from Blogger using the built-in tool. See the problem page at http://technobabbl.es/2008/06/two-types-of-friends-found-poem/

The first comment and the second comment are switched. The IDs and timestamps are correct, but they're being sorted out of any logical order. Blogger doesn't have threaded comments, either, and I have threaded comments in WP turned off.

#15 in reply to: ↑ 14 ; follow-up: @Denis-de-Bernardy
14 years ago

Replying to voyagerfan5761:

The first comment and the second comment are switched. The IDs and timestamps are correct, but they're being sorted out of any logical order. Blogger doesn't have threaded comments, either, and I have threaded comments in WP turned off.

yours is a separate issue which comes from the importer, and which should be fixed (for the most part anyway) in 2.9.

#16 in reply to: ↑ 15 ; follow-up: @voyagerfan5761
14 years ago

Replying to Denis-de-Bernardy:

Replying to voyagerfan5761:
yours is a separate issue which comes from the importer, and which should be fixed (for the most part anyway) in 2.9.

Is there a Trac ticket for it? Sorry to have bothered everyone on this issue.

#17 in reply to: ↑ 16 @nacin
14 years ago

Replying to voyagerfan5761:

Is there a Trac ticket for it? Sorry to have bothered everyone on this issue.

#8019

#18 @hakre
14 years ago

TO summarize: The datahandling for comments depth is broken. In every situation where comments can be added it must be checked wether or not that comment can be added. This has not been done consequently (which is a smell for code duplication) and that is why it is broken.

I suggest that the comments API prevents adding comments which are not valid to be added. This might help as well that we do not need to review the whole code to find all places where this bug is in.

#19 @hakre
14 years ago

I found a fix: Disable javascript, then you can not reply to comments any longer in the admin. okay, jokes aside, wp_new_comment() in admin-ajax.php does not properly check on comment_parent for the depth, it uses wp_insert_comment() w/o any additional depth checks. wp_insert_comment() is not doing any depth-checks as well before inserting the new comment into the database.

#20 @hakre
14 years ago

Related: #8384

#21 @dd32
14 years ago

(In [13932]) Fix out-of-order comments when comment nesting is reduced. Displays child comments on the same level after its "parent" in the case that the max_depth has been hit. See #8841

#22 @dd32
14 years ago

  • Keywords needs-patch removed

The commit fixes the displayment of comments after the comment max_depth has been decreased, It does not attempt to validate the parent comment_ID or anything else.

To quote the commit:

	/**
	 * This function operates the same as Walker::display_element(), with one small change.
	 * Instead of elements being moved to the end of the listing when their threading reaches
	 * $max_depth, the children are displayed inline.
	 * 
	 * Example: max_depth = 2, with 5 levels of nested content.
	 * 1
	 *  1.1
	 *    1.1.1
	 *    1.1.1.1
	 *    1.1.1.1.1
	 *    1.1.2
	 *    1.1.2.1
	 * 2
	 *  2.2
	 *
	 */

If anyone notices any issues, please let me know :)

#23 @dd32
14 years ago

(In [13938]) Move children of nested levels not shown to after the current element, not inside it like children. See #8841

#24 @dd32
14 years ago

(In [13939]) Reduce Code duplication. Rely on parent class to do the heavy lifting, just tack the comment addition on the end. See #8841

#25 @dd32
14 years ago

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

Please re-open if someone notices any issues directly related to this.

Note: See TracTickets for help on using tickets.