Skip to:
Content

Opened 3 years ago

Closed 3 years ago

#3476 closed defect (bug) (fixed)

Change priority of bp_screens (and maybe bp_actions) hooks

Reported by: boonebgorges Owned by:
Milestone: 1.5 Priority: major
Severity: major Version: 1.5
Component: Core Keywords: has-patch
Cc:

Description

When the bp_screens and bp_actions action hooks were introduced, they introduced some much needed sanity and regularity to the way that events were hooked to pageloads. In what I assume was an attempt to mirror the old, haphazard hooking of items directly to wp, both bp_actions and bp_screens were hooked to wp with priority 2.

However, this causes problems with plugins that are written in a certain way. In particular, it used to be recommended (in the Skeleton Component, among other places) that your plugin set up its globals and nav like this:
{{{ function bbg_setup_stuff() {

global $bp;
$bp->boone->is = 'cool';

}
add_action( 'wp', 'bbg_setup_stuff', 2 );}}}

With the introduction of bp_screens and bp_actions, this method now works only sometimes: it will work if you put it in bp-custom.php or in functions.php of your theme (so that you are hooked to wp's 2 priority slot *before* the screen functions), and it will also work if it's in a plugin that *happens* to be loaded before BuddyPress (I think it's done alphabetically by plugin directory name), but in other cases it does not work. That's because bp_screens is the action to which screen functions are hooked, and screen functions generally call bp_core_load_template(), which dies after output.

We can, and I think should, avoid this. By changing the priority of bp_screens to 3 or 4, we make sure that plugins loaded in this outmoded (but still widely used) fashion will continue to work.

bp_actions is a slightly different story. Functions hooked to bp_actions generally do not render templates, and so the same problems don't necessarily arise. However, it's still probably best to make sure that plugins have a chance to load themselves before running bp_actions type functions. Moreover, it makes sense to load bp_actions *before* bp_screens, because bp_actions functions generally redirect to a different page without rendering anything. Right now, bp_actions does indeed fire before bp_screens, but I think it's only accidentally, because it's called first.

My recommended approach (move bp_screens to 4, bp_actions to 3) is in the attached patch. At the very minimum, we should strongly consider moving bp_screens.

Attachments (1)

3476.patch (605 bytes) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (4)

boonebgorges3 years ago

comment:1 DJPaul3 years ago

I think your reasoning is good, but of course the tricky part is figuring out if this change breaks anything subtle and non-obvious. I think this should go in before next beta/rc so that everyone testing has time to notice if it's broken anything.

comment:2 boonebgorges3 years ago

Thanks for the feedback, DJPaul. I've been thinking about this some more (in particular, about your a propos question "will it break anything else") and I'm pretty convinced that it won't. The only possibility is this: someone has hooked a function to 'wp' with priority 3 or 4 (which we don't do in BP, so it'd have to be another plugin); on the current setup, that function would never fire on a BP page, while with this change, it *will* fire. It's very unlikely that a BP-related plugin would ever have done this (the plugin would never have worked!). And if it is happening with a non-BP plugin, I would say that this change actually *fixes* a problem, rather than causes one.

I'm going to make the change and mark the ticket closed. Let's keep an eye (and an ear) out for issues that might be related to screen function load order.

comment:3 boonebgorges3 years ago

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

(In [4977]) Bumps priority of bp_actions and bp_screens, to provide more consistent load order and better backward compatibility with existing plugins and themes. Fixes #3476

Note: See TracTickets for help on using tickets.