WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 19 months ago

#25669 new enhancement

Introduce helper function for AJAX checks

Reported by: toscho Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords:
Focuses: Cc:

Description

Currently, we have to duplicate AJAX checks over and over in our code, so a shorthand function would be nice.

Something like this:

function is_wp_ajax() {
	return defined( 'DOING_AJAX' ) && DOING_AJAX;
}

is_wp_ajax() is easier to type, it would reduce code duplication a little bit, and it would be less error prone, because an IDE would help with auto-completion.

Con: one function more.

Change History (13)

comment:1 @jdgrimes22 months ago

  • Cc jdg@… added

comment:2 @TJNowell22 months ago

  • Cc contact@… added

comment:3 @westonruter22 months ago

  • Cc weston@… added

comment:4 @rmccue22 months ago

Yes please, and ideally filterable too. Constants are not a great way to check this really (I'm sure @bpetty will agree on that point), especially since AJAX calls can be made from other places.

function is_wp_ajax() {
	return apply_filters( 'is_wp_ajax', defined( 'DOING_AJAX' ) && DOING_AJAX );
}

comment:5 follow-ups: @bpetty22 months ago

  • Cc bpetty added

Agreed, and I've been trying to find some time to come up with a similar solution that works well for all of our request state constants. Here's some of our other constants with the exact same problem as DOING_AJAX:

DOING_AUTOSAVE
DOING_CRON
IFRAME_REQUEST
IS_PROFILE_PAGE
WP_ADMIN
WP_BLOG_ADMIN
WP_IMPORTING
WP_INSTALLING
WP_INSTALLING_NETWORK
WP_NETWORK_ADMIN
WP_REPAIRING
WP_SETUP_CONFIG
WP_USER_ADMIN
WP_USE_THEMES
XMLRPC_REQUEST

Also note that WordPress does not use the wp prefix with is_* methods. If we're following convention here, it would just be is_ajax(), or is_ajax_request() if you want to be really clear.

However, do we want to clutter the global namespace with more non-prefixed is_*_request() methods (for all of the constants listed above), or work through a single object like WP_Screen? The latter option might require promoting WP_Screen to wp-includes instead of just admin, or building a somewhat different WP_Request object instead using the same design model.

The reason I mention WP_Screen is because some of these request states are already being managed there. Take a look at is_admin(), which is controlled through WP_Screen. We're already providing this convenient wrapper (like the AJAX one being proposed) for some of these other constants with is_admin(). We're still not deprecating the constants behind that, but that was technically the preferred replacement for the following constants (of the ones I mentioned above):

WP_ADMIN
WP_BLOG_ADMIN
WP_NETWORK_ADMIN
WP_USER_ADMIN

comment:6 in reply to: ↑ 5 @TJNowell22 months ago

Replying to bpetty:

Agreed, and I've been trying to find some time to come up with a similar solution that works well for all of our request state constants. Here's some of our other constants with the exact same problem as DOING_AJAX:

DOING_AUTOSAVE
DOING_CRON
IFRAME_REQUEST
IS_PROFILE_PAGE
WP_ADMIN
WP_BLOG_ADMIN
WP_IMPORTING
WP_INSTALLING
WP_INSTALLING_NETWORK
WP_NETWORK_ADMIN
WP_REPAIRING
WP_SETUP_CONFIG
WP_USER_ADMIN
WP_USE_THEMES
XMLRPC_REQUEST

Agreed, I'd prefer we got the is_ajax_request request committed first so it doesn't succumb to scope creep.

Also note that WordPress does not use the wp prefix with is_* methods. If we're following convention here, it would just be is_ajax(), or is_ajax_request() if you want to be really clear.

Agreed, I'm happy for the suggested rename to is_ajax_request

However, do we want to clutter the global namespace with more non-prefixed is_*_request() methods (for all of the constants listed above), or work through a single object like WP_Screen? The latter option might require promoting WP_Screen to wp-includes instead of just admin, or building a somewhat different WP_Request object instead using the same design model.

The reason I mention WP_Screen is because some of these request states are already being managed there. Take a look at is_admin(), which is controlled through WP_Screen. We're already providing this convenient wrapper (like the AJAX one being proposed) for some of these other constants with is_admin(). We're still not deprecating the constants behind that, but that was technically the preferred replacement for the following constants (of the ones I mentioned above):

WP_ADMIN
WP_BLOG_ADMIN
WP_NETWORK_ADMIN
WP_USER_ADMIN

A good point but perhaps that belongs in a new ticket with the other constants? As mentioned before, scope creep

comment:7 in reply to: ↑ 5 @toscho22 months ago

Replying to bpetty:

Also note that WordPress does not use the wp prefix with is_* methods.

I’m always afraid of too generic names in the global namespace, but … ok. :)

There are also some wp_is_* functions. Our naming scheme is not optimal.

If we're following convention here, it would just be is_ajax(), or is_ajax_request() if you want to be really clear.

is_ajax_request() sounds better.

However, do we want to clutter the global namespace with more non-prefixed is_*_request() methods (for all of the constants listed above), or work through a single object like WP_Screen? The latter option might require promoting WP_Screen to wp-includes instead of just admin, or building a somewhat different WP_Request object instead using the same design model.

WP_Screen might not be the best candidate for that. It’s scope already too wide: it handles states, the help content (might be changed soon), the screen options box and page parent relations. And it is made for the admin pages only.

A WP_State class with static members would be better, but that might need too much time.

Based on @rmccue’s suggestion, I would like to see something like this:

