Make WordPress Core

Opened 3 years ago

Last modified 6 months ago

#51525 new feature request

Add new functions apply_filters_single_type() and apply_filters_ref_array_single_type()

Reported by: jrf's profile jrf Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: 2nd-opinion needs-patch php8.x early
Focuses: Cc:

Description (last modified by jrf)

This is a proposal to add two new functions apply_filters_typesafe() apply_filters_single_type() and apply_filters_ref_array_typesafe() apply_filters_ref_array_single_type().

Underlying reasons

PHP 8 is a lot more strict about data types and depending on what is done with the data will start throwing more notices/warnings, but more concerning exceptions, which will result in fatal errors (= white screen of death) when uncaught.

Problem outline

Now, say you have a filter hook foo which passes an integer as its second argument and expects an integer to be returned.

<?php
$int = apply_filters( 'foo', $int );

Also say that there are two functions hooked into this filter, function plugin_a() and function plugin_b().

apply_filters() will pass the original integer to plugin_a() and subsequently will pass the returned value from plugin_a() as the parameter to plugin_b().

Now take a situation where plugin_a() returns something other than an integer, while plugin_b() rightfully expects to receive an integer and doesn't do additional type checking (which it shouldn't have to do) and this results in an error.

Plugin B will be identified in the error backtrace as the place where the error occurred, so plugin B will be blamed and get a bad reputation, while plugin A is actually at fault.

Code example: https://3v4l.org/uqT9G

Proposed solution

To combat this situation I propose to add two new functions apply_filters_single_type() and apply_filters_ref_array_single_type().

These functions would basically have the same underlying logic as the existing apply_filters() and apply_filters_ref_array() functions, with one big difference:

  • At the start of the function, the variable type of the second parameter (the value being filtered) received is checked and remembered.
  • In the loop which calls the hooked in functions, a check is done on the variable type received back from each function and if it doesn't match the expected variable type, the previous value is used for the call to the next hooked-in function and a __doing_it_wrong() error is thrown identifying the function which returned the incorrectly typed variable. This includes doing this check after the last hooked-in function is called, before returning the value to the original caller of apply_filters_single_type().

For apply_filters_ref_array(), this would apply to the first array item in $args.

Practical advantages

  1. Using these functions will prevent potential fatal errors, diminishing the impact of these type of developer errors for end-users.
  2. The "Doing it wrong" notice will make identifying the function/plugin/theme which caused the problem a lot easier.
  3. Plugins/Themes and WP core using the new functions will be able to count on the parameter/return type of the call to these new functions and can write their code based on that, without excessive defensive coding to defend against the errors made by others.
  4. Using these functions will help in the efforts to identify more PHP 8 related incompatibilities, both in WP Core as well as in plugins and themes.

Technical considerations

  1. I considered adding a new $typesafe parameter to the existing apply_filters() function, but as it can be passed a variable number of arguments to be passed along to the hooked-in functions, this is not an option. This doesn't apply to apply_filters_ref_array(), but for consistency I think introducing a new sister-function for both would be the better choice.
  2. I've also considered type juggling an incorrectly type value within a try/catch, using that value to pass on to the next function if the juggling succeeded and only using the previous value if the juggling did not succeed, but I think that could cause even more problems, so I'd rather opt for ignoring the incorrectly typed returned value altogether.

Future scope

Once these functions are in place, a review should be done of all WP Core calls to apply_filters() and apply_filters_ref_array() and where the filtered value is expected to be a singular type, the function call should be switched out for a call to one of the new functions.

The WP version in which this review is done will need a good dev-note about these changes as they could be considered a BC-break, but then again, they are only a BC_break for hooked-in functions already doing it wrong and likely causing problems already.

Other

I'm tentatively milestoning this for WP 5.6, even though I realize this is late in the game, but as this proposal could help with the PHP 8 efforts, I believe it would be good to address this sooner rather than later.

/cc @omarreiss

Attachments (1)

typed-apply-filters.patch (5.9 KB) - added by herregroen 3 years ago.

Download all attachments as: .zip

Change History (26)

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


3 years ago

#2 @TimothyBlynJacobs
3 years ago

This is super necessary, I'd love to see it happen. I had a couple of questions about the implementation.

At the start of the function, the variable type of the second parameter (the value being filtered) received is checked and remembered. In the loop which calls the hooked in functions, a check is done on the variable type received back from each function and if it doesn't match the expected variable type

How is the type check going to work?

Are we going to allow subclasses?

If the requirement is that a class implement an interface, how will that work they might not share a common base class?

What about places where we want union types?

What if a plugin allows for null to disable the object? How will we notate that?

How strict will the type checking be? If I filtered a float, will a plugin be able to provide an integer?

I'm sure we can probably have some reasonable defaults. But I think this API will really need to support accepting a check parameter. This would still be a benefit over checking the return type manually because it means we won't throwaway the entire filter stack, just the plugin that is broken, and of course is simpler DUX wise.

