Warn against lambda callbacks in add_filter() and add_action()

[azaozz]

(Follow-up from #1485).

Currently (because of PHP 5.2) lambda/anonymous functions cannot be used in core. However once we move on to requiring a more recent PHP version, this will become possible.

Generally using lambda functions should be (strongly) discouraged as they usually miss documentation and context. There are some exceptions: for example very short callbacks in calls to array_filter(), array_udiff(), usort(), etc. may be better to be "inline lambda" rather than defined separately.

However in case of add_filter() and add_action() using lambda functions is "harmful". The anonymous callbacks cannot be removed and "break" remove_filter() and remove_action().

In that terms thinking we need a sniff to warn when a lambda callback is used in general (in case the code can be written better), and perhaps another sniff (or another configuration of the same sniff) specifically targeting add_filter() and add_action() and throwing an error when a lambda callback is detected there.

[GaryJones]

The anonymous callbacks cannot be removed

Not true:

• https://wordpress.stackexchange.com/questions/137688/remove-actions-filters-added-via-anonymous-functions
• https://inpsyde.com/en/remove-wordpress-hooks/

[jrfnl]

The anonymous callbacks cannot be removed

Not true:

Well, I agree with @azaozz that closures can not easily be removed from the $wp_filter queue and as the whole principle of the hook mechanism is intended to make it easy to hook into WP, it should also be easy to unhook.

With that in mind, I would support a sniff which would warn - not error - when a closure is used as the callback parameter when hooking into WP.

Having said that, I totally disagree with prohibiting or even discouraging closures in general. Restricting the use of constructs like eval() is warranted as it is a security risk, restricting closures, which is a perfectly valid PHP construct, seems over the top.
Also keep in mind, that closures are the only real alternative for on the fly function creation now create_function() has been deprecated in PHP 7.2.

The only thing I could imagine doing in that space would be to, again, warn, when a closure is more than five statements with the suggestion of making it a proper function instead.

The number five is of course an arbitrary number and open for discussion. The number of statements could be made configurable, but should be set to a sensible default, with 0 IMO not being acceptable as a sensible default.

[JPry]

I think this paragraph from the second link provided by @GaryJones is relevant (although of course it's beyond the scope of this discussion):

Moreover, I think that the fact some of the functions of plugins API work well with some callback types and not with other is a flaw of the plugin API. Something that was totally understandable in 2004 when PHP was transitioning from version 4 to 5, closures and invokable objects did not existed and OOP PHP was at its first steps. But 14 years later, in my opinion, that can surely be considered a defect that had been better to address long time ago, especially considering that a big rewriting of the plugin API internals have been released just one year ago (with WP 4.7), without even discussing the possibility to address the issue in some way.

If a sniff is still going to be added, then I agree with @jrfnl that the sniff should only be a warning and not an error. I also agree that the number of statements in an anonymous function should be configurable.

[azaozz]

The anonymous callbacks cannot be removed

Not true:

@GaryJones hmmmm, this is a quote from one of the links you referenced:

The problem is that you can't distinguish form an anonymous function and another, so yes, it is possible to remove a closure (i.e. anonymous function) but if more than one closure act on same filter at same priority you have to make a choice, remove them all, or remove only one (without knowing exactly which).

I.e. you can "blindly" remove lambda callbacks in a very hacky way. Don't think that's an acceptable practice, not even close to "best practices", etc. :)

The fact is that using anonymous callbacks in add_filter() and add_action() breaks core functionality: remove_filter() and remove_action() cannot be used then. If plugin authors consciously want to break that part of the API, nothing can stop them. However if they are doing that just for convenience, or if they aren't fully aware what's happening, they should be made aware :)

I agree with @jrfnl that a warning would be sufficient there. Also thinking this particular case can perhaps be handled in the code ("Doing it wrong", etc.).

The general problems I'd like to address with this ticket are that:

• Anonymous functions miss context. A function's name (usually) gives pretty good idea of that a function does.
• Very often anonymous functions miss a docblock or any inline documentation. This is primarily caused by the code formatting/layout.
• Anonymous functions usually add redundant levels of indentation that can reduce readability.

As I've said above and as @jrfnl points out, the purpose here is not to "ban" all lambda functions, but to set "expectations" and "best practices". As they are not yet officially supported in core, now would be a good time to do so.

[lkraav]

