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 :grinning:

[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.

[dingo-d]

I think that the name of the issue should be renamed to Discourage lambda callbacks in add_filter() and add_action() instead of "Ban"... 🙂

Just to avoid the confusion, as I think that this is what @azaozz had in mind in the first place.

[JPry]

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

I think it's worth pointing out that this is a limitation of the API itself. Anonymous functions are an essential part of the PHP language. Like anything else, they could certainly be used improperly. But they have a lot of advantages that shouldn't be set aside simply because the current WordPress API doesn't know how to deal with them.

The fact that the API isn't currently designed with these in mind means that the API needs to be updated to take these into account. As @aristath mentioned, there are instances where using anonymous functions is the best, and sometimes only, way to handle complex situations.

Rather than reinforcing the limitations of the API itself, the API should be updated. This is especially relevant when the move away from PHP 5.2 is taken into account.

If anything, this should be a temporary warning until the API can be made to handle anonymous functions properly.

[dingo-d]

The fact that the API isn't currently designed with these in mind means that the API needs to be updated to take these into account.

As long as WordPress is PHP 5.2 backward compatible, I doubt that is going to change...

[nerrad]

How is this different than a class instance doing this (where the instance is not exposed)?

class SomeClass {
   public function __construct() {
        add_action( 'some_hook', array( $this, 'some_method' ) );
   }
   public function some_method() {
       echo 'Hello';
   }
}

I agree with this statement by @JPry

Rather than reinforcing the limitations of the API itself, the API should be updated. This is especially relevant when the move away from PHP 5.2 is taken into account.

[azaozz]

Hmmm, I'm quite surprised at how many people are so eager to use a coding pattern that breaks an existing API. IMHO it would be helpful if everybody here mentioned what their involvement with coding standards is and why they think it is a good idea to do so :)

Theoretical example:

1. We accept lambda callbacks for filters and actions.
2. We refactor default-filters.php to use all lambda callbacks. (Once deemed acceptable I'd expect core to start using lambda callbacks too).

How many plugins to you think will break? Maybe enough to "bring down the internet"? :)

[azaozz]

How is this different than a class instance doing this (where the instance is not exposed)?

That class can be extended, and the methods overloaded, right? :) But yes, when it comes to OOP the "management" of what runs when is shifted to classes and sub-classes.

[azaozz]

I think it's worth pointing out that this is a limitation of the API itself. Anonymous functions are an essential part of the PHP language. Like anything else, they could certainly be used improperly.

Absolutely agree.

Rather than reinforcing the limitations of the API itself, the API should be updated.

Absolutely agree again. All commenters here are very welcome to add any ideas for updates/fixes to the plugins API that will allow anonymous callbacks to be removed easily, by using remove_filter() and remove_action().

Also agree that until this is patched in core we need to warn against using anonymous callbacks there.

[mundschenk-at]

How is this different than a class instance doing this (where the instance is not exposed)?

That class can be extended, and the methods overloaded, right?

Yes, but that doesn't help with removing a filter using that instance.

[azaozz]

Yes, but that doesn't help with removing a filter using that instance.

Right, but the decision to keep that instance "private" makes that non-removable (actually it can still be removed by doing instance_of(), etc. but not easily).

[rheinardkorf]

I very rarely go against suggestions in this project, but for this one I feel its worth speaking up. I would like to see this discussion going back to #core-php for more input and perhaps a make post. This is quite a drastic measure, even as a warning.

There are times where you might like to extend your own plugin and perhaps you'd like to implement a "reverse hook" mechanism in a class method (or constructor) to perhaps do something like...

public function __construct( $namespace ) {
    add_action( 'the_thing_i_trigger_elsewhere', function( $arg1, $arg2 ) use ( $namespace ) {
         // Do something with $arg1, $arg2 and $namespace.
    } )
}

I'm not saying the above is a valid use case per se, you could assign $namespace to a property (or heaven forbid a static property), but its not inherently bad if you know that you are controlling the hook tightly.

Some more discussion would be welcomed. If the community says, "ban it!" then that's fine. This particular sniff just worries me a bit, especially with seasoned developers who may have a very legit reason for doing this. I guess that warrants the argument for a phpcs:ignore.

[dingo-d]

I personally would be against this (adding add_action/add_filter in the constructor). Just looks like a problem in the long run (tight coupling to class, difficult to test, a problem when removing the hook).

But I'm definitely for discussing this on a larger scale since it touches the subject of rewriting the important API in the WordPress, and would definitely like to hear what some experienced developers have to say about this :)

[rheinardkorf]

@dingo-d I agree. Was using it as an example...

in a class method (or constructor)

[nerrad]

I personally would be against this (adding add_action/add_filter in the constructor). Just looks like a problem in the long run (tight coupling to class, difficult to test, a problem when removing the hook).

Yes in general it is a bad idea. I was just illustrating the point that what is being warned against here doesn't seem much different than a similar pattern I've seen elsewhere. This points to the larger issue of a flaw in the plugin action/filter api (granted not a trivial thing to fix for back-compat etc).

[Rarst]

Vehemently against discouraging closures.

WP is very hook centric and sometimes complex use cases require not just using an existing callback, but manufacturing a very custom callback with scope on the fly. Closures are the most convenient and succinct mechanism for it at the moment. Alternatives like object factory or anonymous class would be just as hard to remove without access to instance.

If removing is the problem then removing should be improved. There are abundant existing materials on removing complex callbacks, as well as battle–tested APIs to do it conveniently such as identifying them by a string representation. For example Brain Monkey testing lib allows to easily identify closures in hooks like this: has_filter('the_title', 'function ($title)' ).

[Rarst]

I've opened a trac ticket with suggestion to improve the API: Improve identifying of non–trivial callbacks in hooks

[azaozz]

Sorry but I think the discussion here is not going in the right direction :)

The facts are that closures in WP actions and filters break the current API. Ideally the coding standards should warn the developers that they are doing something wrong.

Whether closures are allowed in WP hooks or not is not something the "Coding Standards" should decide. However the "Coding Standards" shouldn't fail to warn the developer that an API is used incorrectly. IMHO this is one of the good things about having coding standards in the first place :)

[Rarst]

The facts are that closures in WP actions and filters break the current API. Ideally the coding standards should warn the developers that they are doing something wrong.

So do objects by that logic.

Thought the plan was to move WP development forward, not backwards.

[azaozz]

@Rarst I think you are still talking about how WordPress works and not about "coding standards". The ticket you opened on trac would probably be a better place to decide what to do with plugins and themes that break the existing API? :)

[jrfnl]

Related: https://github.com/PHPCSStandards/PHPCSExtra/issues/192

[jrfnl]

I've opened a PR to address the concern about long closures - PR #2311.

While it doesn't directly address the use of closures for hooks, it does address other remarks which were made in this thread.

Please have a look at the PR and leave an opinion if you have one.