Opened 14 years ago
Closed 7 years ago
#14671 closed enhancement (wontfix)
Deprecate the "accepted args" argument in add_filter() and add_action()
Reported by: | markjaquith | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | Plugins | Keywords: | has-patch dev-feedback |
Focuses: | Cc: |
Description
There is no harm in passing additional arguments to action/hook callbacks. Why do we force developers to explicitly ask for additional arguments to be passed? It is a maximum, and it doesn't matter if the maximum is exceeded. We can just pass all the arguments, all the time. Callbacks can use them, or ignore them. add_action()
and add_filter()
can lose their 4th argument.
Ran this by koopersmith and beaulebens, they couldn't find any reasons this would be a problem.
Attachments (5)
Change History (49)
#3
@
14 years ago
If memory serbes, there is huge harm in passing more arguments than needed to php's built-in functions.
addslahes('test', 'test'); // warning or fatal?
Userland functions can accept additional args without issues.
#4
@
14 years ago
- Cc westi added
- Component changed from General to Plugins
This is definitely one of the common first plugin gotchas.
Sounds like a great fix.
If there is an issue with php built-ins we could make the default behaviour be to pass an infinite number of args and preserve the current behaviour if a number was specified.
#5
@
14 years ago
Removal of a feature will break backwards compatibility and will affect a small percentage of plugin / theme developers. Deprecating the argument using one of the built-in deprecate functions should be used first and then the code should be removed at a future date.
Besides the reasoning you have in the summary, why would you want to remove the functionality?
Several key points of contention:
- I don't think the removal of code is dependent of people who have never worked on the plugin system and most likely have no idea how it would affect those used to this feature you plan on removing. Honestly, I think the vote should be with those who have actually written a patch for the plugin system and have provided support in the past.
- Lets weight the pros and cons. What other reasons do you have for removing the feature?
- Saying a feature is useless, is a matter of opinion that is quite bluntly is useless. It is like saying something sucks. It is subjective, unproven and unprovable, and pointless. There are many features that are rarely used, like for example the blog-by-email. Lets remove that why don't we, oh wait that was tried and the few people that wanted it argued for keeping it.
- This code has been in WordPress since the plugin API has been extended back in 1.2 to 1.5. Why break compatibility now?
- Have you done any statistics on which fraction of public plugins use the feature? We can put the guess at around .01% to 1%.
- Does this really affect beginners? In what way? I suppose you've handled the support, in what way were they confused with the feature. Is it something that inline documentation or Codex level documentation can correct in the future?
#6
@
14 years ago
<blockquote>Removal of a feature will break backwards compatibility</blockquote>
Re-read the ticket. You don't understand it.
<blockquote>If memory serves, there is huge harm in passing more arguments than needed to php's built-in functions.</blockquote>
That was the sort of gotcha I was looking for, but I think we can overcome it.
We could keep the argument, but set the default number of args to one for built-in PHP functions.
This increases peak memory usage by 55K, and uses between 1 and 1.5 ms to generate the initial list. Total time cost on my system, with 388 calls is 2.5 ms.
function is_internal_php_function( $function_name ) { global $php_internal_functions; if ( !isset( $php_internal_functions ) ) { $functions = get_defined_functions(); $php_internal_functions = array_flip( $functions['internal'] ); } return isset( $php_internal_functions[$function_name] ); }
But only a very small subset of those internal PHP functions are ever used in WordPress filters. We might be able to just manually enumerate them, and set the $accepted_args
to 1 by default for those.
WordPress itself only uses three PHP internal functions in filters:
trim(), strip_tags(), and urldecode()
I can think of a few other useful ones (intval()
, for instance.
In either case, we'd be keeping the optional argument, just choosing a more intuitive default, to remove the necessity for 10,2
nonsense.
#7
follow-ups:
↓ 8
↓ 9
@
14 years ago
I guess I need to explain. The backwards compatibility I was describing had nothing to with passing more parameters than the function definition supports, but really had to do with passing less parameters than the function supports.
Function A accepts 3 parameters with 3rd being optional.
The programmer wishes to use this same function for two or more filters, but wants to specify different behavior when the 3rd parameter is set.
Your patch, as it is now, will not allow for this specific use case. Believe me, I have seen people do this and had legitimate reasons for doing so.
Now that I have answered your request, how about answering the rest of mine.
#8
in reply to:
↑ 7
@
14 years ago
Replying to jacobsantos:
I guess I need to explain. The backwards compatibility I was describing had nothing to with passing more parameters than the function definition supports, but really had to do with passing less parameters than the function supports.
Function A accepts 3 parameters with 3rd being optional.
The programmer wishes to use this same function for two or more filters, but wants to specify different behavior when the 3rd parameter is set.
I considered raising the same issue (I occasionally do this), but I then felt that the cost of myself adding an extra function to work around it was quite tiny. It's much tinier than me wasting my time needing to remember to pass ghat 3rd argument anyway...
@mark: re the php built-in functions I'd take the view that we replace trim and so forth with custom wrappers where necessary. And then let WP complain or crash: doing so will make missing function args or extra passed args easier to track.
#9
in reply to:
↑ 7
@
14 years ago
Replying to jacobsantos:
Function A accepts 3 parameters with 3rd being optional.
The programmer wishes to use this same function for two or more filters, but wants to specify different behavior when the 3rd parameter is set.
That's what current_filter()
is for. It's much more powerful than going on the number of parameters passed.
We considered this, but thought that it was both an edge case and an instance of sloppy coding (why do that when we have a function that tells you exactly which hook is being run?)
@mark: re the php built-in functions I'd take the view that we replace trim and so forth with custom wrappers where necessary.
I don't think it's that much work to support internal PHP functions, either using the exhaustive method I provided, or just enumerating common use cases. There aren't that many that are ever used. Or, we could mark as deprecated the use of internal PHP functions, while still supporting them for a while.
#11
follow-up:
↓ 12
@
14 years ago
14671.002.diff
checks internal PHP functions and keeps the default at 1 for those, but sets it to 100 for all others. Using a smaller subset of PHP functions would be faster, but might require some maintenance in the future, if we want to keep the ability to set internal PHP functions as callbacks.
#12
in reply to:
↑ 11
@
14 years ago
Replying to markjaquith:
14671.002.diff
checks internal PHP functions and keeps the default at 1 for those, but sets it to 100 for all others. Using a smaller subset of PHP functions would be faster, but might require some maintenance in the future, if we want to keep the ability to set internal PHP functions as callbacks.
+1
@
14 years ago
Corrections to 14671.002.diff : PHPdoc for new function. Lowercase constant usage. Don't define max arguments, use null instead.
#13
@
14 years ago
Testing determined that passing null
to array_slice() 3rd parameter will get to the end of the array.
#15
@
14 years ago
Related: #14881 - do_action should not pass empty string by default
Related: #14789 - Inconsistency in 'all' hook invocation
Related: #10535 - _wp_filter_build_unique_id issues with the first time a filter is hooked by a class
Like this ticket, we have some basic Hook (Action/Filter) related tickets atm. Probably it's worth to streamline them all together for 3.1?
#18
@
13 years ago
If we do this, I wonder if we can speed things up. Perhaps by avoiding the array_slice().
#19
@
13 years ago
- Cc wp-trac@… added
Nobody should use stock PHP functions in WP actions/filters. But OMG... they might do. Reflection should be the solution. See sample:
function forward_call_reflection($function){ if(!is_callable($function)) return false; // Make sure it's worth continuing $args = func_get_args(); array_shift($args); // Get arguments, shift function (1st arguments) // No Reflection, bail here... false is to prevent autoload if(!class_exists('ReflectionFunction', false)){ return call_user_func_array($function, $args); } // Now gain access to reflection object and continue $refunc = new ReflectionFunction($function); // Call user defined functions directly as they don't care about argument counts if($refunc->isUserDefined()){ return call_user_func_array($function, $args); } // Now let's be cautious about arguments $this_argc = count($args); // Arguments fed to this function $argc = $refunc->getNumberOfParameters(); // Function arguments count (including predefined optionals) $req_argc = $refunc->getNumberOfRequiredParameters(); // Required arguments (with no default values) while($this_argc++ < $req_argc) $args[] = null; // Push NULL to fill gaps if($this_argc > $argc) array_splice($args, $argc); // POP extra arguments return $refunc->invokeArgs($args); // Invoke function }
Performance penalty is double the processing time.
100 calls to addslashes() with no Reflection take 0.0005870 while 100 calls with Reflection take 0.0013950.
It's unnoticeable IMO.
#20
@
13 years ago
To be totally paranoid regarding performance, statics can be used to store ReflectionFunction results for isUserDefined, getNumberOfParameters and getNumberOfRequiredParameters in case of repeat calls to same function [won't work for Closures though].
But I'd say that's a bit overdoing it.
#21
@
13 years ago
Anything except the above code kills performance. Caching in a static the get_defined_functions()[internals] and searching the array() slows things by up to 10 times. Caching ReflectionFunction-> method calls' results slows by about 10%. Anything involving extra array slows things down.
Reflection is blazing fast. Nothing else matches it. And PHP 5.2 has it for sure.
#22
@
13 years ago
Speaking of reflection, I made a little helper that automatically hooks all public methods of a given class:
http://scribu.net/wordpress/reflection-on-filters.html
It takes care of $accepted_args
and you can even set different priorities through DocComments.
@
12 years ago
Patch which allows omission 4th argument by using PHP Reflection to look up the number of accepted parameters from the supplied function
#25
follow-up:
↓ 26
@
12 years ago
I had the idea a couple years ago to use PHP's Reflection capabilities to introspect the number of arguments defined on the function so that the use of the $accepted_args
4th argument could be largely eliminated (as 5ubliminal also suggested above). At that time, WordPress still was required to support PHP4, and so this was a dead end since Reflection was introduced in PHP5. Now, however, since WordPress requires PHP≥5.2, it is now safe to use Reflection and we can use it make our plugin code more DRY. In my supplied patch, note that you can still supply the 4th $accepted_args
parameter, and this will override any attempt to get the argument count via Reflection; this is useful if you are using a function as a hook handler which has optional arguments which may get inadvertently set when the hook is executed with extra arguments.
Regarding performance, I did a comparison of the execution time when using Reflection vs. the baseline code that is in trunk. The test script can be found at https://gist.github.com/4581512#file-test-php
Here is the results of the reference baseline trunk plugins.php:
$ wp eval-file test.php Running original plugin.php Iteration count 100000 Testing function... 2.842000 seconds Testing method... 2.881264 seconds Testing closure... 2.831295 seconds
And then here is the results with my patch:
$ wp eval-file test.php Running patched plugin.php Iteration count 100000 Testing function... 3.783207 seconds Testing method... 3.864082 seconds Testing closure... 3.748331 seconds
As you can see at over 100,000 iterations, the version with Reflection runs about a third slower. I did try a version of my patch that included static caching the Reflection calls, but it seems the logic to generate the cache key (utilizing spl_object_hash()
) actually is about the same as invoking Reflection's getNumberOfParameters
method; after repeated tests, the version of the patch with and the version without would consistently alternate in being faster. Nevertheless, the patch with the cache is also available: https://gist.github.com/4581512#file-plugin-php-alternate-patch
Personally, I think the usability benefits we gain by not having to supply the 4th argument outweighs the overall performance hit to use Reflection to introspect the number of arguments.
I have not yet run the patch through the unit tests.
#26
in reply to:
↑ 25
;
follow-up:
↓ 27
@
12 years ago
Replying to westonruter:
I had the idea a couple years ago to use PHP's Reflection capabilities to introspect the number of arguments defined on the function so that the use of the
$accepted_args
4th argument could be largely eliminated (as 5ubliminal also suggested above). At that time, WordPress still was required to support PHP4, and so this was a dead end since Reflection was introduced in PHP5. Now, however, since WordPress requires PHP≥5.2, it is now safe to use Reflection and we can use it make our plugin code more DRY.
I use parameter matching using Reflection in my own code and it's still reasonably fast. However, with this specific issue, we should just be able to pass all the parameters in (as if you set $accepted_args
to PHP_INT_MAX
) without issue, and use $accepted_args
as a fallback to limit it (I can't think of any good reason not to, given that we have current_filter
).
Also related, I have a class similar to scribu's, but that uses PHPDoc tags. Again, this is still plenty fast enough despite the heavy use of Reflection, so if we did want to go that way, I'd argue the performance isn't really a consideration.
#27
in reply to:
↑ 26
@
12 years ago
Replying to rmccue:
I use parameter matching using Reflection in my own code and it's still reasonably fast. However, with this specific issue, we should just be able to pass all the parameters in (as if you set
$accepted_args
toPHP_INT_MAX
) without issue, and use$accepted_args
as a fallback to limit it (I can't think of any good reason not to, given that we havecurrent_filter
).
There are two reasons why letting PHP_INT_MAX
be the default value for $accepted_args
would be undesirable, as I understand from chatting with markjaquith this weekend at WordCamp Phoenix:
- For internal PHP functions, if you pass more arguments than the function is defined to take, it will issue a warning, for example:
PHP Warning: strtoupper() expects exactly 1 parameter, 3 given
. - For user-defined functions that have optional arguments, sometimes they are designed to behave differently when additional arguments are supplied. If the developer is not aware of a filter being supplied extra arguments, they may assume that any argument defaults they define in their function will be used, only to be surprised when any extra arguments for the filter get supplied the function's optional arguments. This is the point that jacobsantos makes above: comment:7
The 2nd point above would actually be a potential issue with my patch because it uses Reflection's getNumberOfParameters
method instead of getNumberOfRequiredParameters
. Perhaps the latter would be a more suitable default. In any case, if the default is not correct, the developer may supply the 4th argument.
On a completely different line of thinking, another option would be to eliminate the 4th argument and to let the 3rd parameter be a WP query arg string/array. This would not only make the code more readable (as the use of many positional parameters can be poor form), but it would also allow the accepted_args
be supplied without having to supply the priority
:
Index: wp-includes/plugin.php =================================================================== --- wp-includes/plugin.php (revision 23307) +++ wp-includes/plugin.php (working copy) @@ -62,8 +62,18 @@ * @param int $accepted_args optional. The number of arguments the function accept (default 1). * @return boolean true */ -function add_filter($tag, $function_to_add, $priority = 10, $accepted_args = 1) { +function add_filter($tag, $function_to_add, $options = '', $deprecated = 1) { global $wp_filter, $merged_filters; + if (is_numeric($options)) { + _deprecated_argument(__FUNCTION__, '3.6', 'Supply the accepted_args in the 3rd argument options'); + $priority = $options; + $accepted_args = $deprecated; + } else { + extract(wp_parse_args($options, array( + 'priority' => 10, + 'accepted_args' => 1, + ))); + } $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);
Usage:
add_filter('the_content', 'translate_into_klingon', 'accepted_args=3'); add_filter('the_content', 'translate_into_klingon', array( 'accepted_args' => 3 ));
#28
follow-up:
↓ 29
@
12 years ago
If we're going to make a function slower, it should be apply_filters() and do_action(). The functions add_filter() and add_action() get called thousands of times and need to be as fast as possible.
#29
in reply to:
↑ 28
;
follow-up:
↓ 31
@
11 years ago
Replying to nacin:
If we're going to make a function slower, it should be apply_filters() and do_action(). The functions add_filter() and add_action() get called thousands of times and need to be as fast as possible.
apply_filters()
is used in translate for gettext
and routinely adds up to hundreds/thousands calls.
At current point personally I wouldn't think of any performance degradation to these as acceptable. It's a glue that holds everything together and due to core (over)use is hard to manage for performance.
#31
in reply to:
↑ 29
;
follow-up:
↓ 35
@
11 years ago
Replying to Rarst:
At current point personally I wouldn't think of any performance degradation to these as acceptable. It's a glue that holds everything together and due to core (over)use is hard to manage for performance.
Agreed, it'd be nice to see more data though so that these things don't appear to be based on opinion when read by others
#35
in reply to:
↑ 31
@
11 years ago
Replying to TJNowell:
Agreed, it'd be nice to see more data though so that these things don't appear to be based on opinion when read by others
Quick rough count from xhprof profile for my current local stack:
add_action()
243 callsadd_filter()
500 callsdo_action()
198 callsapply_filters()
2201 calls
It has some dirty performance hacks and minimum active plugins, so would probably be even more in average site.
#36
follow-up:
↓ 37
@
11 years ago
mmm, If I am not miss anything but is this will broke such examples?!
// esc_url only need the first arg. add_filter( 'post_link', 'esc_url' ); apply_filters( 'post_link', $url, $post_id ); function esc_url( $url, $protocols, $_context ){ // function code. }
#37
in reply to:
↑ 36
;
follow-up:
↓ 38
@
11 years ago
Replying to alex-ye:
mmm, If I am not miss anything but is this will broke such examples?!
// esc_url only need the first arg. add_filter( 'post_link', 'esc_url' ); apply_filters( 'post_link', $url, $post_id ); function esc_url( $url, $protocols, $_context ){ // function code. }
No, it will not break. See the code in the patch checks to see how many arguments that the esc_url
function accepts via the Reflection API, and it will pass in as many arguments from the apply_filters
call as the supplied filter function accepts.
#38
in reply to:
↑ 37
;
follow-up:
↓ 39
@
11 years ago
Replying to westonruter:
No, it will not break. See the code in the patch checks to see how many arguments that the
esc_url
function accepts via the Reflection API, and it will pass in as many arguments from theapply_filters
call as the supplied filter function accepts.
I think that the use of Reflection API here will downgrade the performance.
As you see esc_url can accept 3 arguments and we use the first only in most cases. The thing I don't want to happen is to pass $post_id as $protocols.
Instead of deprecate the "accepted args" I love to see an ability to pass -1 to indicate that the function can accept any args count.
#39
in reply to:
↑ 38
@
11 years ago
Replying to alex-ye:
Instead of deprecate the "accepted args" I love to see an ability to pass -1 to indicate that the function can accept any args count.
Yes, that's probably what we'll need to do here.
In hindsight, it would have been -1 to begin with, and thus the esc_url() example, you'd force 1. By defaulting to 1, we tied our hands.
#44
@
7 years ago
- Milestone Future Release deleted
- Resolution set to wontfix
- Status changed from new to closed
I think the point has passed where it's possible to make this change. Core was probably already past that point when this ticket was opened seven years ago.
I know that in the past I've written functions that accept more than one parameter, but only receive one parameter when they're used as a filter or action callback. One that comes to mind is a $force
flag for deletion. Example:
function trash_connected_item( $term_id, $force_delete = false ) { if ( $force_delete ) { // delete connected term } else { // trash connected term } } add_action( 'delete_term', 'trash_connected_item' );
This code would break with any of the proposed patches above, because the second parameter from the delete_term
filter ($tt_id
) would be passed as the $force_delete
parameter and act as a truthy value.
Feel free to re-open if there's a way this can be worked around.
Nacin just came over and said he also can't think of a reason that it wouldn't work.
Uploaded patch isn't complete... we'd remove some more code that manages accepted args, but the patch makes it so that all the arguments are passed.