Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#6230 closed defect (bug) (wontfix)

BuddyPress resets their pages to page ID 0

Reported by: pete-hudson's profile Pete Hudson Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.2
Component: Templates Keywords: has-patch
Cc:

Description (last modified by r-a-y)

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.

Attachments (3)

bp-core-theme-compatibility.php.diff (761 bytes) - added by mikelopez 10 years ago.
Set proper page ID if one exists for the current component in bp_theme_compat_reset_post()
bp-core-theme-compatibility.php.1424377688.diff (783 bytes) - added by mikelopez 10 years ago.
Set proper page ID if one exists in $bp->pages and $argsID? is empty (please ignore previous patch)
6230.reset-single-pages.patch (1.1 KB) - added by r-a-y 10 years ago.

Download all attachments as: .zip

Change History (26)

#1 @boonebgorges
10 years ago

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.

#2 @hnla
10 years ago

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

#3 @r-a-y
10 years ago

  • Keywords reporter-feedback added

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.

#4 @netweb
10 years ago

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

This ticket was mentioned in Slack in #buddypress by netweb. View the logs.


10 years ago

#6 @mikelopez
10 years ago

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?

#7 @mikelopez
10 years ago

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
10 years ago

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

#8 @mikelopez
10 years ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


10 years ago

#10 @mikelopez
10 years ago

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

@mikelopez
10 years ago

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

#11 @r-a-y
10 years ago

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.

Last edited 10 years ago by r-a-y (previous) (diff)

#12 @mikelopez
10 years ago

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/?

#13 @mikelopez
10 years ago

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.

#14 @boonebgorges
10 years ago

  • Milestone changed from Awaiting Review to Under Consideration

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.

#15 @r-a-y
10 years ago

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.4 or v2.5), then we should stick with what we're using currently. Technically, the third option makes sense as well.

Feedback welcome.

Last edited 10 years ago by r-a-y (previous) (diff)

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


10 years ago

#17 @Pete Hudson
10 years ago

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

#18 @mikelopez
9 years ago

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

#19 @DJPaul
9 years ago

  • Milestone changed from Under Consideration to Future Release

#20 @r-a-y
9 years ago

  • Keywords reporter-feedback removed

#6628 was marked as a duplicate.

#21 @r-a-y
9 years ago

  • Component changed from Appearance to API - Theme Compat
  • Description modified (diff)
  • Summary changed from BP 2.2 (and 2.2.1) feed is no longer protected by Wishlist Member to BuddyPress resets their pages to page ID 0
  • Version changed from 2.2.1 to 2.2

#22 @DJPaul
7 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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.

#23 @DJPaul
7 years ago

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.

Note: See TracTickets for help on using tickets.