Wrong post position displayed in topic replies

Troky is maintaining this branch and I'm not using the module at the moment. This issue sounds really familiar, though, so I suggest searching the issue queue.

Michelle

Definitely a bug.

This patch should fix the problem. Please try it and report back.

Try new patch.
(roll back old patch before applying new one)

Are you sure you patched advanced_forum_preprocess_comment.inc, too?
Check line 89... it should be:

$post_position = advanced_forum_post_position($node, $comment);

Thanks for testing.
Committed to 7.x-2.x-dev.

There's mistake in #7 patch.

$post_id = $comment->pid;

should be

$post_id = $comment->cid;

Marked #2092575: Incorrect reply numbers in comments as duplicate of this issue.

#13 commited
Thanks!

I installed the latest dev version today (2014-Jun-21) and now all my "Reply to" numbering is broken. It was working fine before the latest dev.

I looked at the patch from #13 where the line $post_id = $comment->pid; was changed to $post_id = $comment->cid;. I edited advanced_forum.module and changed it back to pid and the numbering is working correctly again. So for me this patch breaks Advanced Forum reply-to numbering.

Looking further... The above Reply to numbering was on my Forum. Changing to pid corrects the Forum. But after looking at Node Comments I noticed that neither cid nor pid works correctly after updating to the latest dev version.

I am sorry! I can confirm #18 - my patch from #13 breaks reply-to numbering.

It seems like comment order on forum differs from regular node comments. Quick workaround (needs prior testing, review and work of somebody who understand this difference):

      if ($node->type == 'forum') {
        $post_position = $post_position + 2;
      }

added this thread to release known issues
https://www.drupal.org/node/2290369
Please, feel free to upload patch against latest dev to fix this issue asap.

Thanks in advance

