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.

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.

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

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.

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:

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.

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.

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

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)

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

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.

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.

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