#15327 closed enhancement (fixed)
Strip down admin-ajax.php to be a bare bones ajax handler.
Reported by: | westi | Owned by: | kurtpayne |
---|---|---|---|
Milestone: | 3.4 | Priority: | normal |
Severity: | normal | Version: | 3.1 |
Component: | Administration | Keywords: | westi-likes has-patch dev-feedback |
Focuses: | Cc: |
Description
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 :-)
Attachments (12)
Change History (53)
#2
@
14 years ago
- Milestone changed from Awaiting Review to Future Release
Use Future Release to constrain 3.2-early
#4
@
14 years ago
- Keywords westi-likes added; 3.2-early removed
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
#5
@
14 years ago
- Owner changed from westi to garyc40
- Status changed from new to assigned
- Type changed from defect (bug) to enhancement
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?
#7
@
13 years ago
- Cc kpayne@… added
- Owner changed from garyc40 to kurtpayne
- Status changed from assigned to accepted
- Version changed from 3.1 to 3.3
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.
#11
@
13 years ago
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?)
#12
@
13 years ago
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.
#13
@
13 years ago
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).
#14
@
13 years ago
Resounding +1.
Notes:
- It looks like indentation is off in ajax-actions.
- 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:
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 );
#15
@
13 years ago
- Milestone changed from Future Release to 3.4
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.
#16
@
13 years ago
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.
#17
@
13 years ago
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.
#18
@
13 years ago
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...
#19
@
13 years ago
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 :)
#20
@
13 years ago
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)
#22
@
13 years ago
Committed erroneously in [19738]. Leaving it in. Going to tweak it so it ends up with its own commit message.
#25
@
13 years ago
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...
#27
@
13 years ago
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!
#28
follow-up:
↓ 30
@
13 years ago
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.
#29
@
13 years ago
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.
#30
in reply to:
↑ 28
@
13 years ago
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().
#33
@
13 years ago
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.
#34
follow-up:
↓ 35
@
13 years ago
As discussed in IRC, we can lose function_exists( 'apply_filters' ) in wp_die().
#35
in reply to:
↑ 34
@
13 years ago
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 is loaded after sunrise.php, or is_rtl
, which is loaded after plugins.
+1000.