Make WordPress Core

Opened 9 years ago

Last modified 5 years ago

#33492 new enhancement

Slightly better add_filter

Reported by: iazel's profile Iazel Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: General Keywords:
Focuses: Cc:

Description

I want to suggest a small change for add_filter, adding a new parameter to explicitly define the $idx used.

<?php
function add_filter( $tag, $function_to_add, $priority = 10, $accepted_args = 1, $idx = null ) {
        global $wp_filter, $merged_filters;

        if($idx === null)
                $idx = _wp_filter_build_unique_id($tag, $function_to_add, $priority);
        $wp_filter[$tag][$priority][$idx] = array('function' => $function_to_add, 'accepted_args' => $accepted_args);
        unset( $merged_filters[ $tag ] );
        return true;
}

I've just made $idx explicit and for obvious retrocompatibility reasons this have to be at the end.
Why we should do this small change? Well, this will help a lot and will enable a full use of anonymous functions:

add_action('init', function(){ /* some code */ }, 10, 0, 'plugin_domain');
add_action('the_title', function($t){ return $t; }, 10, 0, 'plugin_domain');

Please notice that I've used the domain name as the $idx, therefore we can:

remove_action('init', 'plugin_domain');

It would be easier and more efficient if we all follow the convention of set $idx = $domain_name, hence if anyone want to remove a specific hook introduced by some plugin, doesn't need to read the code to know it.
Also, if we use anonymous functions instead of named, the namespace pollution will decrease by a lot and performance should increase (it doesn't even have to calculate the $idx).
It will also enable the override of specific hook. Today we need to remove the hook we want to override and add another one, because we cannot redifine a named function. However, with this change all we have to do is:

add_action('init', function(){ /* new code */ }, 10, 0, 'plugin_to_override');

A syntactical sugar will be great:

function new_add_action($hook, $idx, $callback, $priority = 10, $args_count = 1) {
   return add_filter($hook, $callback, $priority, $args_count, $idx);
}

I don't know what the name should be yet, but introducing it is a good compromise between retrocompatibility and good API; in years the first version should be deprecated... Something like that.

All in all, a really small change can simplify a good amount of things.
I don't really know if this is the correct way to propose this change, but I still haven't found the equivalent of git's pull request on SVN, sorry for that ^^"

Change History (4)

#1 follow-up: @dd32
9 years ago

FWIW, the best way to handle this at present is to assign the anonymous function to a variable which you later remove, the following code would print false, true, false for example:

var_dump( has_filter( 'test' ) );

add_filter( 'test', $function = function() {} );

var_dump( has_filter( 'test' ) );

remove_filter( 'test', $function );

var_dump( has_filter( 'test' ) );

It seems to me, that allowing the closures to be named in this way could result in conflicts with other plugins and functions. assigning it to a variable and removing it later also encourages developers not to use anonymous functions when they should be using a real function or method.

#2 in reply to: ↑ 1 ; follow-up: @Iazel
9 years ago

Replying to dd32:

FWIW, the best way to handle this at present is to assign the anonymous function to a variable which you later remove, the following code would print false, true, false for example:

var_dump( has_filter( 'test' ) );

add_filter( 'test', $function = function() {} );

var_dump( has_filter( 'test' ) );

remove_filter( 'test', $function );

var_dump( has_filter( 'test' ) );

It seems to me, that allowing the closures to be named in this way could result in conflicts with other plugins and functions. assigning it to a variable and removing it later also encourages developers not to use anonymous functions when they should be using a real function or method.

Sorry, but I don't see how this could raise more conflicts than using the named functions, furthermore if you use the plugin domain, that should be unique for every installed plugin as far as I know, hence it's impossible to have a name conflict.
That's said, we could introduce a check to see if a name is already present and raise an exception, but I discourage this approach because then we should introduce a new parameter for the override or do it as we already do.
I also don't see why anonymous function should be discouraged, this kind of use case is the perfect one for them!
Obviously we can store it in a variable, but in the 99% of the case, that variable will be out of scope for another plugin/template to replace it, and that's the usual case.

And please, you should take note that this method doesn't replace the actual workflow, therefore if you don't like anonymous functions, you can continue to not use them.

#3 in reply to: ↑ 2 ; follow-up: @dd32
9 years ago

Replying to Iazel:

Sorry, but I don't see how this could raise more conflicts than using the named functions, furthermore if you use the plugin domain, that should be unique for every installed plugin as far as I know, hence it's impossible to have a name conflict.

Since we can't enforce it that other plugins won't have a similarly named function, or that plugin authors would use their plugin slug, that raises the possibility of conflicts. Sure, it's a small case, but it's one that exists.

I also don't see why anonymous function should be discouraged, this kind of use case is the perfect one for them!

I'm not saying they should be discouraged, but that in some situations, they should be avoided in favour of a class method or stand alone function. Yes, in your use-cases they probably make sense, and I'm not arguing against that.

Obviously we can store it in a variable, but in the 99% of the case, that variable will be out of scope for another plugin/template to replace it, and that's the usual case.

And that's a use-case where I feel that an anonymous function should be discouraged, where it's hooked in one place, and unhooked in another context. Personal opinion though.

#4 in reply to: ↑ 3 @Iazel
9 years ago

Replying to dd32:

Since we can't enforce it that other plugins won't have a similarly named function, or that plugin authors would use their plugin slug, that raises the possibility of conflicts. Sure, it's a small case, but it's one that exists.

It's the same thing with named function, isn't it? I could also inject some very common function name, like:

<?php
function format_content($content) { /* code */ }
add_filter('the_content', 'format_content');

I could do that, but anyone who want to publish his plugin know that he must give his function a good name that will not cause conflicts. With this small addition, who want to use a function doesn't need to think about it anymore, just use the plugin domain (that it's already specific) and you're done :)

Regarding the other points, I can't say much because that's your opinion, programming is also nice because you can do it in your way and I think this change respects that principle :)

Version 0, edited 9 years ago by Iazel (next)
Note: See TracTickets for help on using tickets.