WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 15 months ago

#10535 reopened defect (bug)

_wp_filter_build_unique_id issues with the first time a filter is hooked by a class

Reported by: simonwheatley Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.9
Component: Plugins Keywords: 3.2-early has-patch needs-refresh
Focuses: Cc:

Description

Ref #8723

The first time _wp_filter_build_unique_id is used to generate an ID the ID returned is different to the second time it is called. This presents a problem if the first ID is used when adding a filter which then needs to be removed later in the app flow... as the IDs don't match the filter is unremovable.

One workaround proposed is to set a wp_filter_id property before add the filter, and this will cause _wp_filter_build_unique_id to bypass the problem code and effectively forces the "unique" ID which is generated... this workaround feels unpoetic. ;)

Attachments (12)

10535.diff (1.4 KB) - added by dd32 5 years ago.
10535.2.diff (1.5 KB) - added by dd32 5 years ago.
10535.3.diff (679 bytes) - added by mdawaffe 4 years ago.
10535.patch (4.1 KB) - added by hakre 4 years ago.
New approach to fix issues.
10535.2.patch (4.1 KB) - added by hakre 4 years ago.
Global registry is not needed any longer in this helper function.
10535.3.patch (4.3 KB) - added by hakre 4 years ago.
added spl_object_hash replacement function
10535.4.patch (4.4 KB) - added by hakre 4 years ago.
Update spl_object_hash replacement function after running PHP 4 tests.
10535.5.patch (4.7 KB) - added by hakre 4 years ago.
better namespace collision prevention, docblocks added.
10535.6.patch (4.8 KB) - added by hakre 4 years ago.
Introducing wp_object_hash(), fixed some whitespace issues and a variable typo.
10535.7.patch (4.7 KB) - added by hakre 4 years ago.
is_callable() can check for syntax as well and returns the needed string at once - handy.
10535.8.patch (4.9 KB) - added by hakre 4 years ago.
Refreshed against Trunk, added function_exists() cache in wp_object_hash()
10535.9.patch (4.8 KB) - added by hakre 3 years ago.
Updated for PHP 5 and against current trunk, coding standard fixes

Download all attachments as: .zip

Change History (46)

comment:1 follow-up: hakre5 years ago

that's by design, because it should generate a new id each time. anyway the current implementation might have it's design flaws and there might be possibility to improve it. irc suggestions were:

http://wordpress.pastebin.com/m6cac0d84 (http://wordpress.pastebin.com/m34bad8d7)
http://wordpress.pastebin.com/m34bad8d7 (DD32)

comment:2 hakre5 years ago

correct credits are:
http://wordpress.pastebin.com/m6cac0d84 (simonwheatley)

dd325 years ago

comment:3 dd325 years ago

  • Milestone changed from Unassigned to 2.9
  • Version set to 2.9

attachment 10535.diff added

  • Main change is lines 685-7, the rest are just WordPress Coding Standard cleanups.

Testing code:

include 'wp-load.php';

class Foo {	function test() { echo 'TEST'; } }
class Baz extends Foo { }

$a = new Foo();
$b = new Foo();
$c = new Baz();

var_dump( _wp_filter_build_unique_id('test', array(&$a, 'test'), 10) );
var_dump( _wp_filter_build_unique_id('test', array(&$b, 'test'), 10) );
var_dump( _wp_filter_build_unique_id('test', array(&$c, 'test'), 10) );

var_dump( _wp_filter_build_unique_id('test', array(&$a, 'test'), 10) );
var_dump( _wp_filter_build_unique_id('test', array(&$b, 'test'), 10) );
var_dump( _wp_filter_build_unique_id('test', array(&$c, 'test'), 10) );

do_action('test');

Output before:

string 'Footest0' (length=8)
string 'Footest0' (length=8)
string 'Baztest0' (length=8)
string 'Footest3' (length=8)
string 'Footest4' (length=8)
string 'Baztest5' (length=8)

Notice that the FIRST calling will return 0, not 3, 4 or 5?

Testing it again:

var_dump( _wp_filter_build_unique_id('test', array(&$a, 'test'), 10) );
var_dump( _wp_filter_build_unique_id('test', array(&$a, 'test'), 10) );
var_dump( _wp_filter_build_unique_id('test', array(&$a, 'test'), 10) );

