From 5033910569b6e8268fc435d9dd7c3c21fe7636ec Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 9 Mar 2018 17:10:45 -0800 Subject: [PATCH 1/2] Wrap registered widgets in callbacks to track widget validation errors * Include core as a source in addition to plugins and themes. * Refactor source comments to only consist of JSON-encoded source data. * Include the function name in the source. --- includes/utils/class-amp-validation-utils.php | 199 +++++++++--------- tests/test-class-amp-validation-utils.php | 155 +++++++------- 2 files changed, 186 insertions(+), 168 deletions(-) diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index 2ba61273427..c1e4d61f24d 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -597,47 +597,33 @@ public static function print_edit_form_validation_status( $post ) { /** * Get source start comment. * - * @param string $type Extension type. - * @param string $name Extension name. - * @param array $args Args. + * @param array $source Source data. + * @param bool $is_start Whether the comment is the start or end. * @return string HTML Comment. */ - public static function get_source_comment_start( $type, $name, $args = array() ) { - $args_encoded = wp_json_encode( $args ); - if ( '[]' === $args_encoded ) { - $args_encoded = '{}'; - } - return sprintf( '', $type, $name, str_replace( '--', '', $args_encoded ) ); - } - - /** - * Get source end comment. - * - * @param string $type Extension type. - * @param string $name Extension name. - * @return string HTML Comment. - */ - public static function get_source_comment_end( $type, $name ) { - return sprintf( '', $type, $name ); + public static function get_source_comment( array $source, $is_start = true ) { + return sprintf( + '', + $is_start ? '' : '/', + str_replace( '--', '', wp_json_encode( $source ) ) + ); } /** * Parse source comment. * * @param DOMComment $comment Comment. - * @return array|null Source info or null if not a source comment. + * @return array|null Parsed source or null if not a source comment. */ public static function parse_source_comment( DOMComment $comment ) { - if ( ! preg_match( '#^\s*(?P/)?amp-source-stack:(?Ptheme|plugin|mu-plugin):(?P\S+)(?: (?P{.+}))?\s*$#s', $comment->nodeValue, $matches ) ) { + if ( ! preg_match( '#^\s*(?P/)?amp-source-stack\s+(?P{.+})\s*$#s', $comment->nodeValue, $matches ) ) { return null; } - $source = wp_array_slice_assoc( $matches, array( 'type', 'name' ) ); - $source['closing'] = ! empty( $matches['closing'] ); - if ( isset( $matches['args'] ) ) { - $source['args'] = json_decode( $matches['args'], true ); - } - return $source; + $source = json_decode( $matches['args'], true ); + $closing = ! empty( $matches['closing'] ); + + return compact( 'source', 'closing' ); } /** @@ -653,17 +639,17 @@ public static function parse_source_comment( DOMComment $comment ) { */ public static function locate_sources( DOMNode $node ) { $xpath = new DOMXPath( $node->ownerDocument ); - $comments = $xpath->query( 'preceding::comment()[ contains( ., "amp-source-stack:" ) ]', $node ); + $comments = $xpath->query( 'preceding::comment()[ starts-with( ., "amp-source-stack" ) or starts-with( ., "/amp-source-stack" ) ]', $node ); $sources = array(); foreach ( $comments as $comment ) { - $source = self::parse_source_comment( $comment ); - if ( $source ) { - if ( $source['closing'] ) { - array_pop( $sources ); - } else { - unset( $source['closing'] ); - $sources[] = $source; - } + $parsed_comment = self::parse_source_comment( $comment ); + if ( ! $parsed_comment ) { + continue; + } + if ( $parsed_comment['closing'] ) { + array_pop( $sources ); + } else { + $sources[] = $parsed_comment['source']; } } return $sources; @@ -677,7 +663,7 @@ public static function locate_sources( DOMNode $node ) { public static function remove_source_comments( $dom ) { $xpath = new DOMXPath( $dom ); $comments = array(); - foreach ( $xpath->query( '//comment()[ contains( ., "amp-source-stack:" ) ]' ) as $comment ) { + foreach ( $xpath->query( '//comment()[ starts-with( ., "amp-source-stack" ) or starts-with( ., "/amp-source-stack" ) ]' ) as $comment ) { if ( self::parse_source_comment( $comment ) ) { $comments[] = $comment; } @@ -700,20 +686,22 @@ public static function remove_source_comments( $dom ) { public static function callback_wrappers() { global $wp_filter; $pending_wrap_callbacks = array(); - foreach ( $wp_filter as $filter_tag => $wp_hook ) { + + // @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_data = self::get_source( $callback['function'] ); - if ( isset( $source_data ) ) { - $pending_wrap_callbacks[ $filter_tag ][] = array_merge( - $callback, - $source_data, - array( - 'hook' => $filter_tag, - ), - compact( 'priority' ) - ); + $source = self::get_source( $callback['function'] ); + if ( ! $source ) { + continue; } + $source['hook'] = $hook; + + $pending_wrap_callbacks[ $hook ][] = array_merge( + $callback, + compact( 'priority', 'source', 'hook' ) + ); } } } @@ -726,6 +714,23 @@ public static function callback_wrappers() { add_action( $hook, $wrapped_callback, $callback['priority'], $callback['accepted_args'] ); } } + + // Wrap widgets callbacks. + global $wp_registered_widgets; + foreach ( $wp_registered_widgets as $widget_id => &$registered_widget ) { + $source = self::get_source( $registered_widget['callback'] ); + if ( ! $source ) { + continue; + } + $source['widget_id'] = $widget_id; + + $function = $registered_widget['callback']; + $accepted_args = 2; // For the $instance and $args arguments. + $callback = compact( 'function', 'accepted_args', 'source' ); + + $registered_widget['callback'] = self::wrapped_callback( $callback ); + } + } /** @@ -747,10 +752,12 @@ public static function decorate_shortcode_source( $output, $tag ) { if ( empty( $source ) ) { return $output; } + $source['shortcode'] = $tag; + $output = implode( '', array( - self::get_source_comment_start( $source['type'], $source['name'], array( 'shortcode' => $tag ) ), + self::get_source_comment( $source, true ), $output, - self::get_source_comment_end( $source['type'], $source['name'] ), + self::get_source_comment( $source, false ), ) ); return $output; } @@ -767,18 +774,26 @@ public static function decorate_shortcode_source( $output, $tag ) { * } */ public static function get_source( $callback ) { + $reflection = null; + $class_name = null; // Because ReflectionMethod::getDeclaringClass() can return a parent class. try { if ( is_string( $callback ) && is_callable( $callback ) ) { // The $callback is a function or static method. - $exploded_callback = explode( '::', $callback ); - if ( count( $exploded_callback ) > 1 ) { - $reflection = new ReflectionClass( $exploded_callback[0] ); + $exploded_callback = explode( '::', $callback, 2 ); + if ( 2 === count( $exploded_callback ) ) { + $class_name = $exploded_callback[0]; + $reflection = new ReflectionMethod( $exploded_callback[0], $exploded_callback[1] ); } else { $reflection = new ReflectionFunction( $callback ); } } elseif ( is_array( $callback ) && isset( $callback[0], $callback[1] ) && method_exists( $callback[0], $callback[1] ) ) { // The $callback is a method. - $reflection = new ReflectionClass( $callback[0] ); + if ( is_string( $callback[0] ) ) { + $class_name = '\'' . $callback[0]; + } elseif ( is_object( $callback[0] ) ) { + $class_name = get_class( $callback[0] ); + } + $reflection = new ReflectionMethod( $callback[0], $callback[1] ); } elseif ( is_object( $callback ) && ( 'Closure' === get_class( $callback ) ) ) { $reflection = new ReflectionFunction( $callback ); } @@ -786,28 +801,38 @@ public static function get_source( $callback ) { return null; } - $file = isset( $reflection ) ? $reflection->getFileName() : null; - if ( ! isset( $file ) ) { + if ( ! $reflection ) { return null; } - $file = wp_normalize_path( $file ); - $slug_pattern = '([^/]+)'; - if ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WP_PLUGIN_DIR ) ), ':' ) . $slug_pattern . ':s', $file, $matches ) ) { - $type = 'plugin'; - $name = $matches[1]; - } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( get_theme_root() ) ), ':' ) . $slug_pattern . ':s', $file, $matches ) ) { - $type = 'theme'; - $name = $matches[1]; - } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WPMU_PLUGIN_DIR ) ), ':' ) . $slug_pattern . ':s', $file, $matches ) ) { - $type = 'mu-plugin'; - $name = $matches[1]; + $source = array(); + + $file = $reflection->getFileName(); + if ( $file ) { + $file = wp_normalize_path( $file ); + $slug_pattern = '([^/]+)'; + if ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WP_PLUGIN_DIR ) ), ':' ) . $slug_pattern . ':s', $file, $matches ) ) { + $source['type'] = 'plugin'; + $source['name'] = $matches[1]; + } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( get_theme_root() ) ), ':' ) . $slug_pattern . ':s', $file, $matches ) ) { + $source['type'] = 'theme'; + $source['name'] = $matches[1]; + } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WPMU_PLUGIN_DIR ) ), ':' ) . $slug_pattern . ':s', $file, $matches ) ) { + $source['type'] = 'mu-plugin'; + $source['name'] = $matches[1]; + } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( ABSPATH ) ), ':' ) . '(wp-admin|wp-includes)/:s', $file, $matches ) ) { + $source['type'] = 'core'; + $source['name'] = $matches[1]; + } } - if ( isset( $type, $name ) ) { - return compact( 'type', 'name' ); + if ( $class_name ) { + $source['function'] = $class_name . '::' . $reflection->getName(); + } else { + $source['function'] = $reflection->getName(); } - return null; + + return $source; } /** @@ -822,9 +847,7 @@ public static function get_source( $callback ) { * * @type callable $function * @type int $accepted_args - * @type string $type - * @type string $source - * @type string $hook + * @type array $source * } * @return closure $wrapped_callback The callback, wrapped in comments. */ @@ -852,32 +875,14 @@ public static function wrapped_callback( $callback ) { // Keep track of which source enqueued the styles. if ( isset( $wp_styles ) && isset( $wp_styles->queue ) ) { foreach ( array_diff( $wp_styles->queue, $before_styles_enqueued ) as $handle ) { - $source = array_merge( - wp_array_slice_assoc( $callback, array( 'type', 'name' ) ), - array( - 'args' => array( - 'hook' => $callback['hook'], - ), - ) - ); - - AMP_Validation_Utils::$enqueued_style_sources[ $handle ][] = $source; + AMP_Validation_Utils::$enqueued_style_sources[ $handle ][] = $callback['source']; } } // Keep track of which source enqueued the scripts, and immediately report validity . if ( isset( $wp_scripts ) && isset( $wp_scripts->queue ) ) { foreach ( array_diff( $wp_scripts->queue, $before_scripts_enqueued ) as $handle ) { - $source = array_merge( - wp_array_slice_assoc( $callback, array( 'type', 'name' ) ), - array( - 'args' => array( - 'hook' => $callback['hook'], - ), - ) - ); - - AMP_Validation_Utils::$enqueued_script_sources[ $handle ][] = $source; + AMP_Validation_Utils::$enqueued_script_sources[ $handle ][] = $callback['source']; if ( isset( $wp_scripts->registered[ $handle ] ) ) { self::add_validation_error( array( @@ -885,7 +890,7 @@ public static function wrapped_callback( $callback ) { 'handle' => $handle, 'dependency' => $wp_scripts->registered[ $handle ], 'sources' => array( - $source, + $callback['source'], ), ) ); } @@ -894,9 +899,9 @@ public static function wrapped_callback( $callback ) { // Wrap output that contains HTML tags (as opposed to actions that trigger in HTML attributes). if ( ! empty( $output ) && preg_match( '/<.+?>/s', $output ) ) { - echo AMP_Validation_Utils::get_source_comment_start( $callback['type'], $callback['name'], array( 'hook' => $callback['hook'] ) ); // WPCS: XSS ok. + echo AMP_Validation_Utils::get_source_comment( $callback['source'], true ); // WPCS: XSS ok. echo $output; // WPCS: XSS ok. - echo AMP_Validation_Utils::get_source_comment_end( $callback['type'], $callback['name'] ); // WPCS: XSS ok. + echo AMP_Validation_Utils::get_source_comment( $callback['source'], false ); // WPCS: XSS ok. } return $result; }; diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php index 13f889ce2bd..30dc686c5b2 100644 --- a/tests/test-class-amp-validation-utils.php +++ b/tests/test-class-amp-validation-utils.php @@ -283,19 +283,31 @@ public function test_print_edit_form_validation_status() { * * @covers AMP_Validation_Utils::locate_sources() * @covers AMP_Validation_Utils::parse_source_comment() - * @covers AMP_Validation_Utils::get_source_comment_start() - * @covers AMP_Validation_Utils::get_source_comment_end() + * @covers AMP_Validation_Utils::get_source_comment() * @covers AMP_Validation_Utils::remove_source_comments() */ public function test_source_comments() { + $source1 = array( + 'type' => 'plugin', + 'name' => 'foo', + 'shortcode' => 'test', + 'function' => __FUNCTION__, + ); + $source2 = array( + 'type' => 'theme', + 'name' => 'bar', + 'function' => __FUNCTION__, + 'hook' => 'something', + ); + $dom = AMP_DOM_Utils::get_dom_from_content( implode( '', array( - AMP_Validation_Utils::get_source_comment_start( 'plugin', 'foo', array( 'shortcode' => 'test' ) ), - AMP_Validation_Utils::get_source_comment_start( 'theme', 'bar', array( 'hook' => 'something' ) ), + AMP_Validation_Utils::get_source_comment( $source1, true ), + AMP_Validation_Utils::get_source_comment( $source2, true ), 'Test', - AMP_Validation_Utils::get_source_comment_end( 'theme', 'bar' ), - AMP_Validation_Utils::get_source_comment_end( 'plugin', 'foo' ), + AMP_Validation_Utils::get_source_comment( $source2, false ), + AMP_Validation_Utils::get_source_comment( $source1, false ), ) ) ); @@ -315,33 +327,21 @@ public function test_source_comments() { $this->assertInternalType( 'array', $sources ); $this->assertCount( 2, $sources ); - $expected = array( - 'type' => 'plugin', - 'name' => 'foo', - 'args' => array( - 'shortcode' => 'test', - ), - ); - $this->assertEquals( $expected, $sources[0] ); - $expected['closing'] = false; - $this->assertEquals( $expected, AMP_Validation_Utils::parse_source_comment( $comments[0] ) ); - $expected['closing'] = true; - unset( $expected['args'] ); - $this->assertEquals( $expected, AMP_Validation_Utils::parse_source_comment( $comments[3] ) ); - - $expected = array( - 'type' => 'theme', - 'name' => 'bar', - 'args' => array( - 'hook' => 'something', - ), - ); - $this->assertEquals( $expected, $sources[1] ); - $expected['closing'] = false; - $this->assertEquals( $expected, AMP_Validation_Utils::parse_source_comment( $comments[1] ) ); - $expected['closing'] = true; - unset( $expected['args'] ); - $this->assertEquals( $expected, AMP_Validation_Utils::parse_source_comment( $comments[2] ) ); + $this->assertEquals( $source1, $sources[0] ); + $parsed_comment = AMP_Validation_Utils::parse_source_comment( $comments[0] ); + $this->assertEquals( $source1, $parsed_comment['source'] ); + $this->assertFalse( $parsed_comment['closing'] ); + $parsed_comment = AMP_Validation_Utils::parse_source_comment( $comments[3] ); + $this->assertEquals( $source1, $parsed_comment['source'] ); + $this->assertTrue( $parsed_comment['closing'] ); + + $this->assertEquals( $source2, $sources[1] ); + $parsed_comment = AMP_Validation_Utils::parse_source_comment( $comments[1] ); + $this->assertEquals( $source2, $parsed_comment['source'] ); + $this->assertFalse( $parsed_comment['closing'] ); + $parsed_comment = AMP_Validation_Utils::parse_source_comment( $comments[2] ); + $this->assertEquals( $source2, $parsed_comment['source'] ); + $this->assertTrue( $parsed_comment['closing'] ); AMP_Validation_Utils::remove_source_comments( $dom ); $this->assertEquals( 0, $xpath->query( '//comment()' )->length ); @@ -356,7 +356,8 @@ public function test_callback_wrappers() { global $post; $post = $this->factory()->post->create_and_get(); // WPCS: global override ok. $this->set_capability(); - $action_non_plugin = 'foo_action'; + $action_no_tag_output = 'foo_action'; + $action_core_output = 'core_action'; $action_no_output = 'bar_action_no_output'; $action_function_callback = 'baz_action_function'; $action_no_argument = 'test_action_no_argument'; @@ -369,7 +370,8 @@ public function test_callback_wrappers() { add_action( $action_one_argument, array( $this, 'output_notice' ) ); add_action( $action_two_arguments, array( $this, 'output_message' ), 10, 2 ); add_action( $action_no_output, array( $this, 'get_string' ), 10, 2 ); - add_action( $action_non_plugin, 'the_ID' ); + add_action( $action_no_tag_output, 'the_ID' ); + add_action( $action_core_output, 'edit_post_link' ); add_action( $action_no_output, '__return_false' ); $this->assertEquals( 10, has_action( $action_no_argument, array( $this, 'output_div' ) ) ); @@ -378,7 +380,7 @@ public function test_callback_wrappers() { $_GET[ AMP_Validation_Utils::VALIDATE_QUERY_VAR ] = 1; AMP_Validation_Utils::callback_wrappers(); - $this->assertEquals( 10, has_action( $action_non_plugin, 'the_ID' ) ); + $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' ) ) ); @@ -388,23 +390,23 @@ public function test_callback_wrappers() { do_action( $action_function_callback ); $output = ob_get_clean(); $this->assertContains( '
', $output ); - $this->assertContains( 'testafter', + 'beforetestafter', do_shortcode( 'before[test]after' ) ); } @@ -451,16 +460,16 @@ public function test_decorate_shortcode_source() { /** * Test get_source * - * @covers AMP_Validation_Utils::print_edit_form_validation_status() + * @covers AMP_Validation_Utils::get_source() */ public function test_get_source() { - $plugin = AMP_Validation_Utils::get_source( 'amp_after_setup_theme' ); - $this->assertContains( 'amp', $plugin['name'] ); - $this->assertEquals( 'plugin', $plugin['type'] ); - $the_content = AMP_Validation_Utils::get_source( 'the_content' ); - $this->assertEquals( null, $the_content ); - $core_function = AMP_Validation_Utils::get_source( 'the_content' ); - $this->assertEquals( null, $core_function ); + $source = AMP_Validation_Utils::get_source( 'amp_after_setup_theme' ); + $this->assertEquals( 'amp', $source['name'] ); + $this->assertEquals( 'plugin', $source['type'] ); + + $source = AMP_Validation_Utils::get_source( 'the_content' ); + $this->assertEquals( 'wp-includes', $source['name'] ); + $this->assertEquals( 'core', $source['type'] ); } /** @@ -475,9 +484,11 @@ public function test_wrapped_callback() { echo $test_string; // WPCS: XSS OK. }, 'accepted_args' => 0, - 'type' => 'plugin', - 'name' => 'amp', - 'hook' => 'bar', + 'source' => array( + 'type' => 'plugin', + 'name' => 'amp', + 'hook' => 'bar', + ), ); $wrapped_callback = AMP_Validation_Utils::wrapped_callback( $callback ); @@ -488,15 +499,17 @@ public function test_wrapped_callback() { $this->assertEquals( 'Closure', get_class( $wrapped_callback ) ); $this->assertContains( $test_string, $output ); - $this->assertContains( '' . $this->disallowed_tag . '' ); + AMP_Validation_Utils::process_markup( '' . $this->disallowed_tag . '' ); $this->assertCount( 1, AMP_Validation_Utils::$validation_errors ); $this->assertEquals( 'script', AMP_Validation_Utils::$validation_errors[0]['node_name'] ); @@ -659,7 +672,7 @@ public function test_store_validation_errors() { AMP_Validation_Utils::reset_validation_results(); $url = home_url( '/?baz' ); - AMP_Validation_Utils::process_markup( '' . $this->disallowed_tag . '' ); + AMP_Validation_Utils::process_markup( '' . $this->disallowed_tag . '' ); $custom_post_id = AMP_Validation_Utils::store_validation_errors( AMP_Validation_Utils::$validation_errors, $url ); AMP_Validation_Utils::reset_validation_results(); $meta = get_post_meta( $post_id, AMP_Validation_Utils::AMP_URL_META, false ); @@ -668,7 +681,7 @@ public function test_store_validation_errors() { $this->assertContains( $url, $meta ); $url = home_url( '/?foo-bar' ); - AMP_Validation_Utils::process_markup( '' . $this->disallowed_tag . '' ); + AMP_Validation_Utils::process_markup( '' . $this->disallowed_tag . '' ); $custom_post_id = AMP_Validation_Utils::store_validation_errors( AMP_Validation_Utils::$validation_errors, $url ); AMP_Validation_Utils::reset_validation_results(); $meta = get_post_meta( $post_id, AMP_Validation_Utils::AMP_URL_META, false ); @@ -678,7 +691,7 @@ public function test_store_validation_errors() { $this->assertContains( $url, $meta ); AMP_Validation_Utils::reset_validation_results(); - AMP_Validation_Utils::process_markup( '' ); + AMP_Validation_Utils::process_markup( '' ); $custom_post_id = AMP_Validation_Utils::store_validation_errors( AMP_Validation_Utils::$validation_errors, $url ); AMP_Validation_Utils::reset_validation_results(); $error_post = get_post( $custom_post_id ); From 6d5fcc159ead6bb78d3f0a28d552592d2dba68f7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 10 Mar 2018 23:58:12 -0800 Subject: [PATCH 2/2] Re-work filter/action hook wrapping to happen just-in-time * 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. --- includes/utils/class-amp-validation-utils.php | 115 +++++++++++------- tests/test-class-amp-validation-utils.php | 102 ++++++++++++++-- 2 files changed, 166 insertions(+), 51 deletions(-) diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index c1e4d61f24d..9016a51a0d6 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -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' ) ); } @@ -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( '', $is_start ? '' : '/', @@ -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'] ); @@ -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; } /** @@ -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 ) { @@ -805,7 +838,7 @@ public static function get_source( $callback ) { return null; } - $source = array(); + $source = compact( 'reflection' ); $file = $reflection->getFileName(); if ( $file ) { diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php index 30dc686c5b2..fd0ec9e091b 100644 --- a/tests/test-class-amp-validation-utils.php +++ b/tests/test-class-amp-validation-utils.php @@ -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(); } /** @@ -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' => '', + ) ); + update_option( 'sidebars_widgets', array( + $sidebar_id => array( $widget_id ), + ) ); + + ob_start(); + dynamic_sidebar( $sidebar_id ); + $output = ob_get_clean(); + + $this->assertStringStartsWith( + '
  • assertStringEndsWith( + '
    • ', + $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'; @@ -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' ) ); @@ -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(); @@ -438,6 +492,34 @@ public function test_callback_wrappers() { $output = ob_get_clean(); $this->assertNotContains( '', + '', + 'Hello', + '', + '', + ) ), + $output + ); } /**