[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($GLOBALS['wp_filter'][$tag][$priority][$fn_idx]);
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.
Cheers,
d.
--
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