#3 @jrf
3 years ago

@TimothyBlynJacobs Thanks for your response and yes, those are all very good questions.

How is the type check going to work?
Are we going to allow subclasses?
If the requirement is that a class implement an interface, how will that work they might not share a common base class?

My initial line of thinking on this is to just identify the type based on the basic types: boolean, integer, float, string, array, object or resource. Not listing null as in that case with a typesafe filter, there is nothing to filter.

I can imagine that for objects we could add some additional logic to make sure that the returned object is still an instanceof the original object or its parent. Similarly, with resources, we could check that it is still a resource of the same type, but that's as far as I'd go.

Determining full covariance would be taking things too far IMO and could also easily become a big roadblock for this ticket to land.

Along the same lines, I wouldn't want to start checking whether an array still has values of the same type and/or a minimum of the same keys as the original array. That kind of logic has no place in a filter calling mechanism.

What about places where we want union types?
What if a plugin allows for null to disable the object? How will we notate that?

IMO for that the original apply_filters() function should still be used.

From my point of view, the typesafe variants are intended for one type and one type only.

Allowing more than one type would negate some of the advantages of the new functions as type checking logic within the functions calling apply_filters_typesafe() and the functions hooking in, can still not be removed.

Once PHP 8 becomes the minimum PHP version for WordPress (in twenty years or so), union types could be used in the hooked-in functions and a try-catch be added to apply_filters() to catch mismatches, but that's another matter.

Actually, adding the try-catch in apply_filters() already with a doing it wrong would probably not be a bad idea anyway as it would prevent potential fatal errors when plugins already choose to drop PHP 5.6 support and add PHP 7 type declarations, but to be fair, that is outside the scope of this ticket and deserves its own discussion.

How strict will the type checking be? If I filtered a float, will a plugin be able to provide an integer?

That is the one case which I do think the typesafe variant should have tolerance for, though with a float cast when an integer is detected and only when a the actual value would not be affected.
This is similar to how that is handled in PHP itself.

So, when an integer is expected, but a float is returned and $float == (int) $float, the value is accepted and an (int) cast will be applied before passing the value on.
Similarly, if a float is expected, but an integer is returned, the value is accepted as well and a (float) cast is applied before passing the value on.

I'm sure we can probably have some reasonable defaults. But I think this API will really need to support accepting a check parameter.

The only use case I can see for adding that, is if full covariance and union types should be supported, but as I said above, I don't think that's the way to go.

Last edited 3 years ago by jrf (previous) (diff)

#4 @giuseppe.mazzapica
3 years ago

Thanks a lot for this @jrf

A few notes:

  • plugin.php can't have external dependencies, so before doing any _doing_it_wrong it is necessary to check that function is defined.
  • Automatically determine the expected type might not always possible. For example, I can pass a string but what I actually want is a callable. Same with all the other pseudo-types. E.g. an array is passed but any iterable could do.
  • Things become even more complex with objects. If I pass an object to apply_filters_typesafe and then I call a method on the result, I want the object I retrieve to have that method. So checking the result to be an object will not be enough, and checking all parent classes can be wrong either. Let's imagine an object that extends/implements A and B, but on the result of the filter, I call a method of B. If the filter returns an instance of A that is no good.
  • Regarding null there are two cases: the first is that null is the value to be filtered, the second is that null is the value returned by the filters. In the first case, there's basically nothing to do, passing null to apply_filters_typesafe would equal to apply_filters. In the second case, null could be acceptable by the caller.
  • Regarding strictness about int and float, I think the default behavior should be fine with it. Even PHP core does that and there's no need to be stricter than PHP.

To take into consideration all the points above, I think that apply_filters_typesafe should accept a set of args, as in the best WordPress tradition, to fine-tune its behavior.

I can think of accepted_types and nullable options.

That said, because I'm better at PHP than words, I'll paste here what my idea would be.

Note this is meant to only be a discussion point, and I have not tested this at all.

