Strip down admin-ajax.php to be a bare bones ajax handler.

[westi]

We should move all of the handlers we have hardcoded in admin-ajax.php off onto action hooks.

That way they become much more self contained and easier to review.

Also the file becomes much more obvious on how a plugin can hook into it as we have loads of good examples :-)

[nacin]

+1000.

[westi]

Use Future Release to constrain 3.2-early

[westi]

This hasn't made the cut for 3.2

I'm still really interested in doing it and would love to help someone curate a patch set that does this which we could use in the 3.3 cycle

[garyc40]

I'd love to do this :) I thought you were going to do it so I didn't make a move.

Should we go on with this after 3.2 is frozen?

[kurtpayne]

You guys looking for something like 15327.diff​? I've given this some dev testing, but it definitely needs a thorough poking. Let me know if I'm heading down the wrong road.

Also, not sure what version this would go in. None of the strings changed, no new features were added, just moved around.

[kurtpayne]

Refreshed for 3.4

[kurtpayne]

15327.2.patch​ moves everything to a separate file (wp-admin/includes/admin-actions.php, name and location are debatable) and makes admin-ajax a very generic (36 lines!) ajax entry point.

Each case statement became a new function and was hooked to the appropriate "wp_ajax_[hookname]" hook. I scanned with "phpmd ... unusedcode" to look for any unused local variables to ensure no globals were missed in the transition.

Should be good for 3.4 (pending other supporters?)

[ryan]

Discussed this a little bit with nacin. I overall like it, but a couple of points came up:

• The change from global to local scope could break things in core and in plugins. There is probably a plugin somewhere that modified global context in order to affect a core ajax action. Not that that should be encouraged.
• Core ajax handlers can now be overridden. That could get ugly.

[ryan]

Whitelist with add_action at priority 1

[ryan]

We were split on whether to use pure actions or a whitelist of core action that would be directly called. This patch is something of a compromise. The core actions are added from a list after admin_init. This means the core actions cannot be removed easily using remove_action(). The core actions are added with priority 1. For a plugin to override they would have to explicitly use priority 0. These couple of hoops might at least make plugin authors a bit wary of overriding core ajax actions.

This approach could easily be converted from using add_action() to directly calling the core actions.

I think removing the add_action() calls from ajax-actions.php and instead using a list in admin-ajax.php is a good thing either way. We have one easy to read canonical list, it is a bit more DRY, and ajax-actions.php becomes a pure include file (something I wish we were more diligent about).

[nacin]

Resounding +1.

---

Notes:

1. It looks like indentation is off in ajax-actions.

2. We would be well-served to do a svn cp admin-ajax.php includes/ajax-actions.php, to keep the history of individual actions, particularly since we can get away with it in terms of indentation being the same.

3. I would normally like the abstraction in the handler, but since it is a nice heads up display, I would vote it become a bit more obvious, something like:

add_action( 'wp_ajax_nopriv_autosave', 'wp_ajax_nopriv_autosave' ), 1 );

$core_actions = array( ... );
foreach ( $core_actions as $action ) {
    add_action( 'wp_ajax_' . $action, 'wp_ajax_' . str_replace( '-', '_', $action ), 1 );
}
unset( $core_actions, $action );

[nacin]

15327.3.diff​ further extends the work of kurtpayne and ryan. I addressed the three notes in comment 14 and went through every callback in ajax-actions.php, first with a script and then by hand.

​The script identified any local variables that were set before use. These problems were addressed by either globalizing a variable or tweaking the code to make the assignment.

[nacin]

15327.4.diff​ also passes $_REQUESTaction? to the do_action() calls. Some core callbacks require this. While they can be re-jiggered to not require it, some plugins may like it as then multiple ajax actions can go through the same callback.

[sivel]

Ok, it just took me about 10 minutes to figure out how to apply the patches from nacin. For anybody else who tries you need to first copy wp-admin/admin-ajax.php to wp-admin/includes/ajax-actions.php, then the patches will apply cleanly.

[sivel]

A little different take on how to separate the GET and HEAD actions

[sivel]

15327.6.diff​ shows a little different take on how to separate the GET and POST actions, by putting them in a single multidimensional associative array where the keys are the request method.

This allows us to consolidate the 2 if statements into 1 for both GET and POST.

This patch, does not require copying wp-admin/admin-ajax.php

And sorry for the description on the attachment, I obviously meant POST and not HEAD. Muscle memory I guess...

[azaozz]

Still not sure why are we doing this:

We should move all of the handlers we have hardcoded in admin-ajax.php off onto action hooks.

Not a good idea (as noted in the comments above). If we do that it would let plugins remove and/or replace default core AJAX handlers which is not desirable.

That way they become much more self contained and easier to review.

Perhaps a little bit easier to review: long list of functions instead of a long switch{}. I don't mind the long switch{} but that's a personal opinion. Don't see how that makes them more self-contained. It seems to only bring scope issues as noted above.

Also the file becomes much more obvious on how a plugin can hook into it as we have loads of good examples :-)

More obvious than the codex page describing it and the many examples in working plugins? Not sure about this reason :)

[dd32]

If we do that it would let plugins remove and/or replace default core AJAX handlers which is not desirable.

