GiacoCorsiglia / wordpress-stubs

WordPress function, class, and global variable declaration stubs for easier static analysis.
GNU General Public License v2.0
35 stars 10 forks source link

TooManyArguments raised for apply_filters, do_action and others #6

Open Flimm opened 5 years ago

Flimm commented 5 years ago

Take this test file:

<?php
apply_filters( 'foobar', 'example', 1, 2, 3, 4 );
do_action( 'foobar', 'example', 1, 2, 3 );

Running Psalm with wordpress-stubs set up gives these errors:

ERROR: TooManyArguments - test.php:2:1 - Too many arguments for method apply_filters - expecting 2 but saw 6
apply_filters( 'foobar', 'example', 1, 2, 3, 4 );

ERROR: TooManyArguments - test.php:3:1 - Too many arguments for method do_action - expecting 2 but saw 5
do_action( 'foobar', 'example', 1, 2, 3 );

Both apply_filters and do_action take an unlimited number of arguments, as they both call func_get_args.

It looks like the stubs for these functions are not correct:

/**
 * Calls the callback functions added to a filter hook.
 *
 * @since 4.7.0
 *
 * @param mixed $value The value to filter.
 * @param array $args  Arguments to pass to callbacks.
 * @return mixed The filtered value after all hooked functions are applied to it.
 */
public function apply_filters($value, $args)
{
}

/**
 * Executes the callback functions hooked on a specific action hook.
 *
 * @since 4.7.0
 *
 * @param mixed $args Arguments to pass to the hook callbacks.
 */
public function do_action($args)
{
}
szepeviktor commented 5 years ago

Yes, in PHPStan services are for that purpose, I hope Psalm has a similar feature.

szepeviktor commented 5 years ago

You can write write a service for PHPStan - or similar for Psalm - that accepts multiple arguments for these functions.

They are called plugins: https://github.com/vimeo/psalm/blob/master/docs/plugins.md

Flimm commented 5 years ago

I'm confused by your comments, @szepeviktor . The purpose of this repo is to plug in to static analysis tools like Psalm by providing stubs for the WordPress functions. There is a bug in the stubs in wordpress-stubs because they don't indicate which functions accept an unlimited number of arguments. The proposed solution is to fix wordpress-stubs, not to write another plugin.

GiacoCorsiglia commented 5 years ago

I agree this issue should be fixed in the stubs, although I appreciate you sharing your workaround @szepeviktor. The solution will probably be to automatically replace:

function apply_filters($value, $args) -> function apply_filters($value, ...$args)
function do_action($args) -> function do_action(...$args)

after the stubs are generated.

Are there other functions which have this problem? Would be good to get them all.

Flimm commented 5 years ago

@GiacoCorsiglia That sounds good like a good approach. I also wonder if a better approach would be to fix the stubs generator itself so that it can detect when func_get_args is called within a function body. For what it's worth, when you give Pslam the full source code, Psalm can detect this, but it obviously does not detect it with just stubs because stubs don't contain the function body. What do you think?

GiacoCorsiglia commented 5 years ago

That's an interesting thought @Flimm, I didn't realize Psalm did that. The stubs generator currently ignores anything inside function foo() {}. I guess the thing to do would be to:

  1. Keep track of when we're inside a function/method declaration.
  2. Note if func_get_args() is called (but it can't be inside a nested anonymous function or class declaration).
  3. Modify the Function_ node by adding appending a variadic ...$args parameter. That could be done via the NodeVisitor::leaveNode() method here, I believe.

Care might have to be taken in scenarios like apply_filters(), which already has arguments in its call signature. (E.g., you'd have to be careful to not add another param called $args). Perhaps there are other edge cases (maybe a look at Psalm's approach is warranted).

Alas I don't have the time to do this at the moment (blame grad school), but I'd happily look at a PR...

szepeviktor commented 5 years ago

List of WordPress functions using func_get_args()

https://github.com/szepeviktor/phpstan-wordpress/blob/0.2.1/example/phpstan.neon.dist#L24-L30

szepeviktor commented 4 years ago

Is seems to be resolved in WordPress v5.3 Please see https://github.com/php-stubs/wordpress-stubs/issues/2 for details.