output:

string 'Footest0' (length=8)
string 'Footest3' (length=8)
string 'Footest3' (length=8)

So its just the first calling which is failing.

After my patch, Here's the output of the first test case:

string 'Footest3' (length=8)
string 'Footest4' (length=8)
string 'Baztest5' (length=8)
string 'Footest3' (length=8)
string 'Footest4' (length=8)
string 'Baztest5' (length=8)

Which is now correct, $a = 3, $b = 4, $c = 5 each time, Whereas before, All would be 0 first call, and 3/4/5 2nd and every call after..

Lets test filter/action adding/removing.

add_action( 'test', array(&$a, 'test') );
remove_action( 'test', array(&$a, 'test') );

do_action('test');

Before:

TEST

Seems the filter hasnt been removed (As expected, since its ID differs)

After patch:

NULL OUTPUT

No output, Therefor, Action was removed.

Sorry for the long post, But thats the method i've used to test, 99% sure this has no negitive side effects, It works with multiple instances of objects, as well as different objects, Functions and Static calling isnt touched.

comment:4 dd325 years ago

  • Keywords has-patch tested added

comment:5 dd325 years ago

On the other hand, Hakre's solution also works on PHP 5.2+ at least, And the overhead of either is incredibly tiny..

Mine's just fixing the current function, hakre's is replacing the current function with spl_ or uniqid (Which is the same as using the current static var basically.. just doesnt have the current bug of 0)

I'd like to see spl_object_hash() used.. But its not being used correctly in hakre's patch, Theres no reason to store it.

If the hash function was to be used, i'd do it like this:

	} else if (is_object($function[0]) ) { // Object Class Calling
		if ( function_exists('spl_object_hash') )
			return spl_object_hash($function[0]) . $function[1];
		$obj_idx = get_class($function[0]).$function[1];
		if ( !isset($function[0]->wp_filter_id) ) {

Theres no need to store the wp_filter_id in that case, its completely redundant, as it'll return the exact same string the next time its called.

See my 2nd patch for incluson of spl_object_hash (And no, for those interested, the extra function_exists() call adds minimal overhead, about 0.000005 over 10,000 iterations for me..

dd325 years ago

comment:6 simonwheatley5 years ago

Tested 10535.2.diff and this works in my use case(s) where current code doesn't. Tested with and without the spl_object_hash function.

Thanks DD32 and Hakre.

In brief my use case in code is:

class Some_Class {

	// Blah blah

	function widget($args, $instance) {
		
		// Blah blah
		
		// This is the first time anyone has hooked posts_fields in this app
		add_filter( 'posts_fields', array( & $this, 'posts_fields' ) );
		
		$r = new WP_Query( $query_args );
		
		// Blah blah

		// This is the remove_filter call which doesn't work with the current code
		remove_filter( 'posts_fields', array( & $this, 'posts_fields' ) );
	}

	// Blah blah

}

comment:7 simonwheatley5 years ago

  • Cc simon@… added

comment:8 dd325 years ago

  • Keywords commit added

moving to commit-candidate queue

comment:9 westi4 years ago

Given the latest patch here a test and all looks good for both cases.

Makes the unit tests for removal pass when they currently failed.

comment:10 westi4 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [12090]) Improve _wp_filter_build_unique_id to give more consistent behaviour and make filter removal reliable for class based filters. Fixes #10535 props dd32.

comment:11 mdawaffe4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The patched code only works if the filter didn't have anything attached to it already.

Test case in PHP4 (and maybe PHP5 < 5.2)

require 'wp-load.php';

class Kill_It {
        function dead( $i ) {
                return 'DEAD';
        }
}

$kill = new Kill_It; // or $kill =& new Kill_It;  // Doesn't matter

var_dump( get_bloginfo( 'name', 'display' ) );

add_filter( 'bloginfo', array( &$kill, 'dead' ) );

var_dump( get_bloginfo( 'name', 'display' ) );

remove_filter( 'bloginfo', array( &$kill, 'dead' ) );

var_dump( get_bloginfo( 'name', 'display' ) );

Output:

string(6) "My WordPress Site"
string(4) "DEAD"
string(4) "DEAD"

Calling remove_all_filters( 'bloginfo' ) prior to the add_filter() results in:

string(6) "My WordPress Site"
string(4) "DEAD"
string(4) "My WordPress Site"

Confirming the problem is with filters already having attached callbacks.

Attached fixes.

For 3.1, we can simplify the function greatly since spl_object_hash() is always available in PHP >= 5.2.

mdawaffe4 years ago

comment:12 mdawaffe4 years ago

  • Milestone changed from 2.9 to 3.0.2

comment:13 follow-up: Denis-de-Bernardy4 years ago

If I may chime in on a slightly orthogonal, albeit related issue, this won't work either:

function foo() {
remove_action('bar', 'foo');
}

function foo2() {
echo 'test';
}

add_action('bar', 'foo', 10);
add_action('bar', 'foo2', 11);
do_action('bar'); // no output

Or at least it didn't last I checked (writing from an iPad... Can't test.).

comment:14 hakre4 years ago

For those who run into problems removing filter, I've published some code examples on how to safely deal with it. Should do it for any PHP version.

comment:15 in reply to: ↑ 13 jacobsantos4 years ago

Replying to Denis-de-Bernardy:

If I may chime in on a slightly orthogonal, albeit related issue, this won't work either:

function foo() {
remove_action('bar', 'foo');
}

function foo2() {
echo 'test';
}

add_action('bar', 'foo', 10);
add_action('bar', 'foo2', 11);
do_action('bar'); // no output

Or at least it didn't last I checked (writing from an iPad... Can't test.).

This doesn't work because of the behavior of iteration through an array and pointer handling. That said, this should have been fixed when apply_filter() was patched to allow for removing filters. Of course a copy of the filter could be used that might prevent the bug.

Regardless, the full details are a bit vague, since it has been two years since this problem first reared its head.

Lets say you have 4 elements:

element1 -> element2 -> element3 -> element4

You iterate through element2, which removes element2, so you expect it to jump to element3.

element1 -> element2 -x> element3 -> element4

becomes:

element1 -> element3 -> element4

However, the reference from element2 to element3 is broken and PHP is now all like, "WTF? I don't see the other elements, so I guess I'm done." If you were to reset the array and start from the beginning, element2 would be gone and element1 would jump to element3 like it should.

That is what I can gather from tests with the behavior and pinpointing it down 2 years ago. Whether or not it is a PHP problem is not known, but it is something to look out for.

Working with a copy will not have this problem, but then you have a problem of wanting to remove the reference to a filter further down in the chain and not being able to. The solution is a bit more complex, but is workable with a bit of refactoring and keeping track of the current index and then working from there. It would also slow down the filter system.

comment:16 jacobsantos4 years ago

I was right.
{{{<?php

$filter = $wp_filter[ $tag ];
reset( $filter );

do {

foreach ( (array) current($filter) as $the_ )

if ( !is_null($the_function?) )

call_user_func_array($the_function?, array_slice($args, 0, (int) $the_accepted_args?));

} while ( next($filter) !== false );

?>
}}}

Will display 'test' in your example.

comment:17 in reply to: ↑ 1 ; follow-up: mikeschinkel4 years ago

  • Cc mikeschinkel@… added

Replying to hakre:

that's by design, because it should generate a new id each time. anyway the current implementation might have it's design flaws and there might be possibility to improve it. irc suggestions were:

http://wordpress.pastebin.com/m6cac0d84 (http://wordpress.pastebin.com/m34bad8d7)
http://wordpress.pastebin.com/m34bad8d7 (DD32)

Those are all gone. Evidently when you pasted them you did so with an expiration date?

comment:18 in reply to: ↑ 17 hakre4 years ago

Replying to mikeschinkel:

Replying to hakre:

... irc suggestions were:

http://wordpress.pastebin.com/m6cac0d84 (http://wordpress.pastebin.com/m34bad8d7)
http://wordpress.pastebin.com/m34bad8d7 (DD32)

Those are all gone. Evidently when you pasted them you did so with an expiration date?

No, those might just be timed out but I had not specified any. Should be all old discussions so probably not important any longer. Next time I'll attach a textfile. Maybe there is something to find in the IRC logs?

comment:19 hakre4 years ago

