[wp-hackers] Re: [wp-trac] Re: [WordPress Trac] #3875: Proposal for a new plugin architecture

David Schoonover david.schoonover at gmail.com
Sat Jul 21 20:03:20 GMT 2007

Welp, the problems are pretty straightforward. Problem_s_, plural.

Serialize() makes a string out of the CURRENT STATE of an object.  
When we call serialize(array( &$plugin_obj, $func_name )) during  
add_filter(), we get the state of $plugin_obj *at that time*. If the  
state changes before remove_filter() is called, the result of  
serialize() will be different, and we'll leave the filter hooked in.

Additionally, $function_to_add is passed by reference into add_filter 
() and remove_filter(), each of which call serialize(). Serialize(),  
in turn, triggers __sleep() on the object on which it is run. While  
Post Teaser does not take advantage of __sleep()'s magical  
functionality, it's not unreasonable that some especially complex  
plugin might do so. Should that user helpfully pass in  
$function_to_add with a reference to her object, we'll be calling  
__sleep() on the original, live variable, not a copy. Given all this,  
WordPress randomly calling serialize() simply to make a string by  
which to index an object seems neither safe nor fair.

I was unable to reproduce problems with Post Teaser in my testing,  
but I'd guess that whatever issue this causes, it's subtle,  
intermittent and obscure. (Since we only fail when the object's state  
changes between a call to add_filter() and a call to remove_filter(),  
AND failing to remove a filter will only have an effect if the hook  
is yet to come, the execution path for a plugin where this happens is  
probably pretty wild. I'd imagine we'd see some sort of bizarre error  
from the plugin itself, as it was not expecting to have its filter  
function called. But WE HAVE NO IDEA what it will actually do.  
(Additionally, the changes as to how objects are handled in PHP4 vs  
PHP5 has bearing on this problem in several places, so it's also  
likely that the problem will manifest differently on each platform.)

In any case, one potential fix is very easy--replacing calls to  
serialize() with calls to get_class()--and thinking about it will  
illuminate why this strategy will fail unless we slightly change what  
we expect from a function that wishes to hook into a filter.

function add_filter($tag, $function_to_add, $priority = 10,  
$accepted_args = 1) {
	global $wp_filter;

	$fn_idx = (is_array($function_to_add) ? (get_class($function_to_add 
[0]) . $function_to_add[1]) : $function_to_add);

	$wp_filter[$tag][$priority][$fn_idx] = array('function' =>  
$function_to_add, 'accepted_args' => $accepted_args);
	return true;

Unfortunately, all is not well.

This will fail whenever a plugin instantiates more than one instance  
of one of its classes (they'll have the same name according to  
get_class() ), and attempts to register filters from two different  
objects--with different private data, presumably--under the same method.

The standard PHP distribution provides no way (I know of) for us to  
maintain the identity of an object *as a scalar* over time--which is  
what we need so it can be used as a key. All of the essential  
characteristics they have are the same (class, method names, property  
names) and all of their superficial characteristics could change over  
time. It's like that time that set of twins showed up to my party and  
swapped shirts while I was in the bathroom. We can tell they're not  
equal (by comparing them, and seeing, oh hay, there's two of them!)  
but we can't actually keep track of any given one as soon as he and/ 
or she leaves our sight. (Note that we have this problem with  
serialize() as well, as two instances of a class instantiated with  
the same values will yield the same serialized string, even though  
they are not strictly equal.)

The best way I can see to salvage the new filter framework (and keep  
our efficiency gains) is to make each twin give up some DNA for us to  
test. Erm, we need to force objects bearing filter functions to be  
able to identify themselves. PHP5 actually has a function intended  
for this built right in: __toString(). It's magically called whenever  
one would convert an object to a string, the intention being that it  
will return something that picks this object out uniquely (or I guess  
prints out "Hello World" like all the examples :P). (This is unlike  
serialize(), which saves its state irrespective of whether the  
original object persists). This makes it better than any other  
arbitrary method name, as it is not only unlikely to be used for some  
other purpose (similar to 'wp_plugin_obj_to_string' or somesuch), but  
it is, in fact, somewhat likely to be *already* used for *this*  
purpose as it is canonically intended to do what we're asking of it  
(even though the user's version of PHP may or may not care very much).

In the case where a plugin would want to register the same method  
name on two different instances of a class, she would be required to  
implement the method __toString() to uniquely identify that object  
for its duration, or suffer burny, intermittent death.

Note also that it's fine to register '__toString' as a method in  
PHP<5. It just lacks the magical functionality. The same goes for  
when __toString() isn't implemented by the user: casting an object to  
a string simply returns the helpful value of "Object". So as long as  
we don't rely on the magic (or the lack thereof) anywhere else--god  
forbid we have strval($obj) == "Object" somewhere--and we invoke it  
explicitly in add_filter() and remove_filter(), we're fine. Plugins  
that will not run into this problem in the first place--like the vast  
majority of them, as we're seeing now with the buggy serialize() code  
running live--have no need to make any changes. Of the few that do,  
the change is trivial.

One final note. You may be saying, "Dave, this is not a perfect  
solution. It requires action on the part of the plugin author to  
work. In fact, it is a somewhat messy solution." You would be right.

However, looking back on the old code, this problem still existed. / 
We can't solve this by rolling it back./ Technically, it was a form  
of the changing-state-in-serialize() problem, but nonetheless:

// This is a snippet from remove_filter(), starting on line 77 in the  
old plugins.php

foreach ( (array) $wp_filter[$tag]["$priority"] as $filter ) {
	if ( $filter['function'] != $function_to_remove )
		$new_function_list[] = $filter;

Given that $function_to_remove and $filter['function'] are both  
something like array( $object, $function_name ), the comparison will  
end up looping through all the properties of each object, and testing  
to see if their values are equal. If the function-tuple wasn't  
created by reference--and that is up to the plugin author: if her  
plugin is running in PHP4, she has to call $function_to_add = array( & 
$object, $function_name ) and not $function_to_add = array( $object,  
$function_name ) (but vice versa in PHP5!)--the properties of the  
object stored in $wp_filter will have different values than  
$function_to_remove if the state has changed. Slightly inverted from  
our first problem, but in both cases we're in trouble if the author  
doesn't have knowledge of how her object is going to be identified.

Given that rolling back the code doesn't fix the problem, I'm more  
fond of this solution. No, it's not perfect, but on the bright side,  
it does helpfully solve our problem by offloading it to the plugin  
author! And in this case, she is going to be a tiny, infinitesimal  
fraction of all plugin authors anyway--the plugin author who wants to  
make many instances of her objects and attach each one with the same  
method to the same filter. And really, all we're forcing her to do is  
read the documentation. ;) (We really *do* want to doc this, heh. If  
this goes into the trunk, I'll volunteer to write it up.)

So the final code is:

function add_filter($tag, $function_to_add, $priority = 10,  
$accepted_args = 1) {
	global $wp_filter;
	if(is_array($function_to_add)) {
		list($obj,$fn) = $function_to_add;
		$fn_idx = (method_exists($obj,'__toString') ? $obj->__toString() :  
'') . '[]' . get_class($obj) . '[]' . $fn;
	} else
		$fn_idx = $function_to_add;
	$wp_filter[$tag][$priority][$fn_idx] = array('function' =>  
$function_to_add, 'accepted_args' => $accepted_args);
	return true;

function remove_filter($tag, $function_to_remove, $priority = 10,  
$accepted_args = 1) {
	global $wp_filter, $merged_filters;

	if(is_array($function_to_remove)) {
		list($obj,$fn) = $function_to_remove;
		$fn_idx = (method_exists($obj,'__toString') ? $obj->__toString() :  
'') . '[]' . get_class($obj) . '[]' . $fn;
	} else
		$fn_idx = $function_to_add;

	unset( $merged_filters[ $tag ] );

	return true;

ps. I'm sure I violated some sort of style guide or something in this  
message, but I poked around Trac and couldn't find anything. I  
apologize in advance.



David Alan Schoonover

On Jul 21, 2007, at 10:52 a, WordPress Trac wrote:

> #3875: Proposal for a new plugin architecture
> ---------------------------------------------------------------------- 
> ------------+
>  Reporter:   
> FraT                                                                   
> |        Owner:  ryan
>      Type:   
> defect                                                                 
> |       Status:  reopened
>  Priority:   
> normal                                                                 
> |    Milestone:  2.2.2
> Component:   
> Optimization                                                           
> |      Version:  2.1.1
>  Severity:   
> normal                                                                 
> |   Resolution:
>  Keywords:  2nd-opinion plugin plugins plugin.php filters  
> $wp_filter wp-includes  |
> ---------------------------------------------------------------------- 
> ------------+
> Changes (by Nazgul):
>   * keywords:  has-patch 2nd-opinion plugin plugins plugin.php filters
>                $wp_filter wp-includes => 2nd-opinion plugin
>                plugins plugin.php filters $wp_filter wp-
>                includes
> -- 
> Ticket URL: <http://trac.wordpress.org/ticket/3875#comment:9>
> WordPress Trac <http://trac.wordpress.org/>
> WordPress blogging  
> software_______________________________________________
> wp-trac mailing list
> wp-trac at lists.automattic.com
> http://lists.automattic.com/mailman/listinfo/wp-trac

More information about the wp-hackers mailing list