I still question why this is even a problem. Plugins can already destroy the experience by removing or altering plenty of other things, Adding "safe guards" to prevent authors from doing something stupid here isn't something we generally do.

We can make it harder by adding the actions within admin-ajax.php without a action between the add actions and do action, with some specific comments of "do not do this.. we warn you" for removing them, but doing much past that feels like we don't trust Plugins and are doing things differently to avoid a few bad apples in the entire farm.

arguably, being able to hook in before/after code ajax handlers run is a -good- thing, and yes, some plugins could break ajax entirely by replacing the handler with something that has a bug - but if they're in the position that they want to do that, they'll already be doing it (either through php, or through overriding the javascript functions that do the ajax request)

[ryan]

My primary motivation for changing this is to ease unit testing.

[nacin]

Committed erroneously in [19738]. Leaving it in. Going to tweak it so it ends up with its own commit message.

[nacin]

In [19739]:

Strip down admin-ajax.php to be a bare ajax handler. Move core ajax handlers to admin/includes/ajax-actions.php. props kurtpayne, ryan. see #15327. see [19738] for initial commit.

[TobiasBg]

In the last line of the new admin-ajax.php we now send a default response of die('-1'); for authenticated GET and POST requests, where before this has been die('0'); (at the locations where the do_action calls were made). (However, it was die('-1'); for nopriv requests already.)

Suddenly changing from die('0'); to die('-1'); might break some plugins (which, for whatever reason, and maybe wrongly, don't set their own die();).

For keeping backwards compatibility,

if ( is_user_logged_in() ) {
	do_action( 'wp_ajax_' . $_REQUEST['action'], $_REQUEST['action'] );        // Authenticated actions
	die( '0' ); // Default status for authenticated actions
} else {
	do_action( 'wp_ajax_nopriv_' . $_REQUEST['action'], $_REQUEST['action'] ); // Non-admin actions
	die( '-1' ); // Default status for non-admin actions
}

might be necessary, although a clean and consistent die('-1'); would indeed be nice...

[nacin]

0 implies failure, -1 implies an authentication issue.

I'd be okay with making 0 the default status for both authenticated and non-authenticated actions.

Nice catch!

[sivel]

Use multidimensional associative array for $core_actions instead of 2 arrays

[sivel]

I kind of like the multidimensional array with both GET and POST in it, using only a single if and single add_action. Performance testing over 100 result sets, shows a slight performance increase. The single array, with single if and single add_action reduces the execution time by 0.0000379703 seconds. Small I know, but it all adds up.

[TobiasBg]

To be honest, I don't really see that benefit of using a multidimensional array here.
Not only is this still making four conditional checks just as before, but it is much harder to read and understand - which opposes the initial intention of this ticket.

[nacin]

Replying to sivel:

I kind of like the multidimensional array with both GET and POST in it, using only a single if and single add_action. Performance testing over 100 result sets, shows a slight performance increase. The single array, with single if and single add_action reduces the execution time by 0.0000379703 seconds. Small I know, but it all adds up.

The issue I found here is that it's actually a change in behavior.

You could send a POST request to admin-ajax.php?action=test, and wp_ajax_test will fire, even though the request method is POST. I could see someone potentially doing this in an ajax request in JS, accidentally, or using wp_remote_post() instead of wp_remote_get().

[nacin]

In [19762]:

Make 0 the default status for admin-ajax.php. props TobiasBg. fixes #15327.

[kurtpayne]

New wp_die handler for ajax requests.

[kurtpayne]

ajax_die.patch​ came out of an ​IRC discussion to make unit tests possible. The die() statements in the ajax handlers were killing the unit tests.

[kurtpayne]

Forgot check_ajax_referer

[kurtpayne]

Including xmlrpc's die handler, too

[ryan]

As discussed in IRC, we can lose function_exists( 'apply_filters' ) in wp_die().

[nacin]

Replying to ryan:

As discussed in IRC, we can lose function_exists( 'apply_filters' ) in wp_die().

It was likely there for the wp_die() in wp_set_db_vars(), but that is no longer relevant as of r15811.

We can lose the function_exists() in the default handler, as well, for did_action.

We cannot lose it for __, as locale.php l10n.php is loaded after sunrise.php, or is_rtl, which is loaded after plugins.

[kurtpayne]

Removing function_exists check

[nacin]

Thinking that _wp_die_xmlrpc_handler() can probably remain as _wp_die_xmlrpc_handler(), and we can call it _wp_die_ajax_handler() too.

Going to run with this for commit.

[nacin]

In [19801]:

Re-purpose wp_die() for ajax responses.

• Allows unit testing of core ajax actions.
• wp_die() now has separate filters to choose a handler depending on the context (ajax, XML-RPC, else).
• wp_die) in ajax context does not need to be called with a string. Conversion takes place before die().

props kurtpayne, see #15327.

[nacin]

In [19802]:

Use wp_die() in ajax-actions. props kurtpayne. see [19801], fixes #15327.

[ryan]

In [19822]:

Setup now that global is not used. see #15327

[azaozz]

In [19831]:

Fix typo in $core_actions_get in admin-ajax.php, see #15327

[jeremyfelt]

#8420 was marked as a duplicate.