Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#6189 closed defect (bug)

Query string in directory URL breaks AJAX pagination

Reported by: dontdream Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Members Keywords: has-patch
Cc:

Description

Steps to reproduce, with JavaScript enabled:

  1. Visit the Members Directory page with the URL: http://example.com/members/?foo=bar
  1. Check the pagination links, they are like
href='?upage=2'
  1. Click one of the pagination links; the correct page is Ajax loaded
  1. Check the new pagination links, they are like
href='http://example.com/members/?foo=bar&upage=2'
  1. If you click one of the new pagination links, an incorrect page is Ajax loaded.

That happens because, when generating the pagination links, the BP_Core_Members_Template class constructor uses an empty string as $base if the page is not Ajax loaded, but uses the URL as $base if the page is Ajax loaded.

That's usually harmless, but when the URL contains a query string it breaks the pagination.

The attached patch removes this unnecessary difference in the Members, Groups and Messages directories.

Note: a real use case for this happens with BP Profile Search, when the plugin search form uses the GET form method.

Attachments (3)

6189.patch (2.6 KB) - added by dontdream 5 years ago.
6189.2.patch (2.6 KB) - added by dontdream 5 years ago.
6189.diff (1.1 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (18)

@dontdream
5 years ago

#1 @boonebgorges
5 years ago

Thanks for the report, dontdream. Which version of WP are you using? It's possible that this is related to a core bug in 4.1, which will be fixed in 4.1.1: https://core.trac.wordpress.org/ticket/30831

#2 @dontdream
5 years ago

I'm using WP 4.1, but I think my suggested patch is unrelated to paginate_links().

Basically, there is no reason why $base should be set to a different value if the page is being loaded via Ajax, and this patch simply removes that unneeded code.

#3 @dontdream
5 years ago

I've tested again with WP 4.1.1 and BP 2.2.1. There are minor differences, but the main issue remains.

  1. Visit the Members Directory page with the URL: http://example.com/members/?foo=bar
  1. Check the pagination links, they are like
href='?upage=2&foo=bar'
  1. Click one of the pagination links; the correct page is Ajax loaded
  1. Check the new pagination links, they are like
href='http://example.com/members/?foo=bar&upage=2'

If you click one of the new pagination links, an incorrect page is Ajax loaded.

That happens because, when generating the pagination links, the BP_Core_Members_Template class constructor uses an empty string as $base if the page is not being Ajax loaded, but uses the referrer URL as $base if the page is being Ajax loaded.

This difference in setting the $base value is not necessary and, when the URL contains a query string, it's harmful.

The updated patch removes the unneeded code in the Members, Groups and Messages directories.

PS: I guess that the code I'm suggesting to remove was meant to preserve the whole original query string in the pagination links. If that was the idea, probably it has never worked, because the template JavaScript expects the 'upage' variable to be the first in the query string.

@dontdream
5 years ago

@boonebgorges
5 years ago

#4 @boonebgorges
5 years ago

  • Keywords has-patch 2nd-opinion added
  • Milestone changed from Awaiting Review to 2.3

Thanks for following up and for the patch, dontdream.

The DOING_AJAX stuff was added in [8931] #5768. In a nutshell, there are certain query args that we need to blacklist from being added to the pagination links - in that particular case, it was 's'. The whole thing is kind of a mess, to be honest, and I'm not 100% certain of the best way forward. 6189.diff is one attempt: when doing AJAX, parse the query string, unset the 's' key, and pass all other query vars as 'add_args' to paginate_links(). It works, but it is quite homely. I'd like to ask @r-a-y to chime in here, as he's been knee-deep in this particular mud before.

PS: I guess that the code I'm suggesting to remove was meant to preserve the whole original query string in the pagination links. If that was the idea, probably it has never worked, because the template JavaScript expects the 'upage' variable to be the first in the query string.

Oh boy. I had never seen it before, but I'm assuming you're talking about this gem: https://buddypress.trac.wordpress.org/browser/tags/2.2.0/src/bp-templates/bp-legacy/js/buddypress.js#L940 I think we can probably do better than this :) But probably a subject for a separate ticket.

#5 @dontdream
5 years ago

I've tested your patch and it works fine. Thanks!

Long explanation: WP 4.1.1 introduced a change that is relevant to this ticket. As you can see comparing my original post with my comment:3, WP 4.1.1 is the first version that insists on putting all the original query args into the pagination links, even if we set $base to an empty string.

Before 4.1.1, the whole query string was dropped setting $base to an empty string when (DOING_AJAX == false), so I was suggesting to drop the whole query string even when (DOING_AJAX == true).

After 4.1.1, the query string is no longer dropped when (DOING_AJAX == false), so it's reasonable to do the same when (DOING_AJAX == true). 6189.diff does just that, leaves the 'upage' query var in the first position, and keeps the existing JavaScript happy :)

#6 @DJPaul
5 years ago

  • Milestone changed from 2.3 to 2.4

@dontdream, I am sorry that we didn't get time to look at your patch for 2.3. We appreciate it a lot, though, so I've moved it to the next release (2.4) which should give someone time to re-review your work and get the fix into BuddyPress.

#7 @dontdream
5 years ago

That's fine. Actually there are two patches, mine and the one by @boonebgorges. After the changes made to WP 4.1.1, I think that the most logical solution is the latter.

#8 @dontdream
4 years ago

I've found a simpler use case where this same bug appears.

  1. Visit the Members Directory page
  2. Enter sometext (or nothing at all) in the standard Members search box and hit Search
  3. Check the pagination links, their href attributes look like
href="?upage=2&s=sometext&members_search_submit=Search"
  1. Click one of the pagination links, you'll get the correct results page
  2. Check the new pagination links, their href attributes look like
href="http://example.com/members/?members_search_submit=Search&upage=1&s=sometext"

If you click one of the new pagination links, you get a page that does not honor the page length and displays all the results. The usual text Viewing X - Y of N members displays wrong X and Y values.

The bug appeared when the members_search_submit query var was introduced.

The underlying bug is the same originally reported, and all the above discussion is still valid.

#9 @DJPaul
4 years ago

  • Milestone changed from 2.4 to 2.5

#10 @dontdream
4 years ago

I guess we are waiting for r-a-y's opinion here, but probably he didn't get any notification and is unaware of this ticket. The @r-a-y mention in comment:4 above links to @r, so probably a proper notification to r-a-y was never sent.

Last edited 4 years ago by dontdream (previous) (diff)

#11 @r-a-y
4 years ago

@dontdream - Sorry for missing this ticket!

I came across a similar issue while debugging pagination just now - #6745. Your example in comment:8 should be fixed in the patch for #6745. Can you try the patch in that ticket and see if that solves the problems listed in this ticket? If not, let me know what needs to be addressed.

We might want to combine patches.

#12 @dontdream
4 years ago

@r-a-y - No problem! I tested the patch for #6745, and it fixes both the issues reported here. I think we can close this ticket now.

#13 @r-a-y
4 years ago

  • Keywords 2nd-opinion removed
  • Milestone 2.5 deleted
  • Status changed from new to closed

Thanks for testing, @dontdream.

Going to mark this as a duplicate of #6745.

#14 @r-a-y
4 years ago

In 10379:

JS: Fix an issue with parsing the page number when clicking on a pagination link.

Previously, when a paginated link is clicked, we grabbed the page number
from the URL. This isn't great as the URL is unreliable to parse the page
number (see #6189).

This commit partially reverts r7946 to use the older page number parsing,
but with some logic to properly parse numbers.

See #6745 (trunk).

Anti-props r-a-y.

#15 @r-a-y
4 years ago

In 10381:

JS: Fix an issue with parsing the page number when clicking on a pagination link.

Previously, when a paginated link is clicked, we grabbed the page number
from the URL. This isn't great as the URL is unreliable to parse the page
number (see #6189).

This commit partially reverts r7946 to use the older page number parsing,
but with some logic to properly parse numbers.

See #6745 (2.4-branch).

Anti-props r-a-y.

Note: See TracTickets for help on using tickets.