<?php
function apply_filters_typesafe( $tag, $arguments = array(), $value = null, ...$values ) {

    if ( ! has_filter( $tag ) ) {
        return $value;
    }

    static $types_map;
    if (!$types_map) {
        $types_map = [
            'boolean' => 'boolean',
            'integer' => 'numeric',
            'double' => 'numeric',
            'string' => 'string',
            'array' => 'array',
            'resource' => 'resource',
            'resource (closed)' => 'resource',
            'NULL' => 'mixed',
        ];
    }

    $type = gettype( $value );
    $is_object = is_object( $value );
    $accepted_types = isset( $types_map[ $type ] ) ? array( $types_map[ $type ] ) : array();

    // Do not calculate multiple times for same class.
    static $classes_types = [];
    // Skip calculation of accepted types if they are are explicitly passed.
    if ( $is_object && empty ( $arguments['accepted_types'] ) ) {
        $class = get_class( $value );
        if ( isset( $classes_types[ $class ] ) ) {
            $accepted_types = $classes_types[ $class ];
        } else {
            $accepted_types = array( $class );
            $parent = get_parent_class( $class );
            while ( $parent ) {
                $accepted_types[] = $parent;
                $parent = get_parent_class( $parent );
            }

            $accepted_types = array_merge( $accepted_types, class_implements( $class ) );
        }

        $classes_types[ $class ] = $accepted_types;
    }

    $arguments = array_replace(
        array(
            'nullable' => false,
            'accepted_types' => $accepted_types
        ),
        $arguments
    );

    $original = $value;
    // Objects are passed by ref, clone to return original unchanged in case of errors.
    $to_filter = $is_object ? clone $value : $value;

    // `next_filter` function doesn't exist, but you get the point
    $filter = next_filter( $tag );
    $filtered = $filter( $to_filter, ...$values );

    // 'mixed' is a valid PHP 8 pseudo-type so we support for consistency.
    // That said, if mixed is fine then just use apply_filters.
    if ( in_array( 'mixed', (array)$arguments['accepted_types'] ) ) {
        // `has_next_filter` function doesn't exist, but you get the point
        return has_next_filter( $tag )
            ? apply_filters_typesafe( $tag, $arguments, $filtered, ...$values )
            : $filtered;
    }

    static $can_do_it_wrong = false;
    if ( ! $can_do_it_wrong && function_exists( '_doing_it_wrong' ) ) {
        $can_do_it_wrong = true;
    }

    if ( null === $filtered ) {
        if ( !$arguments['nullable'] ) {
            $filtered = $original;

            if ( $can_do_it_wrong ) {
                _doing_it_wrong(
                    __FUNCTION__,
                    "Filters for '$tag' where not expected to return null.",
                    '5.6'
                );
            }
        }

        // `has_next_filter` function doesn't exist, but you get the point
        return has_next_filter( $tag )
            ? apply_filters_typesafe( $tag, $arguments, $filtered, ...$values )
            : $filtered;
    }

    static $functions;
    if ( ! $functions ) {
        $functions = array(
            'int' => 'is_int',
            'integer' => 'is_int',
            'double' => 'is_float',
            'float' => 'is_float',
            'numeric' => 'is_numeric',
            'number' => 'is_numeric',
            'bool' => 'is_bool',
            'boolean' => 'is_boolean',
            'string' => 'is_string',
            'array' => 'is_array',
            'callable' => 'is_callable',
            'function' => 'is_callable',
            'resource' => 'is_resource',
            'iterable' => 'is_iterable',
            'countable' => 'is_countable',
        );
    }

    foreach ( (array)$arguments['accepted_types'] as $type ) {
        if ( isset( $functions[ $type ] ) && call_user_func( $functions[ $type ], $filtered ) ) {
            // `has_next_filter` function doesn't exist, but you get the point
            return has_next_filter( $tag )
                ? apply_filters_typesafe( $tag, $arguments, $filtered, ...$values )
                : $filtered;
        }

        if ( $is_object && is_string ( $type ) && is_a( $filtered, $type ) ) {
            // `has_next_filter` function doesn't exist, but you get the point
            return has_next_filter( $tag )
                ? apply_filters_typesafe( $tag, $arguments, $filtered, ...$values )
                : $filtered;
        }
    }

    if ( $can_do_it_wrong ) {
        $expected = implode( "', '", $arguments['accepted_types'] );
        $actual = is_object( $filtered ) ? 'instance of ' . get_class($filtered) : gettype( $filtered );
        _doing_it_wrong(
            __FUNCTION__,
            "Filters for '$tag' where expected to return a value of one of types: '$expected'. Got '$actual' instead.",
            '5.6'
        );
    }
    
    // `has_next_filter` function doesn't exist, but you get the point
    return has_next_filter( $tag )
        ? apply_filters_typesafe( $tag, $arguments, $original, ...$values )
        : $original;
}

I know it's complex, but there are complex issues to tackle.

Just to leave here a few examples, you could use the above function like this:

<?php
$callable = apply_filters_typesafe('foo', ['accepted_types' => ['callable']], '__return_true');
$callable();

Or:

<?php
$fs = apply_filters_typesafe('foo', ['accepted_types' => ['WP_Filesystem_Base']], $fs);
$fs->find_folder('bar');

Or even:

<?php
/** @var numeric|null $num */
$num = apply_filters_typesafe('foo', ['nullable' => true], 0);
$int = (int)($num ?? 42);
Last edited 3 years ago by giuseppe.mazzapica (previous) (diff)

#5 @giuseppe.mazzapica
3 years ago

Please also note that, of course, in the real implementation I would not expect to call apply_filters or this whole thing would make no sense, so when you see apply_filters in the function above what I mean is "Run the login that runs the next filter".

A proof of concept: https://3v4l.org/pbMST

(edited several times the link because I found bugs.)

Last edited 3 years ago by giuseppe.mazzapica (previous) (diff)

