Improve handling of validation errors in response cache and admin display #1190

westonruter · 2018-06-01T06:00:43Z

Ensure response cache is varied by validation error sanitization status. If the response had been cached when all validation errors had marked as rejected (not-sanitized), then when one or more of those validation errors have their status changed to accepted (sanitized) then needs to make sure that the response cache will be updated. Otherwise, a validation error's status could change but no change would be reflected on the frontend.
Improve searchability of validation errors in invalid AMP URL posts by storing full text of validation error in the post_content, instead of just storing the sources. Note that this means the validation error data is duplicated in the amp_validation_error term description as well as in the post_content of the amp_invalid_url post type. This is not concerning because the size is small, the data will not be updated once written, and denormalization improves performance for searching.
Improve the “incompatible sources” summary so it shows more than just “core: wp-includes”.
Account for amp_validation_error_sanitized filter when displaying validation errors in admin. (And let second filter argument be flatter as just a $error array, instead of $context containing $error.) Prevent offering to change validation error status when filter is forcing a certain status. For example, if I want to make sure that invalid_attribute validation errors are always sanitized, I could add this:

add_filter( 'amp_validation_error_sanitized', function( $sanitized, $error ) {
	if ( $error['code'] === 'invalid_attribute' ) {
		$sanitized = true;
	}
	return $sanitized;
}, 10, 2 );

In the UI now, when looking at an invalid AMP URL post, this is indicated:

Likewise, in the list of validation errors, the checkboxes for doing bulk actions on such sanitization-forced errors are removed, as are the row actions:

…idation errors in admin Prevent offering to change validation error status when filter is forcing a certain status

westonruter · 2018-06-01T06:02:22Z

Unit tests are still needed for the validation classes, but let's not let that hold this up.

ThierryA · 2018-06-01T09:17:16Z

includes/validation/class-amp-invalid-url-post-type.php

 				continue;
 			}
