[wp-trac] [WordPress Trac] #51525: Add new functions apply_filters_single_type() and apply_filters_ref_array_single_type() (was: Add new functions apply_filters_typesafe() and apply_filters_ref_array_typesafe())

Mon Oct 19 17:13:46 UTC 2020

#51525: Add new functions apply_filters_single_type() and
apply_filters_ref_array_single_type()
------------------------------------------+---------------------
 Reporter:  jrf                           |       Owner:  (none)
     Type:  feature request               |      Status:  new
 Priority:  normal                        |   Milestone:  5.6
Component:  General                       |     Version:  trunk
 Severity:  normal                        |  Resolution:
 Keywords:  2nd-opinion needs-patch php8  |     Focuses:
------------------------------------------+---------------------

Old description:

> This is a proposal to add two new functions `apply_filters_typesafe()`
> and `apply_filters_ref_array_typesafe()`.
>
> == 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
> <?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_typesafe()` and `apply_filters_ref_array_typesafe()`.
>
> 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_typesafe()`.
>
> 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

New description:

 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
 <?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

--

Comment (by jrf):

 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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/51525#comment:7>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform