Kill the serialization of $wp_filter in get_terms()

[nacin]

We use this as part of a persistent cache key:

serialize($GLOBALS['wp_filter']['list_terms_exclusions'])

See [8225].

This is not good for two reasons. First, well, it's a not fun to reach into our internal API like that. But worse, it's broken whenever an object method is used, because spl_object_hash() will be unique to that object, thus rendering the cache invalidated on the next pageload.

As an aside, we should probably have _wp_filter_build_unique_id() create a delimiter when dealing with a static class method — $function[0].$function[1] can conflict with a legitimate function name.

I'm not sure how this should be fixed.

That looks incredibly hacky. Besides, closures are not serializable (you get a fatal error).

Here's how you can serialize a Closure instance, and by "serialize" I really mean "create a unique identifier":

ob_start();
echo new ReflectionFunction( function ( $var1, $var2 ) { 
	return $var1 + $var2;
} );
$source = ob_get_clean();
echo md5( $source );

// makes md5 of this:
Closure [ <user> function {closure} ] {
  @@ /Users/scott/Sites/wordpress-core/test.php 64 - 66

  - Parameters [2] {
    Parameter #0 [ <required> $var1 ]
    Parameter #1 [ <required> $var2 ]
  }
}

This works because ReflectionFunction implements the toString magic method and is better than spl_object_hash which will refresh the hash on every request.

​http://www.php.net/manual/en/reflectionfunction.tostring.php

Since PHP 5.2 (the required version for WP is 5.2.4), spl_object_hash exists, which means the else statement and function_exists() check are completely useless in _wp_filter_build_unique_id()

Also, since all of that code is ditched and strings are returned for functions, there is nothing unique about the ID created, it just returns a string representation of a_function, MyClass::method, or whatever represents a hash for a closure

I am working up a patch

FYI, here's a good way to debug this in functions.php or whatever:

function a_function( $filter ) { error_log( 'called' ); return $filter; }
class MyClass1 { function filter( $filter ) { error_log( 'called' ); return $filter; } }
class MyClass2 { static function filter( $filter ) { error_log( 'called' ); return $filter; } }

$my_class_1a = new MyClass1;
$my_class_1b = new MyClass1;

add_filter( 'list_terms_exclusions', 'a_function' );
add_filter( 'list_terms_exclusions', array( $my_class_1a, 'filter' ) );
add_filter( 'list_terms_exclusions', array( $my_class_1b, 'filter' ) );
add_filter( 'list_terms_exclusions', array( 'MyClass2', 'filter' ) );
add_filter( 'list_terms_exclusions', 'MyClass2::filter' );
add_filter( 'list_terms_exclusions', function ( $filter ) { error_log( 'called' ); return $filter; } );

print_r( $GLOBALS['wp_filter']['list_terms_exclusions'] );

echo wp_filter_callbacks_hash( 'list_terms_exclusions' );

get_terms( 'link_category' );
exit();

Attached the patch:

Removed the unreachable code
Created a function to hash a closure: _wp_build_unique_closure_id( $function )
Created a function to hash a list of callbacks by key: wp_filter_callbacks_hash( $tag )

_wp_filter_build_unique_id() now generally returns the string name of a function or class ('MyClass::method') which is what this function was doing anyways - it returns an md5 string representing a closure where applicable. For class instances, it will prepend spl_object_hash( $function[0] )~ which will be removed when the wp_filter_callbacks_hash is generated - allows persistence between requests

get_terms() now internally calls:

$filter_key = wp_filter_callbacks_hash( 'list_terms_exclusions' );

It previously called:

$filter_key = ( has_filter('list_terms_exclusions') ) ? serialize($GLOBALS['wp_filter']['list_terms_exclusions']) : '';

Updated the patch and edited my comments where appropriate

Clever.

Since this is the only time we've had to hash the filters being used, I wonder if this is a bit overkill.

@nacin - do we still care about this, or should we ditch the filter and throw a doing_it_wrong or something? I mean, this could be our chance to have an md5'd output buffer of ReflectionFunction::__toString in Core, which everyone's been asking for

this could be our chance to have an md5'd output buffer of ReflectionFunction::__toString in Core, which everyone's been asking for

Everybode? Really? :P

or should we ditch the filter and throw a doing_it_wrong or something?

Instead of basing the cache key on the list of callbacks attached to that filter, why not just use the term IDs currently returned by the filter?

Or, ugh, the SQL string returned by the filter rather.