Make WordPress Core

Opened 14 years ago

Closed 6 years ago

#14671 closed enhancement (wontfix)

Deprecate the "accepted args" argument in add_filter() and add_action()

Reported by: markjaquith's profile 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)

14671.diff (1.5 KB) - added by markjaquith 14 years ago.
14671.002.diff (4.5 KB) - added by markjaquith 14 years ago.
14671.2.diff (4.5 KB) - added by jacobsantos 14 years ago.
Make default to pass all parameters and allowed_args limits.
14671.3.diff (4.6 KB) - added by jacobsantos 14 years ago.
Corrections to 14671.002.diff : PHPdoc for new function. Lowercase constant usage. Don't define max arguments, use null instead.
14671.4.diff (7.5 KB) - added by westonruter 11 years ago.
Patch which allows omission 4th argument by using PHP Reflection to look up the number of accepted parameters from the supplied function

Download all attachments as: .zip

Change History (49)

@markjaquith
14 years ago

#1 @markjaquith
14 years ago

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.

#2 @scribu
14 years ago

  • Cc scribu added

#3 @Denis-de-Bernardy
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 @westi
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 @jacobsantos
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:

  1. 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.
  2. Lets weight the pros and cons. What other reasons do you have for removing the feature?
  3. 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.
  4. This code has been in WordPress since the plugin API has been extended back in 1.2 to 1.5. Why break compatibility now?
  5. Have you done any statistics on which fraction of public plugins use the feature? We can put the guess at around .01% to 1%.
  6. 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 @markjaquith
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: @jacobsantos
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 @Denis-de-Bernardy
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 @markjaquith
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.

#10 @nbachiyski
14 years ago

  • Cc nbachiyski added

#11 follow-up: @markjaquith
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.

@jacobsantos
14 years ago

Make default to pass all parameters and allowed_args limits.

#12 in reply to: ↑ 11 @jacobsantos
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

@jacobsantos
14 years ago

Corrections to 14671.002.diff : PHPdoc for new function. Lowercase constant usage. Don't define max arguments, use null instead.

#13 @jacobsantos
14 years ago

Testing determined that passing null to array_slice() 3rd parameter will get to the end of the array.

#14 @peaceablewhale
14 years ago

  • Keywords has-patch added

#15 @hakre
13 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?

#16 @markjaquith
13 years ago

  • Milestone changed from 3.1 to Future Release

Not touching this for 3.1

#17 @mikeschinkel
13 years ago

  • Cc mikeschinkel@… added

#18 @nacin
13 years ago

If we do this, I wonder if we can speed things up. Perhaps by avoiding the array_slice().

#19 @5ubliminal
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 @5ubliminal
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 @5ubliminal
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 @scribu
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.

#23 @lkraav
12 years ago

  • Cc lkraav added

#24 @westonruter
11 years ago

  • Cc westonruter@… added

@westonruter
11 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: @westonruter
11 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: @rmccue
11 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 @westonruter
11 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 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).

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:

  1. 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.
  2. 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 ));
Last edited 11 years ago by SergeyBiryukov (previous) (diff)

#28 follow-up: @nacin
11 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: @Rarst
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.

#30 @Rarst
11 years ago

  • Cc contact@… added

#31 in reply to: ↑ 29 ; follow-up: @TJNowell
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

#32 @TJNowell
11 years ago

  • Cc tom@… added

#33 @alex-ye
11 years ago

  • Cc nashwan.doaqan@… added

#34 @toscho
11 years ago

  • Cc info@… added

#35 in reply to: ↑ 31 @Rarst
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 calls
  • add_filter() 500 calls
  • do_action() 198 calls
  • apply_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: @alex-ye
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: @westonruter
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: @alex-ye
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 the apply_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 @nacin
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.

#40 @betzster
11 years ago

  • Cc j@… added

#41 @chriscct7
8 years ago

  • Keywords dev-feedback added

#42 @swissspidy
8 years ago

#36713 was marked as a duplicate.

#43 @swissspidy
7 years ago

#37985 was marked as a duplicate.

#44 @johnbillion
6 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.

Note: See TracTickets for help on using tickets.