callable typehint thows static analysis errors

[dingo_d]

While working on client projects I tend to use a lot of automated tools to make my code better. One such tool is PHPStan.

In a project I have this call

<?php
\add_menu_page(
        \esc_html__('Blocks', 'eightshift-libs'),
        \esc_html__('Blocks', 'eightshift-libs'),
        self::REUSABLE_BLOCKS_CAPABILITY,
        'edit.php?post_type=wp_block',
        '',
        'dashicons-editor-table',
        4
);

Lo and behold PHPStan threw an error

  79     Parameter #5 $function of function add_menu_page expects callable(): mixed, '' given.

I was a bit confused. I know that you can pass a callback to that function. But also, you can pass a string that will be the name of the callback. So I went to the documentation and to the trac, and I found the problem:

https://core.trac.wordpress.org/browser/tags/5.6/src/wp-admin/includes/plugin.php#L1325

The documentation is marked as

* @param callable $function   The function to be called to output the content for this page.

But that's not true. Sure, you can provide a callback here. But you can also provide a string. So I got to thinking. Wait, don't add_action and add_filter suffer from the same issue?

Sure they do. I reckon there are a lot of such cases to be found thought the core. In #42509, John Blackbourn mentions around 80 such occurrences in the core I found 78 matches across 23 files.

So what can we do about it?

Simple, just mention that the callable can also be a string denoting the callback that will be added in places where this can be done (like in the examples above). Some of those cannot be replaced with a string, because they have is_callable check.

So in the case of adding menu page we'd have

* @param callable|string $function   The function to be called to output the content for this page or the string denoting the callback that will output the content for this page.

Since in the case of add_menu_page the default is already set as empty string, that seems like it would solve all the static analysis issues of this kind.

[dingo_d]

A first pass of docblock changes

[johnjamesjacoby]

This is a big +1 from me. Nice patch @dingo_d 👍

Docs change pretty frequently recently, so it would be nice to get eyes on this quickly (maybe from @johnbillion or @SergeyBiryukov?) to avoid it going too stale and causing too much rework.

[johnbillion]

A string is a callable. The problem here is not the string, it's that the default value is an empty string, which is not a callable.

IMO rather than changing the docblock to callable|string the default value should be changed to null so it's nullable. Otherwise passing an arbitrary non-callable string becomes possible which is not useful.

[johnbillion]

For clarification when I say "A string is a callable" I actually mean that a valid function name or FQN as a string is a callable. An empty string isn't.

[johnjamesjacoby]

Good thoughts @johnbillion. Now that you mention them, I agree with you, too.

I’m speculating here, but because callables are strings or arrays (in the case of class methods) it may even be unintentionally confusing to include string in the docs at all.

At the risk of scope-creeping this too much (changing the default parameter values to null wasn’t on my mind) do you see this as an opportunity to consider auditing the names of the relevant variables?

$function and $callback are nice and obvious but lack context like many of the other variable names have, like $sanitize_callback for example.

[dingo_d]

Great point on callables. I get that they are strings because they are used in call_user_func, but there is another issue that I noticed, and that is that in only a few places there is a check for is_callable. But this would go above the scope of the current problem (documentation).

So for a second pass, would it just be enough to convert the empty strings to null and remove the string type-hint from the documentation?

I'm a bit vary with variable name changes, as those could affect how WP behaves on PHP8, with named arguments, no?

[johnbillion]

My preference would be to retain the existing callable type in @param and switch any incompatible default values (eg '') to null, but null is also not valid for a callable without it being made nullable via callable|null. However, core is definitely not consistent in that regard currently, ie. there are instances of parameters that are nullable but not documented as such. That is a wider issue.

Renaming the various callable parameters is sorely needed but I think that can be handled in a separate ticket.

[johnbillion]

Partially related: #42509

[bjorsch]

I was about to file a ticket about this problem related to add_menu_page() and add_submenu_page(), but turned up this one in my preliminary search.

We're running into the same problem while using Phan for static analysis, and we'd appreciate if callable parameters that also accept an empty() value to indicate no-callback were to be updated to be callable|null.