function is_ajax_request() {
	return apply_filters( 'is_ajax_request', defined( 'DOING_AJAX' ) && DOING_AJAX );
}

Which file would be the best place? wp-includes/functions.php is already almost unreadable.
We could move all state functions to a file wp-includes/states.php. Suggestions?

comment:8 follow-up: @nacin22 months ago

It should be noted that the vast majority of those constants are internal use only.

  • IS_PROFILE_PAGE - used on user-edit.php to tell if you are editing the current user (the profile), and really not needed.
    • WP_ADMIN - is_admin()
    • WP_BLOG_ADMIN - is_blog_admin()
    • WP_INSTALLING_NETWORK - used to load translations on wp-admin/network.php
    • WP_NETWORK_ADMIN - is_network_admin()
    • WP_REPAIRING - used as a config-level flag to access the maintenance script
    • WP_SETUP_CONFIG - internal use only
    • WP_USER_ADMIN - is_user_admin()
    • WP_USE_THEMES - should never be referenced outside of core, only maybe defined (but including wp-load.php is better)

The seven remaining ones are a mixture of constants plugins can use and constants only core should use, but would be helpful to modify for testing purposes.

  • DOING_AJAX
  • DOING_AUTOSAVE
  • IFRAME_REQUEST
  • WP_IMPORTING
  • WP_INSTALLING
  • XMLRPC_REQUEST
  • DOING_CRON

Side note, we've discussed using $_ENV as a way to test functions that rely on certain constants (like ABSPATH).

When ryan and I designed WP_Screen, it was indeed meant as a more general bucket than just the administration interface. It just hadn't gotten to that point yet.

is_wp_ajax_request() makes sense. We use wp_is_* sometimes, bpetty is right that we generally omit 'wp_', but we do use wp_is_* sometimes. So why is_wp_ here? Because it's not about prefixing, it's about the specificity of the kind of ajax request. is_ajax_request() would make me think it is checking a X-Requested-With: XMLHttpRequest header, versus specifically being an admin-ajax.php request.

It might also be good to step back for a moment and think about the problem, rather than the solution. Why do people need to check DOING_AJAX? Why does core need to check DOING_AJAX? Why do we need to allow it to be filterable? What are our end goals?

comment:9 @TJNowell22 months ago

The problems:

  • Code relying on DOING_AJAX is harder to test in isolation than code that relies on a function
  • It's easier to filter such a functions output than to redefine it ( after all what would be the point if we swapped redefining a constant with redefining a function? )
  • A lot of codebases and developers define functions exactly like this, thus this function results in a reduction in boilerplate and duplication
  • I can't count the number of times I've assumed something like this was already present and had to either use the boilerplate or implement my own version

As for why one might need to do it, not all code should run on an AJAX request. E.g. Yoasts SEO plugin has no business being loaded in my AJAX call to autocomplete a field name in a frontend form. It makes no sense to add the menus to the Admin toolbar when the response is a json object etc

Last edited 22 months ago by TJNowell (previous) (diff)

comment:10 in reply to: ↑ 8 @bpetty21 months ago

Replying to nacin:

It should be noted that the vast majority of those constants are internal use only.

I agree with @toscho's use case, and you're right that it's mostly irrelevant to internal constants, however, I'd like to ensure that whatever is decided here is compatible with a second goal: to stop abusing constants to store application states that have been tormenting us with unit testing problems. Very few core contributors understand the implications of our hack surrounding @runTestsInSeparateProcesses used without disabling @preserveGlobalState, but with our default prepareTemplate() override that prevents preserving global constants. Ideally we shouldn't even be using @runTestsInSeparateProcesses. It's inefficient, difficult to debug, and we're only doing it because we can't reset these constants for the next test (confirming that they aren't actually constant, and never should have been defined as such).

That specifically applies to internal request state constants, not just ones likely to be used with plugins and themes. Even if it didn't though, telling plugin developers that request states are handled like this, and then using a different approach internally is inconsistent and confusing. On the other hand, so is offering a filterable wp_is_ajax() that could result in a different state than the (still supported) DOING_AJAX constant since it can't be changed, but that's just an argument for deprecating the constant and actually removing it at some point.

Side note, we've discussed using $_ENV as a way to test functions that rely on certain constants (like ABSPATH).

What is the idea proposed here? Have a link to the discussion?

comment:11 @alex-ye21 months ago

  • Cc nashwan.doaqan@… added

comment:12 @griffinjt19 months ago

+1 for this in whatever convention works best.

Ajax requests need to have as little cruft as possible because they are meant to be fast. If all you check for is is_admin(), which is what the majority of plugin/theme developers do, they don't realize that they are loading all their admin junk into ajax requests, including potential checks to remote servers (tracking, updating, etc.). This is likely the reason why admin-ajax.php has so many searches for performance issues - it can cause a performance nightmare that is tough to track down. For now, I use:

is_admin() && ( ! defined( 'DOING_AJAX' ) || ! DOING_AJAX )

otherwise, ajax requests are polluted by everything else I would normally only load in the admin. This is especially troubling when running ajax requests from the front end to admin-ajax.php. It is very confusing.

I usually wrap the code above in a class method called "is_true_admin" to distinguish between admin and ajax states, because while is_admin() is indicative of the current state, it doesn't necessarily reflect the intended use of the state, e.g. for ajax requests. My 2 cents. :-)

comment:13 @nacin19 months ago

  • Component changed from General to Bootstrap/Load
Note: See TracTickets for help on using tickets.