Introduce helper function for AJAX checks

[toscho]

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.

[rmccue]

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 );
}

[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

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

[TJNowell]

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

[toscho]

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?

[nacin]

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?

[TJNowell]

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

[bpetty]

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?

[griffinjt]

+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. :-)

[Mte90]

rmccue suggestion

[Mte90]

I've done a simple patch, contain the docs (to improve) and the rmccue suggestion for that.

[sebastian.pisula]

I think that this function with filter - this is good idea. But I suggest add to core new constant:

<?php
define( 'WP_IS_AJAX', ( ! empty( $_SERVER['HTTP_X_REQUESTED_WITH'] ) && strtolower( $_SERVER['HTTP_X_REQUESTED_WITH'] ) == 'xmlhttprequest' ) );

Patch 25669.2.patch

[Mte90]

From slack log is proposed to rename that function as wp_is_ajax(), improve the docs, add unit tests and few examples.
Someone can look on that steps?

[Mte90]

renamed the function and filter as wp_is_ajax

[swissspidy]

25669.3.patch​ looks good so far, needs some docs improvements (@since, etc.).

Also, if this gets introduced, it would make sense to replace all DOING_AJAX checks in core with this function. Otherwise it's pretty pointless.

[sebastian.pisula]

4.7-alpha-38274

[sebastian.pisula]

4.7-alpha-38274

[Mte90]

Amazing so we need to update 25669.3.2.patch with the docs to get ready for the integration.
What type of docs are missing?

[swissspidy]

Replying to Mte90:

From slack log is proposed to rename that function as wp_is_ajax(), improve the docs, add unit tests and few examples.
Someone can look on that steps?

Do you have the link to the Slack conversation by chance? Because earlier the consensus seemed to be is_wp_ajax_request() or is_wp_ajax().

Replying to Mte90:

25669.4.patch​ improves the docs already quite a bit (thanks @sebastian.pisula!). However, the filter needs to be documented and a description added to the @return.

There's a detailed page about the WordPress PHP documentation standards: ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/

[sebastian.pisula]

I think that this function should be without filter.

[Mte90]

Thanks @swisspidy, for the name: ​https://wordpress.slack.com/archives/core/p1452878327010600 and few row after there is the proposal.

I think that the filter it's important to help when you have strange issues or you want to force something else as the proposals in the top of that ticket.

[swissspidy]

I also think adding a filter makes sense. Let's continue working on 25669.4.patch​. If someone thinks a filter isn't necessary (which would make this function rather useless), we could always remove it in the end.

[helen]

Apologies for the naming bikeshed - I'd suggest wp_doing_ajax() for consistency with the constant and because IMO it reads better.

[swissspidy]

Thanks for chiming in. 25669.diff​ renames the function and adds proper inline docs for the function and the filter.

Adding to the 4.7 milestone for consideration.

[DrewAPicture]

I like 25669.diff​ and agree with making it filterable. I can't really think of a real-world use case for the filter, however it would definitely be useful in the context of testing.

[wonderboymusic]

In 38334:

AJAX: add a new function, wp_doing_ajax(), which can replace... (wait for it...) DOING_AJAX checks via the constant.

Props Mte90, sebastian.pisula, swissspidy.
Fixes #25669.

[jdgrimes]

Was any decision made regarding the other constants:

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

wp_installing() was introduced recently, but the others might be useful. Are we intentionally waiting to see how wp_doing_ajax() works out, or should somebody open up new tickets for any of these others?

[swissspidy]

@jdgrimes Good question. We didn't introduce wp_doing_ajax() to intentionally wait for how it works out. Even if we did, new tickets should be created for each global nonetheless.

IMHO a ticket should be created for replacing each global. For example, a helper function for XMLRPC_REQUEST or REST_REQUEST might make more sense than IFRAME_REQUEST because those are used more often and I think when we do things right, IFRAME_REQUEST will be a gone rather soon.

[SergeyBiryukov]

In 38607:

Docs: Use a third-person singular verb for wp_doing_ajax filter added in [38334].

See #25669.