Skip to:
Content

Opened 4 years ago

Closed 4 years ago

Last modified 2 years ago

#5390 closed enhancement (fixed)

Remove some old BuddyBar stuff

Reported by: DJPaul Owned by: boonebgorges
Milestone: 2.1 Priority: normal
Severity: normal Version:
Component: Toolbar & Notifications Keywords: has-patch commit
Cc:

Description

The BuddyBar was deprecated back in 1.6 in favour of switching to then-new WP Toolbar. We force-upgraded people to the Toolbar in favour of an opt-out during the upgrade wizard. Generally, this seemed to have gone pretty well, and I haven't seen anyone asking for the BuddyBar back since then.

While some of the BuddyBar code is tied into the BP navigation menus and routing, and so should remain, there remains a bunch of code we should be able to pull out now. bp_use_wp_admin_bar() tends to be used as a wrapper around some of these places (e.g. bp_core_load_buddybar_css()).

I suggest we just remove what we can for 2.0 with a view to keeping the BP code as tidy and relevant as possible.

Attachments (2)

5390.01.patch (43.5 KB) - added by DJPaul 4 years ago.
5390.02.patch (42.7 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (18)

#1 @boonebgorges
4 years ago

Stuff that can get pulled out without breaking anything, let's do. And if there's a way to reorganize the codebase to deemphasize the old stuff, that'd be great too (the fact that we have nav stuff in the core-buddybar file doesn't make any sense).

However, let's not sacrifice any backward compatibility for the sake of tidiness. It doesn't hurt anything to have the BuddyBar in there for people who are still using it.

#2 @DJPaul
4 years ago

To your point specifically about leaving the BuddyBar itself in the code. I disagree, and my idea to delete the BuddyBar is what inspired me to create this ticket. Leaving significant amounts of code in the project that is not run for both new installs, and surely the vast majority of existing sites -- considering the forced, opt-out upgrade back in 1.6 -- leaves a significant amount of technical debt in the code that otherwise we'd have to maintain in perpetuity. Why do we want to continue supporting a worse, un-supported, lower quality toolbar when we have a forwards-compatible alternative that's been running in BP for just over a year and a half, powered by WP Core APIs?

The only ways to re-enable the old BuddyBar is to set a site option with a key of "_bp_force_buddybar", define the BP_USE_WP_ADMIN_BAR constant as false, or filter bp_use_wp_admin_bar. It's not controllable via an option in the UI, and we don't even bother to tell people to not use the BuddyBar, because no-one ever sees it or knows about it.

A Google search of plugins.svn.wordpress.org and themes.svn.wordpress.org has no matches for any of these, so no-one is publicly downgrading to the BuddyBar from a theme or a plugin.

Compared to the discussion in #5351 about removing old bbPress, I think this is an easy no-brainer.

#3 @boonebgorges
4 years ago

a significant amount of technical debt in the code that otherwise we'd have to maintain in perpetuity.

I disagree. This code has not been touched in years. I assume we'd only spend any time on it in case of a serious security issue. In other words, it's deprecated (though we perhaps haven't been as explicit as we should've been about this deprecation). It doesn't take any time or effort to leave there, and it doesn't leave us on the hook to support anything but the most extreme cases.

The situation I want to avoid is this: back when we migrated to the WP toolbar, someone set define( 'BP_USE_WP_ADMIN_BAR', false ); in their config file, and their site has been humming along happily since then. If we tear everything out, their site will go whitescreen after upgrading to 2.0. This is unacceptable. (The fact that you didn't find any public plugins or themes doing this is not a good indicator of its popularity. This is a config option that's likely to be set only on individual sites.)

It seems to me that, much like the case of #5351 and bp-forums, we are engineering a solution to a problem that does not exist. IMHO, this is a poor use of our resources. In both cases, the only downsides of the status quo are abstract ideas about technical debt and elegance, which I don't find convincing when the change will have real repercussions for existing sites.

