BuddyPress resets their pages to page ID 0

[Pete Hudson]

Read comment:6 and on.

---

Original message:

WLM protects the BP 2.1.1 Activity Feed just fine, but since the update of 2.2 (and now 2.2.1) WLM no longer protects the BP feed. Non-members are not allowed to comment, but they can see posts and comments in the feed. This has been verified with fresh installs of WP, BP and WLM.

WLM support will not help because they say it's the new BP code (2.2) that is causing the problem and they say it needs to be fixed on the BP end.

[boonebgorges]

WLM support will not help because they say it's the new BP code (2.2) that is causing the problem and they say it needs to be fixed on the BP end.

Well, this delightful attitude doesn't make me eager to dive in. Not your fault, of course :)

Did WLM give any more details about what they thought was causing the problem? Can you please give detailed instructions on how to reproduce? I have not used Wishlist Member.

It looks like it will be difficult to debug the issue, as it appears that Wishlist Member requires a license.

[hnla]

@pete-hudson You do already have a thread in the support forum running on this issue and have asked the question above on it - best to stick to one thread to avoid confusion and duplication of effort, the support forum is probably the more suitable place for the moment:
​https://buddypress.org/support/topic/newest-bp-upgrade-wont-protect-feed-with-wishlist-member/

In that thread I did suggest a possible solution if wishlist was using a is_page() test which you declined as probably too awkward to accomplish which is fair enough (although think our issue, or WP's issue with page Id's of zero would produce the inverse of the issue you're having if they use is_page() checks).

As Boone says this is difficult for us to debug without access to a premium plugin, perhaps though on the thread in the support forum you could provide as much detail as possible as to what settings you set in wishlist to effect this hidden activity stream, i.e does it say something like select 'Pages' from this list to protect or provide the ID of the page ? Some clue as to how wishlist achieves this may clue us in on the issue, again as Boone asked did wishlist suggest any possible cause.

It is a great shame they couldn't be a little more helpful as it's much easier for them to have quickly tested on a local install running BP with their plugin and to probably had a fair idea of where the problem lay.

[r-a-y]

I've replied to the forum thread.

As hnla has also presumed, I'm guessing it is related to the changeset that resets BP pages to the page ID of zero (r9322), similar to the issue noted in #6108.

Pete, please try patching one line in WordPress and see if that resolves the issue:
​https://core.trac.wordpress.org/attachment/ticket/24674/24674.2.diff

My other thought is WLM does not do redirects on the proper hook. They should do redirects on the 'template_redirect' hook as early as possible (like a priority of 0). Though, at this point, without looking at their code, I'm purely speculating.

[netweb]

Ping @mikelopez, let's see if this works ;) (Mike Lopez, Lead Developer for WishList Products.)

[mikelopez]

Currently, WLM uses the following filters for protecting content:

taxonomy_template, page_template, single_template, category_template, tag_template

We could use template_redirect but I'm wondering if this is the one that should be fixed.

Turns out BP creates a dummy post with ID=0 for the activity page. I don't understand why this is needed. The function bp_theme_compat_reset_post says:

This dummy data is necessary because theme compatibility essentially fakes
WordPress into thinking that there is content where, in fact, there is none

when in fact there will always be content as BP requires it.

I also think that there's also the chance for other plugins to be thrown off because of said dummy post.

Thoughts?

[mikelopez]

Ok, I made further investigation and I now see the need for the dummy post.

However, instead of setting post data to 0, wouldn't it be safer to just clone the post data of the parent activity / members page instead?

[mikelopez]

Set proper page ID if one exists for the current component in bp_theme_compat_reset_post()

[mikelopez]

Kindly ignore the previous patch. It breaks the menu highlighting. Working on an update.

[mikelopez]

Set proper page ID if one exists in $bp->pages and $argsID? is empty (please ignore previous patch)

[r-a-y]

Turns out BP creates a dummy post with ID=0 for the activity page. I don't understand why this is needed.

In the past, BP used to use the page ID that was associated with the BP directory page (/members/, /groups/, /activity/). But that caused issues on BP single pages like /members/USERNAME/ and /groups/GROUPNAME/.

We could set the page ID only on BP directory pages. In fact, it was something I originally did in r8821. I think we could bring r8821 back since r9322 changes how BP page items are highlighted in nav menus. Let me experiment a bit.

The alternative is for WLM to do redirect checks on the 'template_redirect' hook. Since BuddyPress overrides $wp_query properties on the 'template_redirect' hook, if WLM wants to do checks before other plugins like BP overrides $wp_query, WLM should do checks on the page in question as early as possible on 'template_redirect' at priority zero. See wp-includes/template-loader.php for the firing sequence.

For example, try this code on a BP directory page:

function test_queried_object() {
    print_r( get_queried_object() );
}
add_action( 'template_redirect', 'test_queried_object', 0 );

The correct page ID is still available then. If you change to your 'page_template' filter, you will notice that the page ID is zero.

So just to conclude either BP or WLM should make the suggested changes above to fix this for now. One way or another, in the future, this will break again when BP moves to rewrite rules and virtual pages and we'll both need to look at a way to engineer another solution then.

[mikelopez]

Thanks for the detailed explanation @r-a-y

We'll experiment on template_redirect on our end and see if that's workable. I can't remember exactly but I'm pretty sure we had our reasons as to why we did not use it before (WLM 2.6). This is why I'm trying to look for a different fix.

Do you think the patch I submitted would break stuff like /members/USERNAME/ and /groups/GROUPNAME/?

[mikelopez]

I hate bumping stuff but...

I was just wondering if the patch I submitted worked? I believe it will not only benefit WishList Member but other plugins expecting the page ID being set.

Thanks.

[boonebgorges]

No worries about the bump, mikelopez.

Your suggestion in bp-core-theme-compatibility.php.1424377688.diff​ is functionally equivalent to reverting (part of) [8821]. There is a fairly long discussion in #5241 about why we moved away from using the bp-pages page for the dummy post, and started using '0' instead. But r-a-y may be right that recent changes in our own handling of current-state menu highlighting etc might mitigate some of these original motivations. I'll let r-a-y be the judge of whether it's worth it, on balance, to go back to the old way.

[r-a-y]

I think there are three approaches we could take:

• Leave all BP pages as page ID zero (current method)
• Move back all BP pages to the corresponding BP directory page ID
• Use page ID zero on BP single pages only

The first option is what we're currently using now. It sets us up for rewrite rules and virtual pages in the future, but breaks functionality with plugins such as WLM expecting a proper WP page and those using is_page() (the is_page() issue is fixed in WP 4.2-bleeding).

The second option is what we were using up until BP 2.1.1. The problem here is if someone is using WP functions like get_queried_object() or the_permalink() on a BP single page, this would reference the WP page properties instead. This offers the most compatibility, but edge cases can occur (is_page() and plugins doing stuff dependent on WP properties). This is basically r8932 (or a version of mikelopez's patch).

6230.reset-single-pages.patch implements the third option, which adds back r8821. It resets the post on single pages (/members/USER/, /groups/GROUP/), but not directory pages (/members/, /groups/, etc.).

This is what I originally envisioned in r8821 as a BP directory page is technically a WP page, but a BP single page is a virtual page. Due to issues with page highlighting (#5810), we had to revert r8821, but this should be fixed as of r9322. When WLM is used with this patch, BP directory pages can be blocked again, however BP single pages will not. So this could be seen as a half-measure for those using WP plugins like WLM with BuddyPress.

You can also view some discussion on Slack about the page ID zero situation here:
​https://wordpress.slack.com/archives/buddypress/p1424375799002601

All of these approaches are valid, but also have issues. If we care about plugin compatibility, then we should go back to the second option with the caveat that we'll have to do this all over again in the future. If we are planning on implementing rewrite rules in an upcoming release (v2.3 or v2.4), then we should stick with what we're using currently. Technically, the third option makes sense as well.

Feedback welcome.

[Pete Hudson]

Hi guys. I'm still running BP 2.2.1 because of the patch that the WLM guys helped us out with.

I'm a little concerned about not updating with all the news about WP vulnerabilities.

Are we safe not upgrading to 2.2.3.1 right now?

Thanks.

Pete

[mikelopez]

Hey guys,

Just a quick note that we've moved WishList Member to use template_redirect for the redirects. Works fine.

We'll include this fix in our next official release of WishList Member.

Note: Still think that there has to be a better way for BP to do it's thing without changing the page ID but that's just me.

I guess we can mark this as resolved now unless you guys think otherwise.

Mike

[r-a-y]

#6628 was marked as a duplicate.

[DJPaul]

Closing most tickets related to BP-Default and BP-Legacy, since the upcoming BP-Nouveau template pack (planned for 3.0) will make these redundant.

[DJPaul]

Closing most tickets related to BP-Default and BP-Legacy, since the upcoming BP-Nouveau template pack (planned for 3.0) will make these redundant.