Race condition processing Ajax requests.

[gary_mazz]

BP ajax requests are triggered with action 'bp_init' in function bp_core_add_ajax_hook(). Plugin initialization triggers at 'bp_init' level 10, but after the ajax request is processed. Any ajax actions and filter are missed due to the early ajax processing.

Workaround:
I have shifted the plugin init to 'bp_init' level 5, seems to fix most the issues

[boonebgorges]

Could you provide the code you're using that's causing the problem? The AJAX in my plugins is all working correctly, hooks and all.

[r-a-y]

gary_mazz makes a good point.

The bp_core_add_ajax_hook() should probably run after 'bp_init' (later than 10) so all plugins can initialize their code before the ajax hook is run.

The same problem happened when I was working on the core BP embed code, which is why the embed code is hooked at priority 9:
​http://buddypress.trac.wordpress.org/browser/tags/1.5.3.1/bp-core/bp-core-functions.php#L938

So currently, if you initialize your plugin on 'bp_init' you should do so at a priority lower than 10 and AJAX will run properly.

[boonebgorges]

So the issue is initiating your plugin at bp_init? Best practice is to load at bp_include ​https://github.com/boonebgorges/buddypress-skeleton-component/blob/master/loader.php#L63, which runs way earlier: ​http://buddypress.trac.wordpress.org/browser/tags/1.5.3.1/bp-core/bp-core-hooks.php

[r-a-y]

Boone is right; best practice is to load your custom code at 'bp_include'.

'bp_init' is kind of a misnomer. Still, is it worth moving the ajax hook further back?

[johnjamesjacoby]

(In [5768]) Change bp_core_add_ajax_hook() priority from 10 to 20 to allow plugins to hook into bp_init at the default priority and still have ajax function as expected. Fixes #3985. Props r-a-y, gary_mazz.

[MikeLittle]

I do not believe this is the correct solution to this problem. Let me give my example:

