Skip to:
Content

BuddyPress.org

Opened 11 years ago

Closed 9 years ago

#2599 closed defect (bug) (fixed)

bp_core_add_ajax_url_js() generating wrong ajaxurl value

Reported by: Kawauso Owned by:
Milestone: 1.6 Priority: normal
Severity: normal Version:
Component: Core Keywords: needs-testing needs-refresh
Cc:

Description

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' ).

Attachments (3)

2599.diff (429 bytes) - added by Kawauso 10 years ago.
2599.2.diff (14.7 KB) - added by kawauso 10 years ago.
Everyone has to die() sometime
2599.3.diff (14.6 KB) - added by kawauso 10 years ago.
Refresh. Won't cover any new AJAX calls since last patch.

Download all attachments as: .zip

Change History (21)

@Kawauso
10 years ago

#1 @Kawauso
10 years ago

  • Keywords has-patch added; ajax removed

#2 follow-up: @DJPaul
10 years ago

  • Keywords reporter-feedback added; has-patch removed

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.

#3 in reply to: ↑ 2 @Kawauso
10 years ago

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.

#4 @DJPaul
10 years ago

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

#5 @Kawauso
10 years ago

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.

#6 @vtowel
10 years ago

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.

#7 @vtowel
10 years ago

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.

#8 @DJPaul
10 years ago

  • Keywords needs-patch added; reporter-feedback removed
  • Milestone changed from 1.3 to 1.4

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.

#9 @kawauso
10 years ago

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.

#10 @kawauso
10 years ago

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

@kawauso
10 years ago

Everyone has to die() sometime

@kawauso
10 years ago

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

#11 follow-up: @boonebgorges
9 years ago

  • Severity set to normal

+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?

#12 @DJPaul
9 years ago

  • Keywords needs-refresh added; has-patch removed

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

#13 in reply to: ↑ 11 @r-a-y
9 years ago

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.

#14 @boonebgorges
9 years ago

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

#15 @DJPaul
9 years ago

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

#16 @boonebgorges
9 years ago

  • Milestone changed from 1.6 to Future Release

#17 @DJPaul
9 years ago

  • Milestone changed from Future Release to 1.6

#18 @djpaul
9 years ago

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

(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.
Note: See TracTickets for help on using tickets.