[wp-trac] [WordPress Trac] #51894: PHP 8: Invalid functions added to hooks now cause fatals

[WordPress Trac]

#51894: PHP 8: Invalid functions added to hooks now cause fatals
-------------------------+---------------------
 Reporter:  dlh          |       Owner:  (none)
     Type:  enhancement  |      Status:  new
 Priority:  normal       |   Milestone:  5.6
Component:  Plugins      |     Version:
 Severity:  normal       |  Resolution:
 Keywords:  php8         |     Focuses:
-------------------------+---------------------

Comment (by jrf):

 > call_user_func() and call_user_func_array() are among the functions in
 PHP 8 that throw a TypeError when passed a parameter of an invalid type,
 which means that do_action() and apply_filters() are capable of generating
 fatal errors when the hooked function no longer exists, has a typo, etc.

 In previous PHP versions this would throw a warning and return `null` and
 yes, in PHP 8 this has become a `TypeError`.

 Warnings and errors like that ''**have a function**'' and should not be
 avoided. They should be solved instead (by the developer who is ''doing it
 wrong'').

 > Unlike invoking a WordPress function that uses its parameters
 immediately, passing an invalid callable to add_action() or add_filter()
 is itself still safe. The fatal error occurs whenever the hook fires, if
 it does at all during the particular request.

 In contrast to what #51525 addresses, the fatal in this case would still
 points to the right culprit - the function being called which doesn't
 exist -, though in some cases it may to hard to figure out which
 plugin/theme/Core did the hook-in for that function.

 > Is it thinkable to create a helper plugin that will emit warnings, but
 let execution go on, to create a total list? And t help site owners and
 developers identify the offending plugins, and plugin developer identify
 all TypeError breaches? For PHP 7 and 8.

 A generic plugin like that would not be that helpful as in a lot of cases
 (probably most), this would point to the ''wrong plugin/theme/Core'' - see
 #51525 for an extensive explanation.


 > If they're throwing a TypeError, could we not wrap the calls within
 do_action and apply_filters in a try-catch segment, that way if it fails
 it could discard that attempt and also log a __doing_it_wrong?

 From https://core.trac.wordpress.org/ticket/51525#comment:3:

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

 So while a `try-catch` in `apply_filters()` would address ''this''
 particular issue, it would also catch `TypeError`s caused by one hook
 function returning the wrong type (or not returning) and another hook
 function then using that value and running into a `TypeError`, which is a
 far more common reason for the `TypeError`s. In that case, the `try-catch`
 would blame the wrong plugin/theme/Core.

 All in all, I think the proposal in #38116 is probably the best one for
 this particular issue:
 * Doing a `is_callable()` before calling the hooked-in function.
 * If `false`, throw a ''doing it wrong'' and move on to the next function.

 I'd advocate for the ''doing it wrong'' notice to be elevated from an
 `E_USER_NOTICE` to an `E_USER_WARNING` in that case though, as an
 `E_USER_NOTICE` is too often silenced, even by developers, or more
 particularly: especially by beginner/inexperienced developers who are more
 often than not the reason for these type of issues occurring.

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