Make WordPress Core

Opened 9 years ago

Last modified 5 years ago

#31206 new enhancement

Move AJAX action parameters out of the method body and into the declaration.

Reported by: morganestes's profile morganestes Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.2
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:


admin-ajax.php has several methods that require an $action parameter, then immediately set that parameter in the body of the method to the desired string if it's not set, which seems a bit counter-intuitive.

I propose removing the conditional check completely (since it's not checking for a value, just the presence) and move the desired string into the method declaration as a default value.

Attachments (1)

31206.diff (4.0 KB) - added by morganestes 9 years ago.
Initial patch with additional missing doc blocks.

Download all attachments as: .zip

Change History (5)

9 years ago

Initial patch with additional missing doc blocks.

#1 @DrewAPicture
9 years ago

  • Keywords has-patch added

#2 @morganestes
9 years ago

  • Keywords dev-feedback added

So one of the issues with both the original version and is that there's no sanity check for what happens if an action is passed that isn't the one we want.

I'm interested in the original thought behind the structure of these functions. Since each one of these functions expects one specific action to be passed, is there a reason for having it as a parameter in the first place instead of just using the string inside the method?

This ticket was mentioned in Slack in #core by morganestes. View the logs.

9 years ago

#4 @nacin
9 years ago

The issue here is that wp_ajax_* hooks pass by default one argument, which for do_action() is an empty string. See #14881.

I think we could possibly change the add_action() calls in admin-ajax.php to pass zero arguments.

These functions do not actually always have a specific action. See wp_ajax_trash_post() versus wp_ajax_untrash_post(). I am not sure why wp_ajax_delete_post() uses the same style, possibly just leftover cruft.

Hopefully this is enough background?

Note: See TracTickets for help on using tickets.