#6 @TimothyBlynJacobs
3 years ago

@jrf I agree Core shouldn't try and actually implement LSP out of the box.

I think instead of a complex args array, we should simplify this to allowing users to pass a callable( $original, $filtered ) : bool. We could maybe create a predefined version that uses the simplest of type mechanics. And create custom ones for things like accepting a callable.

IMO a developer creating a wrong object or callable is not significantly less unlikely than providing the incorrect arg type.

#7 follow-up: @jrf
3 years ago

  • Description modified (diff)
  • Summary changed from Add new functions apply_filters_typesafe() and apply_filters_ref_array_typesafe() to Add new functions apply_filters_single_type() and apply_filters_ref_array_single_type()

Hi @giuseppe.mazzapica, thank you for sharing your thoughts on this. You bring up some good points, though unfortunately I disagree with nearly all.

As the title of the issue seems to leave the impression that these functions should support full type declarations as supported by PHP, I'm going to change the _typesafe in the suggested function names to _single_type.

Let's step through the points you bring up one by one:

Regarding null

Regarding null there are two cases: the first is that null is the value to be filtered, the second is that null is the value returned by the filters. In the first case, there's basically nothing to do, passing null to apply_filters_typesafe would equal to apply_filters. In the second case, null could be acceptable by the caller.

null should never be acceptable. As stated before, passing null as the original value, would mean that running the filters is useless as null would be the only acceptable outcome.
Regarding filter functions returning null, that is also not acceptable as it breaks the principle of this function being typesafe. Filters should always return a value and in the case of filters called via these functions, a value of the same type as originally received, so null would never be an acceptable return.

You are basically suggesting to support "nullable types" in this function. This goes against the principle of one type and one type only and would significantly reduce the advantages of having these functions in the first place.

Regarding callable

I can't think of a single reason for a callable being filterable. To be honest, that just sounds like a security issue waiting to be exploited, which I don't think is a good case to support.

If people still want to do so, let them use apply_filters(), but I'd really wouldn't want to support it in a function with safe in the function name.

Regarding iterable and countable

These I'd need to have a think about, but I'm leaning against a "no", if for no other reason than that it would still require the code in the callback functions hooked into the "typesafe" filter to allow for several different types and several different way to add or remove something from the iterable/countable.

Again, that situation can be handled by using apply_filters().

Regarding number and numeric

First of all, this violates the single type principle again. Second of all, too few people actually understand that is_numeric() will also accept NAN, INF, '1e1' (floats in exponent annotation), ' 1' and more.

Also see: https://phpcheatsheets.com/test/numeric_tests/

Supporting number/numeric would also negate some of the advantages regarding PHP 8, which introduces "saner numeric comparisons" and "saner numeric strings".
In other words, the behaviour of whatever is passed through would become unpredicatable PHP cross-version, which goes against the whole point of these new functions and would make identifying the callbacks doing it wrong again more difficult.

Regarding mixed

No. Just plain no. Use apply_filters(), that's the behaviour you expect there. Just like union types, mixed should not be supported by this function.

Regarding adding an extra argument, let's call it $type_spec.

I tried to avoid adding an extra argument to make switching existing calls from apply_filters() to the new functions as easy as possible, but I can see your point about objects.

If we'd go that way, we would basically need to have a "type" key and an "instanceof" key. If "instanceof" is set, "type" would not need to be set, though setting it to object would be acceptable.

But then, why have two array keys ? We know what the supported basic types are and if whatever is passed (as a string) is not one of the basic types, doing an instanceof could be done automatically.

Also, and again if we'd go the way of adding a "type" argument, then I'd advocate for the original value being passed to apply_filters_single_type() to also be evaluated for validity before any callback functions are called, a doing it wrong being thrown if it is not of the expected type and the function short circuiting (not calling any callbacks) if the wrong type was passed.

This function should never fallback to calling apply_filters() as in that case, it negates the whole point of having this function in place, as callbacks would still not be able to trust the input received from via "typesafe" hooks.

plugin.php can't have external dependencies, so before doing any _doing_it_wrong it is necessary to check that function is defined.

Good point :+1:

// Objects are passed by ref, clone to return original unchanged in case of errors.

Good point, this should definitely be taken into account.

#8 in reply to: ↑ 7 @giuseppe.mazzapica
3 years ago

Thanks @jrf, instead of answering your points one by one, let me try to put in words what I tried to put in code because I think there was some misunderstanding.

To be honest, I'm afraid the direction this ticket is taking lands to nowhere, and that is for the "single type" principle that is being self-imposed.

First: this does not play well with PHP: a variable can be more than one type. An array or a string both can also be callable. And the concept of "single type" is completely not applicable to objects: object is pretty much useless as it guarantees exactly zero safety.

