apply_filters's handling of extra arguments is broken

For now we can change to 2, use array_merge, and not pass extra args to filters that have single arg functions registered by deafult.

Yup, upon further looking, array_merge and 2 was the right solution. But how do you intend to detect single args functions?

Reminder sent to morganiq

Pinging Morgan

This is only a problem with internal (rather then user defined) functions, right? We could search through the internal function array provided by get_defined_functions() and not pass extra args if the function registered with the filter is in that internal list. That could be pretty slow though.

By: rboren (who also forgot to login)

edited on: 02-18-05 01:21

(edit by michel v: I wrote this note, forgot to log in. I didn't write the previous one though) Another solution is to alter add_filter()'s syntax to accept an array or a string for the function name argument.

- add_filter('the_title', 'filterfunc') would only get the $string argument, - add_filter('the_title', array('filterfunc', 2)) would get $string plus up to two extra arguments brought by WP

This approach is not very elegant, but it has the merit of being backwards compatible with existing plugins that call core PHP functions. I'll be working on a patch to add this functionnality to apply_filters, if Morgan doesn't beat me to it during the night.

edited on: 02-18-05 00:59

It seems to me that the _number_ of additional arguments accepted is irrelevant: rather, the only important thing we need to distinguish is whether or not more than one argument can be passed. So a simple boolean flag should be all that we need:

add_filter('the_title', 'trim'); // does not accept multiple arguments add_filter('get_permalink', 'my_permalink_filter', , true); //does accept multiple arguments

michel v, coffee2code: thoughts?

Reminder sent to coffee2code

Pinging Scott.

(For background, coffee2code mentioned in #wordpress that some of his filter functions do "double-duty" and expect to only be passed one argument when called by apply_filters, but get passed other custom arguments when called from elsewhere in his plugin.)

How about something like:

add_filter('the_title', 'callback_function:#')

Where # is the number of args. If no number is specified, the function is assumed to accept a variable number of arguments.

I like that idea, it's very clean and easy to implement. My only qualm with that solution is that every internal, without variation or exception, would have to have :1 appended to it -- which isn't a big deal, but could be a point of confusion to uninitiated plugin devs. It would be nice, since we know that every internal will need to have one and only one argument passed, to either be smart enough to handle them separately, or to just default to one argument unless the user specifies that their function can handle more.

Which brings me back to get_defined_functions: I know that logically, checking each function name against a list is much less efficient than taking user-supplied argument counts -- but surely in_array is highly optimized, and as long as we cache the list from get_defined_functions, I'm starting to think the actual performance hit would be minimal.

We could also speed up in_array by eliminating the functions that logically could never be used as filters, and hard-code the list in. I'm sure an audit of the function list could reduce it by at least 75%.

I like Ryan's solution the best so far. I think the number of arguments a registered filter can accept needs to be passed in at the time of add_filter(). Any sort of introspection of how many arguments a function can take (if this is possible) or pre-associating number of args with function names is still doomed to have its drawbacks.

What I'd like to amend to Ryan's suggestion is the ability to indicate a function will accept all additional arguments, as in:

add_filter('the_title','callback_function:');

A trailing colon without a number to indicate all args; no ":#" at all would indicate no additional arguments (other than the primary argument that has until recently been sent via apply_filters()). It'll be up to future plugins to indicate they can take advantage of additional arguments, which should be easy enough under this scheme. And apply_filters() could continue to add additional arguments to the call without fear of breaking a registered filter function.

If you need to specify the number of arguments you want to accept, I'd say you've hit on the best solution.

Although I'm still a little fuzzy on why one would need to specify an exact number of arguments for the function to take, rather than just specifying that the function can take more than one. Could you give an example?

edited on: 02-18-05 06:25

I don't know if under current conditions my plugins are immediately affected. But let's say we at some point decide to do apply_filters('the_content', $content, $more_link_text). Currently we just send $content, but this is a reasonable example of a probable future change since the_content() does have $more_link_text as argument which we may want to send to filters.

Anyhow, most of the filters I register deal with 'the_content', so this change would impact me. This for example:

function c2c_hyperlink_urls ($text, $mode='0', $trunc_before=, $trunc_after='...', $open_in_new_window=true)

(I tend to use a lot of optional args because they are easy to override, yet provide for reasonable defaults; and I DO sometimes call filter functions like this directly from within a template file)

If I want to adapt to handle $more_link_text (for whatever reason), I'd do: add_filter('the_content', 'c2c_hyperlink_urls:1'); (or would that be ":2" -- are we saying 2 args total, or 1 arg in addition to the primary arg?)

And then I go and insert $more_link_text as the second arg for that function, shifting the other optional args over.

Assuming your situation of all-or-nothing in terms of additional args, if we then later added the_content()'s $strip_teaser arg via apply_filters('the_content', $content, $more_link_text, $strip_teaser), my function just broke/becomes unreliable.

edited on: 02-18-05 07:59

Ahh, I see, for future reliability. Well, you've convinced me. This gets my vote.

I would expect :n to return n args, not n-1. So specifying :1 would have the same behavior as specifying nothing at all, since the default behavior would be to provide only the first argument.

Update: it just occurred to me that this could be useful for getting rid of arguments altogether -- just specify ':0'. Can't think off the top of my head exactly why one would want to do that, but I guess it makes it more flexible. :-)

edited on: 02-18-05 08:14

Ryan: I would say if no number was specified, then it should be assumed the function takes only one argument, $string. This way, we do not break existing plugins that may use core PHP functions, and we do not have to go through WP's code adding :1 everybloodywhere. :) It also keeps it simple for simple plugins; keeps the burden of :X to advanced plugins.

michel v, I think we're all on the same page here; that's the behavior coffee2code & I were proposing.

For additional consideration, since the :# approach is something a sufficiently aware plugin author would be taking advantage of, is there then any harm in defining it via an additional argument to add_filter()?

function add_filter($tag, $function_to_add, $priority = 10, $number_of_arguments = 1)

It's less obscure than :# and more clearly indicates the default value. Though I can see how WP may internally use the notation, but if not, this saves WP from having to parse string:#. As far as changing existing add_filter() calls in WP, it's the same amount of effort either way; those that would've added the :# notation just need now to also specify their priority.

$number_of_arguments is fine too, whichever way everyone likes best.

So, we default to 1 unless otherwise noted. Do we need a "don't care" setting for number of arguments to accommodate functions that accept a variable number of arguments? $number_of_arguments = -1 or something?

Sounds good to me.

Perfect. I like.

Uploaded a patch that adds :num_args support and fixes the "whoops we don't send args" issue. The is_array check is done so we can support multiple args for methods in classes. Array('class', 'method:5') is how you tell WP that the method 'method' in class 'class' supports 5 arguments.

Uploaded a fix to the forementioned patch. (Hat tip: Ryan)

michel v, I thought we decided on a $number_of_arguments argument rather than the :n thing.

michel v: Whoa there buddy, committing already? That's not what we decided on!

OK, working on adding support for both methods...

Uploaded a patch that adds support for both methods, this patch applies on CVS (so after the commit of the previous patch). This patch also fixes do_action so that it too can send extra arguments, and moves the :X parsing to add_filter and remove_filter in order to avoid doing this operation everytime apply_filter is called.

Internally, filters are stored as [tag][priority][array(function, accepted_args)]. It could have been more optimal to store them as [tag][priority][function][accepted_args], since this would allow for simple issets to check for a filter's presence, but this would be incompatible with the usage of class methods (since those are passed as arrays, and we can't use arrays as array keys). To use them as array keys, we could also use serialize/unserialize, but that would add to the operations done when doing apply_filter, so it would be rather bad.

News patch, applies to the new SVN trunk. Fixes and optimises the code from the last patch: remove_filter was bugged, and apply_filter and do_action benefit from avoiding an array_slice if $accepted_args equals 1.

Looks pretty good to me. Any other comments?

Looks good to me too.

edited on: 02-24-05 22:24

Looks good.

We're not incurring much overhead, though, supporting both methods? If so, it'd be better to address it now before anyone uses either. Once deployed, we'd have to continue to support both methods.

It seems to me that the 'function:2' method would add yet more overhead, and I seem to recall reading we're trying to cut out overhead, not add more in. I'd go with only the function add_filter($tag, $function_to_add, $priority = 10, $number_of_arguments = 1) method.

Uploaded a version of the patch that doesn't handle the :X addition.

So, long story short: to accept more than one argument, one must specify a fourth argument to add_filter() and add_action().

http://trac.wordpress.org/changeset/2398

Now we need to identify those places where we should change the value for accepted args.

While using this functionality I realized something strange. To get an extra argument (say for 'bloginfo' filter) I have to specify not 2 but actually 4 in add_filter. The returned data are: first argument, filter name, first argument, second argument

BTW: One place where this function is needed is for writing 'bloginfo' filter. It passes two arguments - value and key.

So when I add_filter I have to write: add_filter('bloginfo', 'my_filter',10, 4 );

And then I access the second argument as: function remove_css($first_argument,$dummy,$dummy,$second_argument)

I haven't tried for more than 2 arguments but I suspect it will be equally funky.

Works fine for me.

function blog_filter($string, $show) {
        echo "String: $string Show: $show <br />";
        return $string;
}

add_filter('bloginfo', 'blog_filter', 10, 2 );

You can't have any more than those two arguments since apply_filters() as called in bloginfo() supplies only two.

worksforryan(andme)