From 41d99afaf7f58da924a928f712bf429db4ccba6f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 14 Oct 2019 22:10:12 -0700 Subject: [PATCH 01/26] Include hook priority in source information --- includes/validation/class-amp-validation-manager.php | 7 ++++--- .../php/validation/test-class-amp-validation-manager.php | 8 ++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index f68f24f4f95..64ced775b13 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -1255,9 +1255,10 @@ public static function wrap_hook_callbacks( $hook ) { continue; } - $source['hook'] = $hook; - $original_function = $callback['function']; - $wrapped_callback = self::wrapped_callback( + $source['hook'] = $hook; + $source['priority'] = $priority; + $original_function = $callback['function']; + $wrapped_callback = self::wrapped_callback( array_merge( $callback, compact( 'priority', 'source' ) diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index c2ed4ef9182..017fb240b82 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -1056,11 +1056,11 @@ public function test_callback_wrappers() { implode( '', [ - '', - '', + '', + '', 'Hello', - '', - '', + '', + '', ] ), $output From 977ecd4b5328633901b48fe4662524ea15ff8cc3 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 14 Oct 2019 22:55:27 -0700 Subject: [PATCH 02/26] Include relative file name and line number in source information --- .../class-amp-validation-manager.php | 32 +++++++++++++------ .../test-class-amp-validation-manager.php | 24 ++++++++++---- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 64ced775b13..353eaf8b6dc 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -1239,6 +1239,11 @@ public static function wrap_hook_callbacks( $hook ) { continue; } + /** + * Reflection. + * + * @var ReflectionFunctionAbstract $reflection + */ $reflection = $source['reflection']; unset( $source['reflection'] ); // Omit from stored source. @@ -1446,22 +1451,31 @@ public static function get_source( $callback ) { 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 ) ) { + $slug_pattern = '(?[^/]+)'; + if ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WP_PLUGIN_DIR ) ), ':' ) . $slug_pattern . '/(?P.*$):s', $file, $matches ) ) { $source['type'] = 'plugin'; - $source['name'] = $matches[1]; - } elseif ( preg_match( ':' . preg_quote( trailingslashit( self::$template_directory ), ':' ) . $slug_pattern . ':s', $file ) ) { + $source['name'] = $matches['slug']; + $source['file'] = $matches['file']; + } elseif ( preg_match( ':' . preg_quote( trailingslashit( self::$template_directory ), ':' ) . '(?P.*$):s', $file, $matches ) ) { $source['type'] = 'theme'; $source['name'] = self::$template_slug; - } elseif ( preg_match( ':' . preg_quote( trailingslashit( self::$stylesheet_directory ), ':' ) . $slug_pattern . ':s', $file ) ) { + $source['file'] = $matches['file']; + } elseif ( preg_match( ':' . preg_quote( trailingslashit( self::$stylesheet_directory ), ':' ) . '/(?P.*$):s', $file, $matches ) ) { $source['type'] = 'theme'; $source['name'] = self::$stylesheet_slug; - } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WPMU_PLUGIN_DIR ) ), ':' ) . $slug_pattern . ':s', $file, $matches ) ) { + $source['file'] = $matches['file']; + } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WPMU_PLUGIN_DIR ) ), ':' ) . $slug_pattern . '/(?P.*$):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['name'] = $matches['slug']; + $source['file'] = $matches['file']; + } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( ABSPATH ) ), ':' ) . '(?Pwp-admin|wp-includes)/(?P.*$):s', $file, $matches ) ) { $source['type'] = 'core'; - $source['name'] = $matches[1]; + $source['name'] = $matches['slug']; + $source['file'] = $matches['file']; + } + + if ( isset( $source['file'] ) ) { + $source['line'] = $reflection->getStartLine(); } } diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 017fb240b82..1390d92f124 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -816,10 +816,16 @@ public function get_block_data() { 'latest_posts' => [ '', sprintf( - '', + '', $is_gutenberg ? 'plugin' : 'core', $is_gutenberg ? 'gutenberg' : 'wp-includes', - $latest_posts_block->render_callback + $latest_posts_block->render_callback, + wp_json_encode( + $is_gutenberg + ? preg_replace( ':.*gutenberg/:', '', $reflection_function->getFileName() ) + : preg_replace( ':.*wp-includes/:', '', $reflection_function->getFileName() ) + ), + $reflection_function->getStartLine() ), [ 'element' => 'ul', @@ -906,6 +912,8 @@ public function test_add_block_source_comments( $content, $expected, $query ) { * @covers AMP_Validation_Manager::wrap_widget_callbacks() */ public function test_wrap_widget_callbacks() { + $this->markTestIncomplete( 'Need to deal with file and line.' ); + global $wp_registered_widgets, $_wp_sidebars_widgets; $widget_id = 'search-2'; @@ -1042,11 +1050,13 @@ public function test_callback_wrappers() { do_action( 'inner_action' ); $that->assertEquals( 10, has_action( 'inner_action', $handle_inner_action ) ); }; + $outer_reflection = new ReflectionFunction( $handle_outer_action ); $handle_inner_action = static 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 'Hello'; }; + $inner_reflection = new ReflectionFunction( $handle_inner_action ); add_action( 'outer_action', $handle_outer_action ); add_action( 'inner_action', $handle_inner_action ); AMP_Theme_Support::start_output_buffering(); @@ -1056,11 +1066,11 @@ public function test_callback_wrappers() { implode( '', [ - '', - '', + sprintf( '', $outer_reflection->getStartLine() ), + sprintf( '', $inner_reflection->getStartLine() ), 'Hello', - '', - '', + sprintf( '', $inner_reflection->getStartLine() ), + sprintf( '', $outer_reflection->getStartLine() ), ] ), $output @@ -1090,6 +1100,8 @@ public function test_has_parameters_passed_by_reference() { * @covers AMP_Validation_Manager::decorate_filter_source() */ public function test_decorate_shortcode_and_filter_source() { + $this->markTestIncomplete( 'Need to figure out what to do with the file and line information.' ); + AMP_Validation_Manager::add_validation_error_sourcing(); add_shortcode( 'test', From fb8c82c10d4a1174fd9fa7e67012e110b103c2c3 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 16 Oct 2019 12:36:04 -0700 Subject: [PATCH 03/26] Add special case for sourcing WP_Widget::display_callback --- .../class-amp-validation-manager.php | 73 +++++++++---------- .../test-class-amp-validation-manager.php | 61 ++++++++++++---- 2 files changed, 79 insertions(+), 55 deletions(-) diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 353eaf8b6dc..4d9b4681940 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -1405,7 +1405,6 @@ public static function decorate_filter_source( $value ) { public static function get_source( $callback ) { $reflection = null; $class_name = null; // Because ReflectionMethod::getDeclaringClass() can return a parent class. - $file = null; try { if ( is_string( $callback ) && is_callable( $callback ) ) { // The $callback is a function or static method. @@ -1424,21 +1423,17 @@ public static function get_source( $callback ) { $class_name = get_class( $callback[0] ); } - /* - * Obtain file from ReflectionClass because if the method is not on base class then - * file returned by ReflectionMethod will be for the base class not the subclass. - */ - $reflection = new ReflectionClass( $callback[0] ); - $file = $reflection->getFileName(); - // This is needed later for AMP_Validation_Manager::has_parameters_passed_by_reference(). $reflection = new ReflectionMethod( $callback[0], $callback[1] ); + + // Handle the special case of the class being a widget, in which case the display_callback method should + // actually map to the underling widget method. It is the display_callback in the end that is wrapped. + if ( 'WP_Widget' === $reflection->getDeclaringClass()->getName() && 'display_callback' === $reflection->getName() ) { + $reflection = new ReflectionMethod( $callback[0], 'widget' ); + } } elseif ( is_object( $callback ) && ( 'Closure' === get_class( $callback ) ) ) { $reflection = new ReflectionFunction( $callback ); } - if ( $reflection && ! $file ) { - $file = $reflection->getFileName(); - } } catch ( Exception $e ) { return null; } @@ -1449,34 +1444,34 @@ public static function get_source( $callback ) { $source = compact( 'reflection' ); - if ( $file ) { - $file = wp_normalize_path( $file ); - $slug_pattern = '(?[^/]+)'; - if ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WP_PLUGIN_DIR ) ), ':' ) . $slug_pattern . '/(?P.*$):s', $file, $matches ) ) { - $source['type'] = 'plugin'; - $source['name'] = $matches['slug']; - $source['file'] = $matches['file']; - } elseif ( preg_match( ':' . preg_quote( trailingslashit( self::$template_directory ), ':' ) . '(?P.*$):s', $file, $matches ) ) { - $source['type'] = 'theme'; - $source['name'] = self::$template_slug; - $source['file'] = $matches['file']; - } elseif ( preg_match( ':' . preg_quote( trailingslashit( self::$stylesheet_directory ), ':' ) . '/(?P.*$):s', $file, $matches ) ) { - $source['type'] = 'theme'; - $source['name'] = self::$stylesheet_slug; - $source['file'] = $matches['file']; - } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WPMU_PLUGIN_DIR ) ), ':' ) . $slug_pattern . '/(?P.*$):s', $file, $matches ) ) { - $source['type'] = 'mu-plugin'; - $source['name'] = $matches['slug']; - $source['file'] = $matches['file']; - } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( ABSPATH ) ), ':' ) . '(?Pwp-admin|wp-includes)/(?P.*$):s', $file, $matches ) ) { - $source['type'] = 'core'; - $source['name'] = $matches['slug']; - $source['file'] = $matches['file']; - } - - if ( isset( $source['file'] ) ) { - $source['line'] = $reflection->getStartLine(); - } + // Identify the type, name, and relative file path. + $file = wp_normalize_path( $reflection->getFileName() ); + $slug_pattern = '(?[^/]+)'; + if ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WP_PLUGIN_DIR ) ), ':' ) . $slug_pattern . '/(?P.*$):s', $file, $matches ) ) { + $source['type'] = 'plugin'; + $source['name'] = $matches['slug']; + $source['file'] = $matches['file']; + } elseif ( ! empty( $template_directory ) && preg_match( ':' . preg_quote( trailingslashit( c ), ':' ) . '(?P.*$):s', $file, $matches ) ) { + $source['type'] = 'theme'; + $source['name'] = self::$template_slug; + $source['file'] = $matches['file']; + } elseif ( ! empty( self::$stylesheet_directory ) && preg_match( ':' . preg_quote( trailingslashit( self::$stylesheet_directory ), ':' ) . '/(?P.*$):s', $file, $matches ) ) { + $source['type'] = 'theme'; + $source['name'] = self::$stylesheet_slug; + $source['file'] = $matches['file']; + } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WPMU_PLUGIN_DIR ) ), ':' ) . $slug_pattern . '/(?P.*$):s', $file, $matches ) ) { + $source['type'] = 'mu-plugin'; + $source['name'] = $matches['slug']; + $source['file'] = $matches['file']; + } elseif ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( ABSPATH ) ), ':' ) . '(?Pwp-admin|wp-includes)/(?P.*$):s', $file, $matches ) ) { + $source['type'] = 'core'; + $source['name'] = $matches['slug']; + $source['file'] = $matches['file']; + } + + // If a file was identified, then also supply the line number. + if ( isset( $source['file'] ) ) { + $source['line'] = $reflection->getStartLine(); } if ( $class_name ) { diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 1390d92f124..ef2b4e0d432 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -912,20 +912,25 @@ public function test_add_block_source_comments( $content, $expected, $query ) { * @covers AMP_Validation_Manager::wrap_widget_callbacks() */ public function test_wrap_widget_callbacks() { - $this->markTestIncomplete( 'Need to deal with file and line.' ); - global $wp_registered_widgets, $_wp_sidebars_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] ); - $this->assertSame( 'display_callback', $wp_registered_widgets[ $widget_id ]['callback'][1] ); + $search_widget_id = 'search-2'; + $this->assertArrayHasKey( $search_widget_id, $wp_registered_widgets ); + $this->assertInternalType( 'array', $wp_registered_widgets[ $search_widget_id ]['callback'] ); + $this->assertInstanceOf( 'WP_Widget_Search', $wp_registered_widgets[ $search_widget_id ]['callback'][0] ); + $this->assertSame( 'display_callback', $wp_registered_widgets[ $search_widget_id ]['callback'][1] ); + $archives_widget_id = 'archives-2'; + $this->assertArrayHasKey( $archives_widget_id, $wp_registered_widgets ); + $this->assertInternalType( 'array', $wp_registered_widgets[ $archives_widget_id ]['callback'] ); + $wp_registered_widgets[ $archives_widget_id ]['callback'][0] = new AMP_Widget_Archives(); AMP_Validation_Manager::wrap_widget_callbacks(); - $this->assertInstanceOf( 'AMP_Validation_Callback_Wrapper', $wp_registered_widgets[ $widget_id ]['callback'] ); - $this->assertInstanceOf( 'WP_Widget', $wp_registered_widgets[ $widget_id ]['callback'][0] ); - $this->assertSame( 'display_callback', $wp_registered_widgets[ $widget_id ]['callback'][1] ); + $this->assertInstanceOf( 'AMP_Validation_Callback_Wrapper', $wp_registered_widgets[ $search_widget_id ]['callback'] ); + $this->assertInstanceOf( 'AMP_Validation_Callback_Wrapper', $wp_registered_widgets[ $archives_widget_id ]['callback'] ); + $this->assertInstanceOf( 'WP_Widget', $wp_registered_widgets[ $search_widget_id ]['callback'][0] ); + $this->assertInstanceOf( 'WP_Widget', $wp_registered_widgets[ $archives_widget_id ]['callback'][0] ); + $this->assertSame( 'display_callback', $wp_registered_widgets[ $search_widget_id ]['callback'][1] ); + $this->assertSame( 'display_callback', $wp_registered_widgets[ $archives_widget_id ]['callback'][1] ); $sidebar_id = 'amp-sidebar'; register_sidebar( @@ -934,18 +939,42 @@ public function test_wrap_widget_callbacks() { 'after_widget' => '', ] ); - $_wp_sidebars_widgets[ $sidebar_id ] = [ $widget_id ]; + // Test core search widget. + $_wp_sidebars_widgets[ $sidebar_id ] = [ $search_widget_id ]; AMP_Theme_Support::start_output_buffering(); dynamic_sidebar( $sidebar_id ); - $output = ob_get_clean(); - + $output = ob_get_clean(); + $reflection = new ReflectionMethod( 'WP_Widget_Search', 'widget' ); $this->assertStringStartsWith( - '
  • getFileName() ) ), + $reflection->getStartLine(), + wp_json_encode( $reflection->getDeclaringClass()->getName() . '::' . $reflection->getName() ), + wp_json_encode( $search_widget_id ) + ), + $output + ); + $this->assertRegExp( + '#
    • ', + + // Test plugin-extended archives widget. + $_wp_sidebars_widgets[ $sidebar_id ] = [ $archives_widget_id ]; + AMP_Theme_Support::start_output_buffering(); + dynamic_sidebar( $sidebar_id ); + $output = ob_get_clean(); + $reflection = new ReflectionMethod( 'AMP_Widget_Archives', 'widget' ); + $this->assertStringStartsWith( + sprintf( + '
  • getFileName() ) ), + $reflection->getStartLine(), + wp_json_encode( $reflection->getDeclaringClass()->getName() . '::' . $reflection->getName() ), + wp_json_encode( $archives_widget_id ) + ), $output ); } From 4e0993ae5a797c84b43f0c4d593f918e3ee6cad7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 16 Oct 2019 13:35:24 -0700 Subject: [PATCH 04/26] Refactor test_decorate_shortcode_and_filter_source Also fix VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable and keyboard typo --- .../class-amp-validation-manager.php | 2 +- .../test-class-amp-validation-manager.php | 226 +++++++++++++++++- 2 files changed, 218 insertions(+), 10 deletions(-) diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 4d9b4681940..75ac3c25d11 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -1451,7 +1451,7 @@ public static function get_source( $callback ) { $source['type'] = 'plugin'; $source['name'] = $matches['slug']; $source['file'] = $matches['file']; - } elseif ( ! empty( $template_directory ) && preg_match( ':' . preg_quote( trailingslashit( c ), ':' ) . '(?P.*$):s', $file, $matches ) ) { + } elseif ( ! empty( self::$template_directory ) && preg_match( ':' . preg_quote( trailingslashit( self::$template_directory ), ':' ) . '(?P.*$):s', $file, $matches ) ) { $source['type'] = 'theme'; $source['name'] = self::$template_slug; $source['file'] = $matches['file']; diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index ef2b4e0d432..a66ef14bda4 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -1127,33 +1127,241 @@ public function test_has_parameters_passed_by_reference() { * @covers AMP_Validation_Manager::add_validation_error_sourcing() * @covers AMP_Validation_Manager::decorate_shortcode_source() * @covers AMP_Validation_Manager::decorate_filter_source() + * @throws Exception If assertion fails. */ public function test_decorate_shortcode_and_filter_source() { - $this->markTestIncomplete( 'Need to figure out what to do with the file and line information.' ); - AMP_Validation_Manager::add_validation_error_sourcing(); + $shortcode_fallback = static function() { + return 'test'; + }; add_shortcode( 'test', - static function() { - return 'test'; - } + $shortcode_fallback ); $filtered_content = apply_filters( 'the_content', 'before[test]after' ); if ( version_compare( get_bloginfo( 'version' ), '5.0', '>=' ) && has_filter( 'the_content', 'do_blocks' ) ) { - $source_json = '{"hook":"the_content","filter":true,"sources":[{"type":"core","name":"wp-includes","function":"WP_Embed::run_shortcode"},{"type":"core","name":"wp-includes","function":"WP_Embed::autoembed"},{"type":"plugin","name":"amp","function":"AMP_Validation_Manager::add_block_source_comments"},{"type":"core","name":"wp-includes","function":"do_blocks"},{"type":"core","name":"wp-includes","function":"wptexturize"},{"type":"core","name":"wp-includes","function":"wpautop"},{"type":"core","name":"wp-includes","function":"shortcode_unautop"},{"type":"core","name":"wp-includes","function":"prepend_attachment"},{"type":"core","name":"wp-includes","function":"wp_make_content_images_responsive"},{"type":"core","name":"wp-includes","function":"capital_P_dangit"},{"type":"core","name":"wp-includes","function":"do_shortcode"},{"type":"core","name":"wp-includes","function":"convert_smilies"}]}'; + $sources = [ + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'WP_Embed::run_shortcode', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'WP_Embed::autoembed', + ], + [ + 'type' => 'plugin', + 'name' => 'amp', + 'function' => 'AMP_Validation_Manager::add_block_source_comments', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'do_blocks', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'wptexturize', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'wpautop', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'shortcode_unautop', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'prepend_attachment', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'wp_make_content_images_responsive', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'capital_P_dangit', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'do_shortcode', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'convert_smilies', + ], + ]; } elseif ( has_filter( 'the_content', 'do_blocks' ) ) { - $source_json = '{"hook":"the_content","filter":true,"sources":[{"type":"plugin","name":"amp","function":"AMP_Validation_Manager::add_block_source_comments"},{"type":"plugin","name":"gutenberg","function":"do_blocks"},{"type":"core","name":"wp-includes","function":"WP_Embed::run_shortcode"},{"type":"core","name":"wp-includes","function":"WP_Embed::autoembed"},{"type":"core","name":"wp-includes","function":"wptexturize"},{"type":"core","name":"wp-includes","function":"wpautop"},{"type":"core","name":"wp-includes","function":"shortcode_unautop"},{"type":"core","name":"wp-includes","function":"prepend_attachment"},{"type":"core","name":"wp-includes","function":"wp_make_content_images_responsive"},{"type":"core","name":"wp-includes","function":"capital_P_dangit"},{"type":"core","name":"wp-includes","function":"do_shortcode"},{"type":"core","name":"wp-includes","function":"convert_smilies"}]}'; + $sources = [ + [ + 'type' => 'plugin', + 'name' => 'amp', + 'function' => 'AMP_Validation_Manager::add_block_source_comments', + ], + [ + 'type' => 'plugin', + 'name' => 'gutenberg', + 'function' => 'do_blocks', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'WP_Embed::run_shortcode', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'WP_Embed::autoembed', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'wptexturize', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'wpautop', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'shortcode_unautop', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'prepend_attachment', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'wp_make_content_images_responsive', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'capital_P_dangit', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'do_shortcode', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'convert_smilies', + ], + ]; } else { - $source_json = '{"hook":"the_content","filter":true,"sources":[{"type":"core","name":"wp-includes","function":"WP_Embed::run_shortcode"},{"type":"core","name":"wp-includes","function":"WP_Embed::autoembed"},{"type":"core","name":"wp-includes","function":"wptexturize"},{"type":"core","name":"wp-includes","function":"wpautop"},{"type":"core","name":"wp-includes","function":"shortcode_unautop"},{"type":"core","name":"wp-includes","function":"prepend_attachment"},{"type":"core","name":"wp-includes","function":"wp_make_content_images_responsive"},{"type":"core","name":"wp-includes","function":"capital_P_dangit"},{"type":"core","name":"wp-includes","function":"do_shortcode"},{"type":"core","name":"wp-includes","function":"convert_smilies"}]}'; + $sources = [ + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'WP_Embed::run_shortcode', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'WP_Embed::autoembed', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'wptexturize', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'wpautop', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'shortcode_unautop', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'prepend_attachment', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'wp_make_content_images_responsive', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'capital_P_dangit', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'do_shortcode', + ], + [ + 'type' => 'core', + 'name' => 'wp-includes', + 'function' => 'convert_smilies', + ], + ]; } + foreach ( $sources as &$source ) { + $function = $source['function']; + unset( $source['function'] ); + if ( strpos( $function, '::' ) ) { + $method = explode( '::', $function, 2 ); + $reflection = new ReflectionMethod( $method[0], $method[1] ); + } else { + $reflection = new ReflectionFunction( $function ); + } + + if ( 'core' === $source['type'] ) { + $source['file'] = preg_replace( ':.*/' . preg_quote( $source['name'], ':' ) . '/:', '', $reflection->getFileName() ); + } elseif ( 'plugin' === $source['type'] ) { + $source['file'] = preg_replace( ':.*/' . preg_quote( basename( WP_PLUGIN_DIR ), ':' ) . '/[^/]+?/:', '', $reflection->getFileName() ); + } else { + throw new Exception( 'Unexpected type: ' . $source['type'] ); + } + $source['line'] = $reflection->getStartLine(); + $source['function'] = $function; + } + + $source_json = wp_json_encode( + [ + 'hook' => 'the_content', + 'filter' => true, + 'sources' => $sources, + ] + ); + + $shortcode_fallback_reflection = new ReflectionFunction( $shortcode_fallback ); + $expected_content = implode( '', [ "", - '

    beforetestafter

    ' . "\n", + sprintf( + '

    beforetestafter

    ' . "\n", + wp_json_encode( substr( $shortcode_fallback_reflection->getFileName(), strlen( AMP__DIR__ ) + 1 ) ), + $shortcode_fallback_reflection->getStartLine() + ), "", ] ); From e317de2bfec8c5abe3b3b2873f225276f49f29a9 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 16 Oct 2019 16:07:09 -0700 Subject: [PATCH 05/26] Improve display of validation error details, including informational messages --- .../css/src/amp-validation-error-taxonomy.css | 29 +- .../src/amp-validation-single-error-url.css | 12 +- .../class-amp-validation-error-taxonomy.php | 249 ++++++++++++------ ...st-class-amp-validation-error-taxonomy.php | 4 +- 4 files changed, 200 insertions(+), 94 deletions(-) diff --git a/assets/css/src/amp-validation-error-taxonomy.css b/assets/css/src/amp-validation-error-taxonomy.css index d6c91eaa3a7..290ce0e0c01 100644 --- a/assets/css/src/amp-validation-error-taxonomy.css +++ b/assets/css/src/amp-validation-error-taxonomy.css @@ -78,7 +78,34 @@ details.single-error-detail[open] .single-error-detail-summary::after { .notice .detailed details .detailed { padding-left: 32px; - font-family: Consolas, Monaco, monospace; +} + +dl.detailed dt { + font-weight: bold; + margin-bottom: 0.5em; +} +dl.detailed dd + dt { + margin-top: 1em; +} + +dl.detailed .element-attributes th { + font-weight: bold; + text-align: right; +} +dl.detailed .element-attributes th:after { + content: ":"; +} +dl.detailed .element-attributes th, +dl.detailed .element-attributes td { + padding: 2px; +} + +dl.detailed a { + text-decoration: underline; +} + +dl.detailed { + padding-bottom: 16px; } .details-attributes__title code, diff --git a/assets/css/src/amp-validation-single-error-url.css b/assets/css/src/amp-validation-single-error-url.css index ca000ca3db6..c3a38dfcf56 100644 --- a/assets/css/src/amp-validation-single-error-url.css +++ b/assets/css/src/amp-validation-single-error-url.css @@ -63,19 +63,9 @@ table.striped > tbody > tr.even { cursor: pointer; } -.details ul.detailed { - padding: 0 32px; - margin-top: 0; -} - -.details div.detailed { +.details dl.detailed { padding-left: 30px; margin-top: 10px; - font-family: Consolas, Monaco, monospace; -} - -.details .detailed details { - padding-bottom: 16px; } .details .detailed summary code { diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index c1c995f2b1b..6dee454527c 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -1753,11 +1753,11 @@ public static function filter_manage_custom_columns( $content, $column_name, $te '', $filtered_content ); + $this->assertStringStartsWith( $initial_content . '
    • + + + + Date: Wed, 16 Oct 2019 22:01:08 -0700 Subject: [PATCH 10/26] Jump to the requested line when opening the file editor --- .../class-amp-validation-error-taxonomy.php | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index af5a288f12a..03b0c67a183 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -964,6 +964,36 @@ static function( $primary_column ) { return $primary_column; } ); + + // Jump to the requested line when opening the file editor. + add_action( + 'admin_enqueue_scripts', + function ( $hook_suffix ) { + if ( ! isset( $_GET['line'] ) ) { + return; + } + $line = (int) $_GET['line']; + + if ( 'plugin-editor.php' === $hook_suffix || 'theme-editor.php' === $hook_suffix ) { + wp_add_inline_script( + 'wp-theme-plugin-editor', + sprintf( + ' + ( + function( originalInitCodeEditor ) { + wp.themePluginEditor.initCodeEditor = function init() { + originalInitCodeEditor.apply( this, arguments ); + this.instance.codemirror.doc.setCursor( %d - 1 ); + }; + } + )( wp.themePluginEditor.initCodeEditor ); + ', + wp_json_encode( $line ) + ) + ); + } + } + ); } /** From b99ba62b8f189f595fef1f9637b603868ae17a61 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 16 Oct 2019 22:13:34 -0700 Subject: [PATCH 11/26] Fix CSS linting errors --- assets/css/src/amp-validation-error-taxonomy.css | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/assets/css/src/amp-validation-error-taxonomy.css b/assets/css/src/amp-validation-error-taxonomy.css index 9399a4ab1b6..2d6300d3cf5 100644 --- a/assets/css/src/amp-validation-error-taxonomy.css +++ b/assets/css/src/amp-validation-error-taxonomy.css @@ -84,6 +84,7 @@ dl.detailed dt { font-weight: bold; margin-bottom: 0.5em; } + dl.detailed dd + dt { margin-top: 1em; } @@ -92,9 +93,11 @@ dl.detailed .element-attributes th { font-weight: bold; text-align: right; } -dl.detailed .element-attributes th:after { + +dl.detailed .element-attributes th::after { content: ":"; } + dl.detailed .element-attributes th, dl.detailed .element-attributes td { padding: 2px; @@ -349,6 +352,7 @@ body.taxonomy-amp_validation_error .wp-list-table .new th.check-column input { margin: 0; padding: 6px 0; } + .validation-error-sources > li:first-child { border-top: 0; } From 98bba39c0e8064bd1777a9e44477ce00c1ac968e Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 16 Oct 2019 22:18:12 -0700 Subject: [PATCH 12/26] Ignore WordPress.Security.NonceVerification.Recommended --- includes/validation/class-amp-validation-error-taxonomy.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 03b0c67a183..7012767a423 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -969,10 +969,10 @@ static function( $primary_column ) { add_action( 'admin_enqueue_scripts', function ( $hook_suffix ) { - if ( ! isset( $_GET['line'] ) ) { + if ( ! isset( $_GET['line'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended return; } - $line = (int) $_GET['line']; + $line = (int) $_GET['line']; // phpcs:ignore WordPress.Security.NonceVerification.Recommended if ( 'plugin-editor.php' === $hook_suffix || 'theme-editor.php' === $hook_suffix ) { wp_add_inline_script( From 04c2f40b119745d287af2ccfb8580b0b0aa11c78 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 16 Oct 2019 22:31:36 -0700 Subject: [PATCH 13/26] Skip including AMP_Validation_Manager::add_block_source_comments in sources --- .../validation/class-amp-validation-error-taxonomy.php | 2 +- includes/validation/class-amp-validation-manager.php | 5 +++++ .../validation/test-class-amp-validation-manager.php | 10 ---------- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 7012767a423..5f1ed0f89e1 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2518,7 +2518,7 @@ public static function get_source_key_label( $key, $validation_error ) { if ( self::INVALID_ATTRIBUTE_CODE === $validation_error['code'] ) { return __( 'Attribute name', 'amp' ); } elseif ( self::INVALID_ELEMENT_CODE === $validation_error['code'] ) { - return __( 'Element name', 'amp' ); + return __( 'Element name', 'amp' ); } else { return __( 'Node name', 'amp' ); } diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index da0027a63c7..d233694c866 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -1239,6 +1239,11 @@ public static function wrap_hook_callbacks( $hook ) { continue; } + // Skip considering ourselves. + if ( 'AMP_Validation_Manager::add_block_source_comments' === $source['function'] ) { + continue; + } + /** * Reflection. * diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index a66ef14bda4..8a0b2db460a 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -1153,11 +1153,6 @@ public function test_decorate_shortcode_and_filter_source() { 'name' => 'wp-includes', 'function' => 'WP_Embed::autoembed', ], - [ - 'type' => 'plugin', - 'name' => 'amp', - 'function' => 'AMP_Validation_Manager::add_block_source_comments', - ], [ 'type' => 'core', 'name' => 'wp-includes', @@ -1206,11 +1201,6 @@ public function test_decorate_shortcode_and_filter_source() { ]; } elseif ( has_filter( 'the_content', 'do_blocks' ) ) { $sources = [ - [ - 'type' => 'plugin', - 'name' => 'amp', - 'function' => 'AMP_Validation_Manager::add_block_source_comments', - ], [ 'type' => 'plugin', 'name' => 'gutenberg', From 2e99772f9c1992a42fd5f7cb40f5f9eb74c381f7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 17 Oct 2019 13:12:54 -0700 Subject: [PATCH 14/26] Improve presentation of boolean attributes Co-Authored-By: Pierre Gordon <16200219+pierlon@users.noreply.github.com> --- assets/css/src/amp-validation-error-taxonomy.css | 2 +- includes/validation/class-amp-validation-error-taxonomy.php | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/assets/css/src/amp-validation-error-taxonomy.css b/assets/css/src/amp-validation-error-taxonomy.css index 2d6300d3cf5..3fbd639e887 100644 --- a/assets/css/src/amp-validation-error-taxonomy.css +++ b/assets/css/src/amp-validation-error-taxonomy.css @@ -94,7 +94,7 @@ dl.detailed .element-attributes th { text-align: right; } -dl.detailed .element-attributes th::after { +dl.detailed .element-attributes th.has-attr-value::after { content: ":"; } diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 5f1ed0f89e1..ac1c453238e 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2125,7 +2125,8 @@ public static function render_single_url_error_details( $validation_error, $term $attr_value ) : ?> %s', esc_html( $attr_name ) ); + $attr_name_class = empty( $attr_value ) ? '' : 'has-attr-value'; + printf( '%2$s', esc_attr( $attr_name_class ), esc_html( $attr_name ) ); echo ''; if ( ! empty( $attr_value ) ) { printf( '%s', esc_html( $attr_value ) ); From dfd87bb4d8c9b5049abd2e3502d0f7a55b1ea98d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 17 Oct 2019 13:14:03 -0700 Subject: [PATCH 15/26] Improve translatability of strings Co-Authored-By: Pascal Birchler --- .../class-amp-validation-error-taxonomy.php | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index ac1c453238e..0bbe41d8b25 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2045,7 +2045,19 @@ public static function render_single_url_error_details( $validation_error, $term <script> tags unless they are for loading AMP components (which the AMP plugin will add for you automatically). In order for a page to be served as AMP, the invalid JS code must be removed from the page, which is what happens when you accept sanitization. Learn more about how AMP works. As an alternative to using custom JS, please consider using a pre-built AMP functionality, including actions and events (as opposed to JS event handler attributes like onclick) and the amp-bind component; you may also add custom JS if encapsulated in the amp-script.', 'amp' ) + sprintf( + /* translators: 1: script, 2: Documentation URL, 3: Documentation URL, 4: Documentation URL, 5: onclick, 6: Documentation URL, 7: amp-bind, 8: Documentation URL, 9: amp-script */ + __( 'Arbitrary JavaScript is not allowed in AMP. You cannot use JS %1$s tags unless they are for loading AMP components (which the AMP plugin will add for you automatically). In order for a page to be served as AMP, the invalid JS code must be removed from the page, which is what happens when you accept sanitization. Learn more about how AMP works. As an alternative to using custom JS, please consider using a pre-built AMP functionality, including actions and events (as opposed to JS event handler attributes like %5$s) and the %7$s component; you may also add custom JS if encapsulated in the %9$s.', 'amp' ), + '<script>', + 'https://amp.dev/documentation/components/', + 'https://amp.dev/about/how-amp-works/', + 'https://amp.dev/documentation/guides-and-tutorials/learn/amp-actions-and-events/', + 'onclick', + 'https://amp.dev/documentation/components/amp-bind/', + 'amp-bind', + 'https://amp.dev/documentation/components/amp-script/', + 'amp-script' + ) ) ?> @@ -2057,7 +2069,12 @@ public static function render_single_url_error_details( $validation_error, $term AMP HTML specification. If an element or attribute is not allowed in AMP, it must be removed for the page to cached and be eligible for prerendering.', 'amp' ) + sprintf( + /* translators: 1: Documentation URL, 2: Documentation URL. */ + __( 'AMP has specific set of allowed elements and attributes that are allowed in valid AMP pages. Learn about the AMP HTML specification. If an element or attribute is not allowed in AMP, it must be removed for the page to cached and be eligible for prerendering.', 'amp' ), + 'https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/', + 'https://amp.dev/documentation/guides-and-tutorials/learn/amp-caches-and-cors/how_amp_pages_are_cached/' + ) ) ?> @@ -2203,7 +2220,7 @@ private static function render_sources( $sources ) { $source_count = count( $sources ); echo esc_html( sprintf( - /* translators: %s is the number */ + /* translators: %s: number of sources. */ _n( '%s source', '%s sources', From 499d279ef69e5166bfa4dae30b2e720093ac2fbb Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 17 Oct 2019 09:33:03 -0700 Subject: [PATCH 16/26] Eliminate square brackets for attributes in favor of angle brackets for tags --- includes/validation/class-amp-validated-url-post-type.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index a4dceecc390..661b99e3a20 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -998,18 +998,18 @@ public static function output_custom_column( $column_name, $post_id ) { if ( ! empty( $error_summary[ AMP_Validation_Error_Taxonomy::REMOVED_ELEMENTS ] ) ) { foreach ( $error_summary[ AMP_Validation_Error_Taxonomy::REMOVED_ELEMENTS ] as $name => $count ) { if ( 1 === (int) $count ) { - $items[] = sprintf( '%s', esc_html( $name ) ); + $items[] = sprintf( '<%s>', esc_html( $name ) ); } else { - $items[] = sprintf( '%s (%d)', esc_html( $name ), $count ); + $items[] = sprintf( '<%s> (%d)', esc_html( $name ), $count ); } } } if ( ! empty( $error_summary[ AMP_Validation_Error_Taxonomy::REMOVED_ATTRIBUTES ] ) ) { foreach ( $error_summary[ AMP_Validation_Error_Taxonomy::REMOVED_ATTRIBUTES ] as $name => $count ) { if ( 1 === (int) $count ) { - $items[] = sprintf( '[%s]', esc_html( $name ) ); + $items[] = sprintf( '%s', esc_html( $name ) ); } else { - $items[] = sprintf( '[%s] (%d)', esc_html( $name ), $count ); + $items[] = sprintf( '%s (%d)', esc_html( $name ), $count ); } } } From 82a87a696e0687e8d663c23a3e256ec4a4d91f49 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 17 Oct 2019 21:41:01 -0700 Subject: [PATCH 17/26] Fix phpcs linting issues --- includes/validation/class-amp-validation-error-taxonomy.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 4878ac783b4..f4d398e8f45 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2079,7 +2079,7 @@ public static function render_single_url_error_details( $validation_error, $term /* translators: 1: Documentation URL, 2: Documentation URL. */ __( 'AMP has specific set of allowed elements and attributes that are allowed in valid AMP pages. Learn about the AMP HTML specification. If an element or attribute is not allowed in AMP, it must be removed for the page to cached and be eligible for prerendering.', 'amp' ), 'https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/', - 'https://amp.dev/documentation/guides-and-tutorials/learn/amp-caches-and-cors/how_amp_pages_are_cached/' + 'https://amp.dev/documentation/guides-and-tutorials/learn/amp-caches-and-cors/how_amp_pages_are_cached/' ) ) ?> From 5b77a645bfcb3af5d5815e4cc826fcc59b03627d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 17 Oct 2019 21:53:01 -0700 Subject: [PATCH 18/26] Swap status column with error type column; rename Details to Context --- .../amp-validated-url-post-edit-screen.js | 2 +- .../class-amp-validated-url-post-type.php | 20 +++++++++---------- .../class-amp-validation-error-taxonomy.php | 4 ++-- ...test-class-amp-validated-url-post-type.php | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/assets/src/amp-validation/amp-validated-url-post-edit-screen.js b/assets/src/amp-validation/amp-validated-url-post-edit-screen.js index 0703a64469c..edb1807655a 100644 --- a/assets/src/amp-validation/amp-validated-url-post-edit-screen.js +++ b/assets/src/amp-validation/amp-validated-url-post-edit-screen.js @@ -245,7 +245,7 @@ const handleSearching = () => { const detailsQuery = document.querySelectorAll( 'tbody .column-details' ); /* - * Iterate through the 'Details' column of each row. + * Iterate through the 'Context' (formerly 'Details') column of each row. * If the search query is not present, hide the row. */ let numberErrorsDisplaying = 0; diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index a8ffd664b0e..3fe5d384bef 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -934,30 +934,30 @@ public static function add_single_post_columns() { return [ 'cb' => '', 'error' => __( 'Error', 'amp' ), - 'status' => sprintf( + 'error_type' => __( 'Type', 'amp' ), + 'details' => sprintf( '%s', - esc_html__( 'Status', 'amp' ), + esc_html__( 'Context', 'amp' ), esc_attr( sprintf( '

    %s

    %s

    ', - esc_html__( 'Status', 'amp' ), - esc_html__( 'An accepted validation error is one that will not block a URL from being served as AMP; the validation error will be sanitized, normally resulting in the offending markup being stripped from the response to ensure AMP validity.', 'amp' ) + esc_html__( 'Context', 'amp' ), + esc_html__( 'The parent element of where the error occurred.', 'amp' ) ) ) ), - 'details' => sprintf( + 'sources_with_invalid_output' => __( 'Sources', 'amp' ), + 'status' => sprintf( '%s', - esc_html__( 'Details', 'amp' ), + esc_html__( 'Status', 'amp' ), esc_attr( sprintf( '

    %s

    %s

    ', - esc_html__( 'Details', 'amp' ), - esc_html__( 'The parent element of where the error occurred.', 'amp' ) + esc_html__( 'Status', 'amp' ), + esc_html__( 'An accepted validation error is one that will not block a URL from being served as AMP; the validation error will be sanitized, normally resulting in the offending markup being stripped from the response to ensure AMP validity.', 'amp' ) ) ) ), - 'sources_with_invalid_output' => __( 'Sources', 'amp' ), - 'error_type' => __( 'Type', 'amp' ), ]; } diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index f4d398e8f45..eadcefb38b6 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -831,11 +831,11 @@ static function( $old_columns ) { ), 'details' => sprintf( '%s', - esc_html__( 'Details', 'amp' ), + esc_html__( 'Context', 'amp' ), esc_attr( sprintf( '

    %s

    %s

    ', - esc_html__( 'Details', 'amp' ), + esc_html__( 'Context', 'amp' ), esc_html__( 'The parent element of where the error occurred.', 'amp' ) ) ) diff --git a/tests/php/validation/test-class-amp-validated-url-post-type.php b/tests/php/validation/test-class-amp-validated-url-post-type.php index d5c9884d838..173465ab36a 100644 --- a/tests/php/validation/test-class-amp-validated-url-post-type.php +++ b/tests/php/validation/test-class-amp-validated-url-post-type.php @@ -558,7 +558,7 @@ public function test_add_single_post_columns() { 'cb' => '', 'error' => 'Error', 'status' => 'Status', - 'details' => 'Details', + 'details' => 'Context', 'sources_with_invalid_output' => 'Sources', 'error_type' => 'Type', ], From dcb00f7144865efcb789d95909d4f563f1571151 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 17 Oct 2019 22:19:39 -0700 Subject: [PATCH 19/26] Allow validation error source file edit link to be filtered --- .../class-amp-validation-error-taxonomy.php | 33 +++++++++++++++---- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index eadcefb38b6..a8310756d07 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2242,12 +2242,11 @@ private static function render_sources( $sources ) { rawurlencode( $plugin['name'] ), 'file' => rawurlencode( $file ), @@ -2268,7 +2267,7 @@ private static function render_sources( $sources ) { ); } } elseif ( 'theme' === $source['type'] && current_user_can( 'edit_themes' ) ) { - $file_link = add_query_arg( + $edit_url = add_query_arg( [ 'file' => rawurlencode( $file ), 'theme' => rawurlencode( $source['name'] ), @@ -2278,6 +2277,22 @@ private static function render_sources( $sources ) { ); } } + + /** + * Filters the URL for opening the file to edit. + * + * Users of IDEs that support opening files in via web protocols can use this filter to override + * the edit link to result in their editor opening rather than the theme/plugin editor. + * + * @since 1.4 + * + * @param string|null $edit_url Edit URL for the file. Line number with colon will be appended to end. Null when user cannot access theme/plugin editor. + * @param array $source Source information. + */ + $edit_url = apply_filters( 'amp_validation_error_source_file_edit_url', $edit_url, $source ); + + // Prevent duplicated info. + unset( $source['file'], $source['line'] ); } ?>
  • @@ -2304,11 +2319,15 @@ private static function render_sources( $sources ) { file: - - + + - + From b4779dae6db31c32fc8d026dd31dd7efdfecdafb Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 17 Oct 2019 22:42:03 -0700 Subject: [PATCH 20/26] Prevent status dropdown from wrapping after status icon --- .../src/amp-validation-single-error-url.css | 5 +++ .../class-amp-validation-error-taxonomy.php | 40 ++++++++++--------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/assets/css/src/amp-validation-single-error-url.css b/assets/css/src/amp-validation-single-error-url.css index c3a38dfcf56..f97f4472b41 100644 --- a/assets/css/src/amp-validation-single-error-url.css +++ b/assets/css/src/amp-validation-single-error-url.css @@ -149,7 +149,12 @@ table.striped > tbody > tr.even { border-right: 1px solid #a0a5aa; } +.amp-validation-error-status-dropdown { + display: flex; +} + .amp-validation-error-status { + margin-left: 5px; width: auto; float: none; } diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index a8310756d07..99bbfdb22b7 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -1851,29 +1851,31 @@ public static function filter_manage_custom_columns( $content, $column_name, $te ob_start(); ?> - - - + term_group || self::VALIDATION_ERROR_NEW_REJECTED_STATUS === $term->term_group ) : ?> term_group ) : ?> - - + + - - - - + + + Date: Fri, 18 Oct 2019 10:20:47 -0700 Subject: [PATCH 21/26] Fix phpcs complaints regarding indentation --- .../class-amp-validation-error-taxonomy.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 99bbfdb22b7..29a109cc660 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2321,13 +2321,17 @@ private static function render_sources( $sources ) { file: - - - + ', + // Note that esc_attr() used instead of esc_url() to allow IDE protocols. + esc_attr( $edit_url ), + // Open link in new window unless the user has filtered the URL to open their system IDE. + in_array( wp_parse_url( $edit_url, PHP_URL_SCHEME ), [ 'http:', 'https:' ], true ) ? 'target="_blank"' : '' + ); + } + ?> From e6cbc5225bcf58e21d0123f1c0030e00c90dfd3b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 18 Oct 2019 11:26:38 -0700 Subject: [PATCH 22/26] Introduce file editor URL template and filter for mapping to host machine --- .../class-amp-validation-error-taxonomy.php | 177 +++++++++++++----- 1 file changed, 127 insertions(+), 50 deletions(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 29a109cc660..aa0558e1ff4 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2215,6 +2215,131 @@ public static function get_plugin_from_slug( $plugin_slug ) { return null; } + /** + * Get the URL for opening the file for a AMP validation error in an external editor. + * + * @since 1.4 + * + * @param array $source Source for AMP validation error. + * @return string|null File editor URL or null if not available. + */ + private static function get_file_editor_url( $source ) { + if ( ! isset( $source['file'], $source['line'], $source['type'], $source['name'] ) ) { + return null; + } + + $edit_url = null; + + /** + * Filters the template for the URL for linking to an external editor to open a file for editing. + * + * Users of IDEs that support opening files in via web protocols can use this filter to override + * the edit link to result in their editor opening rather than the theme/plugin editor. + * + * The initial filtered value is null, requiring extension plugins to supply the URL template + * string themselves. If no template string is provided, links to the theme/plugin editors will + * be provided if available. For example, for an extension plugin to cause file edit links to + * open in PhpStorm, the following filter can be used: + * + * add_filter( 'amp_validation_error_source_file_editor_url_template', function () { + * return 'phpstorm://open?file={{file}}&line={{line}}'; + * } ); + * + * For a template to be considered, the string '{{file}}' must be present in the filtered value. + * + * @since 1.4 + * + * @param string|null $editor_url_template Editor URL template. + */ + $editor_url_template = apply_filters( 'amp_validation_error_source_file_editor_url_template', null ); + + // Supply the file path to the editor template. + if ( null !== $editor_url_template && false !== strpos( $editor_url_template, '{{file}}' ) ) { + $file_path = null; + if ( 'core' === $source['type'] ) { + if ( 'wp-includes' === $source['name'] ) { + $file_path = ABSPATH . WPINC . '/' . $source['file']; + } elseif ( 'wp-admin' === $source['name'] ) { + $file_path = ABSPATH . 'wp-admin/' . $source['file']; + } + } elseif ( 'plugin' === $source['type'] ) { + $file_path = WP_PLUGIN_DIR . '/' . $source['name']; + if ( $source['name'] !== $source['file'] ) { + $file_path .= '/' . $source['file']; + } + } elseif ( 'mu-plugin' === $source['type'] ) { + $file_path = WPMU_PLUGIN_DIR . '/' . $source['name']; + } elseif ( 'theme' === $source['type'] ) { + $theme = wp_get_theme( $source['name'] ); + if ( $theme instanceof WP_Theme && ! $theme->errors() ) { + $file_path = $theme->get_stylesheet_directory() . '/' . $source['file']; + } + } + + if ( $file_path && file_exists( $file_path ) ) { + /** + * Filters the file path to be opened in an external editor for a given AMP validation error source. + * + * This is useful to map the file path from inside of a Docker container or VM to the host machine. + * + * @since 1.4 + * + * @param string|null $editor_url_template Editor URL template. + * @param array $source Source information. + */ + $file_path = apply_filters( 'amp_validation_error_source_file_path', $file_path, $source ); + if ( $file_path ) { + $edit_url = str_replace( + [ + '{{file}}', + '{{line}}', + ], + [ + rawurlencode( $file_path ), + rawurlencode( $source['line'] ), + ], + $editor_url_template + ); + } + } + } + + // Fall back to using the theme/plugin editors if no external editor is offered. + if ( ! $edit_url ) { + if ( 'plugin' === $source['type'] && current_user_can( 'edit_plugins' ) ) { + $plugin = self::get_plugin_from_slug( $source['name'] ); + if ( $plugin ) { + $file = $source['file']; + + // Prepend the plugin directory name to the file name as the plugin editor requires. + if ( false !== strpos( $plugin['name'], '/' ) ) { + $file = strtok( $plugin['name'], '/' ) . '/' . $file; + } + + $edit_url = add_query_arg( + [ + 'plugin' => rawurlencode( $plugin['name'] ), + 'file' => rawurlencode( $file ), + 'line' => rawurlencode( $source['line'] ), + ], + admin_url( 'plugin-editor.php' ) + ); + } + } elseif ( 'theme' === $source['type'] && current_user_can( 'edit_themes' ) ) { + $edit_url = add_query_arg( + [ + 'file' => rawurlencode( $source['file'] ), + 'theme' => rawurlencode( $source['name'] ), + 'line' => rawurlencode( $source['line'] ), + ], + admin_url( 'theme-editor.php' ) + ); + } + } + + return $edit_url; + } + /** * Render sources. * @@ -2244,56 +2369,9 @@ private static function render_sources( $sources ) { rawurlencode( $plugin['name'] ), - 'file' => rawurlencode( $file ), - 'line' => rawurlencode( $line ), - ], - admin_url( 'plugin-editor.php' ) - ); - } - } elseif ( 'theme' === $source['type'] && current_user_can( 'edit_themes' ) ) { - $edit_url = add_query_arg( - [ - 'file' => rawurlencode( $file ), - 'theme' => rawurlencode( $source['name'] ), - 'line' => rawurlencode( $line ), - ], - admin_url( 'theme-editor.php' ) - ); - } - } - - /** - * Filters the URL for opening the file to edit. - * - * Users of IDEs that support opening files in via web protocols can use this filter to override - * the edit link to result in their editor opening rather than the theme/plugin editor. - * - * @since 1.4 - * - * @param string|null $edit_url Edit URL for the file. Line number with colon will be appended to end. Null when user cannot access theme/plugin editor. - * @param array $source Source information. - */ - $edit_url = apply_filters( 'amp_validation_error_source_file_edit_url', $edit_url, $source ); - - // Prevent duplicated info. + $file_text = $source['file'] . ':' . $source['line']; unset( $source['file'], $source['line'] ); } ?> @@ -2340,7 +2418,6 @@ private static function render_sources( $sources ) { -
    • From 413d41b9abfceba4f2c71e8c318dd753c31460dc Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 18 Oct 2019 12:17:49 -0700 Subject: [PATCH 23/26] Improve display of source information with icons and translated strings --- .../class-amp-validation-error-taxonomy.php | 145 +++++++++++++++++- 1 file changed, 141 insertions(+), 4 deletions(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index aa0558e1ff4..5f735370efa 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2340,6 +2340,39 @@ private static function get_file_editor_url( $source ) { return $edit_url; } + /** + * Render source name. + * + * @since 1.4 + * + * @param string $name Name. + * @param string $type Type. + */ + private static function render_source_name( $name, $type ) { + $nicename = null; + switch ( $type ) { + case 'theme': + $theme = wp_get_theme( $name ); + if ( ! $theme->errors() ) { + $nicename = $theme->get( 'Name' ); + } + break; + case 'plugin': + $plugin = self::get_plugin_from_slug( $name ); + if ( $plugin && ! empty( $plugin['data']['Name'] ) ) { + $nicename = $plugin['data']['Name']; + } + break; + } + echo ' '; + + if ( $nicename ) { + printf( '%s (%s)', esc_html( $nicename ), esc_html( $name ) ); + } else { + echo '' . esc_html( $name ) . ''; + } + } + /** * Render sources. * @@ -2355,8 +2388,8 @@ private static function render_sources( $sources ) { sprintf( /* translators: %s: number of sources. */ _n( - '%s source', - '%s sources', + '%s possible source', + '%s possible sources', $source_count, 'amp' ), @@ -2374,17 +2407,121 @@ private static function render_sources( $sources ) { $file_text = $source['file'] . ':' . $source['line']; unset( $source['file'], $source['line'] ); } + $is_filter = ! empty( $source['filter'] ); + unset( $source['filter'] ); + + $priority = null; + if ( isset( $source['priority'] ) ) { + $priority = $source['priority']; + unset( $source['priority'] ); + } + ?>
  • $value ) : ?>
    - : + + + '; + esc_html_e( 'Theme', 'amp' ); + break; + case 'plugin': + echo ' '; + esc_html_e( 'Plugin', 'amp' ); + break; + case 'mu-plugin': + echo ' '; + esc_html_e( 'Must-Use Plugin', 'amp' ); + break; + case 'core': + echo ' '; + esc_html_e( 'Core', 'amp' ); + break; + default: + echo esc_html( (string) $value ); + } + ?> + + + + + + + + + + + labels->singular_name ) ) { + echo esc_html( $post_type->labels->singular_name ); + printf( ' (%s)', esc_html( $value ) ); + } else { + printf( '%s', esc_html( $value ) ); + } + ?> @@ -2396,7 +2533,7 @@ private static function render_sources( $sources ) {
    - file: + : Date: Fri, 18 Oct 2019 12:26:56 -0700 Subject: [PATCH 24/26] Keep track of whether a handle is a script or a style --- .../class-amp-validation-callback-wrapper.php | 12 ++++++++++-- .../class-amp-validation-error-taxonomy.php | 16 ++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/includes/validation/class-amp-validation-callback-wrapper.php b/includes/validation/class-amp-validation-callback-wrapper.php index 552c9d23049..2ad17236b56 100644 --- a/includes/validation/class-amp-validation-callback-wrapper.php +++ b/includes/validation/class-amp-validation-callback-wrapper.php @@ -72,7 +72,11 @@ public function __invoke() { // Keep track of which source enqueued the styles. if ( isset( $wp_styles, $wp_styles->queue ) ) { foreach ( array_diff( $wp_styles->queue, $before_styles_enqueued ) as $handle ) { - AMP_Validation_Manager::$enqueued_style_sources[ $handle ][] = array_merge( $this->callback['source'], compact( 'handle' ) ); + AMP_Validation_Manager::$enqueued_style_sources[ $handle ][] = array_merge( + $this->callback['source'], + [ 'dependency_type' => 'style' ], + compact( 'handle' ) + ); } } @@ -87,7 +91,11 @@ public function __invoke() { } foreach ( $handles as $handle ) { - AMP_Validation_Manager::$enqueued_script_sources[ $handle ][] = array_merge( $this->callback['source'], compact( 'handle' ) ); + AMP_Validation_Manager::$enqueued_script_sources[ $handle ][] = array_merge( + $this->callback['source'], + [ 'dependency_type' => 'script' ], + compact( 'handle' ) + ); } } } diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 5f735370efa..ec90964e79f 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2410,6 +2410,12 @@ private static function render_sources( $sources ) { $is_filter = ! empty( $source['filter'] ); unset( $source['filter'] ); + $dependency_type = null; + if ( isset( $source['dependency_type'] ) ) { + $dependency_type = $source['dependency_type']; + unset( $source['dependency_type'] ); + } + $priority = null; if ( isset( $source['priority'] ) ) { $priority = $source['priority']; @@ -2434,7 +2440,13 @@ private static function render_sources( $sources ) { esc_html_e( 'Post Type', 'amp' ); break; case 'handle': - esc_html_e( 'Handle', 'amp' ); + if ( 'script' === $dependency_type ) { + esc_html_e( 'Script Handle', 'amp' ); + } elseif ( 'style' === $dependency_type ) { + esc_html_e( 'Style Handle', 'amp' ); + } else { + esc_html_e( 'Handle', 'amp' ); + } break; case 'block_content_index': esc_html_e( 'Block Index', 'amp' ); @@ -2510,7 +2522,7 @@ private static function render_sources( $sources ) { ?> - + Date: Fri, 18 Oct 2019 14:43:49 -0700 Subject: [PATCH 25/26] Prepare CSS informational message for translation --- .../validation/class-amp-validation-error-taxonomy.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index ec90964e79f..5327de7b046 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2071,7 +2071,13 @@ public static function render_single_url_error_details( $validation_error, $term style your pages using CSS in much the same way as regular HTML pages, however there are some restrictions. Nevertheless, the AMP plugin automatically inlines external stylesheets, transforms !important qualifiers, and uses tree shaking to remove the majority of CSS rules that do not apply to the current page. Nevertheless, AMP does have a 50KB limit and tree shaking cannot always reduce the amount of CSS under this limit; when this happens an excessive CSS error will result.', 'amp' ) + sprintf( + /* translators: 1: Documentation URL, 2: Documentation URL, 3: !important */ + __( 'AMP allows you to style your pages using CSS in much the same way as regular HTML pages, however there are some restrictions. Nevertheless, the AMP plugin automatically inlines external stylesheets, transforms %3$s qualifiers, and uses tree shaking to remove the majority of CSS rules that do not apply to the current page. Nevertheless, AMP does have a 50KB limit and tree shaking cannot always reduce the amount of CSS under this limit; when this happens an excessive CSS error will result.', 'amp' ), + 'https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/', + 'https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/style_pages/', + '!important' + ) ) ?> From 1b1e1dc88c47742d1e810249cd6173cbc5a47cd3 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 18 Oct 2019 14:49:34 -0700 Subject: [PATCH 26/26] Prevent duplicating char findiing; add parens; fix scheme check --- .../validation/class-amp-validation-error-taxonomy.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 5327de7b046..39147fdaaff 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -2137,7 +2137,7 @@ public static function render_single_url_error_details( $validation_error, $term $value ) : ?>