That said, if we absolutely must do something, I see a couple viable options.

  1. Formally deprecate Buddybar code by moving it to deprecated files and adding _deprecated_function() notices, but do not disable it. This could nudge people away.
  2. Move the Buddybar into a plugin, and use a to-be-determined technique to automatically migrate folks to that plugin. (See #5351) It may turn out that the process of migrating to the plugin is manual, in which case we will probably want to put a big admin notice on the Dashboard for a release cycle: "Warning: the Buddybar will be torn out soon. Download the plugin to avoid catastrophe"
  3. Force-migrate users to the WP bar by no longer respecting bp_use_wp_admin_bar(). This will cause Buddybar lovers/customizers some consternation, but at least it will leave them with a working toolbar. Convert Buddybar functions to stubs and deprecate them, in case anyone's calling them directly.

If we're going to move forward with this, I have a strong preference *against* 3. We should avoid alienating users when there's no good technical reason to do so. 2 seems the most elegant, if we can sort out the details. We could even do 1 now, and then plan to phase in 2 over the course of a release or two.

#4 @johnjamesjacoby
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 2.0
  • Type changed from enhancement to task

Starting with Boone's 1 and moving to 2 makes the most sense to me. Let's get the BuddyBar code deprecated and moved into files that can neatly be extracted, should we so choose to do so later.

#5 @boonebgorges
4 years ago

  • Milestone changed from 2.0 to 2.1

I started to deprecate the functions, but it's turning out to be a pain. It results in some empty files (bp-blogs-buddybar.php, etc) and some files that ought to be renamed without wrecking their history (bp-core-buddybar.php) and I don't want to rush this fix. Let's do it for 2.1.

@DJPaul
4 years ago

#6 @DJPaul
4 years ago

  • Keywords has-patch added; needs-patch removed

I still maintain my opinion that the remnants of the buddybar should be removed now. Obv, the navigation building code needs to remain, but everything else is game. See 5390.01.patch.

#7 @boonebgorges
4 years ago

I think this is the wrong approach. Replacing existing functions with stubs will prevent sites from experiencing fatal errors on upgrade, but it will also break existing functionality. I'm all for deprecating the functions - in the sense that we move them to the deprecated files (so a site admin can prevent them from being loaded) and put _deprecated_function() notices into them. But there's no reason why we can't also leave the functions intact. (Side note: to gut the functions is not deprecation at all - it's removal. So in that case it's misleading to put the functions in the deprecated files.)

I have a feeling that DJPaul and I are not going to come to agreement on this point, so I think this is a place where jjj is probably going to have to step in and make an executive decision.

#8 @r-a-y
4 years ago

I'm in agreement with Boone.

My thoughts are we can deprecate the BuddyBar functions, but retain its functionality if needed.

In bp_core_load_admin_bar(), if bp_use_wp_admin_bar() is false, we could then load up the deprecated BuddyBar functions file (eg. bp-core/deprecated/2.1/buddybar.php) and everything should still work. This is how I envision Boone's point no. 1 in comment:3.

#9 @johnjamesjacoby
4 years ago

If someone is holding off updating from 1.6, they're doing it for more than the BuddyBar. If we deprecate it, I imagine it will effect a very small percentage of installations.

That said, BuddyPress is too mature of a project now to completely remove any code without a bulletproof migration plan. The way we moved people off the BuddyBar and onto using the WordPress toolbar was really nicely done; let's not let perfection get in the way of greatness, at least in this instance.

I agree with Boone and Ray above... I'd like to see as much of that old code moved into a file that can be hot-loaded as necessary, but I'm not comfortable completely abandoning any functionality that risks upsetting an unsuspecting user who happily clicked the "Update Now" button in WordPress's dashboard, only to be rewarded with a broken site.

#10 @boonebgorges
4 years ago

5390.02.patch does the following:

  • Moves BuddyBar functions, unmodified into deprecated/2.1.php
  • Instead of adding _deprecated_function() notices to every single one (resulting in a torrent of deprecated notices for sites using the BuddyBar), add a single doing_it_wrong() notice in bp_core_load_admin_bar() if BuddyBar is enabled
  • Leaves all the BuddyBar loading logic in place, but removes all the settings interface

This way, sites using BP_USE_WP_ADMIN_BAR=false will continue to have the BuddyBar work seamlessly, but will see an error. Meanwhile, we get rid of lots of cruft in our main plugin files. Installations that have chosen to block the loading of deprecated functions (BP_IGNORE_DEPRECATED) will not load the functions at all. (Users who mistakenly have define( 'BP_USE_WP_ADMIN_BAR', false ); along with define( 'BP_IGNORE_DEPRECATED', true ); will have no toolbar at all, but the site will continue to load.)

#11 @johnjamesjacoby
4 years ago

  • Keywords commit added

5390.02.patch is exactly what I had in mind.

#12 @boonebgorges
4 years ago

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

In 8568:

Formally deprecate the BuddyBar.

BuddyPress has been fully compatible with the WordPress toolbar since 1.6. We
have provided backward compatible support for the BuddyBar through the
BP_USE_WP_ADMIN_BAR constant. This changeset retains this support, but
officially deprecates it, with the following changes:

  • All UI related to the BuddyBar has been removed from the Dashboard setings panels
  • BuddyBar-related code has been moved to the deprecated functions folder
  • A _doing_it_wrong() notice is now thrown when loading the BuddyBar

Fixes #5390

Props DJPaul for an initial patch

#13 @johnjamesjacoby
4 years ago

In 8579:

Updates to bp_use_wp_admin_bar():

  • Default to true
  • Use BP_USE_WP_ADMIN_BAR constant as true/false.
  • Use bp_force_buddybar() rather than accessing the option directly.

See #5390.

#14 @boonebgorges
4 years ago

In 8602:

Undeprecate bp_force_buddybar()

The change introduced in r8579 means that bp_force_buddybar() is still used in
a BP core function, and thus should not be deprecated. (If kept in the
deprecated functions file, a fatal error is caused when these files are not
loaded.)

See #5390

#15 @DJPaul
2 years ago

  • Component changed from General - Toolbar/BuddyBar to Toolbar & Notifications

#16 @DJPaul
2 years ago

  • Type changed from task to enhancement
Note: See TracTickets for help on using tickets.