Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 5 years ago

#3625 closed enhancement (no action required)

Core functions should not hook to bp_init with default priority

Reported by: boonebgorges Owned by:
Milestone: Priority: normal
Severity: normal Version: 1.5
Component: Core Keywords: needs-refresh
Cc:

Description

I had a good discussion on ticket #3622 about the bp_init hook and plugins. Conceptually, I think that the idea behind bp_init is to let plugins attach certain actions to a point in the load order when they can be sure that BP's core functionality has been fully loaded. This mostly works - bp_setup_globals, bp_setup_nav, and other initialization hooks all happen before bp_init priority 10. However, there are a few important places in BP where we use priority 10, with the result that if a plugin happens to be loaded by WordPress *before* BP is, those hooks could fail to find the corresponding action. To wit:

bp_core_load_admin_bar_css()
bp_core_load_buddybar_css()
bp_core_admin_menu_init() (initializes the admin menu loader)
bp_core_add_ajax_hook() (creates the wp_ajax_ hooks on the front-end that WP has by default on the back end)
bp_core_wpsignup_redirect() (redirects users away from wp-signup.php when BP is installed)

There are also some places in the friends component where we hook to bp_init, but IMO that's not as crucial, since our optional components essentially operate like plugins anyway.

I propose that all of the above function be moved from bp_init priority 10 to 8 or 9. This will obviously need some thorough testing (though I'm pretty certain that making these particular functions fire a bit *earlier* is not going to hurt anything). I'll post a patch.

Attachments (2)

3625.01.patch (2.4 KB) - added by boonebgorges 10 years ago.
3625.02.patch (8.2 KB) - added by webraket 10 years ago.

Download all attachments as: .zip

Change History (14)

#1 @webraket
10 years ago

I think setting it to 8 does hurt some things, for example the title is set at priority 9.
http://buddypress.trac.wordpress.org/browser/trunk/bp-core/bp-core-hooks.php#L33

My suggestion would still be to never hook core functions on bp_init.

WordPress also never hooks core functions to their init action.
http://codex.wordpress.org/Plugin_API/Action_Reference/init

Even if a plugin hooks to bp_init with priority 1, BuddyPress should be fully initialised at that point.

Last edited 10 years ago by webraket (previous) (diff)

#2 @boonebgorges
10 years ago

I think setting it to 8 does hurt some things, for example the title is set at priority 9.

http://buddypress.trac.wordpress.org/browser/trunk/bp-core/bp-core-hooks.php#L33

I don't think that hurts anything.

Even if a plugin hooks to bp_init with priority 1, BuddyPress should be fully initialised at that point.

Maybe. But that will require a pretty massive overhaul of load order. Our entire initialization routine runs on bp_init.

#3 @webraket
10 years ago

Sorry but your patch doesn't fixes the problem.

BuddyPress should support add_action('wp_ajax_') on the default priority 10 of bp_init.

Lowering the priority from the bp_core_add_ajax_hook() function that calls do_action('wp_ajax_') from 10 to 8 only makes it worse.

#4 @boonebgorges
10 years ago

Sorry but your patch doesn't fixes the problem.

You may be right. I wasn't able to reproduce the problem in the first place, so I was kinda guessing.

Lowering the priority from the bp_core_add_ajax_hook() function that calls do_action('wp_ajax_') from 10 to 8 only makes it worse.

I don't understand how it could "make it worse". I guess I assumed that the problem was that, by hooking to bp_init with priority 10, you were sometimes attempting to hook to wp_ajax_ before BP was there to listen for the hooks. Moving the priority up would mean that the wp_ajax_ hook mechanism would be in place by the time you tried adding your own stuff at bp_init:10. It's possible that I'm misdiagnosing the issue, but in any case I don't know how it would make it worse, since it either works or it doesn't work - there's not really room for "better" or "worse" here.

If you have ideas about how this issue could be solved in a broader way, I would welcome a patch.

#5 @webraket
10 years ago

You probably can't reproduce because I load plugins from /mu-plugins.
/mu-plugins are always loaded before /plugins/buddypress

Your patch makes this a little worse, because if you move the ajax function to bp_init priority=8, it also breaks (plugins hooked to bp_init with default priority=10) when BuddyPress "IS" loaded before the actual plugin.

#6 @boonebgorges
10 years ago

In that case, I guess I don't really understand the problem at all. I thought the issue was that BP core functions were loading too *late*. But you're saying that they load too early?

#7 @webraket
10 years ago

I'm a little confused myself, my previous reply was incorrect, sorry.

Your patch makes this a little worse, because if you move the ajax function to bp_init priority=8, it also breaks (plugins hooked to bp_init with default priority=10) when BuddyPress "IS" loaded before the actual plugin.

My wp_ajax plugin is only activated for the rootblog, so it's in /plugins instead.
BuddyPress is actually loaded "before" this plugin.

That's why BuddyPress calls do_action('wp_ajax_') before my plugin has called add_action('wp_ajax_') when they are both hooked to bp_init with the default priority 10.

So yes, these BP core actions load too early, hooking into bp_setup_globals is the only way to get the ajax call working (for now).

---

I thought the issue was that BP core functions were loading too *late*.

That was related to the other ticket when a plugin hooks to init from /mu-plugins with default priority 10.
At that point bp_init is not called (yet), so you can't get activity slugs or other BuddyPress globals.

Hooking to bp_init instead of init already fixes that problem.
However when a developer only tests on /plugins and BuddyPress is loaded before the plugin, they won't have problems hooking into init. (until a multisite user drops their plugin into /mu-plugins)

@webraket
10 years ago

#8 @webraket
10 years ago

Just an idea, not tested.
Plugins already hooked to bp_init (with default/10 or higher priority) will not be affected.

#9 @cnorris23
10 years ago

This is already confusing, but I just wanted to jump in too see if I can bring some clarity. First, while I don't think it's necessary, it definitely wouldn't hurt to load our more important business functions, like the ones listed by boone, a little earlier for those hooking into bp_init priority 10.

Second, the biggest challenge with hooks is knowing when/where to hook. In the case of the wp_ajax_ hook example, it seems webraket is trying something like this

<?php
class Webraket_Class {
    
	public function __construct(){
				
		add_action('wp_ajax_', array(&$this,'my_wp_ajax_'));
		
	}
	
	function my_wp_ajax_() {    
		//
	}
		
}

function Init_Webraket_Class(){
	$webraket = new Webraket_Class;
}
add_action('bp_init', 'Init_Webraket_Class');
?>

The problem with this is that you're trying to hook a function to wp_ajax_, which itself is hooked to bp_init priority 10, within a class that's not instantiated until bp_init priority 10. While this doesn't preclude success (something I learned from Jeff Sayre's WordPress Hook Sniffer), it's doesn't mean that it will fail more times that not. In the case of the wp_ajax_ hook, the latest you could load your class would be like this