I have a plugin that registers some custom taxonomies for users. This plugin is not dependent on BuddyPress, but if BuddyPress is active, it adds some more hooks (on plugins loaded, because bp_include won't work in the middle of a class!) to display these user taxonomies on the profile page, for example.

A colleague decided to highlight one of the user's taxonomies on the members page. It works fine on first view of the members page and correctly display one of the custom taxonomy terms for each user. But when using the order by drop down and triggering an ajax refresh, it stops working.

On investigation, I found that the taxonomies have not been created by the time the template code runs again!

The taxonomies are created on the 'init' hook at the default priority level. The problem comes because bp_init(), the function, is also hooked on 'init' at the default level. The final piece in this problem is that bp_core_add_ajax_hook() is hooked on bp_init too.
So here's the bad scenario.

plugin 1 hooks function_1() on init
buddypress hooks bp_init() on init, and thus hooks bp_core_add_ajax_hook()
plugin_2 hooks function_2 on init

on a normal request everything is fine: all the functions hooked on init run long before any template files are displayed.

On an ajax request though, function_1() runs, then bp_init() runs, bp_init it calls do_action(bp_init) which calls the ajax_handler. That loads a template file, and starts generating some html, which wants to use the taxonomies registered by function_2().
Poor old function_2() hasn't run yet!, because WordPress hasn't finished processing all the init hooks yet!

Function_2 is my function which defines the taxonomies and is sensibly hooked on init. (register_taxonomy() should not be called before init according to it's docs)

Now, I can work around the problem by hooking my taxonomy creation at a higher priority than bp_init. But that is a work-around.

The problem is you should not be calling locate_template or even ajax handling from a default priority 'init' hook, it's way too early.

Try hooking the ajax handler on 'wp_loaded' if you can't, for some reason, use the proper WP ajax handling (wp-admin/admin-ajax.php).

In fact right at the bottom of wp-settings.php just after it calls do_action('init'), it recommends using the proper ajax handling, then suggests (implies) you can used the wp_loaded action for such things.

So, please, the correct solution to this problem, and other problems I've seen around non-initialisation during ajax processing, is to move the ajax processing to the proper admin-ajax.php processing. OR failing that, shift it to the 'wp_loaded' action.

[johnjamesjacoby]

Replying to MikeLittle:

I do not believe this is the correct solution to this problem. (...)
move the ajax processing to the proper admin-ajax.php processing. OR failing that, shift it to the 'wp_loaded' action.

Seems sane to me. BuddyPress core doesn't have proper subactions for the ajax requests that it does, mostly because it's heavily theme dependent. If we're going to do this, I'd suggest we have a core BuddyPress subaction to piggy back the WordPress core ajax action, then audit the bp-default ajax actions and hook them onto the new BuddyPress subaction.

[DJPaul]

Re-hooking to admin-ajax.php also involves checking that our AJAX requests don't set the "action" parameter to anything already in use by WordPress (in the switch in admin-ajax.php).

[DJPaul]

As we are near the end of the 1.6 cycle, I suggest that we switch the ajaxurl variable to point to admin-ajax.php, which should solve the problem in this ticket. I think this is pretty safe and it should just work.

We could even add the new subaction that John suggests (the benefits of this for AJAX handling are not immediately obvious to me), but I really don't want to touch BP-Default's AJAX in 1.6; it's all going to be re-written in 1.7, so we can do larger changes then.

[boonebgorges]

See also #2599.

I don't see any reason off the top of my head why we can't just switch to admin-ajax.php. Seems like it should just work - since we'll be moving the hooks to *later* in the load process, it should be totally backward compatible.

John, are you suggesting a wrapper for WP's wp_ajax_ logic? I guess the gain would be that plugin actions hooked to this would not fire if BP were not installed? If that's the only gain, I guess I would rather not introduce the overhead (plugins should be loading their BP content on bp_loaded anyway, and in any case there may good reasons why one might want to hook an AJAX handler that fires even in the absence of BP, and having our own hook would require conditional hooking in that case)

[DJPaul]

First attempt at a patch attached (3985-01.patch). Please test on default theme in trunk, and on child themes inheriting Default's ajax.php (e.g. Status theme), and on themes that include a copy of the old ajax.php. Please also test on bug reported in this ticket (custom taxonomy problem).

A key difference is that admin-ajax.php always returns "0" at the end of output, unless the AJAX handler terminates script execution, e.g. exit(). This caused breakage in a lot of places because all the return values had "0" suffixed to them; in some places this was visible and/or broke things.

There was and still is a mismatch of all different sorts of return types in ajax.php, and with the intention of not breaking backpat or requiring sizeable updates to BP-Default's javascript, I've kept things as simple as possible. Suffice to say, I'm looking forward to 1.7 when we can re-build all of this from scratch.
---

Resolve problems with load order when handling AJAX requests in BuddyPress.

• Reworks BP-Default's AJAX calls to use correct receiver in WordPress.
• Hooks BP-Default's AJAX handlers to both 'wp_ajax_nopriv_' and 'wp_ajax'.
• Updates associated parts in BP core to support this, and deprecates old handling.
• Renames Activity spam/unspam AJAX actions (new to BP 1.6).
• Adds full PHPDoc to ajax.php.
• Code standards pass of ajax.php.

[boonebgorges]

Thanks for all your work on this, Paul. Looks pretty good, with one problem.

I've applied the patch against the current trunk, with the following theme setups:

1. BP-Default
2. Child theme of BP-Default, which allows bp-default to use its native ajax.php and global.js (Status)
3. A theme adapted for BP using a recent version of the BP Template Pack (loads ajax.php and global.js directly from bp-default, so this is essentially the same as 2)
4. A theme with manually-added BP compatibility: global.js and ajax.php (old versions) copied into the theme, and enqueued/included manually

As expected, 1-3 all work correctly. Since they all use ajax.php directly from bp-default, they all get the improvements automatically.

In case (4), I'm getting stray '0's. (When clicking Favorite in the activity stream, for instance.) Basically, the problem that Paul outlines above. I have a feeling that this is going to affect a pretty fair number of folks (especially people who have done heavy customization, who are probably some of our eagerest users), so we should come up with some way to help them adjust. I have a couple of ideas:

• Publicize the change heavily. Tell theme devs: Either you've got to copy over your included ajax.php with the new one, *or* you have to manually merge the changes in (if you have customized the file heavily, this might be the better option). Or, we could tell them to unhook our new admin-ajax.php method and manually provide their own ajaxurl, like we always have (they could fire up the deprecated function, maybe). (This latter idea seems bad for a number of reasons, but I'm just throwing it out there.)
• Hack together a fix for these users. Since the problem is that the handler is not killing PHP execution before hitting the end of admin-ajax.php, we could rig BP to do it on behalf of the offending theme. The only place where we can really do it is on the wp_ajax_ and wp_ajax_nopriv_ hooks. Something like this, in one of our core files (*not* the theme):

// Only load when we detect that the old ajax.php is present
if ( !function_exists( 'bp_dtheme_register_actions' ) ) :
	function bp_kill_ajax_output() {
		$actions = array(
			// List all the offending callback functions
			'activity_mark_fav'           => 'bp_dtheme_mark_activity_favorite',
			'activity_mark_unfav'         => 'bp_dtheme_unmark_activity_favorite'
			// etc
		);
	
		foreach( $actions as $name => $function ) {
			add_action( 'wp_ajax_'        . $name, create_function( '', 'die();' ), 9999 );
			add_action( 'wp_ajax_nopriv_' . $name, create_function( '', 'die();' ), 9999 );
		}
	}
	add_action( 'after_setup_theme', 'bp_kill_ajax_output', 20 );
endif;

This is an ugly and pretty hackish solution. But: it fixes the '0' problem on all setups, for more graceful degradation.

My feeling about this is that we should be as generous as possible to theme developers, who really got jerked around when 1.5 dropped. IMO we should do *both* of these routes (education as well as a fallback method).

[DJPaul]

I like your crazy hack idea. Maybe put this helper function in the 1.6 deprecated file, so people can skip loading it by setting $bp->load_deprecated = true ?

Do we revert the return handling changes that I made in that patch, or keep both in case someday we remove this new function from core?

[boonebgorges]

Maybe put this helper function in the 1.6 deprecated file, so people can skip loading it by setting $bp->load_deprecated = true ?

Good idea. I will try to work up a patch today or tomorrow.

Do we revert the return handling changes that I made in that patch, or keep both in case someday we remove this new function from core?

Keep them. Your way is the "right" way - server-side callbacks should be responsible for killing PHP output on their own. That's how WP does it; see (in trunk) /wp-admin/includes/ajax-actions.php.

[boonebgorges]

DJPaul, I've amended your patch to include my hack. As suggested, I've put it in the 1.6 deprecated file. I haven't put a _deprecated_x() call in the function, as I'm not quite sure what it would say - but maybe it's worth considering ("If you're seeing this message, update your ajax.php"?)

To make this patch, I assembled a list of the hook names whose functions did not exit; or die; in BP 1.5.

Seems to work well for me, but a second look would be good.

[DJPaul]

Looks good and works here. Thanks for the updated patch and the testing. I'm going to commit it.

[djpaul]

(In [6003]) Resolve problems with load order when handling AJAX requests in BuddyPress. Fixes #2599 and #3985, props DJPaul and boonebgorges.

• Reworks BP-Default's AJAX calls to use correct receiver in WordPress.
• Hooks BP-Default's AJAX handlers to both 'wp_ajax_nopriv_' and 'wp_ajax'.
• Updates associated parts in BP core to support this, and deprecates old handling.
• Renames Activity spam/unspam AJAX actions (new to BP 1.6).
• Adds full PHPDoc to ajax.php.
• Code standards pass of ajax.php.
• Remove unnecessary globals.
• Backwards compatible with themes based on versions of BP-Default earlier 1.6.