Introduce a safe function for apply_filter

[MathieuLamiot]

Context

• In the continuation of this WP Core issue: https://core.trac.wordpress.org/ticket/51525 - See the comment below with the PR
• @CrochetFeve0251 mentioned having something a bit related in WP Launchpad -> See Dispatcher lib. Could be used for "internal" (but seems to complex for WP Core)
• This is useful for WP Rocket, but also WP Media in general and maybe WP Core. So we need to identify the proper way to implement a first version that could easily be re-used for all those projects. Start only in WP Rocket? Or a library/file easily movable to Launchpad and/or other plugins? That could easily be transmitted to WP Core? The proper way to implement this must be identify.

Expected behavior

• Add a apply_filters_typed function, as per https://core.trac.wordpress.org/ticket/51525, that does apply_filter and throws a _doing_it_wrong in case of type mismatch.
• Add a apply_filters_safetyped function, as per https://core.trac.wordpress.org/ticket/51525, that does apply_filte, throws a _doing_it_wrong in case of type mismatch and skips the $next_value to keep a valid one.
• Add custom types to the is_type method, based on what we usually need
  • positive_integer: type integer and >= 0
  • strictly_positive_integer: type integer and > 0

Targeted implementation

The goal is to have a method we can start using in at least WP Rocket (and if possible in other places) that we can also share as a PoC to WP Core.

[MathieuLamiot]

Here is an up-to-date branch with the patch from Trac, but on GH to facilitate our work. I had to adapt a few things, and also switch a behavior: Now, if a callback returns something that is not of the right type, this value is skipped so the typed function is really safe (otherwise, it was just a warning).

Here is a Code Snippet to run some integration tests, the ugly way.

error_log('Starting tests');

function test_filter_int($the_data) {
   return 3;
}
function test_filter_string($the_data) {
   return '1';
}
function test_filter_array($the_data) {
	return ['toto'];
}

// Should keep previous filtered result when following filter fails
add_filter('filter_it1', 'test_filter_int', 10, 1);
add_filter('filter_it1', 'test_filter_string', 20, 1);

$original_value = 2 ;
$result = apply_filters_typesafe('filter_it1', $original_value);
if($result!=3){
	error_log("Apply Filter safe - Test 1 Failed. Returned: " . $result);
}

// Should keep following filtered result when previous filter fails
add_filter('filter_it2', 'test_filter_string', 10, 1);
add_filter('filter_it2', 'test_filter_int', 20, 1);

$original_value = 1 ;
$result = apply_filters_typesafe('filter_it2', $original_value);
if($result!=3){
	error_log("Apply Filter safe - Test 2 Failed");
}

// Should filter array
add_filter('filter_it3', 'test_filter_array', 20, 1);

$original_value = [] ;
$result = apply_filters_typesafe('filter_it3', $original_value);
if($result!=['toto']){
	error_log("Apply Filter safe - Test 3 Failed");
}

error_log("All tests ran.");

// Should filter out from array.
add_filter('filter_it4', 'test_filter_int', 20, 1);

$original_value = [] ;
$result = apply_filters_typesafe('filter_it4', $original_value);
if($result!=[]){
	error_log("Apply Filter safe - Test 4 Failed");
}

// Should run 'all' correctly.
add_action('all', 'check_it_is_called', 20, 2);

function check_it_is_called($current_hook, ...$args){
	if ('filter_it5' != $current_hook)
		return;
	if($args[0] == 'the_secret')
		error_log("Test 5 succeeded.");
		return;
	error_log("Test 5 failed!");
}

$original_value = 'the_secret' ;
$result = apply_filters_typesafe('filter_it5', $original_value);
error_log("Did Test 5 succeed? Check above!");

error_log("All tests ran.");

It seems to be working well so far. This should be a good place to start to:

1. Contribute to the original Trac issue, sharing this update and implementing tests in the repo properly.
2. I believe this won't be picked up quickly. We should create our own version of it. The logic would remain a bit the same: create our own apply_filters_typesafe that would: call apply_filters, then apply the is_type checks based on the type of the value arg. Note that, in our version, Tests 1, 2 & 5 would not be relevant. We would just need to check that we filter in/filter out correctly on each type we create/use.

[remyperona]

Where is the branch we can use to look into this?

[MathieuLamiot]

Sorry, forgot the link: https://github.com/wp-media/wordpress-develop/tree/enhancement/typed-apply-filters

[MathieuLamiot]

Instructions to follow-up: https://wp-media.slack.com/archives/CUT7FLHF1/p1718998379118069

[CrochetFeve0251]

@MathieuLamiot I added a new PR that implements this on the dispatcher which can be a good way to have this feature for us in a first time while getting closer to Launchpad. (The dispatcher doesn't require the core from launchpad)

[MathieuLamiot]

Thank you @CrochetFeve0251

About the PR itself on dispatcher:

• I see there are dedicated methods defined for apply_filter with int, and float for example. Having to create a dedicated methodfor each type makes this a bit heavy. Are they still needed? Could we have a generic approach like the PR from @Tabrisrp where the desired type (or actually not necessarily a type, could be any string such as 'mixed') could be passed? This would make the implementation and the way to use it much simpler I think.
• what if the output does not pass the sanitizer? The default value is applied?
• I think we are missing a method that takes the desired type as argument. It is a nice possibility in @Tabrisrp 's PR.

Now, about this topic more generally: I think it's important to reconcile both approaches (the WP one, and the Launchpad one) as I would really be in favor of pushing this change to WP Core. We need one implementation as a library that we can use in our plugins and that would be very similar to what we could put in WP Core, so that it serves as a PoC and once in Core, we can easily do the switch.

Why did you prefer to go with re-implementing the library started by @Tabrisrp in the dispatcher way? We need to settle on one implementation that we can use and that could be pushed to Core almost as is. I am afraid this dispatcher would be too abstract and complex, I think, to be pushed to Core ; I am thinking we could get the same level of flexibility just using filters, staying in thebcommon toolbox of WP. WDYT? Did you discuss it already with the team?

Let's settle on a common approach for this, to avoid doing the same thing several time. Let's discuss this next week.

[CrochetFeve0251]

@MathieuLamiot the typed functions(int, string etc) are here as it was the way it was introduced at the base. That way we will have to keep them as they are already introduced and used in Launchpad framework.

Concerning the specified type parameter we can introduce that.

Finally the value returned when we don't match as it is shown in the integration tests is the default value.

I went on the dispatcher way as the implementation doesn't really matter but rather the contract passed by the user. ( most important is to have same behaviour on tests and we can replace it later with the WP core logic when it is added )

Using a sanitizer or rewriting the logic is not something the end used using the filter would see but only the output. That prevents having two ways to work for the same logic in the dispatcher.

To be honest I didn't discussed it with the team as I discovered @Tabrisrp library existence when I got notified yesterday evening and it is also why I sent a link to the PR on the dispatcher which is from 4 days ago and that I did as a draft from what we talked at WC EU.

[MathieuLamiot]

Ok, so we need to settle on one suitable solution for us and possibly WP.

[MathieuLamiot]

Once we have the PR ✅ for our internal lib, how can we submit this as well to WP Core? Based on this comment, I think it'd be much appreciated if we could add the tests we did in our lib to the branch I created on the wordpress-develop repo, to suggest a PR with code + tests. If that needs a bit of work, we'd need a dedicated GH issue (to separate matters). Thanks