Skip to:
Content

BuddyPress.org

Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#7931 closed defect (bug) (fixed)

Possible regression in bp_core_new_subnav_item()

Reported by: dontdream Owned by: r-a-y
Milestone: 3.2.0 Priority: normal
Severity: normal Version: 3.0.0
Component: Navigation Keywords: has-patch has-unit-tests
Cc:

Description

The attached code snippet adds a new tab to a members's Settings menu, and works fine on BP 2.9.4 and earlier versions.

On BP 3.0.0 and 3.1.0 it adds the new tab but, when I click the tab link, I get a 404 page instead of the tab content.

Reverting changeset 11814 in bp-core-buddybar.php makes the code work again.

Thanks for looking into this!

Attachments (3)

testing.php (972 bytes) - added by dontdream 15 months ago.
demonstrates possible regression in bp_core_new_subnav_item()
7931.01.patch (780 bytes) - added by r-a-y 15 months ago.
7931.unit-test.patch (1.6 KB) - added by r-a-y 15 months ago.

Download all attachments as: .zip

Change History (12)

@dontdream
15 months ago

demonstrates possible regression in bp_core_new_subnav_item()

#1 @r-a-y
15 months ago

  • Keywords 2nd-opinion added

Thanks for the report.

You might want to check out the note here:
https://buddypress.trac.wordpress.org/ticket/7659#comment:7

The problem is hooking to 'bp_setup_nav'. Try hooking to 'bp_settings_setup_nav' or 'bp_setup_nav' at a later priority as you want to ensure that the Settings nav is setup before registering your subnav.

@r-a-y
15 months ago

#2 @r-a-y
15 months ago

  • Keywords has-patch added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 3.2.0

Actually, you are right, @dontdream !

Instead of returning if no parent nav is found, we can check if there is a parent nav, if so, we can use reset().

I tested the example code in #7659 and your test example and both test cases work.

#3 @boonebgorges
15 months ago

I think that 7931.01.patch will work for the issue described in https://buddypress.trac.wordpress.org/ticket/7659#comment:7. Can you think of any problems that might arise from this "loosening"? That is, we'll no longer be rejecting items registered against non-existent parents. I think that these items would just be ignored during normal rendering anyway, but it's worth thinking about.

#4 @r-a-y
15 months ago

I think we should be safe due to the conditional checking for the slug with bp_is_current_action() and with the parent nav screen function check:
https://buddypress.trac.wordpress.org/browser/tags/3.1.0/src/bp-core/bp-core-buddybar.php?marks=617,618,625#L588

#5 @dontdream
15 months ago

I've tested 7931.01.patch and it works for me too. Thank you!

#6 @r-a-y
15 months ago

  • Keywords has-unit-tests added

I've added a unit test that exhibits the problem.

@boonebgorges - Can you go over the naming of the unit test? I'm not usually good at naming these things.

#7 @boonebgorges
15 months ago

Test and name seems fine to me :)

#8 @r-a-y
15 months ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

In 12194:

Navigation: Ensure subnav menus can be registered before the parent nav is registered.

In #7659, a bug was fixed where restricted subnav menus should be
redirected instead of throwing a 404. The side-effect of the fix is this
broke how plugins can register subnav menus before the parent nav is
created.

This commit rectifies the problem by not bailing out if the parent nav
cannot be found during subnav menu registration. Includes a unit-test.

Props dontdream, boonebgorges, r-a-y.

Fixes #7931 (3.x branch).

#9 @r-a-y
15 months ago

In 12195:

Navigation: Ensure subnav menus can be registered before the parent nav is registered.

In #7659, a bug was fixed where restricted subnav menus should be
redirected instead of throwing a 404. The side-effect of the fix is this
broke how plugins can register subnav menus before the parent nav is
created.

This commit rectifies the problem by not bailing out if the parent nav
cannot be found during subnav menu registration. Includes a unit-test.

Props dontdream, boonebgorges, r-a-y.

See #7931 (trunk).

Note: See TracTickets for help on using tickets.