Skip to:
Content

bbPress.org

Opened 2 years ago

Closed 2 years ago

#3123 closed defect (fixed)

Audit `nopaging` and `posts_per_page`

Reported by: thebrandonallen Owned by: johnjamesjacoby
Milestone: 2.6 Priority: normal
Severity: normal Version: trunk
Component: General Keywords: has-patch
Cc: contato@…

Description

In [6506], widget classes were audited and changes were made to the WP_Query args to improve performance. This broke widgets (https://bbpress.org/forums/topic/bbpress-2-6-beta/page/4/#post-185174).

The issue here is the use of nopaging set to true. When you use this, WP_Query will ignore any other paging parameters, and just return all the results.

Attached patch fixes the widget issue by removing the nopaging argument.

I also went through and audited the usage of nopaging and posts_per_page. It's a micro-optimization, but I've set posts_per_page => -1 when were setting nopaging => true, and I've set nopaging => true where we're setting posts_per_page => -1.

Attached patch fixes an issue where the query in bbb_forum_query_last_reply_id() was returning all reply ids, rather than a single reply id, due to incorrect usage of nopaging => true.

Attachments (1)

3123.1.diff (6.7 KB) - added by thebrandonallen 2 years ago.

Download all attachments as: .zip

Change History (5)

#1 @espellcaste
2 years ago

  • Cc contato@… added

That's interesting to know this side effect of 'nopaging'...

#2 @johnjamesjacoby
2 years ago

  • Owner set to johnjamesjacoby

Hmm. Yeah, I forgot that widgets had counts.

Setting posts_per_page to -1 forces nopaging to true if it's not already set.

if ( !isset($q['nopaging']) ) {
        if ( $q['posts_per_page'] == -1 ) {
                $q['nopaging'] = true;
        } else {
                $q['nopaging'] = false;
        }
}

Then, this is how sticky posts are loaded inside of the WP_Query class:

// Fetch sticky posts that weren't in the query results
if ( !empty($sticky_posts) ) {
        $stickies = get_posts( array(
                'post__in' => $sticky_posts,
                'post_type' => $post_type,
                'post_status' => 'publish',
                'nopaging' => true
        ) );

        foreach ( $stickies as $sticky_post ) {
                array_splice( $this->posts, $sticky_offset, 0, array( $sticky_post ) );
                $sticky_offset++;
        }
}

I'm going to look through the patch to confirm everything, but thanks a lot for catching & patching this. <3

#3 @johnjamesjacoby
2 years ago

In 6606:

Widgets: Remove nopaging keys from widget queries.

This reverts part of r6506 that broke the max_rows setting of widgets.

Props thebrandonallen. See #3123.

#4 @johnjamesjacoby
2 years ago

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

In 6607:

Queries: nopaging audit.

  • In r6506 the nopaging query argument was added to various queries to avoid paginating results when it wasn't necessary. This resulted in a few queries (widgets mainly) not obeying their specific settings.
  • In #3123, other inconsistencies in our query arguments were uncovered, triggering the need to audit our query usages and equalize them once again.

This change brings all queries back to par with one another, specifically in regards to posts_per_page => -1 style queries, and queries where filters can be suppressed and meta/term caches should not be primed.

It also groups together the get_user_object_ids functions. These are now unused in bbPress proper, though were previously useful before the engagements API was in place. These queries are considered too inefficient to rely upon in large-scale applications, but are included to provide filterable wrappers should someone need them, or should we need to bring them back later.

Props thebrandonallen. Fixes #3123.

Note: See TracTickets for help on using tickets.