Skip to content
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

Omit validation errors sanitized by filter or tree-shaking option #1413

Merged
merged 4 commits into from Sep 9, 2018

Conversation

@westonruter
Copy link
Member

@westonruter westonruter commented Sep 9, 2018

Note: Tree-shaking option was eliminated in v1.2, per #2501.

When CSS tree-shaking is accepted by an options or by filter, the removed_unused_css_rules validation error is always being shown on an invalid URL screen. Similarly, when a theme is configured to forcibly accept sanitization for other errors, such as an illegal CSS rule, as in the core theme sanitizer:

https://github.com/Automattic/amp-wp/blob/60fe63749e273c938d6ab2375ff89725ed1f2389/includes/sanitizers/class-amp-core-theme-sanitizer.php#L191-L201

These are also being shown always for every invalid URL. This adds noise and creates confusion.

We should just omit such validation errors from ever being included in validation results so that they never show up. By omitting validation errors that are forcibly-accepted as sanitized by a filter, we avoid needlessly creating amp_validation_error taxonomy terms when they'll always be accepted anyway. This is particularly important for validation errors such as created by scripts with variable contents. Consider, for example, a template that does:

<script>
var serverRandom = <?php echo wp_json_encode( wp_rand() ); ?>;
</script>

(For a more common scenario, consider the case when a nonce is output.) You may have considered adding a filter to automatically accept this validation error:

add_filter( 'amp_validation_error_sanitized', function( $sanitized, $error ) {
	$is_random_script = (
		AMP_Validation_Error_Taxonomy::INVALID_ELEMENT_CODE === $error['code']
		&&
		'script' === $error['node_name']
		&&
		isset( $error['text'] )
		&&
		false !== strpos( $error['text'], 'var serverRandom' )
	);
	if ( $is_random_script ) {
		$sanitized = true;
	}
	return $sanitized;
}, 10, 2 );

However, what this results in is a new amp_validation_error term added to the database every time that the URL is re-validated. Since the text is random, then that validation error will become disassociated with the URL the next time it is checked, and another validation error will come and take its place. This repeats until many duplicated amp_validation_error terms fill up the database.

🎗 At some point we need to add a cronjob to delete amp_validation_error terms that have zero URLs associated with them.

So, the changes in this PR also prevent this from being a problem. Such filter-accepted validation errors will never see the light of day.

The changes in this PR extend the “staleness” determination beyond the active theme/plugins to include changes to whether tree shaking will be automatically accepted. In this way, if the user re-checks a URL after enabling auto tree-shaking then the validation error for tree shaking will no longer appear.

Before

image

After

image

@westonruter westonruter added this to the v1.0 milestone Sep 9, 2018
@westonruter westonruter requested review from amedina and kienstra Sep 9, 2018
@westonruter westonruter changed the title Update/validation error omission Omit validation errors sanitized by filter or tree-shaking option Sep 9, 2018
amedina
amedina approved these changes Sep 9, 2018
Copy link
Member

@amedina amedina left a comment

LGTM.
One comment: Let's an a backlog issue for " At some point we need to add a cronjob to delete amp_validation_error terms that have zero URLs associated with them."

@westonruter westonruter merged commit b3e1b4d into develop Sep 9, 2018
2 checks passed
westonruter added a commit that referenced this issue Sep 17, 2018
@swissspidy swissspidy deleted the update/validation-error-omission branch Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants