Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#20699 closed defect (bug) (fixed)

AJAX Actions now pass the action name as an arg

Reported by: sivel's profile sivel Owned by: ryan's profile ryan
Milestone: 3.4 Priority: high
Severity: critical Version: 3.4
Component: Administration Keywords: 2nd-opinion dev-feedback
Focuses: Cc:

Description

Previous to the admin-ajax.php refactoring in #15327 the do_action calls looked like:

do_action( 'wp_ajax_nopriv_' . $_REQUEST['action'] );
do_action( 'wp_ajax_' . $_GET['action'] );
do_action( 'wp_ajax_' . $_POST['action'] );

They now look like:

do_action( 'wp_ajax_' . $_REQUEST['action'], $_REQUEST['action'] );
do_action( 'wp_ajax_nopriv_' . $_REQUEST['action'], $_REQUEST['action'] );

The change is to add the action name as an argument to do_action to pass to the callback, where before there was none.

As a result,plugin authors were previously able to rely on passing an arg to determine if the function was called by an ajax action or not based on if the $arg was empty. Now the $arg is always populated, as we are sending the action name as the argument, instead of allowing do_action to send it's default empty string.

I had multiple plugins affected by this, where I was determining if the call to the function was from the ajax call, and performing different tasks. I don't have a problem with fixing it in my code, to perhaps do it the right way like it should have been done from the beginning, but I am worried that it could potentially cause issues with other plugins that may be doing this.

Some sample code that started failing:

add_action( 'wp_ajax_nopriv_sivel', 'sivel' );
function sivel( $arg ) {
	if ( $arg )
		// Do something if an arg was passed
	else
		// Do something if an arg was not passed
}

In the above example, only the code under if ( $arg ) is executed since 3.4. Switching to something like if ( $arg === true ) fixes the issue in the plugin.

I also realize that passing the action as an arg gives us context for a single callback called from multiple ajax actions, but that could also be retrieved using $_REQUEST['action'] from the callback anyway.

In case we should remove it, I've attached the diff.

Attachments (3)

20699.diff (691 bytes) - added by sivel 12 years ago.
20699-unit-test-fix.patch (509 bytes) - added by kurtpayne 12 years ago.
Keep the unit test framework in sync with this change
20699-default-action-params.patch (1.8 KB) - added by kurtpayne 12 years ago.

Download all attachments as: .zip

Change History (19)

@sivel
12 years ago

#1 @sivel
12 years ago

  • Summary changed from AJAX Actions now passes the action name as an arg to AJAX Actions now pass the action name as an arg

@kurtpayne
12 years ago

Keep the unit test framework in sync with this change

#2 @TobiasBg
12 years ago

Why don't you just check DOING_AJAX to determine if your callback was called by an AJAX action?

#4 @sivel
12 years ago

Replying to TobiasBg:

Why don't you just check DOING_AJAX to determine if your callback was called by an AJAX action?

That is a good point as well, and in general would work. Although this is not necessarily about my plugin, but the potential for it to break more than just mine. I have a fix in place that works with my more advanced requirement.

In any case, if it stays, it may be worth at least a wpdevel post. In either case, I'm fine with the outcome. My particular issue has been resolved already.

#5 @ryan
12 years ago

Alas, this is not easily grepped in the plugin repo to see how many this might affect. Passing the action sure is handy and I'd like to keep it if the impact is minimal.

#6 follow-up: @nacin
12 years ago

While it's handy, current_filter() also works.

Given the wide use of DOING_AJAX, I doubt many people were relying on this behavior.

At the moment, I'm not sure whether to keep it or ditch it.

#7 @jane
12 years ago

How about a compromise: keep it for now, look into further for 3.5?

#8 in reply to: ↑ 6 @westi
12 years ago

Replying to nacin:

While it's handy, current_filter() also works.

Given the wide use of DOING_AJAX, I doubt many people were relying on this behavior.

At the moment, I'm not sure whether to keep it or ditch it.

Seeing as current_filter() exists and is a better way of finding the information consistently I would say we should go back to the old behaviour and reduce the risk of any plugin bugs.

#9 @ryan
12 years ago

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

In [20857]:

Don't pass action ID to the ajax actions to avoid possible back compat problems. Props sivel. fixes #20699

#10 @kurtpayne
12 years ago

  • Cc kpayne@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Just ran across this after the unit test patch was committed. It looks like some of the ajax functions require a parameter:

function wp_ajax_delete_post( $action )
function wp_ajax_trash_post( $action )
function wp_ajax_untrash_post( $action )
function wp_ajax_add_link_category( $action )
function wp_ajax_get_comments( $action )
function wp_ajax_replyto_comment( $action )
function wp_ajax_add_user( $action )

The $action parameter is passed to check_ajax_referer. The unit test is currently broken.

#11 @nacin
12 years ago

  • Priority changed from normal to high
  • Severity changed from normal to critical

#12 @markjaquith
12 years ago

Let's just grab $action from $_REQUEST['action'] for now. And in 3.5-early, we can circle back, announce this change properly, and drop that global access.

#13 @nacin
12 years ago

Me in IRC with MarkJaquith: What had happened in 3.3 is we had done $action = $_GETaction? (or $_POST) in global scope, then just used $action down the line. Once we encapsulated things into functions, since we decided to pass in $action, it was just convenient. I think sivel was relying on some pretty damn edge behavior that I'd be okay with breaking compatibility on that. But since we didn't advertise it (I didn't realize it, none of us did), I'd be okay with holding off on it until 3.5.

So let's do $action = $_REQUESTaction?; in each of these functions, and try this again in early 3.5, and warn people about it. Unfortunately we didn't recognize it as a backwards incompatible behavioral change at the time.

#14 @kurtpayne
12 years ago

One of these wp_ajax functions with $action as a parameter is called directly (not via hook) in at least one instance:

function wp_ajax_untrash_post( $action ) {
	if ( empty( $action ) )
		$action = 'untrash-post';
	wp_ajax_trash_post( $action );
}

#15 @markjaquith
12 years ago

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

In [20866]:

Set $action for core ajax callbacks, as it will not be provided by the do_action() call until WordPress 3.5. props kurtpayne. fixes #20699

#16 @nacin
12 years ago

In [20888]:

_wp_ajax_add_hierarchical_term() no longer gets passed an action, so pull it from POST. see #20699.

Note: See TracTickets for help on using tickets.