Skip to:
Content

BuddyPress.org

Opened 2 years ago

Last modified 3 months ago

#7729 new enhancement

Remove BuddyBar?

Reported by: DJPaul Owned by:
Milestone: 7.0.0 Priority: normal
Severity: normal Version:
Component: Navigation Keywords: has-patch needs-dev-note 2nd-opinion
Cc: m_uysl

Description

See https://imgur.com/a/KEiM5

The Buddybar was deprecated in 2.1 (2014). You can turn it on define( 'WP_DEBUG', false ); define( 'BP_USE_WP_ADMIN_BAR', false ); but it doesn't fully work nowadays and it throws PHP notices.

I know a lot of its code base is tied to the navigation bar logic, but it does load easy-to-remove CSS assets.
What do you think of taking advantage of our major 3.0 release and removing as much of the Buddybar code as easily possible (basically removing the backwards compatibility)?

Attachments (4)

7729.01.patch (35.4 KB) - added by r-a-y 2 years ago.
7729.02.patch (24.6 KB) - added by r-a-y 2 years ago.
7729.03.patch (33.4 KB) - added by r-a-y 5 months ago.
7729.04.patch (34.7 KB) - added by imath 5 months ago.

Download all attachments as: .zip

Change History (27)

#1 @r-a-y
2 years ago

+1 from me here.

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


2 years ago

@r-a-y
2 years ago

#3 @r-a-y
2 years ago

  • Keywords has-patch added

01.patch is a first pass at removing BuddyBar functionality.

The removal of bp_loggedin_user_domain() to each component's setup_nav() method is due in part to the generation of the BuddyBar. Since we're no longer supporting the BuddyBar, we can remove those instances and only check if we're on a user profile page.

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

#4 @DJPaul
2 years ago

  • Milestone changed from Awaiting Review to 3.1

Nice. I think this doesn't fit into 3.0 as we're short on time, and the risk is raised because of the setup_nav() changes you found (unless you can very test this next week (before Friday) maybe).

#5 @DJPaul
2 years ago

The patch looks OK so far. I need to get my brain around the setup_nav change (or we get a second opinion from someone).

Are the functions in bp-core-buddybar.php basically route parsing/registration?

@r-a-y
2 years ago

#6 @r-a-y
2 years ago

the risk is raised because of the setup_nav() changes you found (unless you can very test this next week (before Friday) maybe).

There is one issue with 01.patch and that is those that rely on the BuddyBar / older buddypress()->bp_nav for generating the logged-in navigation.

01.patch breaks this unit test:
https://buddypress.trac.wordpress.org/browser/trunk/tests/phpunit/testcases/routing/core.php

Since we are removing the BuddyBar, there isn't much of a use-case for keeping logged-in navigation. We also introduced BuddyPress integration into "Appearance > Menus" a long time ago as well (#5122), which most sites are probably using for menu generation.

If we want to keep compatibility with older, logged-in navigation, then see 02.patch.

However, if we're not concerned with the older logged-in navigation, I've already tested 01.patch for all frontend user profile navigation items and this still works.

Are the functions in bp-core-buddybar.php basically route parsing/registration?

bp-core-buddybar.php is poorly named, but that file is critical to user nav registration.

Could be renamed to bp-core-nav.php or something like that.

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

#7 @DJPaul
2 years ago

  • Milestone changed from 3.1 to 4.0

Milestone renamed

#8 @DJPaul
2 years ago

  • Keywords dev-feedback added

It would be interesting to get feedback from other core contributors here about which patch we should proceed with.

#9 @r-a-y
2 years ago

Just saw someone post an example using the older $bp->bp_nav syntax in the forums, so let's go with the conservative 02.patch.

#10 @DJPaul
22 months ago

  • Keywords commit added

#11 @boonebgorges
22 months ago

  • Milestone changed from 4.0 to Up Next

#12 @m_uysl
21 months ago

  • Cc m_uysl added

#13 @imath
9 months ago

  • Keywords needs-refresh added; commit removed
  • Milestone changed from Up Next to 6.0.0

Thanks a lot @r-a-y for your work on these patches. Let's try to have this ticket fixed during the 6.0.0 development cycle as we were very close to have the conservative version of the patch committed.

#14 @espellcaste
5 months ago

@imath Are you still considering committing this one in this development cycle?

#15 @imath
5 months ago

  • Keywords dev-feedback removed

Yes but the patch needs to be refreshed (eg: deprecated/6.0.0 and not 2.1.0). I'll take care of it if @r-a-y doesn't beat me to it ☺️

@r-a-y
5 months ago

#16 @r-a-y
5 months ago

  • Keywords needs-refresh removed

03.patch is a refreshed patch of 02.patch, but also deprecates bp_force_buddybar(), removes the admin option for the BuddyBar and updates the PHPDoc in a few places.

Let me know what you think.

@imath
5 months ago

#17 @imath
5 months ago

  • Keywords needs-dev-note added

Hi @r-a-y

Thanks for the refresh, I just tested the patch and I'm having a doubt about line 596 of src/bp-groups/classes/class-bp-groups-component.php. In your patch you're removing it, but then we get a failing test: BP_Tests_Core_Nav_BpCoreNewNavItem::test_group_nav().

I've been tested the patch with or without this line and it doesn't seem to have an impact. But I'd probably keep it this way, just in case.

If you're sure we don't need it anymore, then remove the BP_Tests_Core_Nav_BpCoreNewNavItem::test_group_nav() test ;)

I believe we probably need to write a dev note about this removal on bpdevel 🤔

#18 @r-a-y
5 months ago

Hi @imath, do you remember why you wrote the unit test? (r9818) :)

I believe the -1 thing is a hack because the BuddyBar required it so the group nav wouldn't show up in the BuddyBar. See its introduction in r3818 by JJJ (wow that's a long time ago!). Since we're no longer using the BuddyBar, this shouldn't be required, but I can understand why you would feel safer leaving it in. If we want to be cautious, let's just leave the line in.

#19 @imath
5 months ago

Hi! Probably for the same reason I'm worried about it today :)

I think I'd leave it as I believe it's the reason of this code into the bp_core_register_nav_screen_function()

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


5 months ago

#21 @imath
5 months ago

  • Keywords 2nd-opinion added

@r-a-y I'd like to publish 6.0.0-beta1 early next week. Do you think we'd better move this ticket in next milestone ?

#22 @imath
4 months ago

  • Milestone changed from 6.0.0 to Up Next

Let's do it early during 7.0.0 dev cycle.

#23 @imath
3 months ago

  • Milestone changed from Up Next to 7.0.0
Note: See TracTickets for help on using tickets.