Query string in directory URL breaks AJAX pagination

[dontdream]

Steps to reproduce, with JavaScript enabled:

1. Visit the Members Directory page with the URL: ​http://example.com/members/?foo=bar

2. Check the pagination links, they are like

href='?upage=2'

3. Click one of the pagination links; the correct page is Ajax loaded

4. Check the new pagination links, they are like

href='http://example.com/members/?foo=bar&upage=2'

5. 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.

[boonebgorges]

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

[dontdream]

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.

[dontdream]

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

2. Check the pagination links, they are like

href='?upage=2&foo=bar'

3. Click one of the pagination links; the correct page is Ajax loaded

4. 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.

[boonebgorges]

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.

[dontdream]

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 :)

[DJPaul]

@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.

[dontdream]

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.

[dontdream]

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"

4. Click one of the pagination links, you'll get the correct results page
5. 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.

[dontdream]

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.

[r-a-y]

@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.

[dontdream]

@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.

[r-a-y]

Thanks for testing, @dontdream.

Going to mark this as a duplicate of #6745.

[r-a-y]

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.

[r-a-y]

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.