Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#5768 closed defect (bug) (fixed)

Pagination link error for search results in Messages > Inbox

Reported by: mercime Owned by: djpaul
Milestone: 2.1 Priority: normal
Severity: normal Version: 1.6
Component: Messages Keywords: has-patch commit
Cc:

Description

I was able to reproduce the bug which was initially reported in the BP forums http://buddypress.org/support/topic/error-in-inboxoutbox/

  1. Go to your Messages > Inbox
  2. Search for a word in messages search form e.g. "Test" in my case
  3. If there are more than 10 results, clicking on next page, i.e., #2 pagination link leads to http://www.example.com/wp-admin/admin-ajax.php?mpage=2 showing a white page with only a "0" on screen

Attachments (3)

5768.01.patch (878 bytes) - added by r-a-y 5 years ago.
5768.02.patch (1.3 KB) - added by r-a-y 5 years ago.
5768.03.patch (3.3 KB) - added by r-a-y 5 years ago.

Download all attachments as: .zip

Change History (17)

#1 @DJPaul
5 years ago

  • Component changed from Core to Messaging
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 2.1

@r-a-y
5 years ago

#2 @r-a-y
5 years ago

  • Keywords has-patch added; needs-patch removed
  • Version set to 1.6

Attached patch fixes the PM pagination links when using AJAX and search terms.

The only caveat is if you decide to blank out the search results and search again, the old search term is still used.

This ticket was mentioned in IRC in #buddypress-dev by r-a-y. View the logs.


5 years ago

#4 @DJPaul
5 years ago

  • Keywords dev-feedback added

#5 @DJPaul
5 years ago

This needs feedback from another core dev, please.

#6 @boonebgorges
5 years ago

  • Keywords commit added; dev-feedback removed

The only caveat is if you decide to blank out the search results and search again, the old search term is still used.

Yergh. Only ways around this that I can see are:

(a) disable AJAX search altogether
(b) use some history.pushstate magic to change the URL so that the ?s=foo URL param doesn't bleed through

Both of these seem way outside of what we can do right now.

That said, I think the patch is much, much better than the current situation, so I say go with it for 2.1 and make a note to fix it betterer in the future.

@r-a-y
5 years ago

#7 @r-a-y
5 years ago

02.patch addresses using a blank search, but it hacks bp-legacy's JS to explicitly check for the messages object and allows blank searches to pass through instead of falling back to the querystring in the address bar.

#8 @boonebgorges
5 years ago

Gah. 5768.02.patch is fine as long as it's adequately documented. Otherwise we'll go back in six months and be baffled by this exception. Thanks for trudging through this.

#9 follow-up: @r-a-y
5 years ago

Any reason why we fallback on the querystring for search terms in the JS to begin with? Shouldn't the template loops handle this instead?

#10 in reply to: ↑ 9 @boonebgorges
5 years ago

Replying to r-a-y:

Any reason why we fallback on the querystring for search terms in the JS to begin with? Shouldn't the template loops handle this instead?

I think it's happening here: https://buddypress.trac.wordpress.org/browser/tags/2.0.2/bp-messages/bp-messages-template.php#L168 (line 200). Moving this logic into the template won't make a difference because the AJAX pagination loads the entire template content, so members/single/messages/messages-loop.php will get loaded up in its entirety during an AJAX call, in which case we run into the same issue.

One possible solution is to stop using $_REQUEST, and instead check $_GET and $_POST separately, giving precedence to what's in $_POST. I think that would do it?

#11 @r-a-y
5 years ago

One possible solution is to stop using $_REQUEST, and instead check $_GET and $_POST separately, giving precedence to what's in $_POST. I think that would do it?

Actually, whatever we pass as 'search_terms' to the loop gets precedence so it doesn't matter if we leave the $_REQUEST in place.

My main issue is does anyone have any qualms about removing these lines from bp-legacy's JS in general?

if ( bp_get_querystring('s') && !search_terms ) {
        search_terms = bp_get_querystring('s');
}

03.patch removes these lines and offers better pagination support when JS is disabled for all our main paginated loops. I'm probably missing a few secondary loops though.

From my browser testing on the members and groups directories (both with JS on and off), I don't see any problems.

@r-a-y
5 years ago

#12 @boonebgorges
5 years ago

Ah, I see - so the calling function would be responsible for passing values to bp_filter_request().

This seems like a fine change to me. There's a slight possibility that it'll break implementations that are calling bp_filter_request() directly and then expecting the search term to be fetched out of the query string. But bp_filter_request() is such a mess that I doubt anyone has ever tried to use it in a plugin before (aside from me - I did once, but even in my implementation, this change wouldn't break anything).

#13 @DJPaul
5 years ago

Looks OK to me, too.

#14 @djpaul
5 years ago

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

In 8931:

Correctly generate pagination links when using AJAX and search terms have been set.
Also improve pagination support for main loops in the Groups/Members/Messages components when JS is disabled.

Fixes #5768, props r-a-y

Note: See TracTickets for help on using tickets.