PHP 8: Invalid functions added to hooks now cause fatals

[dlh]

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.

A lot of amazing work has gone into ensuring that WordPress calls internal PHP functions with correct types (e.g. #51423). The PHP 8 dev note also helpfully reviewed the new strict-typing behavior.

But I might argue that the risk to apply_filters() represents a special "gotcha" worth highlighting for developers (and I hope that I haven't missed that it was) because of its delayed occurrence and relative unpredictability:

• 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.

• Also unlike invoking most WordPress functions, it's possible to hook an invalid callable that later becomes valid, e.g. if the function is loaded later in the bootstrap process.

On a more basic level, the change in behavior might be worth just acknowledging as arguably a backwards compatibility break in core: Previously, invalid callables added to hooks didn't cause fatals, and now they do.

#38116 raised the issue of invalid callables generating warnings and proposed an is_callable() check. That certainly would bypass the fatal now, but as yet there is no counterargument in that ticket to the concerns about performance and general prudence.

Another thought that would be specific to PHP 8 would be to catch TypeErrors thrown by the call_user_func* block and trigger a warning but let execution continue. That might address the performance concerns but not the prudential ones.

So, the best course of action for core might be to do nothing, but even so, perhaps this ticket can at least note that consensus.

[afragen]

Related #51525

[knutsp]

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.

I mean, possibly by some temporary hooks in 5.6.1, later removed, just for this helper plugin?

It seems the helper plugin for jQuery migration was useful.

[SergeyBiryukov]

Thanks for the ticket! Moving to the milestone for visibility.

[Clorith]

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?

[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 TypeErrors 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 TypeErrors. 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.

[SergeyBiryukov]

Related: #49728

Trac ticket: https://core.trac.wordpress.org/ticket/51894

Adds _doing_it_wrong() when the callback is not callable to prevent PHP 8 TypeErrors fatal error when passing the callback to call_user_func() or call_user_func_array().

TODO:

• [x] Add to WP_Hook::apply_filters() (also for WP_Hook::do_action())
• [ ] Add to WP_Hook::do_all_hook()

[hellofromTonya]

First pass implementation is ready for review and discussion in ​PR 777.

• Applies _doing_it_wrong() to WP_Hook::apply_filters (which also applies it to WP_Hook::do_action.
• Adds test coverage for the changes.

Once we get agreement on it, then the same treatment can be applied to WP_Hook::do_all_hook() and elevate the error level to E_USER_WARNING.

[ayeshrajans]

I added two code review comments in GitHub PR, but I don't think they get sync'd to Trac yet. They are about dealing with associative arrays and empty arrays.

I think E_USER_WARNING is the appropriate one as @jrf said, if not for throwing an exception right away.

[TimothyBlynJacobs]

Has this been benched?

Separately, I don't really understand the motivation for this change. This is a clear programming error that isn't likely to vary at runtime. The function is either defined, or isn't defined. Unlike the typesafe hook issues, it would be highly unlikely for a function to no longer be defined because of persisted data or other plugins being active. As such, we'd be adding a performance cost to the hottest code path in WordPress to catch a quite unlikely event.

Additionally, this is something that Recovery Mode should be able to let the site owner recover from.

On a more basic level, the change in behavior might be worth just acknowledging as arguably a backwards compatibility break in core: Previously, invalid callables added to hooks didn't cause fatals, and now they do.

In my personal opinion, I think we should go down this route. Though I think it is worth being clear that this isn't really a break in WordPress Core but in PHP 8. This behavior was always invalid, PHP 8 is just making it more severe.

[knutsp]

Must agree with @TimothyBlynJacobs . This ticket seems to want to combat PHP8 strictness, rather that ensure compatibility. Like an "undoing".

Making this a warning should only be temporary, by a constant or a plugin, to help migration/upgrading. Core, with no modifications, should just accept it as is, a fatal error caused by a developer.

Make the change to PHP8 as soft as possible, offer help for some time or under conditions, but not "undo" permanently.

[DavidAnderson]

I agree with knutsp and TimothyBlynJacobs. The common mindset in the PHP world that fatal errors must not be allowed to occur at almost any cost, is harmful. PHP 8 is doing more to combat this anti-pattern. If there's code that intends to call a function/method, but that function/method is not available, then part of the program is missing. Safe or consistent execution can no longer be guaranteed. An exception should be raised rather than trying to make the best of it. This may cause a few more white screens in the short term as bad code is unearthed, but will improve the quality of the program and therefore experience in the long term.

c.f. This post-mortem of a critical remote code execution vulnerability fixed in WP core 4.7.2, which would not have occurred if an abort had been allowed once safe/consistent execution was no longer possible (see particularly under 9.) - ​https://lukeplant.me.uk/blog/posts/wordpress-4.7.2-post-mortem/

[hellofromTonya]

Punting this ticket to 5.7 as today is 5.6 RC2.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[hellofromTonya]

With 5.7 Beta 1 landing in less than 2 weeks, no activity on this ticket, and no consensus yet, punting this ticket to Future Release.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[hellofromTonya]

After talking with @jrf, moving to close this ticket as wontfix. Why?

From Juliette:

Hey Tonya, to be honest, I think that ticket should probably be a "won't fix".
If we'd decide to add some wrapper code, the above would be the way to do it, but to be fair, the fatal error is there for a reason and I don't necessarily think it's a good idea to "hide" it.

The discussion feedback supports this same conclusion.

Closing as the Trac ticket has been marked as wontfix.

[jrf]

#52784 was marked as a duplicate.