Short-circuit filters can't return common values

[andy]

#37930 contemplates adding another short-circuit filter, pre_option. This follows a pattern that is already duplicated several times and even somewhat fragmented in its implementations.

The short-circuit filter pattern has this problem: the return value is overloaded. Its meaning is either "nothing changed" or "this is the new value" and there are collisions between these meanings in cases where you might want to short-circuit-return false or null, depending on which short-circuit filter is being used. One proposed improvement is to use only null as the value meaning "nothing changed", as this is less likely than false is to be a value in any short-circuit situation.

The pattern using null can be formalized in a new function that does not overload the return value. This is accomplished by returning multiple values, which we can do in PHP by reference passing. A simple implementation using null and a usage example:

function apply_filters_pre( $filter, &$value ) {
	$value = apply_filters( $filter, null );
	return !is_null( $value );
}

if ( apply_filters_pre( 'get_option_pre', $value ) ) {
	return $value;
}

(Other proposed function names include if_apply_filters and apply_filters_short_circuit.)

This centralizes the pattern and its documentation to a single core function, encapsulates the overloaded parameter as an implementation detail, and simplifies every existing and future call site. The diff would be net red. However, we are still unable to return null from a short-circuit position, which becomes a plausible use case if we enshrine this pattern in a core function. The collision space can be minimized by using a special type used nowhere else:

class WP_Unfiltered_Value {}

function apply_filters_pre( $filter, &$value ) {
	$value = apply_filters( $filter, new WP_Unfiltered_Value );
	return !is_object( $value ) || !is_a( $value, 'WP_Unfiltered_Value' );
}

[DrewAPicture]

I'm not sure I'd call centralizing the documentation a benefit since we're talking about a huge variety of hooks and contexts – they're very deliberately documented (and parsed) individually for this reason.

We can bend the hooks API to a certain point, but at some point it gets pretty close to abusing the original intent.

That said, I wonder if maybe moving completely away from the "apply filters" vernacular altogether would help globally clarify the intent rather than further confuse the space of actions and filters.

e.g.

if ( wp_short_circuit( 'tag_name', $value ) {
    return $value;
}

[flixos90]

I like the approach of a centralized function since it is easier to use than the currently existing checks and provides a standardized way to short-circuit functions that core and plugins could benefit from.

Regarding the implementation we would need to make sure that the function can accept further values properly as the regular one can as well.

Replying to DrewAPicture:

That said, I wonder if maybe moving completely away from the "apply filters" vernacular altogether would help globally clarify the intent rather than further confuse the space of actions and filters.

I'm not sure removing the apply_filters from the name is more clarifying. Also with having apply_filters in there, it becomes more clear that calls of that function should be documented in a similar way as regular apply_filters() calls. See also apply_filters_deprecated() which wraps apply_filters() with custom logic as well.

That core is sometimes using false and at other times null indicates the lack of a standard here. Even if we decide against such a function, I think we should figure out a uniform way for short-circuiting and document that in the handbook going forward.

[flixos90]

After reviewing #37930 (and considering similar situations in Core of which there are quite a few) I figured we should continue working towards a sophisticated solution on short-circuiting processes.
Passing false doesn't help in some cases (for example in Core's pre_option_{$option} filter, as you might actually wanna return false), and using another value such as null doesn't solve that problem, but only diminishes the chance for a collision (as we don't use null often in Core, but still sometimes, and we shouldn't rely on such silent conventions anyway). We need something like the above proposals.

40280.diff​ is a patch with a possible implementation that takes both suggestions by @andy and @DrewAPicture into account.

The patch introduces a simple class WP_Short_Circuit_Result that provides a storage for whether a process should be short-circuited and, optionally, for a value to use for the result of the short-circuit (only needed in case of a filter). To make it easy to use this feature, there are two new functions wp_short_circuit_filter() and wp_short_circuit_action().

Here is an example of how the filter function could be used:

Possible patch for #37930:

/**
 * Allows to short-circuit the process of retrieving the value of an existing option.
 *
 * @since 4.8.0
 *
 * @param mixed  $short_circuit_value Value to be filled with the short-circuit result.
 * @param string $option              Option name.
 * @param mixed  $default             The default value to return if the option does not
 *                                    exist in the database.
 */
if ( wp_short_circuit_filter( 'pre_option', $short_circuit_value, array( $option, $default ) ) ) {
        return $short_circuit_value;
}

Example usage of the above call for short-circuiting:

add_filter( 'wp_short_circuit_filter_pre_option', function( $short_circuit_result, $option, $default ) {
        if ( 'foo' === $option ) {
                $short_circuit_result->short_circuit( true );
                $short_circuit_result->value( 'bar' );
        }

        return $short_circuit_result;
}, 10, 3 );

wp_short_circuit_action() works similarly, except that it does not deal with any result value (so you would simply return; to stop execution of the function in case the wp_short_circuit_action() call returns true).

