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

[WordPress Trac]

#51525: Add new functions apply_filters_typesafe() and
apply_filters_ref_array_typesafe()
-----------------------------+------------------------------------------
 Reporter:  jrf              |      Owner:  (none)
     Type:  feature request  |     Status:  new
 Priority:  normal           |  Milestone:  5.6
Component:  General          |    Version:  trunk
 Severity:  normal           |   Keywords:  2nd-opinion needs-patch php8
  Focuses:                   |
-----------------------------+------------------------------------------
 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

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