function Init_Webraket_Class(){
	$webraket = new Webraket_Class;
}
add_action('bp_init', 'Init_Webraket_Class', 9);

but even better would be

function Init_Webraket_Class(){
	$webraket = new Webraket_Class;
}
add_action('bp_include', 'Init_Webraket_Class');

or like boone stated on #3622

function Init_Webraket_Class(){
	$webraket = new Webraket_Class;
}
add_action('bp_loaded', 'Init_Webraket_Class');

However when a developer only tests on /plugins and BuddyPress is loaded before the plugin, they won't have problems hooking into init. (until a multisite user drops their plugin into /mu-plugins)

It's true that plugins in the mu-plugins folder using the init hook will find that BP isn't already loaded, but that's the nature of the mu-plugins folder. However, just because a plugin is in the /plugins folder doesn't mean that one will have success. It's a crap shoot, which I've learned from personal experience, and again, from Jeff Sayre's WordPress Hook Sniffer.

@webraket I'll post some example code on #3622 on how to get your code working better.

#10 @boonebgorges
10 years ago

  • Milestone changed from 1.6 to Future Release

Groan, this issue is so complicated. No time in this milestone. I hope someone can step in and clarify everything in just a few sentences, or a beautifully simple patch :)

#11 @DJPaul
9 years ago

  • Keywords needs-refresh added; has-patch needs-testing removed

#12 @DJPaul
5 years ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from new to closed

I am pretty baffled with this ticket. Going to close it and if someone wants to clarify and thinks it's still an issue in modern BuddyPress, please open a new ticket.

Note: See TracTickets for help on using tickets.