Component subnav items should use displayed user domain, not loggedin user

[boonebgorges]

Several components set up their subnav using bp_loggedin_user_domain(). See, for example, how Groups does it: ​https://buddypress.trac.wordpress.org/browser/trunk/bp-groups/bp-groups-loader.php Line 354 sets $parent_link based on the logged-in user's domain, and that loggedin user's domain is then used to create links.

This is pretty obviously wrong. We should be using the *displayed* user's domain to create these subnav items, since they should be relative to the currently viewed user. Moreover, at the moment, if you're not logged in, bp_loggedin_user_domain() returns an empty string, which means that the subnav hrefs become relative links (sorta by accident), which don't always work as expected.

The reason why this rather big bug has never come up in the past is that the components in question (Groups, Friends, Profile) only have a single subnav item when viewing another user's profile, and bp-default hides these subnavs. See, eg, ​https://buddypress.trac.wordpress.org/browser/trunk/bp-themes/bp-default/members/single/groups.php#L14. However, the problem becomes evident when calling bp_get_options_nav() manually in your own theme.

I'm happy to go ahead and fix this, but first I want to get a sanity check from another dev that changing to bp_displayed_user_domain() is indeed the right thing to do.

[r-a-y]

I think the reason why we don't use bp_displayed_user_domain() is because of the BuddyBar, since the BuddyBar and the options nav use the same code to render themselves.

To workaround this in my own plugin, I do something like this:

		$domain = bp_displayed_user_domain() ? bp_displayed_user_domain() : bp_loggedin_user_domain();

		$link = trailingslashit( $domain . $this->slug );

Maybe it's an approach that can be adopted for the rest of the core components.

Let me know what you guys think.

[boonebgorges]

Thanks for the clarification, r-a-y. I forgot that the BuddyBar uses $bp->bp_options_nav. That's a real mess, IMO, and I wish we could just drop backward compatibility for it :-/

Anyway, we can't simply do a test like you suggest. If I'm logged in as boone, and I visit example.com/members/, my BuddyBar links will point to *my* Profile, Groups, etc. This is expected, legacy behavior. However, when visiting a page like example.com/members/r-a-y/, those links would then point to *your* Profile, Groups, etc, which is not what we'd expect.

My preference is to fix the components so that they work they way they ought to work (which is to say, using bp_displayed_user_domain()), and then to patch the BuddyBar for backward compatibility. If we're going to have a hack, I'd rather have it in a place where it's deprecated. The hack could be as simple as str_replace( bp_displayed_user_domain(), bp_loggedin_user_domain(), $link ) - ugly, but straightforward.

[r-a-y]

The BuddyBar does a str_replace() to swap out the displayed user domain with the logged in user's domain:
​https://buddypress.trac.wordpress.org/browser/tags/1.6.1/bp-core/bp-core-buddybar.php#L517

This enables the code snippet I posted above to work the way we want to.

I don't mind how we go about fixing this.

---

I just realized that what I referenced above should solve the problem I was initially worried about! Having an off-day! Going to test out switching bp_loggedin_user_domain() for bp_displayed_user_domain() to see what happens.

Update: Okay, it works! Carry on, Boone!

[boonebgorges]

Ha, leave it to me to come up with an existing solution :)

If the appropriate str_replace() is already happening, then changing the component's setup_nav() methods to use bp_displayed_user_domain() shouldn't break the BuddyBar, right?

[r-a-y]

Ha, leave it to me to come up with an existing solution :)

:)

then changing the component's setup_nav() methods to use bp_displayed_user_domain() shouldn't break the BuddyBar?, right?

If you're not on a user profile page, then the BuddyBar link won't work since bp_loggedin_user_domain() isn't passed.

We could add in bp_loggedin_user_domain() inside bp_adminbar_account_menu() and that will solve our problems! What do you think?

[boonebgorges]

If you're not on a user profile page, then the BuddyBar? link won't work since bp_loggedin_user_domain() isn't passed.

Groan. In theory, we shouldn't be building the navigation at all on pages where it's not going to be displayed, which is to say that it's wasting cpu cycles to build $bp->bp_options_nav on pages where ! bp_is_user(). (Aside from the Buddybar, of course.)

What bothers me about using bp_loggedin_user_id() is that it will create an options_nav that is nonsensical on non-member pages. Eg, when looking at the Groups Directory, the $bp->bp_options_nav global will contain references to the logged-in user's nav. Why is that relevant, aside from the Buddybar? It all seems very inelegant and inefficient.

As much as I would like to rebuild this stuff so that the nav is only built when it's actually needed (and then to build a backpat bridge for the Buddybar), I can almost guarantee that there are people out there who are using the current, peculiar behavior for their own purposes. A total refactor, even if it made sense from an architectural point of view, would probably break those people's plugins/sites. So, with great sadness, I concede that we're probably going to have to go with your solution: first try bp_displayed_user_domain(), then fall back on bp_loggedin_user_domain(). (We do this already in, eg, the Activity component.)

[DJPaul]

Moving to 1.7 because it doesn't sound essential for a minor release.

[johnjamesjacoby]

Punting to 1.8 unless we get a quick patch. I think because the toolbar always loads the logged in users nav links, and because the code is commingled with displayed user options nav, we need to invest some time in 1.8 to separate these two navigational arrays where appropriate.

[boonebgorges]

Is this an issue anymore? It looks like we prefer the displayed user when it's available. ​https://buddypress.trac.wordpress.org/browser/tags/2.5.3/src/bp-groups/classes/class-bp-groups-component.php#L424