WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 10 months ago

#52539 new defect (bug)

callable typehint thows static analysis errors

Reported by: dingo_d Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: docs Cc:

Description

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.

Attachments (1)

52539.diff (52.6 KB) - added by dingo_d 10 months ago.
A first pass of docblock changes

Download all attachments as: .zip

Change History (9)

@dingo_d
10 months ago

A first pass of docblock changes

#1 @dingo_d
10 months ago

  • Keywords has-patch added; needs-patch removed

#2 @johnjamesjacoby
10 months ago

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.

#3 @johnbillion
10 months ago

  • Keywords dev-feedback 2nd-opinion removed
  • Version trunk deleted

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.

#4 @johnbillion
10 months ago

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.

#5 @johnjamesjacoby
10 months ago

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.

#6 @dingo_d
10 months ago

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?

#7 @johnbillion
10 months ago

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.

#8 @johnbillion
10 months ago

Partially related: #42509

Note: See TracTickets for help on using tickets.