While having the fingers in the file (Related: #14789), I just created this:

PHP 4.2.0+ code to obtain unique object id per request:

As spl_object_hash is not available on WP's current minimum PHP requirements:

ob_start();
debug_zval_dump( $some_object_instance );
list( $object_id ) = explode(' ', ob_get_clean(), 2);
echo $object_id;

Design Flaw on Function / Static Class Function (same unique ID per different callback)

class myplugin {
  static function _breaks() {
  }
}

function myplugin_breaks() {
}

Register both on the same hook with the same priority. The later will overwrite the first one or vice-versa.

Suggested fix: add :: between class name and static function name.


With both suggestions the function it should be possible to make the function more safe in handling data.

hakre4 years ago

New approach to fix issues.

hakre4 years ago

Global registry is not needed any longer in this helper function.

comment:20 hakre4 years ago

Hmm, looks like debug_zval_dump() does not return a unique ID per request on PHP 4 :/. Maybe some other replacement strategy is better.

hakre4 years ago

added spl_object_hash replacement function

hakre4 years ago

Update spl_object_hash replacement function after running PHP 4 tests.

hakre4 years ago

better namespace collision prevention, docblocks added.

comment:21 hakre4 years ago

  • Keywords dev-feedback added; tested commit removed

Removing those two keywords for now and looking for dev-feedback

hakre4 years ago

Introducing wp_object_hash(), fixed some whitespace issues and a variable typo.

comment:22 follow-up: hakre4 years ago

I tested the routine now. It works with Zend Egine 1 and Zend Egine 2 w/o spl_object_hash(). This covers all three scenarios:

  1. PHP 4 - works properly on the fallback variant.
  2. PHP < 5.2.0 - works properly on the fallback variant.
  3. PHP >= 5.2.0 - works (as not tested in life systems) with sp_object_hash().

My tests did run against the StdClass Class, user-defined classes and build in classes (e.g. DomDocument).

comment:23 in reply to: ↑ 22 hakre4 years ago

Replying to hakre:

  1. PHP >= 5.2.0 - works (as not tested in life systems) with sp_object_hash().

That's a mistake. Usage of spl_object_hash() is in core since longer, so it is tested.

comment:24 hakre4 years ago

  • Keywords tested added; dev-feedback removed

hakre4 years ago

is_callable() can check for syntax as well and returns the needed string at once - handy.

hakre4 years ago

Refreshed against Trunk, added function_exists() cache in wp_object_hash()

comment:25 nacin3 years ago

  • Milestone changed from 3.0.3 to 3.1

comment:26 follow-up: scribu3 years ago

  • Keywords needs-patch 3.2-early added; has-patch tested removed
  • Milestone changed from 3.1 to Future Release

I don't think we should mess with this in 3.1 beta, especially since in 3.2 we will be able to use spl_object_hash() directly.

comment:27 scribu3 years ago

Related: #9968

comment:28 garyc403 years ago

  • Keywords has-patch added; needs-patch removed

comment:29 in reply to: ↑ 26 hakre3 years ago

Replying to scribu:

I don't think we should mess with this in 3.1 beta, especially since in 3.2 we will be able to use spl_object_hash() directly.

Unless PHP 5.3, the SPL extension can be disabled. See SPL Installation. So we might need to offer a spl_object_hash() replacement function -or- encapsulate it inside a function on our own (similar to wp_object_hash() in 10535.6.patch, it's a php 4 compatible implementation)

comment:30 follow-up: scribu3 years ago

Yes, except it doesn't need to be PHP 4 compatible anymore.

comment:31 in reply to: ↑ 30 hakre3 years ago

Replying to scribu:

Yes, except it doesn't need to be PHP 4 compatible anymore.

Yeah, finally :) No more tests against PHP 4 - I'm finally done with those for WP :)

hakre3 years ago

Updated for PHP 5 and against current trunk, coding standard fixes

comment:32 hakre3 years ago

I'm just wondering about the $priority parameter, the rest looks pretty okay to me.

In case spl_object_hash is disabled but the function is a PHP 5.3.x closure, this will run into a Catchable fatal error: Closure object cannot have properties, but this is so much likely not to happen.

comment:33 azizur3 years ago

  • Cc azizur added

comment:34 wonderboymusic15 months ago

  • Keywords needs-refresh added

Related / equally gross: #21267 - I think the patch in #21267 handles serialization of closures better, but it's still nasty

Note: See TracTickets for help on using tickets.