Check hook callback is not boolean

[GaryJones]

Is your feature request related to a problem?

A client wanting to return false to a filter, did:

	add_filter( 'liveblog_entry_enable_embeds', false );

...instead of:

	add_filter( 'liveblog_entry_enable_embeds', '__return_false' );

Describe the solution you'd like

When a boolean false or true is given as the second arg of add_filter() or add_action(), make it an Error with a suggestion of what to use instead.

This could be extended to look for other __return_() functions, like null, 0, array(), etc.

Perhaps an error with a more vague message if it's anything non-callable?

[jrfnl]

Perhaps an error with a more vague message if it's anything non-callable?

Well, a tool like PHPCS can't determine if something is callable, it can only look for the format expected for a callable, i.e. (string) 'function_name' or (array) array('ClassName' or $obj, 'function_name').

Note: the callable can, of course, also be passed as a variable or combination of tokens, in which case, we're out of luck.
The below are all perfectly valid and IMO no error/warning should be thrown for any of these:

add_action('action_name', $my_callable);
add_action('action_name', $my->callable);
add_action('action_name', ClassName::$callable);
add_filter('filter_name', ($foo ? $foo_prefix : $bar_prefix ) . 'function_name');

Having said that, I think a sniff which can pick up on some typical mistakes does have a place WPCS, but with the above in mind, IMO, it should only look for and throw errors for verified mistakes, such as outlined in the original use-case.

[zanona]

I am currently invoking add_action with a callback which isn't in a form o string as I do get validations using @sirbrillig's phpcs-import-detection. However, it looks like the correct way of adding a callback is to declare:

add_action(  'init', 'my_callback' );

rather than

add_action( 'init', my_callback );

Considering this, would it be acceptable to request that this plugin adds checks instead, considering add_action and add_filter are WordPress methods? Or is still OK to declare it as in the first example?
Thanks

[jrfnl]

@zanona Your question is not clear to me, would you mind trying to rephrase ?

As for the following code sample:

add_action( 'init', my_callback );

This could be perfectly valid code if there is a constant declared called my_callback which contains the name of the callback function/method.

[zanona]

@jrfnl sorry, I meant it like this:

function my_callback {...}
add_action('init', my_callback);

So by using my_callback without quotes, with the plugin I mentioned earlier, I am able to catch errors like the callback being declared but not defined previously. but if I use quotes, since it's just a string, that plugin cannot check if the method indeed exists since it doesn't have knowledge about add_filter or add_action and that the second argument should be an existent function.

But, perhaps by defining as I am at the moment, using the method name directly without quotes is not a big problem?

thanks

[jrfnl]

But, perhaps by defining as I am at the moment, using the method name directly without quotes is not a big problem?

Using the method name without quotes will break your plugin as that's not a valid way to declare a callback (unless you define constants for all of them).

Please read up on callbacks to understand how it works: https://www.php.net/manual/en/language.types.callable.php

but if I use quotes, since it's just a string, that plugin cannot check if the method indeed exists since it doesn't have knowledge about add_filter or add_action and that the second argument should be an existent function.

That sounds like something which should be fixed in that external standard, especially as that standard already has some work-arounds in place for WordPress anyway.

[zanona]

@jrfnl thanks for the insight. Great to learn about the proper way of doing this. Perhaps it can be implemented on phpcs-import-detection, then. cc/ @sirbrillig.

[sirbrillig]

Won't phpcs-import-detection already warn you about using something like add_action('init', my_callback); without a constant definition for my_callback? Or are you suggesting that we add special detection for functions like add_action() which knows that the second argument should be callable? If it's the latter, I'd suggest just using types instead; making a custom function as a delegate, like:

function wp_add_action( string $tag, callable $callback, int $priority = 10, int $accepted_args = 1 ) {
  add_action( $ag, $callback, $priority, $accepted_args );
}

[zanona]

@sirbrillig, currently phpc-import-dection does warn you but only if the callback is not wrapped in quotes (as a string). so add_action('init', 'my_callback') wouldn't trigger any warning since it doesn't know that the second argument (string) should be a method name and search for already declared ones. Apparently not using quotes for declaring a callback isn't correct as previously mentioned?

[sirbrillig]

add_action('init', 'my_callback') wouldn't trigger any warning since it doesn't know that the second argument (string) should be a method name and search for already declared ones.

Correct. We'd need either a special list of functions or static analysis to be able to know if a string argument is a function reference. Unfortunately this is just how PHP uses function references. We could add such a special list (in fact, we have a special list for functions which mutate their arguments already), but it would only work for functions in that list. That might be a good feature request.

Apparently not using quotes for declaring a callback isn't correct as previously mentioned?

You're right. It's not just an incorrect pattern, it's actually invalid PHP, even though the current version of PHP will allow it by automatically converting the symbol into a string with a warning. In the next version of PHP it's likely to be a fatal error instead of just a warning.