[flixos90]

Revising my previous patch, I think there's a simpler solution to the problem. The class itself already does the trick - we don't need new functions to short-circuit and can instead use actions and filter hooks as we know. Therefore 40280.2.diff​ is the new WP_Short_Circuit_Result class only, without new functions.

Possible patch for #37930 (now more clear than the one with the previous patch):

$short_circuit_result = new WP_Short_Circuit_Result();

/**
 * Allows to short-circuit the process of retrieving the value of an existing option.
 *
 * @since 4.9.0
 *
 * @param WP_Short_Circuit_Result $short_circuit_result Short-circuit result to adjust in case the process should be short-circuited.
 * @param string                  $option               Option name.
 * @param mixed                   $default              The default value to return if the option does not
 *                                                      exist in the database.
 */
do_action( 'short_circuit_get_option', $short_circuit_result, $option, $default );

if ( $short_circuit_result->short_circuit() ) {
        return $short_circuit_result->value();
}

The above could of course be a filter as well, but I used an action since this makes it unnecessary to return the instance here. Maybe a filter may be better for code clarity, but this needs to be discussed.

[spacedmonkey]

It would really help me to understand the patch better, if there were an example of how it would be implemented in core. Could this new class be used to replace for some (not all because different return types) the existing pre filters in core. An example of how developers would interact with would also be useful.

[flixos90]

Replying to spacedmonkey:

It would really help me to understand the patch better, if there were an example of how it would be implemented in core. Could this new class be used to replace for some (not all because different return types) the existing pre filters in core. An example of how developers would interact with would also be useful.

https://core.trac.wordpress.org/ticket/40280#comment:6 has an example of how this could be used in core to address #37930. :)

Regarding existing hooks, I don't think it's possible to adjust them at all, since the old return types need to remain as is for BC. This ticket aims at providing a consistent foundation for all future development.

[andy]

Thanks for considering this issue. The current patch will treat the problem. It's much more verbose and explicit than what I proposed and that might be an improvement. I just want to take the pitfalls out of short-circuiting and this patch helps.

Whatever the new short-circuit pattern looks like, it should be the only pattern in core. It can be used alongside legacy short-circuit filters and those old filters marked for deprecation to break BC in a specified later version, targeting 18-24 months in the future.

[johnjamesjacoby]

I'm not convinced it will be possible to change existing filters with the hooks API as it is today.

Strict type comparisons really solidified the fact that the null or false values are permanently a part of those pre_ style short-circuits.

I spent the past hour trying to abstract the several existing usages, and boiled it down to this terribleness:

/**
 * Pre apply filters to a specific tag, allowing it to be short-circuited.
 *
 * @since 4.9.0
 *
 * @return mixed
 */
function pre_apply_filters() {
	$args = func_get_args();
	$tag  = "pre_{$args[0]}";

	// Don't pass the tag name
	array_shift( $args );

	// Apply filters & return
	return apply_filters_ref_array( $tag, $args );
}

It basically does nothing, but there is also nothing else left to abstract if:

• the initial variable could be anything
• the number of arguments could be 0 to infinity

---

If we agree to leave existing filters as-is and invent something new, then... how about a constant to solidify the value of the default variable in all of our minds, like WP_PRE_FILTER_VAL or something?

[flixos90]

Replying to johnjamesjacoby:

I'm not convinced it will be possible to change existing filters with the hooks API as it is today.

I agree. Whatever we come up with here will likely be impossible to be applied in any backward-compatible way, so it should become the new canonical way of short-circuiting, while leaving the existing filters untouched.

If we agree to leave existing filters as-is and invent something new, then... how about a constant to solidify the value of the default variable in all of our minds, like WP_PRE_FILTER_VAL or something?

A constant doesn't solve the problem in my opinion. What value would we give to that constant? In any case, it could still be a value that could conflict with something that we'd actually wanna return.

A more verbose solution like that in 40280.2.diff​ ensures that we can return anything we like for the return value of a filter operation, since that value no longer dictates whether to short circuit or not. Although it's different from how it has been done, it's still very easy to use.

https://core.trac.wordpress.org/ticket/40280#comment:6 shows how to use it when running the hook:

• These hooks should always be actions, never filters. Since the WP_Short_Circuit_Result object instance is passed to it, it can simply modified, it doesn't need to be returned. If the short-circuit hook exists to short-circuit a function that requires a return value, just set that value via $short_circuit_result->value( $my_new_value ). The benefit of this is that it is ensured that we work with that one object instance. Developers adding hooks can simply use the passed-in object and do not need to worry about instantiating it on their own or about what to return.
• We should also think about how to consistently name those hooks in the future. While several of those filters currently use pre_ in their name, there are also other hooks with that prefix, that do not short-circuit. I think using a more verbose term, like actually short_circuit_ makes more sense.

