Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

#7935 closed defect (bug) (fixed)

bp_core_new_nav_default() not working since v3.0.0

Reported by: r-a-y Owned by: boonebgorges
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.0.0
Component: Core Keywords: has-patch
Cc:

Description

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.

Attachments (4)

7935.01.patch (945 bytes) - added by r-a-y 3 years ago.
7935.02.patch (1.5 KB) - added by r-a-y 3 years ago.
7935.03.patch (2.9 KB) - added by r-a-y 3 years ago.
7935.04.patch (3.0 KB) - added by r-a-y 3 years ago.

Download all attachments as: .zip

Change History (19)

@r-a-y
3 years ago

#1 @r-a-y
3 years ago

  • Milestone changed from Awaiting Review to 3.2.0

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

#2 @DJPaul
3 years ago

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

#3 @imath
3 years ago

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

#4 @DJPaul
3 years ago

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

#5 @DJPaul
3 years ago

  • Milestone changed from 3.2.0 to 3.3.0

#6 @r-a-y
3 years ago

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.

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

#7 @imath
3 years ago

@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 :)

#8 @r-a-y
3 years ago

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!

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

@r-a-y
3 years ago

@r-a-y
3 years ago

#9 @imath
3 years ago

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

#10 @r-a-y
3 years ago

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?

#11 @DJPaul
3 years ago

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.

#12 @DJPaul
3 years ago

  • Milestone changed from 3.3.0 to 4.0

#13 @boonebgorges
3 years ago

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

#14 @boonebgorges
3 years ago

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.

#15 @boonebgorges
3 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.