Also thinking this particular case can perhaps be handled in the code ("Doing it wrong", etc.).

"Doing it wrong" seems excessive. Isn't that prohibitively difficult to disable for private projects, where all @azaozz listed lambda function related concerns can be handled perfectly fine?

Coding Standards is nicely configurable via XML, as long I can turn these off, whatever above sounds good to me for core guidelines.

[GaryJones]

The anonymous callbacks cannot be removed

I.e. you can "blindly" remove lambda callbacks in a very hacky way.

I agree - it's not clean, and it's not intuitive, but it is possible, which is exactly the opposite of what you first claimed. Please use accurate statements and avoid absolutes.

I agree with @jrfnl that a warning would be sufficient there.

Also agreed. There may be cases when a developer adds a filter with the intent that at least one callback is "always on"(for whatever reason), and using an anonymous function / closure would intentionally make it harder for consuming developers to remove that initial callback.

Also thinking this particular case can perhaps be handled in the code ("Doing it wrong", etc.).

Disagree. Using a closure as a hook callback isn't doing things wrong - it's just potentially against the coding standard, and shouldn't result in a change of behaviour at run-time.

Anonymous functions miss context. A function's name (usually) gives pretty good idea of that a function does.

Be clear here - anonymous functions can be assigned to a variable, so what you're really wanting to target is where the second arguments of add_filter() and add_action() contain/start with the T_FUNCTION T_CLOSURE token.

[jrfnl]

where the second arguments of add_filter() and add_action() contain/start with the T_FUNCTION token.

I think you mean the T_CLOSURE token or the function keyword 😀

[azaozz]

@GaryJones uh, ok, lets try this again, hopefully without misunderstandings this time :)

The problem with using lambda functions as callbacks in add_filter() and add_action() is that it breaks part of the plugins API, namely remove_filter() and remove_action(). Then:

• Plugin authors that break the API without realizing it should be warned about it.
• Plugin authors that consciously break the API should be told that they are "doing it wrong".

Whether it is warranted to throw "doing it wrong" when we detect such use is another question (for a core ticket). There have been some discussions about it but no clear conclusion for now. I personally would prefer if this is handled in the coding standards, however there is a possibility to handle that in the core code as well.

...but it is possible, which is exactly the opposite of what you first claimed.

Yes, it is possible (in a hacky, unsupported way) to remove a single lambda callback, however it is not possible to remove a callback when there are two or more lambda callbacks with the same priority. This type of removal will also be "doing it wrong" as it messes with the underlying (private) API data.

[aristath]

There are some valid use-cases for anonymous functions in add_filter & add_action...

Take for example something like this:

$sidebars = apply_filters( 'my_custom_filter_1', array(
	'sidebar_1' => true,
	'sidebar_2' => true,
	'sidebar_3' => false,
) );
add_filter( 'my_custom_filter_2', function( $args ) use ( $sidebars ) {
	foreach ( $sidebars as $key => $val ) {
		// Do something.
	}
	return $args;
} );

Though the above example is pretty simple and can be written without an anonymous function, depending on the data we have and operations we want to perform sometimes closures is the best option.

[azaozz]

the above example is pretty simple and can be written without an anonymous function.

@aristath right, I think so too. Also, could you add an example of how you can use remove_filter() to remove that callback from 'my_custom_filter_2'? :)

The whole point here is that anonymous callbacks break that part of the API.

[aristath]

could you add an example of how you can use remove_filter() to remove that callback from 'my_custom_filter_2'? :)

There is no way that I know of.

The point I wanted to make is that I have encountered instances where using anonymous functions is the best (if not only) way to handle some complex situations.

I'm not against discouraging the use of anonymous methods in add_action & add_filter. But there's a difference between discouraging and banning. :)

Also in case this is implemented, it would be really useful to have a whitelisting tag like // WPCS: Closures ok. or something similar. If that's not available (and memorable) we'll have to fallback to using // phpcs:ignore {rulename} which is just a bit annoying in most cases.

[jrfnl]

Also in case this is implemented, it would be really useful to have a whitelisting tag like // WPCS: Closures ok. or something similar. If that's not available (and memorable) we'll have to fallback to using // phpcs:ignore {rulename} which is just a bit annoying in most cases.

Just so you know: The // phpcs:ignore way of whitelisting is now the recommended method and the native WPCS whitelist comments are slated for removal.