[johnjamesjacoby]

Replying to flixos90:

I don't like the WP_Short_Circuit_Result idea, yet. It's too complicated to use for as simple (and relatively useless) as the class itself ends up actually being in helping me actually override a value.

// Override front page
add_action( 'short_circuit_get_option', function( $ss, $option, $value ) {
	if ( $option === 'show_on_front' && $value !== 'page' ) {
		$ss->short_circuit( true );
		$ss->value( 'page' );
	}

	if ( $option === 'page_on_front' && $value !== 131 ) {
		$ss->short_circuit( true );
		$ss->value( 131 );
	}
	var_dump( $ss );
}, 10, 3 );

With your patch applied, this results in many different nondescript WP_Short_Circuit_Result objects being created inside of get_option(), all with different internal memory pointers. Then my action needs to manipulate these objects indirectly, without a filter? It seems backwards, clunky, and less intuitive than what we have now that we don't like.

object(WP_Short_Circuit_Result)[504]
  private 'short_circuit' => boolean false
  private 'value' => null
object(WP_Short_Circuit_Result)[507]
  private 'short_circuit' => boolean false
  private 'value' => null
object(WP_Short_Circuit_Result)[511]
  private 'short_circuit' => boolean false
  private 'value' => null
object(WP_Short_Circuit_Result)[557]
  private 'short_circuit' => boolean true
  private 'value' => string 'page' (length=4)
object(WP_Short_Circuit_Result)[557]
  private 'short_circuit' => boolean true
  private 'value' => int 131

---

My thinking with WP_PRE_FILTER_VAL is to maintain today's ease of use with a predefined, consistently named variable. It could be something like this (or a randomized value):

	// Set pre-filter short-circuit value
	if ( ! defined( 'WP_PRE_FILTER_VAL' ) ) {
		define( 'WP_PRE_FILTER_VAL', NONCE_SALT );
	}

Basically, just a naming convention for humans to agree to agree to agree on.

---

It will be hard to conceptualize anything simpler than the current bespoke approach, and anything more complex hardly addresses the human problem of inconsistency because it introduces several other ways we can be even less consistent. :)

[flixos90]

That's a good point that many of those objects would be instantiated without any benefit.

So I'm not heavily against the simple constant approach, however I think in the original discussion where this ticket came up there were some concerns expressed over a simple value-based solution like we have now. I can't quite remember what it was, maybe someone else does. It was probably nitpicking, since a constant-based solution with a really truly 99.999999% unique value should be solid enough for this. What do other people think?

[spacedmonkey]

I had a go at implementing this my way. Ended up this something pretty similar.
​https://gist.github.com/spacedmonkey/eeeb20e73238d9ef5376db43202f6b41

Part of the problem is the fact it isn't global I think. Filtered values are global for a reason. My implemtention will not work in PHP 5.2 because of lack of anonymous functions.

[johnjamesjacoby]

In bbPress, @jmdodd and I are tossing this idea around:

/**
 * Generate a default intercept value.
 *
 * @since 2.6.0
 *
 * @staticvar mixed $rand Null by default, random string on first call
 * @return string
 */
function bbp_default_intercept() {
	static $rand = null;

	// Generate a new random and unique string
	if ( null === $rand ) {

		// If ext/hash is not present, compat.php's hash_hmac() does not support sha256.
		$algo = function_exists( 'hash' ) ? 'sha256' : 'sha1';

		// Old WP installs may not have AUTH_SALT defined.
		$salt = defined( 'AUTH_SALT' ) && AUTH_SALT ? AUTH_SALT : (string) wp_rand();

		// Create unique ID
		$rand = '{' . hash_hmac( $algo, uniqid( $salt, true ), $salt ) . '}';
	}

	return $rand;
}

/**
 * Allow interception of a method or function call.
 *
 * @since 2.6.0
 *
 * @param string $action Typically the name of the function
 * @param array  $args   Typically the results of func_get_args()
 *
 * @return mixed         Intercept results. Default bbp_default_intercept().
 */
function bbp_do_intercept( $action = '', $args = array() ) {

	// Sanitize the hook same
	$key       = sanitize_key( "pre_{$action}_intercept" );

	// Default return value
	$default   = bbp_default_intercept();

	// Intercept?
	$intercept = apply_filters( $key, $default, extract( $args ) );

	// Bail if intercepted
	if ( $intercept !== $default ) {
		return $intercept;
	}

	// Not intercepted
	return $default;
}

Then the in-function intercept would look something like:

	// Maybe intercept
	$intercept = bbp_do_intercept( __FUNCTION__, func_get_args() );
	if ( bbp_default_intercept() !== $intercept ) {
		return $intercept;
	}

---

WordPress has other considerations, namely backwards compatibility, but in an ideal world this could be relatively simple.