-			$errors[] = array(
+			if ( $term && null !== $forced_sanitization ) {


The conditional statement on line 213 is strict conditioning against true while here it is against null, is that on purpose?

Yes. true means that the validation error is forced to be accepted (sanitized), whereasfalse means that the validation error is forced to be rejected (not sanitized). A null value means that there is no forcing because no filter is applying to force the sanitized state.

ThierryA · 2018-06-01T09:18:30Z

includes/validation/class-amp-invalid-url-post-type.php

 			}
-			if ( $args['ignore_accepted'] && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group ) {
+
+			if ( $args['ignore_accepted'] && ( true === $forced_sanitization || ( $term && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group ) ) ) {


Since $term->term_group is used in the conditional statement, it would be more bullet proof to check for isset( $term->term_group ) instead of just $term.

Actually, the $term->term_group property is always set because it is defined on the WP_Term class:

https://github.com/WordPress/wordpress-develop/blob/a26c24226c6b131a0ed22c722a836c100d3ba254/src/wp-includes/class-wp-term.php#L43-L49

ThierryA · 2018-06-01T09:26:24Z

includes/validation/class-amp-invalid-url-post-type.php

+
+			$forced_sanitization = null;
+			if ( $args['check_forced_sanitization'] ) {
+				$error = $stored_validation_error['data'];


Will throw PHP warning if $stored_validation_error['data'] isn't set. While probability is low, it would more bullet proof to check. In fact, this is what I got when reload a validation page before "Re-checking" it https://cloudup.com/cPPds8QsYVL.

You're right but you only had this problem because the data format of the post type is changed as of 3b484e8. Now that the validation errors are stored in the post type itself, we don't need to parse it out of the term as well. So it is expected that there would be a PHP warning as you observed. But it will go away once re-checking. Since we're in alpha I didn't think it was important to ensure back-compat for a previous alpha state.

ThierryA · 2018-06-01T10:04:14Z

includes/validation/class-amp-invalid-url-post-type.php

@@ -966,33 +968,49 @@ public static function print_validation_errors_meta_box( $post ) {
 		} );
 		</script>

+		<?php
+		$forced_sanitized_count = 0;


Since the notice is displayed if $forced_sanitized_count is bigger than 0, we could display the notice as soon as a $validation_error['forced_sanitization'] is found and break to avoid unnecessarily continuing the loop.

Originally I showed a different message when all of the sanitizations were forced vs some. So yes, this could break now.

ThierryA · 2018-06-01T10:05:17Z

includes/validation/class-amp-invalid-url-post-type.php

 		<div class="amp-validation-errors">
 			<ul>
 				<?php foreach ( $validation_errors as $error ) : ?>
 					<?php
 					$collapsed_details = array();
 					$term              = $error['term'];
 					$select_name       = sprintf( '%s[%s]', AMP_Validation_Manager::VALIDATION_ERROR_TERM_STATUS_QUERY_VAR, $term->slug );
+					$term_status       = $term->term_group;


Not sure I see the reason to store this in a variable.

You're right. It's no longer needed since the term_group is overridden in get_invalid_url_validation_errors.

ThierryA · 2018-06-01T10:06:48Z

includes/validation/class-amp-validation-error-taxonomy.php

@@ -155,11 +155,43 @@ public static function register() {
 			),
 		) );

+		add_filter( 'user_has_cap', array( __CLASS__, 'filter_user_has_cap_for_hiding_term_list_table_checkbox' ), 10, 3 );


Is this filter reset somewhere?

No. It should not be reset. This is a permanent filter for the user caps.

ThierryA · 2018-06-01T10:10:50Z

includes/validation/class-amp-validation-manager.php

+		$term = get_term_by( 'slug', $term_data['slug'], AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG );
+		if ( isset( self::$validation_error_status_overrides[ $term_data['slug'] ] ) ) {
+			$sanitized = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === self::$validation_error_status_overrides[ $term_data['slug'] ];
+		} elseif ( ! empty( $term ) && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group ) {


This could be simplified to:

// ... else { $sanitized = ! empty( $term ) && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group; }

ThierryA · 2018-06-01T10:11:39Z

includes/validation/class-amp-validation-manager.php

+		 * @param bool|null $sanitized Whether sanitized, or null if no default predetermined.
+		 * @param array     $error     Validation error being sanitized.
+		 */
+		$sanitized = apply_filters( 'amp_validation_error_sanitized', $sanitized, $error );


return apply_filters( 'amp_validation_error_sanitized', $sanitized, $error );

ThierryA · 2018-06-01T10:15:46Z

includes/validation/class-amp-invalid-url-post-type.php

@@ -966,33 +968,49 @@ public static function print_validation_errors_meta_box( $post ) {
 		} );
 		</script>

+		<?php
+		$forced_sanitized_count = 0;
+		foreach ( $validation_errors as $validation_error ) {


Another approach:

$forced_sanitized_count = array_reduce( $validation_errors, function( $sum, $validation_error ) { return $sum + (int) isset( $validation_error['forced_sanitization'] ); } );

…reen Tidy up some code

…d normalize logic

…xonomy

westonruter · 2018-06-01T16:51:27Z

@ThierryA there, I'm feeling way better about this after https://github.com/Automattic/amp-wp/pull/1190/files/3e2ba8a..5458e9a

westonruter · 2018-06-01T18:11:29Z

In 6dbdb0d a key improvement is made when accepting/rejecting validation errors from the invalid URL screen: now the URL will be automatically re-checked after the validation errors have been updated. This is very important in the case of errors like removed_unused_css_rules in particular, because if it is sanitized then all of the subsequent excessive_css validation errors automatically go away:

Now when changing the status of just removed_unused_css_rules and clicking Update, the result is then:

…recheck_url()

…error's sanitization status Also prevent removable query vars from erroneously being carried into subsequent requests via referrer

kienstra

Approved

Hi @westonruter,
This looks good. There are some minor points, but no blocker as long as you're happy with this.

kienstra · 2018-06-01T18:49:02Z

includes/validation/class-amp-invalid-url-post-type.php

-		}
+		$actions[ self::RECHECK_ACTION ] = sprintf(
+			'<a href="%s">%s</a>',
+			self::get_recheck_url( $post, get_edit_post_link( $post->ID, 'raw' ) ),


Should this use esc_url()?

kienstra · 2018-06-01T18:49:42Z

includes/validation/class-amp-invalid-url-post-type.php

@@ -708,9 +693,9 @@ public static function print_admin_notice() {
 			$errors_remain     = ! empty( $_GET[ self::REMAINING_ERRORS ] ); // WPCS: CSRF ok.
 			if ( $errors_remain ) {
 				$class   = 'notice-warning';
-				$message = _n( 'The rechecked URL still has validation errors.', 'The rechecked URLs still have validation errors.', $count_urls_tested, 'amp' );
+				$message = _n( 'The rechecked URL still has blocking validation errors.', 'The rechecked URLs still have validation errors.', $count_urls_tested, 'amp' );


Nice use of _n() to account for singular and plural.

kienstra · 2018-06-01T18:58:25Z

includes/validation/class-amp-invalid-url-post-type.php

@@ -792,6 +806,10 @@ public static function handle_validation_error_status_update() {
 		if ( empty( $_POST[ AMP_Validation_Manager::VALIDATION_ERROR_TERM_STATUS_QUERY_VAR ] ) || ! is_array( $_POST[ AMP_Validation_Manager::VALIDATION_ERROR_TERM_STATUS_QUERY_VAR ] ) ) {
 			return;
 		}
+		$post = get_post();
+		if ( ! $post || self::POST_TYPE_SLUG !== get_post_type( $post ) ) {


This is a very minor point, but could this use $post->post_type instead of get_post_type( $post )?

Yes, it wouldn't make any difference though. 👍

kienstra · 2018-06-01T19:29:18Z

includes/validation/class-amp-invalid-url-post-type.php

+		<?php
+		$has_forced_sanitized = false;
+		foreach ( $validation_errors as $validation_error ) {
+			if ( $validation_error['forced'] ) {


You're probably aware of this, but I saw notices on an "Invalid AMP Page (URL)" pages:

( ! ) Notice: Undefined index: forced in /srv/www/loc/public_html/wp-content/plugins/amp/includes/validation/class-amp-invalid-url-post-type.php on line 1015

This occurred even after clicking "Re-check."

These could be from errors saved from before this PR. I tried bulk moving "Invalid Pages" to the trash, but this didn't help.

Still, you mentioned that we don't need backwards compatibility with an earlier "checkout" of 1.0. And the way validation errors are stored is very different from in 0.7.

…_sanitization

westonruter · 2018-06-01T20:24:53Z

Aside, it's a good idea to reset validation state to a blank slate after this:

wp post delete --force $( wp post list --post_type=amp_invalid_url --format=ids );
wp term delete amp_validation_error $( wp term list amp_validation_error --format=ids );

westonruter added 4 commits May 31, 2018 20:58

Ensure response cache is varied by validation error sanitization status

e2f7bc7

Fix obtaining theme/plugin source in summarize_validation_errors

2831654

Store full validation error in invalid URL post for sake of searching

3b484e8

Account for amp_validation_error_sanitized filter when displaying val…

d12dba7

…idation errors in admin Prevent offering to change validation error status when filter is forcing a certain status

westonruter added this to the v1.0 milestone Jun 1, 2018

westonruter requested a review from ThierryA June 1, 2018 06:00

westonruter assigned ThierryA Jun 1, 2018

ThierryA reviewed Jun 1, 2018

View reviewed changes

westonruter added 3 commits June 1, 2018 07:36

Move amp_validation_error delete_term check only to relevant admin sc…

3e2ba8a

…reen Tidy up some code

Add get_validation_error_sanitization method to remove duplication an…

442bae8

…d normalize logic

Move methods from AMP_Validation_Manager into AMP_Validation_Error_Ta…

5458e9a

…xonomy

westonruter added 2 commits June 1, 2018 11:23

Eliminate unused recheck_url param on AMP_Invalid_URL_Post_Type::get_…

313065e

…recheck_url()

Automatically re-check validation of an invalid URL when changing an …

2b089f3

…error's sanitization status Also prevent removable query vars from erroneously being carried into subsequent requests via referrer

westonruter force-pushed the fix/response-cache-varying branch from 6dbdb0d to 2b089f3 Compare June 1, 2018 18:24

kienstra self-assigned this Jun 1, 2018

kienstra approved these changes Jun 1, 2018

View reviewed changes

kienstra removed their assignment Jun 1, 2018

Prevent notice about forced not being defined in get_validation_error…

decebc2

…_sanitization

westonruter merged commit 90acca4 into develop Jun 1, 2018

westonruter deleted the fix/response-cache-varying branch June 1, 2018 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of validation errors in response cache and admin display #1190

Improve handling of validation errors in response cache and admin display #1190

westonruter commented Jun 1, 2018

westonruter commented Jun 1, 2018

ThierryA Jun 1, 2018

westonruter Jun 1, 2018

ThierryA Jun 1, 2018

westonruter Jun 1, 2018

ThierryA Jun 1, 2018

westonruter Jun 1, 2018

ThierryA Jun 1, 2018

westonruter Jun 1, 2018

ThierryA Jun 1, 2018

westonruter Jun 1, 2018

ThierryA Jun 1, 2018

westonruter Jun 1, 2018

ThierryA Jun 1, 2018

ThierryA Jun 1, 2018

ThierryA Jun 1, 2018

westonruter commented Jun 1, 2018

westonruter commented Jun 1, 2018

kienstra left a comment

kienstra Jun 1, 2018

kienstra Jun 1, 2018

kienstra Jun 1, 2018

westonruter Jun 1, 2018

kienstra Jun 1, 2018 •

edited

Loading

westonruter commented Jun 1, 2018

Improve handling of validation errors in response cache and admin display #1190

Improve handling of validation errors in response cache and admin display #1190

Conversation

westonruter commented Jun 1, 2018

westonruter commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jun 1, 2018

westonruter commented Jun 1, 2018

kienstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

westonruter commented Jun 1, 2018

kienstra Jun 1, 2018 •

edited

Loading