bp_core_new_nav_default() not working since v3.0.0

[r-a-y]

Reported here:
​https://buddypress.org/support/topic/removing-settings-general-tab-redirect-to-other-tab/

Problem is due to the conditional loading that I added in v3.0.0 (#7218).

Attached patch fixes the issue by trying to find the screen function code before the screen function check is made in bp_core_new_nav_default().

Will try and write a unit test for this before committing.

[r-a-y]

Can't really write a unit test for this since unit tests preloads all screen code.

[DJPaul]

As long as all those paths are sanitised then we can do this patch.

[imath]

@r-a-y I'm just wondering if $parent_nav->slug is always the same than the component ID. I'm thinking about the xProfile component in particular.

[DJPaul]

Let's get clarification on that before using this patch.

[r-a-y]

I'm just wondering if $parent_nav->slug is always the same than the component ID. I'm thinking about the xProfile component in particular.

@imath - Good catch. I've made two changes:

1. For the path, I'm now using $bp->core->path because we're only concerned with the BuddyPress core path anyway.
2. Due to the fact that the slugs can be modified by a developer, I'm now using $parent_slug->css_id since that uses the component ID for the most part - ​https://buddypress.trac.wordpress.org/browser/tags/3.1.0/src/bp-core/bp-core-buddybar.php?marks=139-141,147#L113. XProfile component sets link_css_id as the component ID - ​https://buddypress.trac.wordpress.org/browser/tags/3.1.0/src/bp-xprofile/classes/class-bp-xprofile-component.php?marks=257#L235.

It might make sense to set the link_css_id for all components to the component ID just to be safe. For example, for the Settings component, we would add the link_css_id parameter directly after this line - ​https://buddypress.trac.wordpress.org/browser/tags/3.1.0/src/bp-settings/classes/class-bp-settings-component.php?marks=146#L124 (Update - looks like the Settings component is the only one that doesn't set the 'link_css_id' parameter, so I've added it to 02.patch).

Let me know what you think.

[imath]

@r-a-y Thanks for the update and your work on this.

Shouldn't it be 'item_css_id' instead of 'link_css_id' ?

Moreover it's probably minor, as this parameter can be used to style the Template Pack, and as if this parameter is empty it falls back to the slug, there's probably a chance, the css rules (if any) applied to #general-personal-li fail.

What about adding a new component_id parameter to nav functions and make the slug and css_id parameters fall back to it ?

Maybe i'm overthinking it :)

[r-a-y]

Shouldn't it be 'item_css_id' instead of 'link_css_id' ?

@imath - Good catch, again. I shouldn't code at night! :) I've updated 02.patch.

What about adding a new component_id parameter to nav functions and make the slug and css_id parameters fall back to it ?

I like this idea. Instead of making the ID fall back to slug or css_id, I left it empty. See 03.patch. I'm setting the ID in class-bp-component.php, so the ID should always be set for most developers extending the BP_Component class. A minor thing is if a plugin is overriding the setup_nav method and not calling the parent::setup_nav() method, but this isn't really a big deal since this ticket is only dealing with core components requiring their screen function code.

Logically, I think 03.patch is better, but 02.patch is simpler.

Let me know what you think!

[imath]

Thanks a lot for your patches @r-a-y I prefer .03 :)

I'd probably won't use 'id' but 'component_id' as it won't be unique for each nav items so it seems a bit confusing to me.

[r-a-y]

I'd probably won't use 'id' but 'component_id' as it won't be unique for each nav items so it seems a bit confusing to me.

We have a second function argument called $component in bp_core_new_nav_default(), so I didn't initially want to name the new parameter $args['component_id'] because it might be confusing.

But, @imath, you're right that 'id' also is too vague. 04.patch changes the parameter to 'component_id'. Anyone else have anything to add before committing?

[DJPaul]

I like it. As long as that file path is guaranteed to not be dynamic. If it could be dynamic, need some sort of appropriate escaping there.

[boonebgorges]

Fix looks good. I'll add some validation to prevent path traversal, to address Paul's concern about escaping/validation.

[boonebgorges]

I'm also going to ensure that components calling bp_core_create_nav_link() (and by extension bp_core_new_nav_item()) without going through BP_Component have the component_id set - by falling back on slug.

[boonebgorges]

In 12288:

Nav: Dynamically load component screen file when changing nav default.

BP 3.0.0 moved most screen controllers into files that are loaded only when
viewing the component. This introduced a regression into
bp_core_new_nav_default(), since the screen_function would not be available
(ie ! is_callable()) at the time that the nav item is modified.

We resolve this by loading the component's screen functions in
bp_core_new_nav_default() when we detect that they're not present. To work
around cases where component slugs don't match the official component ID,
which corresponds to the file path, we introduce a new 'component_id' parameter
to be used when registering a new nav item.

Props imath, r-a-y.
Fixes #7935.