Second: it does not play well with WordPress. If I understand correctly, the idea is to use of these functions you propose in the core. I'm fairly certain that there's no way it is possible to do that in many places if a "single type" is accepted. There are a huge amount of filter hooks in WordPress that depending on several factors can pass null or a string or an integer. The first example into my mind: a post ID that comes from the DB will be a string, if it comes from cache might very well be an integer. Sometimes a $post variable passed by a core filter is a WP_Post, sometimes is an integer and sometimes a string. To make it short: if you want to use these functions in a relevant number of places in WP core, that will just not possible without giving up on the "single type" thing.

Third: it makes the whole thing much less useful and powerful. As a developer who advocates type-safe code for years, I know very well the "single type" approach that PHP itself had for a long time only resulted in much more untyped code. There's no possibility to develop easily with a type system that only allows a single and invariant type and so the alternative to that is use no type at all. What I foresee if the "single type" approach sticks is that not only will not be possible to use it in the core besides very few exceptions, but it will be much less useful for plugin developers.

I'll try to expand on these topics below, and also answer some of the things you posted.

Typed hooks

When I first saw this ticket I thought "too good to be true", because I thought about the possibility to finally have typed hooks in WordPress.

At Inpsyde, we are very strict about types. We have a couple of PHPCS rules that force us to add a type declaration (argument and return) to every function and method. We automatically exclude from those rules functions attached to hooks for the simple reason that hooks are not type-safe.
If we do something like this:

<?php
add_filter('hook', function (string $x): string { /*...*/ });

We can be sure that this will explode because the core itself of some other plugin will pass something that is not a string.

If we analyze the code above, the piece of code that calls apply_filters has (in theory for now) the possibility to define the type of the hook.

In fact, there's a single place where the result of apply_filters is consumed. That means the code that calls apply_filters is the code that decides the type of the hook, the various places where add_filter is called should stick with that type.

Unfortunately, this does not happen right now: apply_filters declares a type, but code that uses add_filter returns whatever, and that is happily accepted by WordPress.

I thought that this could be a chance to stop this.

The way I imagined this was the same mechanism of a function having the so-called "type hints" for parameters. When that happens, whoever calls the function has to pass a type that is compatible with the type the function declared to accept.

Similarly to that, whoever calls apply_filters_typesafe (let me stick with this naming for now) would be able to declare the type of the hook, and then callbacks added to it would be required to return compatible types.

<?php
/** @var string $my_string */
$my_string = apply_filters_typesafe( 'hook', /* ... */ );

Doing so, I would finally be able to safely things like this:

<?php
add_filter('hook', function (string $x): string { /*...*/ });

This would be safe because core would refuse to pass to the next hooking callback something that does not respect the declared hook type. This would be a huge improvement to WordPress, besides the support of PHP 8.

Determining the type

Now that is (I hope) clear what for me is the purpose of an apply_filters_typesafe, that is having typed hooks, we can surely proceed in saying that we could enforce the rule that a typed hook must have a single type.

I'll come back to this later, but for now, let's assume we decide to stick with that.

There would still be two problems:

  • it will not always be possible to infer the type from the given filter "subject"
  • it will not work in a multitude of existing WordPress hooks

The callable issue

You said:

I can't think of a single reason for a callable being filterable.
To be honest, that just sounds like a security issue

The security issue argument is not existent. A callable to be executed must be loaded, that is the file that declares it must be loaded by WordPress. And if a file is loaded, it could do anything, including wiping the entire database, insert new super-admin users, and so on. The fact that it could also filter a callback is not the problem there. If unchecked PHP files can be included in the application that is a huge security issue, but that's a different story and has nothing to do with the fact that a callable is filterable.
WordPress even allows in several places to re-write declared functions (drop-ins, child-themes), there's no security issue there.

Regarding "a single reason for a callable being filterable", I don't have to think. I can show you some examples in the WP core: https://developer.wordpress.org/?s=_handler&post_type%5B%5D=wp-parser-hook

Any of those "handler" hooks accepts a callable. The link above was the one I find more effective to show a list of functions that expect callbacks with a single link, but I think there are other examples in core. Moreover, I can tell you that the "filterable callback" is a pattern that I myself use very often, and because I often review my colleague's code, I can assure you I'm not the only one. Finally, I'm very sure that extremely popular plugins do it here and there.

Now, considering that WordPress passes a string to those functions if WP would auto-determine the type that would be string. Which of course is not valid because an anonymous function or an object method that is correct would be refused and a string that is not callable would be accepted.

Objects

How do we determine the "single type" for objects? You said to stick with the object type.

How this is useful?

When using object-oriented programming the piece of code that declares the type of the hook (that is the piece of code that calls apply_filters_single_type) can decide that anything implementing an interface will be fine. For example:

<?php
/** @var Iterator $my_iterator */
$my_iterator = apply_filters_single_type( /* ... */ );

while( $my_iterator->valid() ) {
  $my_iterator->next();
}

