New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect attribution of theme as source for content validation errors #1467

Merged
merged 2 commits into from Sep 25, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Member

westonruter commented Sep 25, 2018

Given post_content such as:

See post on Make XWP:

https://make.xwp.co/2018/09/24/amp-plugin-release-v1-0-beta4/

This is something: <button onclick="ok">food</button>

The Invalid URL screen currently displays sources as the following (the New Rejected errors are from the content here):

image

Note how they are incorrectly being attributed to “Twenty Sixteen (?)”. This was due to a change I made in 3be5767 from #1449. But it is clearly not right. The four validation errors should all be contributed to the content, not guessing the theme.

This PR fixes that problem by listing the outermost hook running in the source stack, if available, and if no other source was obtained. So the result is this:

image

Note how each source now indicates the_content as the source.

@westonruter westonruter added this to the v1.0 milestone Sep 25, 2018

@westonruter westonruter requested a review from hellofromtonya Sep 25, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 25, 2018

In the future we could attempt to refine this further by specifically identifying embeds as sources.

WIP patch for this:

diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php
index 64a9402d..38a09606 100644
--- a/includes/validation/class-amp-validation-error-taxonomy.php
+++ b/includes/validation/class-amp-validation-error-taxonomy.php
@@ -706,6 +706,8 @@ class AMP_Validation_Error_Taxonomy {
 
 				if ( isset( $source['type'], $source['name'] ) ) {
 					$invalid_sources[ $source['type'] ][] = $source['name'];
+				} elseif ( isset( $source['embed'] ) ) {
+					$invalid_sources['embed'] = true;
 				}
 			}
 		}
diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php
index 4956934a..f905cda4 100644
--- a/includes/validation/class-amp-validation-manager.php
+++ b/includes/validation/class-amp-validation-manager.php
@@ -475,7 +475,8 @@ class AMP_Validation_Manager {
 			add_filter( $wrapped_filter, array( __CLASS__, 'decorate_filter_source' ), PHP_INT_MAX );
 		}
 
-		add_filter( 'do_shortcode_tag', array( __CLASS__, 'decorate_shortcode_source' ), -1, 2 );
+		add_filter( 'do_shortcode_tag', array( __CLASS__, 'decorate_shortcode_source' ), PHP_INT_MAX, 2 );
+		add_filter( 'embed_oembed_html', array( __CLASS__, 'decorate_embed_source' ), PHP_INT_MAX, 3 );
 
 		$do_blocks_priority  = has_filter( 'the_content', 'do_blocks' );
 		$is_gutenberg_active = (
@@ -1237,6 +1238,28 @@ class AMP_Validation_Manager {
 		return $output;
 	}
 
+	/**
+	 * Filters the output created by embeds.
+	 *
+	 * @since 1.0
+	 *
+	 * @param string $output Embed output.
+	 * @param string $url    URL.
+	 * @param array  $attr   Attributes.
+	 * @return string Output.
+	 */
+	public static function decorate_embed_source( $output, $url, $attr ) {
+		$source = array(
+			'embed' => $url,
+			'attr'  => $attr,
+		);
+		return implode( '', array(
+			self::get_source_comment( $source, true ),
+			trim( $output ),
+			self::get_source_comment( $source, false ),
+		) );
+	}
+
 	/**
 	 * Wraps output of a filter to add source stack comments.
 	 *

This doesn't actually work, at present, because the amp-iframe is getting moved outside the source stack comments:

<p><!--amp-source-stack {"embed":"https:\/\/make.xwp.co\/2018\/09\/24\/amp-plugin-release-v1-0-beta4\/","attr":{"width":840,"height":1000}}--></p>
<blockquote class="wp-embedded-content" data-secret="NFEzj31wEP"><p><a href="https://make.xwp.co/2018/09/24/amp-plugin-release-v1-0-beta4/">AMP Plugin Release v1.0-beta4</a></p></blockquote>
<p><!--/amp-source-stack {"embed":"https:\/\/make.xwp.co\/2018\/09\/24\/amp-plugin-release-v1-0-beta4\/","attr":{"width":840,"height":1000}}--></p>
<amp-iframe class="wp-embedded-content amp-wp-enforced-sizes amp-wp-5e9b0a2" sandbox="allow-scripts" src="https://make.xwp.co/2018/09/24/amp-plugin-release-v1-0-beta4/embed/#?secret=NFEzj31wEP" data-secret="NFEzj31wEP" width="600" height="338" title="“AMP Plugin Release v1.0-beta4” — Make XWP" frameborder="0" scrolling="no" layout="intrinsic"><div placeholder="" class="amp-wp-iframe-placeholder"></div></amp-iframe>

The culprit is:

https://github.com/Automattic/amp-wp/blob/c5ebe1f38ff58d58ee1cbdb735cd97bb14a316fe/includes/sanitizers/class-amp-iframe-sanitizer.php#L110-L112

Which was introduced in #112. It may not be relevant anymore.

We can follow-up on this later.

@amedina

LGTM.

@westonruter westonruter merged commit 227579d into develop Sep 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/embed-source branch Sep 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment