WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 3 months ago

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

15327.diff (111.9 KB) - added by kurtpayne 3 years ago.
15327.2.patch (106.3 KB) - added by kurtpayne 2 years ago.
Refreshed for 3.4
15327.2.diff (103.0 KB) - added by ryan 2 years ago.
Whitelist with add_action at priority 1
15327.3.diff (68.4 KB) - added by nacin 2 years ago.
15327.4.diff (68.5 KB) - added by nacin 2 years ago.
15327.5.diff (69.3 KB) - added by nacin 2 years ago.
15327.6.diff (102.5 KB) - added by sivel 2 years ago.
A little different take on how to separate the GET and HEAD actions
15327.7.diff (2.8 KB) - added by sivel 2 years ago.
Use multidimensional associative array for $core_actions instead of 2 arrays
ajax_die.patch (28.2 KB) - added by kurtpayne 2 years ago.
New wp_die handler for ajax requests.
ajax_die.2.patch (28.6 KB) - added by kurtpayne 2 years ago.
Forgot check_ajax_referer
ajax_die.3.patch (29.9 KB) - added by kurtpayne 2 years ago.
Including xmlrpc's die handler, too
ajax_die.4.patch (30.1 KB) - added by kurtpayne 2 years ago.
Removing function_exists check

Download all attachments as: .zip

Change History (53)

comment:1 nacin3 years ago

  • Milestone changed from Awaiting Triage to Awaiting Review

+1000.

comment:2 westi3 years ago

  • Milestone changed from Awaiting Review to Future Release

Use Future Release to constrain 3.2-early

comment:3 Backie3 years ago

  • Cc Backie added

comment:4 westi3 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

comment:5 garyc403 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?

comment:6 SergeyBiryukov3 years ago

  • Keywords needs-patch added

kurtpayne3 years ago

comment:7 kurtpayne3 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.

comment:8 SergeyBiryukov3 years ago

  • Keywords has-patch added; needs-patch removed
  • Version changed from 3.3 to 3.1

comment:9 SergeyBiryukov3 years ago

  • Keywords dev-feedback added

comment:10 Chouby2 years ago

  • Cc Chouby added

kurtpayne2 years ago

Refreshed for 3.4

comment:11 kurtpayne2 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?)

comment:12 ryan2 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.

ryan2 years ago

Whitelist with add_action at priority 1

comment:13 ryan2 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).

Last edited 2 years ago by ryan (previous) (diff)

comment:14 nacin2 years ago

Resounding +1.


Notes:

  1. It looks like indentation is off in ajax-actions.
  1. 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.
  1. 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 );

nacin2 years ago

comment:15 nacin2 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.

nacin2 years ago

comment:16 nacin2 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.

nacin2 years ago

comment:17 sivel2 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.

sivel2 years ago

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

comment:18 sivel2 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.

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

Version 0, edited 2 years ago by sivel (next)

comment:19 azaozz2 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 :)

comment:20 dd322 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)

comment:21 ryan2 years ago

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

comment:22 nacin2 years ago

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

comment:23 nacin2 years ago

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.

comment:24 scribu2 years ago

  • Keywords dev-feedback removed

comment:25 TobiasBg2 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...

comment:26 TobiasBg2 years ago

  • Keywords dev-feedback added

comment:27 nacin2 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!

sivel2 years ago

Use multidimensional associative array for $core_actions instead of 2 arrays

comment:28 follow-up: sivel2 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.

comment:29 TobiasBg2 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.

comment:30 in reply to: ↑ 28 nacin2 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().

comment:31 WraithKenny2 years ago

  • Cc Ken@… added

comment:32 nacin2 years ago

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

In [19762]:

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

kurtpayne2 years ago

New wp_die handler for ajax requests.

comment:33 kurtpayne2 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.

kurtpayne2 years ago

Forgot check_ajax_referer

kurtpayne2 years ago

Including xmlrpc's die handler, too

comment:34 follow-up: ryan2 years ago

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

comment:35 in reply to: ↑ 34 nacin2 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 l10n.php is loaded after sunrise.php, or is_rtl, which is loaded after plugins.

Last edited 2 years ago by nacin (previous) (diff)

kurtpayne2 years ago

Removing function_exists check

comment:36 nacin2 years ago

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.

comment:37 nacin2 years ago

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.

comment:38 nacin2 years ago

In [19802]:

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

comment:39 ryan2 years ago

In [19822]:

Setup now that global is not used. see #15327

comment:40 azaozz2 years ago

In [19831]:

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

comment:41 jeremyfelt3 months ago

#8420 was marked as a duplicate.

Note: See TracTickets for help on using tickets.