If I pass an ArrayIterator, the code that tries to determine the "single accepted type" will have a few choices:

  • accept with object: this will obviously break the snippet above because the hook type is not respected. Moreover, the problem this issue tries to solve is not solved being the "typed hook" not typed anymore: if I add a filter using a function that declares Iterator as param type a fatal error will happen, and my plugin will be blamed even if it was another plugin to return an object that is not an iterator as desired.
  • accept only instances of ArrayIterator. Could work, but it is extremely limiting. As a dev who has practiced OOP in the everyday job for a few years, the most common reason why you pass an object to a filter is to allow consumers to replace that object with a different implementation of the same interface. In fact, if you want consumers to only interact with the object you use an action, not a filter. (What, for example, WordPress does with pre_get_posts passing a WP_Query instance).
  • accept any of the interfaces/parent classes of ArrayIterator. Again not useful at all. A class having only the Serializable interface would be accepted, but the "hook single type" here is Iterator not Serializable, and the code snippet above would end up in a fatal error.

Other types


For other types, e.g. the auto-casting from int to float and vice-versa, that is something PHP happily does even in PHP 8 (https://3v4l.org/alj77) and I really don't see any reason to be stricter than PHP, but if that lands in core... I would not complain.

The only reason why in my proof-of-concept I accepted any numeric value when an int or a float was given to the filter was that this is how WordPress currently handles those values.

When a post ID is passed around in hooks it might be very well a numeric string or an integer. So I thought that supporting is_numeric instead of is_int would have made the integration in existing WordPress hooks much easier.

After all, if I know that the type is numeric, I can safely do a casting downstream:

<?php
/** @var int $post_id */
$post_id = (int)apply_filters_single_type( 'hook', /* ... */ );

Cast to int will never fail if I have the guarantee that the filter returns a value that passed the is_numeric check.

Regarding functions hooking this filter, in PHP 8 they could even have type-declarations:

<?php
add_filter( 'hook', fn(int|string|float $id) => related_id( $id ) );

That said, once again, if the desired direction is to be very strict about this I will not complain but suspect that this will reduce the possibility of this ticket to be merged.

About null

I'll directly start with an example from core: https://developer.wordpress.org/reference/hooks/posts_pre_query/

That filter passes null because it allows overriding the results of the query before the query runs and null, which is passed as the default value, is a signal that it has not being overridden. The reason is simple: if a filter returns an empty array what it means is that the query is forced to have no results at all. So null allow distinguishing "not filtered" from "no results".

This kind of "pre" filtering is very common in WordPress: https://developer.wordpress.org/?s=pre_&post_type%5B%5D=wp-parser-hook

Sometimes false is used as the discriminant in place of null but the idea is the same. How are we going to handle this very common case sticking with "the single type principle"?

If we think in terms of "typed hook" it makes total sense that WordPress declares a hook to have the type array|null. This is something that for PHP supports since PHP 5.1 (https://3v4l.org/fVVnX).

If we stick with the "single type" approach, there will be no possibility to leverage the new apply_filters_single_type for such cases, which means that WP would stick with the apply_filters: again, when the type system is not flexible the only possibility is having no types. That means that all the filters like posts_pre_query would be "condemned" to be untyped and thus break WP with a fatal error if they receive an unexpected type.

Configurable type

All the examples above try to prove that having sensitive defaults to automatically infer the hook type is very fine, but there should be a way to support the explicit declaration of the hook type.

Let's call that explicit declaration $type_spec, as @jrf proposed, so we don't have yet to define the form it takes.

How to do that? Because defaulting to the type of the filter subject is a sensitive choice it would make totally sense to have this as the last parameter of the functions, making it possible to have a default value.

Unfortunately, apply_filters is a variadic function so if we want to have a signature that resembles that function it is not possible to append a new parameter.

And if we agree that having a different approach for the two functions discussed in this ticket is not desirable, I see two possibilities:

  • Prepend the argument, having something like apply_filters_single_type($tag, $type_spec, $subject, ...$args) and apply_filters_ref_array_single_type($tag, $type_spec, $args)
  • Only have the ref_array variant: apply_filters_ref_array_single_type($tag, $args, $type_spec). Which at that point could be renamed apply_filters_typed($tag, $args, $type_spec). That means that if one wants a typed filter they has to use this function, which is not anymore a variant of the existing apply_filters / apply_filters_ref_array but it is a new function whose happen to accept in its simplest form the same two parameters of apply_filters_ref_array: the hook name and an array of args (using the default for $type_spec). Considering that any occurrence of apply_filters could be replaced with this function, and considering that any code doing add_filter could be left unchanged, I don't see this as an issue, actually, it would reduce the additional API introduced which can only be good.

The examples about null above IMO proved there's the necessity to, at the very least, allow for nullable types to make this typed hook function anywhere useful.

If we assume there's a single type, and so $type_spec can be a string, we could still support nullable types. We don't have to think up a syntax, PHP did that for us:

<?php
$this->posts = apply_filters_typed( 'posts_pre_query', array( null, $this ), 'array|null' );

or an example with a filter currently using apply_filters instead of apply_filters_ref_array:

<?php
// was: $check = apply_filters( 'pre_delete_post', null, $post, $force_delete );
// see: https://github.com/WordPress/WordPress/blob/master/wp-includes/post.php#L3013

$check = apply_filters_typed( 'pre_delete_post', array( null, $post, $force_delete ), 'bool|null' );

This would be IMO a very effective solution:

  • Reduced new API: one functions instead of two
  • Sensitive defaults: the third parameter could default to null, and when that's the case, the accepted type would be determined by WordPress.

Regarding how to calculate the default, e.g. if passing an int strictly accept int only, or int|float or numeric that is an implementation detail and, once again, I would be very fine with any of the choices but I suspect that the more permissive choice is what will make this more easily integrated into the core codebase.

Single type?


The question now is: if we allow nullable types which are a form of union types, why don't accept the full union types specification, considering that WordPress has many places where the filtered types can assume several types and also considering that this ticket happens in the context of support for PHP 8, a version that introduces union types?

In other words, if we have an implementation that allows for:

<?php
apply_filters_typed( 'hook', array( null, $value ), 'bool|null' );

there would be close to zero complications to also making it allow for:

<?php
apply_filters_typed( 'hook', array( null, $value ), 'bool|int' );

but that would provide huge benefits to plugin developers and at the same time will, once again, ease the process of using this kind approach in the core, when there are filters whose type is int|string|\WP_Post|null and moving away from that would require an immense refactoring that will never happen (at least not in the next decade).

Regarding using the string form like bool|int or the array form array('bool', 'int'), or yet something else, is an implementation detail that I think is not really relevant now.

Permit me to say that the "single type principle" is a self-imposed principle that not only overlook the current state of WP code, but also the evolution of PHP that in recent versions has put a lot of effort in moving away from the "single type" approach, introducing pseudo-types like callable (PHP 5.4) and iterable (PHP 7.1), covariant and contravariant type declarations (PHP 7.4), and union types (PHP 8.0).
All these things were done for the only reason that "single type" is just not flexible enough and that developers to overcome this lack of flexibility were forced to have no-type. If that teaches us anything, that is that the only consequence of having a not flexible type system is having a no-type system.

About pseudo-types

I personally see a lot of value in having a "type spec" that allows for things like numeric, that is types that are not valid PHP type declarations but have a core way to be checked (is_numeric), but if the decision is made to don't support them, no strong feelings from me.

Only consider that allowing for numeric will provide the possibility to be much more tolerant with existing code. I'm very sure out there, besides core, exist thousands of plugins that return a numeric string where a post ID is expected. Making "typed hook function(s)" very strict about the type will be such a huge breaking change that I hardly see it even thinkable in WordPress, so either "typed hook function(s)" would be used much less in the core (and by plugins), or they would not see the light at all.

That said, considering that to make "typed hooks" anywhere useful the support for pseudo-type like callable is kind-of mandatory, I don't really see the reason why a pseudo-type like iterable, that is also a valid type declaration in PHP, should not be allowed.

Last edited 3 years ago by giuseppe.mazzapica (previous) (diff)

#9 @overclokk
3 years ago

I would like to add some sugar to this ticket because I'd like to have more "typed code".

I thought at two alternative:

1 - Have as many functions as many type we want to support, one function per type:

<?php
\apply_filters_type_string( 'hook', 'string', ...$args );
\apply_filters_type_int( 'hook', 1, ...$args );
\apply_filters_type_array( 'hook', ['val1', 'val2'], ...$args );
// ... and so on

Why? Because I think of having more declarative function reduce the amount of logic inside the function itself, separation of concern, and I don't see any issue with having more API functions in core if they can help to write better code, and we don't have to write functions for all type, only for some and let the developers to write its own if they need them.
With this we could simply switch from \apply_filters() to \apply_filters_type_*() simpler.

2 - A system more similar to PSR-14 and only for one type, object:

<?php
function do_action_psr( 'hook', object $obj ) {
    // Do some stuff
}

//...

$obj = new My_Object( /* With arguments */ );

\do_action_psr( 'hook', $obj );

Why? Because with this I could use the Value Object pattern and if I want to do stuff more strictly I can write a decorator function like this:

<?php
function do_action_my_interface( 'hook', My_Interface $obj ) {
    // Do some stuff
    \do_action_psr( 'hook', object $obj );
}

//...

$obj = new My_Object( /* With arguments */ ) // extends My_Interface;

\do_action_my_interface( 'hook', object $obj );

With objects I don't even need to use apply_filters() because they are passed by reference.

Beetween the two I prefer the second even if I can see that there's one issue with this, if we want more types we have to write more decorators.

Obviously these are only my two cents.

#10 @johnbillion
3 years ago

  • Version trunk deleted

#11 @noisysocks
3 years ago

  • Milestone changed from 5.6 to 5.7

Bumping this from 5.6 as we're past the cut-off for enhancements.

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


3 years ago

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


3 years ago

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


3 years ago

This ticket was mentioned in Slack in #core-php by timothybjacobs. View the logs.


3 years ago

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


3 years ago

#17 @hellofromTonya
3 years ago

Summarizing discussions during 5.7 Core Scrub:

From @ jrf:

To be fair, the responses so far were IMO too ambitious. We need to learn to crawl before we can walk. Take it one step at a time, start with single type and have a potential roadmap for the future for making it more complex, but we do need to start with this as it will save a lot of plugins a world of pain.

I agree: baby steps. Start small and take one step forward. This approach gets us started while giving us the ability to learn and iterate.

What is the next step?

From @ jrf:

I think we need to have some more people vocalize that one step at a time is a good idea and get some visible consensus on that before creating a patch.

⭐️Next Step Proposal ⭐️

Baby step approach adding one type at a time. Start with one single type.

  1. Start with single type filtering.
  2. Add a lot of unit tests for this single type.
  3. Then create/update roadmap for the future steps to add more types.
  4. Repeat.

📣Seeking your feedback 📣

Do you agree with the one step at a time approach?

#18 @hellofromTonya
3 years ago

  • Keywords early added
  • Milestone changed from 5.7 to Future Release

It's too late in the 5.7 alpha cycle for this ticket as we are still working to gain consensus on the proposed baby step approach. Raising the ticket in Dev Chat. Let's see if we can get consensus and then pull it into 5.8 early.

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


3 years ago

#20 @audrasjb
3 years ago

The approach proposed by @hellofromTonya makes sense to me.

  1. Start with single type filtering.
  2. Add a lot of unit tests for this single type.
  3. Then create/update roadmap for the future steps to add more types.
  4. Repeat.

#21 @herregroen
3 years ago

I would suggest two functions

apply_filters_typesafe( $tag, $value, ...$args ) which uses gettype to retrieve the type of the $value and the uses the appropriate is_ function to validate that type.

apply_filters_typed( $type, $tag, $value, ...$args ) in which $type is either a primitive or a class name. Primitives are validated using the appropriate is_ function and class names are validated using is_a || is_subclass_of. In the future we could allow for arrays as $type to allow multiple return types but that can be added later or even more complex objects to, for example, describe the shape of arrays. But the first implementation should remain simple and focus on just a string $type argument.

The mixed type should also be supported for the simple reason that this will allow apply_filters, apply_filters_typesafe and apply_filters_typed to share one single implementation.

apply_filters( ... ) is simply apply_filters_typed( 'mixed', ... ).
apply_filters_typesafe( ... ) is simply apply_filters_typed( gettype( $value ), ... ).

This means we have just one function to maintain and test as opposed to multiple.

Type-checking would be the following function:

<?php
/**
 * Checks whether the given variable is a certain type.
 *
 * Returns whether `$value` is certain type.
 *
 * @since 5.8.0
 *
 * @param string $type  The type to check.
 * @param mixed  $value The variable to check.
 * @return bool Whether the variable is of the type.
 */
function is_type( $type, $value ) {
        switch( $type ) {
                case 'boolean':
                        return is_bool( $value );
                case 'integer':
                        return is_int( $value );
                case 'double':
                        return is_float( $value );
                case 'string':
                        return is_string( $value );
                case 'array':
                        return is_array( $value );
                case 'object':
                        return is_object( $value );
                case 'resource':
                case 'resource (closed)':
                        return is_resource( $value );
                case 'NULL':
                        return is_null( $value );
                case 'unknown_type':
                        return false;
                case 'mixed':
                        return true;
                default:
                        return is_a( $value, $type ) || is_subclass_of( $value, $type );
        }
}

The main apply_filters loop in WP_Hook would have to be as follows

<?php
// Avoid the array_slice() if possible.
if ( 0 == $the_['accepted_args'] ) {
        $next_value = call_user_func( $the_['function'] );
} elseif ( $the_['accepted_args'] >= $num_args ) {
        $next_value = call_user_func_array( $the_['function'], $args );
} else {
        $next_value = call_user_func_array( $the_['function'], array_slice( $args, 0, (int) $the_['accepted_args'] ) );
}
if ( ! is_type( $type, $next_value ) ) {
        _doing_it_wrong(
                $the_['function'],
                sprintf(
                        __( 'Invalid type returned in filter. Expected %1$s but received %2$s' ),
                        $type,
                        gettype( $next_value )
                ),
                '5.8'
        );
} else {
        $value = $next_value;
}
Last edited 3 years ago by herregroen (previous) (diff)

#22 @herregroen
3 years ago

I've added a very quick and dirty patch of what I propose for reference. Code itself likely still needs work but should help to illustrate what I intend.

This ticket was mentioned in Slack in #core-coding-standards by jrf. View the logs.


2 years ago

#24 @hellofromTonya
12 months ago

  • Keywords php8.x added; php8 removed

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


6 months ago

Note: See TracTickets for help on using tickets.