Skip to content

Commit

Permalink
Re-work filter/action hook wrapping to happen just-in-time
Browse files Browse the repository at this point in the history 
* Ensure that filters/actions added during execution of page get wrapped.
* Prevent hook wrapping from happening on callbacks for functions with parameters passed by reference.
* Split widget callback wrapping logic out from hook callback wrapping.
  • Loading branch information
@westonruter
westonruter committed Mar 11, 2018
1 parent 5033910 commit 6d5fcc1
Show file tree
Hide file tree
Showing 2 changed files with 166 additions and 51 deletions.
115 changes: 74 additions & 41 deletions includes/utils/class-amp-validation-utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,8 @@ public static function print_dashboard_glance_styles() {
* Add hooks for doing validation during preprocessing/sanitizing.
*/
public static function add_validation_hooks() {
self::callback_wrappers();
self::wrap_widget_callbacks();
add_action( 'all', array( __CLASS__, 'wrap_hook_callbacks' ) );
add_filter( 'do_shortcode_tag', array( __CLASS__, 'decorate_shortcode_source' ), -1, 2 );
add_filter( 'amp_content_sanitizers', array( __CLASS__, 'add_validation_callback' ) );
}
Expand Down Expand Up @@ -602,6 +603,7 @@ public static function print_edit_form_validation_status( $post ) {
* @return string HTML Comment.
*/
public static function get_source_comment( array $source, $is_start = true ) {
unset( $source['reflection'] );
return sprintf(
'<!--%samp-source-stack %s-->',
$is_start ? '' : '/',
Expand Down Expand Up @@ -674,48 +676,12 @@ public static function remove_source_comments( $dom ) {
}

/**
* Wraps callbacks in comments to indicate to the sanitizer which extension added them.
*
* Iterates through all of the registered callbacks for actions and filters.
* If a callback is from a plugin and outputs markup, this wraps the markup in comments.
* Later, the sanitizer can identify which theme or plugin the illegal markup is from.
* Wrap callbacks for registered widgets to keep track of queued assets and the source for anything printed for validation.
*
* @global array $wp_filter
* @return void
*/
public static function callback_wrappers() {
global $wp_filter;
$pending_wrap_callbacks = array();

// @todo Consider doing this at the 'all' action instead. This would be more reliable and we could reset $wp_filter at PHP_INT_MAX.
// Wrap all added filters and actions.
foreach ( $wp_filter as $hook => $wp_hook ) {
foreach ( $wp_hook->callbacks as $priority => $callbacks ) {
foreach ( $callbacks as $callback ) {
$source = self::get_source( $callback['function'] );
if ( ! $source ) {
continue;
}
$source['hook'] = $hook;

$pending_wrap_callbacks[ $hook ][] = array_merge(
$callback,
compact( 'priority', 'source', 'hook' )
);
}
}
}

// Iterate over hooks to replace after iterating over all to begin with to prevent infinite loop in PHP<=5.4.
foreach ( $pending_wrap_callbacks as $hook => $callbacks ) {
foreach ( $callbacks as $callback ) {
remove_action( $hook, $callback['function'], $callback['priority'] );
$wrapped_callback = self::wrapped_callback( $callback );
add_action( $hook, $wrapped_callback, $callback['priority'], $callback['accepted_args'] );
}
}

// Wrap widgets callbacks.
public static function wrap_widget_callbacks() {
global $wp_registered_widgets;
foreach ( $wp_registered_widgets as $widget_id => &$registered_widget ) {
$source = self::get_source( $registered_widget['callback'] );
Expand All @@ -730,7 +696,72 @@ public static function callback_wrappers() {

$registered_widget['callback'] = self::wrapped_callback( $callback );
}
}

/**
* Wrap filter/action callback functions for a given hook.
*
* Wrapped callback functions are reset to their original functions after invocation.
* This runs at the 'all' action. The shutdown hook is excluded.
*
* @global WP_Hook[] $wp_filter
* @param string $hook Hook name for action or filter.
* @return void
*/
public static function wrap_hook_callbacks( $hook ) {
global $wp_filter;

if ( ! isset( $wp_filter[ $hook ] ) || 'shutdown' === $hook ) {
return;
}

foreach ( $wp_filter[ $hook ]->callbacks as $priority => &$callbacks ) {
foreach ( $callbacks as &$callback ) {
$source = self::get_source( $callback['function'] );
if ( ! $source ) {
continue;
}

/*
* A current limitation with wrapping callbacks is that the wrapped function cannot have
* any parameters passed by reference. Without this the result is:
*
* > PHP Warning: Parameter 1 to wp_default_styles() expected to be a reference, value given.
*/
if ( self::has_parameters_passed_by_reference( $source['reflection'] ) ) {
continue;
}
unset( $source['reflection'] ); // No longer needed.

$source['hook'] = $hook;
$original_function = $callback['function'];
$wrapped_callback = self::wrapped_callback( array_merge(
$callback,
compact( 'priority', 'source', 'hook' )
) );

$callback['function'] = function() use ( &$callback, $wrapped_callback, $original_function ) {
$callback['function'] = $original_function; // Restore original.
return call_user_func_array( $wrapped_callback, func_get_args() );
};
}
}
}

/**
* Determine whether the given reflection method/function has params passed by reference.
*
* @since 0.7
* @param ReflectionFunction|ReflectionMethod $reflection Reflection.
* @return bool Whether there are parameters passed by reference.
*/
protected static function has_parameters_passed_by_reference( $reflection ) {
foreach ( $reflection->getParameters() as $parameter ) {
if ( $parameter->isPassedByReference() ) {
return true;
}
}
return false;
}

/**
Expand Down Expand Up @@ -769,8 +800,10 @@ public static function decorate_shortcode_source( $output, $tag ) {
* @return array|null {
* The source data.
*
* @type string $type Source type.
* @type string $type Source type (core, plugin, mu-plugin, or theme).
* @type string $name Source name.
* @type string $function Normalized function name.
* @type ReflectionMethod|ReflectionFunction $reflection
* }
*/
public static function get_source( $callback ) {
Expand Down Expand Up @@ -805,7 +838,7 @@ public static function get_source( $callback ) {
return null;
}

$source = array();
$source = compact( 'reflection' );

$file = $reflection->getFileName();
if ( $file ) {
Expand Down
102 changes: 92 additions & 10 deletions tests/test-class-amp-validation-utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,16 +68,33 @@ class Test_AMP_Validation_Utils extends \WP_UnitTestCase {
*/
const TAG_NAME = 'img';

/**
* Backed up $wp_registered_widgets.
*
* @var array
*/
protected $original_wp_registered_widgets;

/**
* Setup.
*
* @inheritdoc
* @global $wp_registered_widgets
*/
public function setUp() {
parent::setUp();
$dom_document = new DOMDocument( '1.0', 'utf-8' );
$this->node = $dom_document->createElement( self::TAG_NAME );
AMP_Validation_Utils::reset_validation_results();
$this->original_wp_registered_widgets = $GLOBALS['wp_registered_widgets'];
}

/**
* After a test method runs, reset any state in WordPress the test method might have changed.
*/
public function tearDown() {
$GLOBALS['wp_registered_widgets'] = $this->original_wp_registered_widgets; // WPCS: override ok.
parent::tearDown();
}

/**
Expand Down Expand Up @@ -348,12 +365,52 @@ public function test_source_comments() {
}

/**
* Test callback_wrappers
* Test wrap_widget_callbacks.
*
* @covers AMP_Validation_Utils::wrap_widget_callbacks()
*/
public function test_wrap_widget_callbacks() {
global $wp_registered_widgets;

$widget_id = 'search-2';
$this->assertArrayHasKey( $widget_id, $wp_registered_widgets );
$this->assertInternalType( 'array', $wp_registered_widgets[ $widget_id ]['callback'] );
$this->assertInstanceOf( 'WP_Widget_Search', $wp_registered_widgets[ $widget_id ]['callback'][0] );

AMP_Validation_Utils::wrap_widget_callbacks();
$this->assertInstanceOf( 'Closure', $wp_registered_widgets[ $widget_id ]['callback'] );

$sidebar_id = 'amp-sidebar';
register_sidebar( array(
'id' => $sidebar_id,
'after_widget' => '</li>',
) );
update_option( 'sidebars_widgets', array(
$sidebar_id => array( $widget_id ),
) );

ob_start();
dynamic_sidebar( $sidebar_id );
$output = ob_get_clean();

$this->assertStringStartsWith(
'<!--amp-source-stack {"type":"core","name":"wp-includes","function":"WP_Widget_Search::display_callback","widget_id":"search-2"}--><li id="search-2"',
$output
);
$this->assertStringEndsWith(
'</li><!--/amp-source-stack {"type":"core","name":"wp-includes","function":"WP_Widget_Search::display_callback","widget_id":"search-2"}-->',
$output
);
}

/**
* Test wrap_hook_callbacks.
*
* @covers AMP_Validation_Utils::callback_wrappers()
* @covers AMP_Validation_Utils::wrap_hook_callbacks()
*/
public function test_callback_wrappers() {
global $post;
$that = $this;
$post = $this->factory()->post->create_and_get(); // WPCS: global override ok.
$this->set_capability();
$action_no_tag_output = 'foo_action';
Expand All @@ -365,6 +422,8 @@ public function test_callback_wrappers() {
$action_two_arguments = 'example_action_two_arguments';
$notice = 'Example notice';

AMP_Validation_Utils::add_validation_hooks();

add_action( $action_function_callback, '_amp_print_php_version_admin_notice' );
add_action( $action_no_argument, array( $this, 'output_div' ) );
add_action( $action_one_argument, array( $this, 'output_notice' ) );
Expand All @@ -374,18 +433,13 @@ public function test_callback_wrappers() {
add_action( $action_core_output, 'edit_post_link' );
add_action( $action_no_output, '__return_false' );

// All of the callback functions remain as-is. They will only change for a given hook at the 'all' action.
$this->assertEquals( 10, has_action( $action_no_tag_output, 'the_ID' ) );
$this->assertEquals( 10, has_action( $action_no_output, array( $this, 'get_string' ) ) );
$this->assertEquals( 10, has_action( $action_no_argument, array( $this, 'output_div' ) ) );
$this->assertEquals( 10, has_action( $action_one_argument, array( $this, 'output_notice' ) ) );
$this->assertEquals( 10, has_action( $action_two_arguments, array( $this, 'output_message' ) ) );

$_GET[ AMP_Validation_Utils::VALIDATE_QUERY_VAR ] = 1;
AMP_Validation_Utils::callback_wrappers();
$this->assertNotEquals( 10, has_action( $action_no_tag_output, 'the_ID' ) );
$this->assertNotEquals( 10, has_action( $action_no_output, array( $this, 'get_string' ) ) );
$this->assertNotEquals( 10, has_action( $action_no_argument, array( $this, 'output_div' ) ) );
$this->assertNotEquals( 10, has_action( $action_one_argument, array( $this, 'output_notice' ) ) );
$this->assertNotEquals( 10, has_action( $action_two_arguments, array( $this, 'output_message' ) ) );

ob_start();
do_action( $action_function_callback );
$output = ob_get_clean();
Expand Down Expand Up @@ -438,6 +492,34 @@ public function test_callback_wrappers() {
$output = ob_get_clean();
$this->assertNotContains( '<!--amp-source-stack ', $output );
$this->assertNotContains( '<!--/amp-source-stack ', $output );

// Ensure that nested actions output the expected stack, and that has_action() works as expected in spite of the function wrapping.
$handle_outer_action = function() use ( $that, &$handle_outer_action, &$handle_inner_action ) {
$that->assertEquals( 10, has_action( 'outer_action', $handle_outer_action ) );
$that->assertEquals( 10, has_action( 'inner_action', $handle_inner_action ) );
do_action( 'inner_action' );
$that->assertEquals( 10, has_action( 'inner_action', $handle_inner_action ) );
};
$handle_inner_action = function() use ( $that, &$handle_outer_action, &$handle_inner_action ) {
$that->assertEquals( 10, has_action( 'outer_action', $handle_outer_action ) );
$that->assertEquals( 10, has_action( 'inner_action', $handle_inner_action ) );
echo '<b>Hello</b>';
};
add_action( 'outer_action', $handle_outer_action );
add_action( 'inner_action', $handle_inner_action );
ob_start();
do_action( 'outer_action' );
$output = ob_get_clean();
$this->assertEquals(
implode( '', array(
'<!--amp-source-stack {"type":"plugin","name":"amp","function":"{closure}","hook":"outer_action"}-->',
'<!--amp-source-stack {"type":"plugin","name":"amp","function":"{closure}","hook":"inner_action"}-->',
'<b>Hello</b>',
'<!--/amp-source-stack {"type":"plugin","name":"amp","function":"{closure}","hook":"inner_action"}-->',
'<!--/amp-source-stack {"type":"plugin","name":"amp","function":"{closure}","hook":"outer_action"}-->',
) ),
$output
);
}

/**
Expand Down

0 comments on commit 6d5fcc1

Please sign in to comment.