CUNY Academic Commons

I've reapplied our BuddyPress 12 compatiblity fix in https://github.com/cuny-academic-commons/cac/commit/e5f6a7d5ad6a52bb89e91eb828347239dc389d0a and fixed an issue with a duplicate search form being displayed when making a forum search in https://github.com/cuny-academic-commons/cac/commit/2f3465cb78dff3cf6e4566b238d2a8d648c1886e . Both commits are pushed to production as well.

I'm also checking production and finding that logged-out users see an empty forum page. For example: https://commons.gc.cuny.edu/groups/the-group-for-group-admins/forum/ . Logged-in users are unaffected and single forum threads are okay. I'm looking into this for a tiny bit, but might not be able to address this until tomorrow as it is getting late over here.

Looks like https://bbpress.trac.wordpress.org/changeset/7262 is the culprit. Unfortunately, I won't be able to address this tonight. Boone, when you're up, can you take a look? The bug can be reproduced on cdev.

Ray, I'm sorry that I didn't think about this before yesterday's release. I noticed that bbPress had been updated, but I didn't think about the fact that we'd patched it.

You've correctly identified the bbPress changeset that causes the problem. Prior to the changeset, the structure of bbp_pre_get_posts_normalize_forum_visibility() was like this:

$post_types = $posts_query->get( 'post_type' );

...

if ( in_array( bbp_get_forum_post_type(), $post_types, true ) ) {
   ...
} elseif ( ! array_diff( $post_types, bbp_get_post_types() ) ) {
   ...
}

After the changeset, the structure is instead:

if ( in_array( bbp_get_forum_post_type(), $post_types, true ) ) {
   ...
} 

if ( ! array_diff( $post_types, bbp_get_post_types() ) ) {
   ...
}

The main bbp_has_forums() query on the group /forum/ page hits the first if clause, and prior to the changeset, it would hit only that clause - it never got to the elseif. After the changeset, it hits both clauses.

The commit message explicitly says that this is by design https://bbpress.trac.wordpress.org/changeset/7262: "Tweaks the logic inside of bbp_pre_get_posts_normalize_forum_visibility() to always handle both of its internal conditions (forum query, or any query that includes forums/topics/replies connected via meta data)"

It's not completely clear to me why the clause is causing problems in our case. The problematic forum ID that's being excluded is 11540. This is the "groups root" forum that bbPress uses as the parent for all group forums. I recall having problems with this feature in the past, but I don't remember whether it's something specific to the Commons, or whether it's a more general architectural problem with bbPress. In any case, I first tried solving this with the large hammer of wiping out bbPress's forum-id exclusion list on /groups/my-group/forum https://github.com/cuny-academic-commons/cac/commit/86851520ac6a09bacc80d161774082590a16a9ea, but once I realized that the issue was specific to 11540, I dialed in to ensure that the current group's forum ID and the group-root ID are always excluded from the exclusion list: https://github.com/cuny-academic-commons/cac/commit/54962daaf92b0c5a93625aab067fc948ceee8659 I'm fairly sure this is safe, so I've pushed it to the production site.

But I don't know whether this is an appropriate fix for bbPress itself. It seems like the group forum root, specifically, should never be excluded from 'forum' queries. But Ray, perhaps you have additional thoughts or insight about this.

After looking into this a bit more, I think the bbPress changeset is meant to deal either with when a logged-out user is on the root bbPress /forums/ page and is using the search form on that page or when a logged-out user is using the bbPress search widget.

However, the changeset also alters the forum query when on a single group forum index page for logged-out users to:

SELECT   wp_1_posts.*
             FROM wp_1_posts  INNER JOIN wp_1_postmeta ON ( wp_1_posts.ID = wp_1_postmeta.post_id )
             WHERE 1=1  AND wp_1_posts.ID = GROUP_FORUM_ID AND ( 
  ( wp_1_postmeta.meta_key = '_bbp_forum_id' AND CAST(wp_1_postmeta.meta_value AS SIGNED) NOT IN (LARGE FORUM ID COMMA-DELIMITED LIST OF PRIVATE AND HIDDEN FORUMS) )
) AND wp_1_posts.post_type = 'forum' AND ((wp_1_posts.post_status = 'publish' OR wp_1_posts.post_status = 'hidden' OR wp_1_posts.post_status = 'private'))
             GROUP BY wp_1_posts.ID
             ORDER BY wp_1_posts.menu_order ASC, wp_1_posts.post_title ASC

I think the intention is for the NOT IN query to exclude hidden forums by forum ID, but what is actually happening is the query is excluding hidden forums by the parent forum ID. The _bbp_forum_id postmeta is actually used to store the parent forum ID, not the forum ID. All BuddyPress group forums uses the group forum root as the parent forum ID.

The problematic forum ID that's being excluded is 11540. This is the "groups root" forum that bbPress uses as the parent for all group forums.

The real wrinkle is on the Commons, 11540's 'post_status' is set to 'private', so this is why the logged-out DB query returns nothing as 11540 would match the postmeta query.

It makes sense to exclude hidden forums from the global forum search, however I don't think the bbPress team considered BuddyPress group forums and using a hidden parent forum category to house all group forums. Secondly, I think their NOT IN query is wrong when on a single forum.

The NOT IN query isn't needed when you're on a single forum. (It's also a huge inefficient NOT IN clause for us!) If a user is on a single forum, we've already determined that this is the correct forum ID so we do not need this exclusion query. The bbPress changeset was meant to deal with search for logged-out users, however with single forum search, we do not need to worry about this either because my bbp-single-forum-search plugin already filters forum search to the specific group forum ID.

So I think I actually like your first sledgehammer fix :) What do you think, Boone?

Thanks for thinking more about this, Ray. I'm convinced by your reasoning. Given what you've said, my context checks in https://github.com/cuny-academic-commons/cac/commit/86851520ac6a09bacc80d161774082590a16a9ea should be safe, right? (remove the forum_id restriction only when looking at the /forum/ action in a single group)

Given what you've said, my context checks in https://github.com/cuny-academic-commons/cac/commit/86851520ac6a09bacc80d161774082590a16a9ea should be safe, right? (remove the forum_id restriction only when looking at the /forum/ action in a single group)

Yes, your first commit is a suitable fix for the group forum index issue for logged-out users. I've reverted https://github.com/cuny-academic-commons/cac/commit/54962daaf92b0c5a93625aab067fc948ceee8659 and pushed this to production. I'm going to try and write up a ticket to bbPress Trac explaining this issue.