bp_core_add_ajax_url_js() generating wrong ajaxurl value

[Kawauso]

site_url( 'wp-load.php' ) does not load any of WordPress' AJAX functionality and breaks anything relying on ajaxurl does not set it later. Correct URL should be admin_url( 'admin-ajax.php' ).

[DJPaul]

admin-ajax.php loads wp-load.php near the top of the file. By not using admin-ajax.php, we avoid potentially processing requests that might trigger some of admin-ajax.php's handling for /wp-admin.

Did you find another plugin using this variable name, and it conflicting? We could rename ours to bp_ajaxurl.

[Kawauso]

Just found bp_core_add_ajax_hook(), so I'm much less perplexed about things.

The variable name is suggested in the WordPress Codex article on ​AJAX in Plugins. I'm not specifically aware of any plugins besides my own using it, but it does behave quite differently to the native WP hook (no wp_ajax_nopriv / wp_ajax fires on all requests, DOING_AJAX not defined and WP_ADMIN is false), which I suspect is what threw my plugins.

[DJPaul]

Three solutions: 1) Leave as-is as I haven't seen any complaints a JS name conflict in the wild before, 2) Rename to bp_ajaxurl, or 3) Switch to admin-ajax.php (need to specify a custom 'action' in the POST, which may conflict with BP AJAX variable names).

[Kawauso]

Might as well stick with (1), as (2) would potentially break existing themes and (3) would result in unexpected behaviour. Apologies for not researching this properly initially, will provide further feedback if I find conflicts in the wild.

[vtowel]

We are using BP on our site, and I find it annoying that the BP code causes WP's AJAX functionality to behave differently than is documented in the ​WordPress Codex article AJAX in Plugins.

WP plugin developers reading this Codex document - such as myself - will probably expect the global JS variable ajaxurl, if defined, to point to admin-ajax.php. I however didn't notice that the variable definition was different from the Codex's version (thanks to BP being installed), and as a result the hooks that I've written my plugin around (thinking admin-ajax.php has been run) are not fired. In particular, "admin_init" is not fired. I'm still investigating to find out which initialization hook I can use so that my plugin will work in any WP installation, whether BP has appropriated the ajaxurl global or not.

So while option (2) will probably break existing themes, I do think it's the most logical option. I think it's good design to prefix all globals, whether in PHP or in JS, with a prefix unique to any given software module to avoid name collisions. It only makes sense to me that BP should, too.

[vtowel]

Or... since option (2) is probably going to annoy more people than option (1) will, I'll just follow the advice ​here (which I also found through the WP Codex), and recommend that the section in the Codex about using the ajaxurl global be removed altogether, and the aforementioned article's advice by used instead.

[DJPaul]

On reflection, I would like BuddyPress to use option 3 as described in a previous comment. Renaming or namespacing the variable will break a lot of plugins and as much as I appreciate this issue is annoying to you, I've only seen a handful of posts about this in several months. If we're going to change this and potentially break a lot of plugins, let's make sure the new implementation is doing it properly.

I am changing the milestone to 1.4 as I don't feel it is something we have time for in 1.3 unless someone contributes a patch which we can test thoroughly.

[kawauso]

Attached a first pass at converting BuddyPress' AJAX functions to WordPress standard compliant ones and also stripped out some redundant global $bp; calls.

Key differences:

• WordPress has different actions for logged in and non-logged in users, wp_ajax and wp_ajax_nopriv respectively. BuddyPress combines these under wp_ajax.

• WordPress requires that all functions end with die(), otherwise a 0 will be appended. This noticeably breaks JSON-based functions.

• BuddyPress runs with DOING_AJAX and WP_ADMIN both undefined. These are defined as true under WordPress.

I've taken out is_user_logged_in() where present in favour of using just wp_ajax and I think there are wp_ajax_nopriv hooks where needed, but I'm not overly familiar with the code, so it could use some testing.

[kawauso]

Everyone has to die() sometime

[kawauso]

Refresh. Won't cover any new AJAX calls since last patch.

[boonebgorges]

+1 for something like this patch, though we'll need to ensure backward compatibility with existing plugins. As long as they're using ajaxurl in their ajax calls, it should just work, right?

[DJPaul]

Not sure what the status of these patches are, but unless someone takes ownership and updates and tests, this will be punted.

[r-a-y]

kawauso's patch looks good for the most part, but backwards compatibility is an issue. Also needs a refresh.

---

Replying to boonebgorges:

+1 for something like this patch, though we'll need to ensure backward compatibility with existing plugins. As long as they're using ajaxurl in their ajax calls, it should just work, right?

Not necessarily. Most plugins using the BP ajax hook do not use the wp_ajax_nopriv hook. nopriv is necessary for frontend AJAX functions for non-logged-in users. So depending on the AJAX use case, it could break some existing plugins.

---

Paul makes a great point here:
​http://buddypress.trac.wordpress.org/ticket/2599#comment:2

If we use admin-ajax.php, it loads all WP admin functions, which is unnecessary bloat.

The major con for not implementing admin-ajax.php is the DOING_AJAX constant never works for BP ajax calls. I've had to implement my own conditional to find out if AJAX is firing.

I might suggest option (4), create our own version of admin-ajax.php. It would be a super-lightweight version of admin-ajax.php, but bypassing all the requires and WP post code that admin-ajax.php currently has. This might be considered too extreme though.

[boonebgorges]

Good point about the admin-ajax.php overhead. IMO, that rules out that possibility altogether. If name conflicts are truly an issue, then we should change to bpajaxurl, though we shouldn't do this either unless we can come up with a clever way to ensure backward compatibility with plugins using our implementation of ajaxurl (which is, I'd reckon, every BP plugin that uses AJAX - certainly every one I've built).

[DJPaul]

But it's not really going to cause any extra overhead over any standard AJAX function implementation that does use admin-ajax.php. Since we'd might also need to rename or refactor some of the javascripts (e.g. to not use an "ACTION" variable, and so on), might be best to look at this in 1.7

[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.