Making WordPress.org

Opened 2 years ago

Last modified 18 months ago

#6443 new defect (bug)

/page/2/ being adding to reply links incorrectly

Reported by: sterndata's profile sterndata Owned by:
Milestone: Priority: high
Component: Support Forums Keywords:
Cc:

Description

Replies on the first page of a topic and getting "/page/2/" added with the links, breaking them. For example, see https://wordpress.org/support/topic/smileys-not-working-on-mobile/?view=all

This reply is still on the first page:

https://wordpress.org/support/topic/smileys-not-working-on-mobile/page/2/?view=all#post-15900871

but doesn't work unless /page/2/ is removed.

It may be significant that this is the 10th reply in the chain. Subsequent replies also have /page/2/

Attachments (1)

6443.patch (1.5 KB) - added by tellyworth 21 months ago.
Call bbp_update_reply_position() after removing replies from a thread

Download all attachments as: .zip

Change History (25)

#1 @dd32
2 years ago

I suspect this is a bug from bbPress or WordPress trunk, threads show 15 replies per page, but I guess the pagination code is assuming 10-per-page, I'm not sure where it's inheriting that number from though.

#2 @sterndata
2 years ago

Does this matter, under SETTINGS->READING?https://i.imgur.com/SuZ61Wh.png

#3 @dd32
2 years ago

Does this matter, under SETTINGS->READING?

I did check that setting, but as it's set to 12 I disregarded it as being potentially related.. since this is kicking in at post 10 and there didn't appear to be any hidden replies prior to that.

#4 @sterndata
2 years ago

Just a note that this is affecting real users. They get an email from the forums with a link to a post and when they click it, they don't see it because the pagination is wrong. Can we bump up the priority a bit on this?

#5 @dd32
2 years ago

Took a quick look into this.. Per-forum-page is set to 15, and the page a reply is on is generated through basically bbp_get_reply_position() / bbp_get_replies_per_page().

The problem is that bbp_get_reply_position() is returning the incorrect result for the thread, and other threads.

On https://wordpress.org/support/topic/smileys-not-working-on-mobile/?view=all that should be returning 1 through 15, but instead, the position is being returned as... (with > 15 being page=2)

1, 3, 4, 6, 7, 9, 10, 12, 13, 15, 17, 18, 18, 22, 23 (archived)

It's not immediately obvious to me as to why there are missing reply position numbers in there..

#6 @dd32
2 years ago

As somewhat of a bandaid stop-gap fix, we could redirect /page/xx/ to the first-page to avoid the situation where accessing a too-high-page pagination url returns the thread starter without any replies.

edit: replaced reference to last-page with first page, as last-page is probably inaccurate for affected threads.

Last edited 2 years ago by dd32 (previous) (diff)

This ticket was mentioned in Slack in #forums by dd32. View the logs.


2 years ago

#8 @sterndata
2 years ago

There is one archived post in that topic, but it's near the bottom of the first page of replies.

This ticket was mentioned in Slack in #forums by dd32. View the logs.


2 years ago

This ticket was mentioned in Slack in #forums by ipstenu. View the logs.


22 months ago

#12 follow-up: @bcworkz
22 months ago

It's not immediately obvious to me as to why there are missing reply position numbers in there..

I'm pretty sure the missing replies were spam replies that have been deleted. Apparently the reply count does not get updated when spam is removed, causing the pagination to behave as though the spam is still there.

#13 in reply to: ↑ 12 @dd32
22 months ago

Replying to bcworkz:

It's not immediately obvious to me as to why there are missing reply position numbers in there..

I'm pretty sure the missing replies were spam replies that have been deleted. Apparently the reply count does not get updated when spam is removed, causing the pagination to behave as though the spam is still there.

That would certainly account for it!

Looking at bbPress, I only see bbp_update_reply_position() being called for merged/split topics, and from within the reply-position repair tool. (edit: the menu_order repair tool, I'm not sure if that's the same thing)

Perhaps what we should be looking at here is having bbp_update_reply_position() run for topics after any reply is archived/spammed/deleted.

This feels like it's an upstream bbPress bug, but one that we can probably patch over on w.org while we figure out a way to reproduce it in a test case for upstream fixing.

Last edited 22 months ago by dd32 (previous) (diff)

This ticket was mentioned in Slack in #forums by zoonini. View the logs.


22 months ago

#15 @sterndata
21 months ago

I'd like to bump this on the priority list. I get at least 3 emails a day with bad pagination links. It makes it harder to moderate stuff. BUT, more importantly, it affects "real" users, too, and as it's a publicly facing bug, we should get it fixed ASAP.

This ticket was mentioned in Slack in #forums by zoonini. View the logs.


21 months ago

#17 @tellyworth
21 months ago

From a brief dive into the code, it looks like the Recalculate position of each reply in each topic repair job in Tools / Forums / Repair Forums will run bbp_update_reply_position(). Though I'm not sure it'll run it on the topics where pagination is broken.

I haven't run it because it's listed as High Overhead. @dd32 would you say it's safe to run once? (Once a week?)

#18 @dd32
21 months ago

I haven't run it because it's listed as High Overhead. dd32 would you say it's safe to run once? (Once a week?)

I would not say it's safe to run at all on WordPress.org/support.

From my comment above:

Perhaps what we should be looking at here is having bbp_update_reply_position() run for topics after any reply is archived/spammed/deleted.

I think that's safe though, I'm not sure if it'll fix the issue or not though.

@tellyworth
21 months ago

Call bbp_update_reply_position() after removing replies from a thread

#19 @tellyworth
21 months ago

I think attachment:6443.patch gets us part of the way there. There might be some caching issues to deal with, and I haven't tested it thoroughly. There are probably other places it should hook into as well (unspammed, unarchived?).

Last edited 21 months ago by tellyworth (previous) (diff)

#20 @dd32
20 months ago

Unfortunately bbp_update_reply_position() doesn't completely do the trick here it seems.

It turns out this is a deeper problem than I thought.. In addition to the problem where a reply simply no longer exists (ie. fully deleted), when replies are spam or archived, they still occupy a reply ID, and so the problem is that the URL pagination generation is unaware of some replies not being visible to non-moderators.

https://wordpress.org/support/topic/smileys-not-working-on-mobile/ is now an example of that. The 15th reply is archived, and so when it's not being viewed by a moderator with ?view=all the 15th reply (last reply on the first page) has a /page/2/ url.

Either

  1. bbp_get_reply_url() needs to handle the reply id being incorrect OR
  2. bbp_get_reply_position_raw() needs to change to not give reply IDs to hidden replies.

The problem with these is that with 1 it means iterating over all replies prior to the reply in the thread to determine how many are hidden (..or missing?), and lower the reply_id by that many, and with 2 is that when a moderator is viewing it, the reply_ids need to be in the correct order to render the replies in the correct order. 1 would also mean that the links would be incorrect for moderators (but that's okay I guess).

As much as it seems like a performance hit, 1 is the obvious choice by filtering bbp_get_reply_url, I guess we can get away without much of an impact by only checking when the URL is >= /page/2 as replies on the first page aren't affected. The replies should be being displayed anyway so should also be cached in memory already.

Last edited 20 months ago by dd32 (previous) (diff)

This ticket was mentioned in Slack in #forums by yui. View the logs.


20 months ago

#22 @dd32
20 months ago

I've filed this bug upstream with bbPress: https://bbpress.trac.wordpress.org/ticket/3503

This ticket was mentioned in Slack in #forums by sterndata. View the logs.


18 months ago

#24 @sterndata
18 months ago

#6725 was marked as a duplicate.

Note: See TracTickets for help on using tickets.