Can anyone install #20 patch and test it?
If it is working, I`ll commit/tag quickfix release

It seems like the check for node type being "forum" should really be checking for all node types designated for Adv Forum. In my case, that would be "forum", "poll", and "advpoll".

The issue here isn't the math, it's the logic in the code that's generating the link text. There are two nodes you're worried about here -- parent and child. With the exception of the link, all the data that's being used is from the child node. Here's the problem, just in these two lines in advanced_forum_preprocess_comment.inc (line 94-96, I think, but I've been messing with the file and not neatly):

if ($comment->pid > 0) {
    // Find the display position of the parent post.
    $post_position = advanced_forum_post_position($node, $comment);

You're checking that the parent id exists, and then find the position of the child node -- the one that you're currently rendering -- when what you want is the position of the parent node. Since the pagination is tied to the $post_position the links will also break if the parent post is on another page.
A quick solution, though not terribly efficient, would be the following:

  if ($comment->pid > 0) {
    // Find the display position of the parent post.
    $parent = comment_load($comment->pid);
    $post_position = advanced_forum_post_position($node, $parent);

I've checked this and it works, at least for flat or threaded ascending sorts. Descending sorts are a disaster, but that's to be expected, I suppose, as the page/position calculations assume ascending sorts. I've added a patch, if someone wants to give it a try.

This patches the logic error in advanced_forum_preprocess_comment.inc where the child node was being used in place of the parent node for position calculations.

Oops. No more patches after midnight for me. Let's see if this one flies.

comment_load() in #32 can affect perfomance (anyway, #32 works)

here is improved version of #20. As mentioned by @ExTexan in #27, we need to check for all node types with the forum style applied.
This patch works for me, please try it.

I thought about that, but the only way I can see around it is to rewrite advanced_forum_post_position to use $comment->id instead of $comment. I didn't want to do that without knowing what else depended on that function. As it happens, this is the only call to advanced_forum_post_position in the module.
That said, rewriting advanced_forum_post_position would remove the check for deleted or otherwise invalid comments.

Patch 1724080-34.patch from #34 works for me.
Thanks for the patch!

Thanks for the patch and the comments. I'm just getting back into AF again and will try to have a look soon.

Patch in #34 works for me as well.

Sorry for the long delay. Life's been kinda crazy this last month and I didn't get to Drupally things like I thought I would. I'm off for a week of training at my new job but will try to set aside time for contrib as soon as I can.

I applied the patch from #34 and it fixed everything. Well almost everything. The only problem now is that when you click on a node comment (reply to #) link they all work correctly except for (reply to #1). When I select (reply to #1) the screen refreshes and all of the comment numbering is all messed up. For instance the first comment is numbered -49. Then going down the list they are -48, -47, etc. Clicking on any of the other links besides (reply to #1) sets everything back as it should be.

Here is an example node comment

On Forum posts there is no (reply to #1). They start out at (reply to #2). So I'd give #34 a "close but no cigar" rating. If I knew anything about programming I'd take a run at trying to fix it.

Bill, thanks for testing!
Please try that (for me it fixed the problem that you described):

includes/advanced_forum_preprocess_comment.inc

-      $page_number = floor(($post_position - 2) / $posts_per_page);
+      $page_number = floor(($post_position - 1) / $posts_per_page);

@dberror that seems to have fixed it. Thanks!

#41 commited. Thanks!

Sorry to come (back) so late to this thread. For some strange reason, that patch doesn't work in my case. I had to change this:

      if (in_array($node->type, $advanced_forum_styled_node_types)) {
        $post_position = $post_position + 1;
      }

...back to "+2".

But there's also another related issue... If you are styling all comments like forum posts, the reply-to post number is off by one. So, I had to add an "else" to the above "if" to add 1. The permanent fix should really check the setting for "style all comments like forum posts", but I was trying to get this fixed quickly, so I did the simple "else" patch.

Hi all,

The first cause of this simple issue not succeeding is that the function advanced_forum_post_position has been patched to take a comment object as the second argument, rather than the original comment id - and it uses $comment->cid, so it returns the position of the comment, not his parent...

So, I restored the arg list, and call it with $comment->pid.

Also, add 1 to the return value, as comment with index 0 should be displayed '#1'.

The reason that it has to add 2 for forum posts and add 1 for posts styled like forum posts is because...

1. For forum threads, the node is #1 (add 1 for that), and the $post_position starts with 0 (add one for that).
2. For non-forum threads, the node is NOT #1 (don't add 1 for that), but still add one because $post_position starts with 0.

I've found that the same situation applies to the part of the above patch that is for advanced_forum_preprocess_comment.inc where it calculates the page number for the link back to the "replied-to" post. That calculation needs to subtract 2 for forum posts, and subtract 1 for non-forum posts styled as forum posts.

Edit: @gifad, your reply was posted while I was writing mine. I'm not sure how your changes will affect/solve the situations I wrote about just now. Could you revisit the patch based on my observations and see if there's still an issue? My feeling is that it will still need to handle the forum posts vs. non-forum posts styled as forum posts situation.

I just updated to 7.x-2.5. I realize that the release date is right around the same time as the last few posts above, so I'm not surprised that some of the latest "fixes" didn't make it into that version. Since I had to redo the fixes just yesterday (and they are fresh in my mind), I thought I'd post my code here, in case it helps.

I have three content types that are tagged as forum content, and 2 or 3 others that are not, but whose comments are styled like forum comments. The following code is what's working for me in both cases:

This is at line 1372 of advanced_forum.module. If I let it use cid, the "Reply to #x" values are all wrong. For example, if #4 replies to #2, it will show "Reply to #4".

  $node_id = $node->nid;
//  $post_id = $comment->cid;
  $post_id = $comment->pid;

At line 1414 (may actually be line 1413 because of my extra commented-out line above) of advanced_forum.module, I have this...

      if (in_array($node->type, $advanced_forum_styled_node_types)) {
        $post_position = $post_position + 2;
      } else {
        $post_position = $post_position + 1;
      }

The reason for that is explained in my post #49 above.

And at line 102 of advanced_forum_preprocess_comment.inc, I now have this...

      if (in_array($node->type, $advanced_forum_styled_node_types)) {
        $page_number = floor(($post_position - 2) / $posts_per_page);
      } else {
        $page_number = floor(($post_position - 1) / $posts_per_page);
      }

Again, same reason as above. Without this change, the "page=x" value (in the "Reply to #x" link) is wrong for posts that are at the top of the current page, and/or the bottom of the previous page.

I guess it still needs work?

Well, I can't explain why using cid is working(?) for others, because using pid is the only thing that works for me.

But the other two patches are because, for forum nodes, the node itself is counted as post #1, but for non-forum nodes it isn't. To say it another way, the first comment in forum threads is #2 - the first comment in non-forum threads is #1.

I think if you use the code from #50, it should work. Besides, it will certainly be "reviewed and tested by the community" here.

instead of fighting about choosing $comment->cid or $comment->pid , I'd suggest in #48 to revert the function signature to its original :

function advanced_forum_post_position($node, $post_id)

and let the caller choose which post he/she refers to...

@gifad,

Perhaps the reason it didn't work when I tried it might be related to this part of your reply... "let the caller choose which post s/he refers to". I suppose that would necessitate changes to other parts of advanced_forum not addressed by your patch. I don't know, I'm just speculating here.

But aside from that, regarding this from your #48 post... "Also, add 1 to the return value, as comment with index 0 should be displayed '#1'." -- I think that would need the "add 1 or 2" logic I described above - to handle the forum-thread / non-forum-thread situations - because comment index 0 will be #2 or #1 (respectively) in those two cases.

I implemented this solution which inside the function verifies if it is a threaded reply and uses the comment parent to calculate the reply to position, or if on flat view uses the default logic.

Recently I've just noticed that quoted post number is always leading to the previous post, for example in post #6 it will say 'In reply to post #5' and so on. The link to the quoted post is correct though. Has anyone experienced this very behaviour?
I am using version 7.x-2.5.

@c470ip, yes, that's what this thread is trying to fix, so you've posted in the right place.

@rDeeb, I've never had to add 2 or 3... only 1 or 2 (depending).

The code I posted in #50 (sorry it's not in the form of a patch), has been working for me for several months in a site with a mix of forum comments, and non-forum comments styled like forum comments. It seems pretty straight forward to me. How about we put that in the dev version and get to the "testing by community" stage?

@ExTexan Just tried your solution. Changed advanced_forum.module and advanced_forum_preprocess_comment.inc according to #50.
Got the following errors:
Notice: Undefined variable: advanced_forum_styled_node_types in function _advanced_forum_preprocess_comment() (line 101 in file /home/***/www/sites/all/modules/advanced_forum/includes/advanced_forum_preprocess_comment.inc).
and
Warning: in_array() expects parameter 2 to be array, null given in function _advanced_forum_preprocess_comment() (line 101 in file /home/***/www/sites/all/modules/advanced_forum/includes/advanced_forum_preprocess_comment.inc).
several times per each opened forum threads (possibly the same times as quote referrers were met).

I just reverted advanced_forum_preprocess_comment.inc to its original version, now the errors are gone, quoted post numbers still remaining correct.

Oops. I guess I should have included the line of code before that too. I had already applied a patch from earlier in this thread - one that determines if the node type is styled as a forum post. It didn't work completely (as per my post #50), but I kept the line that determines styled node types. Sorry for the confusion.

Here's the more complete code, with my rather verbose comments.

In advanced_forum.module, around line 1405...

  // Find the position of the passed in post ID.
  $post_position = 0;
  if (is_array($post_order[$node_id])) {
    if (($index = array_search($post_id, $post_order[$node_id])) !== FALSE) {
      $post_position = $index;

      $advanced_forum_styled_node_types = variable_get('advanced_forum_styled_node_types', array('forum'));
      // If "Style all comments like forum replies" is selected, ALL comments are
      // being processed by advanced_forum, which includes forum posts and non-forum
      // posts.  For non-forum posts, we need to add 1 to $post_position because the
      // first comment starts at zero.  For forum posts, we need to add 2; 1 for
      // $post position starting at zero, and 1 because the topic node is considered
      // post #1, but is not included in the index.
      if (in_array($node->type, $advanced_forum_styled_node_types)) {
        $post_position = $post_position + 2;
      } else {
        $post_position = $post_position + 1;
      }
    }
  }

In advanced_forum_preprocess_comment.inc, just after "if ($post_position > 0) {" - (line 98?) - and before "// Assemble the link."...

      $advanced_forum_styled_node_types = variable_get('advanced_forum_styled_node_types', array('forum'));
      // If "Style all comments like forum replies" is selected, ALL comments are
      // being processed by advanced_forum, which includes forum posts and non-forum
      // posts.  For non-forum posts, we need to adjust $post_position down by 1 to
      // get it back to where the first comment starts at zero.  In the case of forum
      // posts, we need to subtract 2; 1 for first comment starting at zero, 1 because
      // the topic node is considered post #1, but is not actually in the index.
      if (in_array($node->type, $advanced_forum_styled_node_types)) {
        $page_number = floor(($post_position - 2) / $posts_per_page);
      } else {
        $page_number = floor(($post_position - 1) / $posts_per_page);
      }

As the Advanced forum module has renewed to 2.6, indicated post positions became wrong again.

@ExTexan I just tried the patch in #59, unfortunately what it does is just change quoted link from [previous_post] to [current_post], that means quoted posts now always point to themselves.
#50 still works fine for me (only changes in module, not the .inc file).
Thank you for your work.

@c470ip,

That's really strange, because I have carried that code through three or four versions of AF now. My site has more than 75,000 posts on 3500 topics, with a mix of posts that are actually on AF content types and ones that are "styled like forum posts". All post numbers (and underlying links) are working perfectly.

I just looked back at #50. Perhaps the first code block there takes care of the "parent" vs. "current" issue you mentioned.

[Sorry the code in my comments ended up in different posts.]