Skip to content

Commit 90acca4

Browse files
westonruterwestonruter
authored
Merge pull request #1190 from Automattic/fix/response-cache-varying
Improve handling of validation errors in response cache and admin display
2 parents 70b81ae + decebc2 commit 90acca4

5 files changed

Lines changed: 327 additions & 160 deletions

‎includes/class-amp-theme-support.php‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,7 +1058,7 @@ public static function prepare_response( $response, $args = array() ) {
10581058
'allow_dirty_styles' => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes).
10591059
'allow_dirty_scripts' => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
10601060
'enable_response_caching' => (
1061-
( ! defined( 'WP_DEBUG' ) || true !== WP_DEBUG )
1061+
! ( defined( 'WP_DEBUG' ) && WP_DEBUG )
10621062
&&
10631063
! AMP_Validation_Manager::should_validate_response()
10641064
),
@@ -1080,8 +1080,20 @@ public static function prepare_response( $response, $args = array() ) {
10801080

10811081
$response_cache = wp_cache_get( $response_cache_key, self::RESPONSE_CACHE_GROUP );
10821082

1083-
if ( ! empty( $response_cache ) ) {
1084-
return $response_cache;
1083+
// Make sure that all of the validation errors should be sanitized in the same way; if not, then the cached body should be discarded.
1084+
if ( isset( $response_cache['validation_results'] ) ) {
1085+
foreach ( $response_cache['validation_results'] as $validation_result ) {
1086+
$should_sanitize = AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $validation_result['error'] );
1087+
if ( $should_sanitize !== $validation_result['sanitized'] ) {
1088+
unset( $response_cache['body'] );
1089+
break;
1090+
}
1091+
}
1092+
}
1093+
1094+
// Short-circuit response with cached body.
1095+
if ( isset( $response_cache['body'] ) ) {
1096+
return $response_cache['body'];
10851097
}
10861098
}
10871099

@@ -1102,14 +1114,13 @@ public static function prepare_response( $response, $args = array() ) {
11021114
1
11031115
);
11041116
}
1105-
$dom = AMP_DOM_Utils::get_dom( $response );
1106-
1107-
$xpath = new DOMXPath( $dom );
11081117

1118+
$dom = AMP_DOM_Utils::get_dom( $response );
11091119
$head = $dom->getElementsByTagName( 'head' )->item( 0 );
11101120

1121+
// Make sure scripts from the body get moved to the head.
11111122
if ( isset( $head ) ) {
1112-
// Make sure scripts from the body get moved to the head.
1123+
$xpath = new DOMXPath( $dom );
11131124
foreach ( $xpath->query( '//body//script[ @custom-element or @custom-template ]' ) as $script ) {
11141125
$head->appendChild( $script );
11151126
}
@@ -1208,7 +1219,17 @@ public static function prepare_response( $response, $args = array() ) {
12081219

12091220
// Cache response if enabled.
12101221
if ( true === $args['enable_response_caching'] ) {
1211-
wp_cache_set( $response_cache_key, $response, self::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS );
1222+
$response_cache = array(
1223+
'body' => $response,
1224+
'validation_results' => array_map(
1225+
function( $result ) {
1226+
unset( $result['error']['sources'] );
1227+
return $result;
1228+
},
1229+
AMP_Validation_Manager::$validation_results
1230+
),
1231+
);
1232+
wp_cache_set( $response_cache_key, $response_cache, self::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS );
12121233
}
12131234

12141235
return $response;

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

Lines changed: 122 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ public static function add_admin_menu_new_invalid_url_count() {
177177
*
178178
* @type bool $ignore_accepted Exclude validation errors that are accepted. Default false.
179179
* }
180-
* @return array List of errors.
180+
* @return array List of errors, with keys for term, data, status, and (sanitization) forced.
181181
*/
182182
public static function get_invalid_url_validation_errors( $post, $args = array() ) {
183183
$args = array_merge(
@@ -198,20 +198,18 @@ public static function get_invalid_url_validation_errors( $post, $args = array()
198198
continue;
199199
}
200200
$term = get_term_by( 'slug', $stored_validation_error['term_slug'], AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG );
201-
if ( ! $term ) {
202-
continue;
203-
}
204-
if ( $args['ignore_accepted'] && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group ) {
201+
202+
$sanitization = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $stored_validation_error['data'] );
203+
if ( $args['ignore_accepted'] && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $sanitization['status'] ) {
205204
continue;
206205
}
207-
$errors[] = array(
208-
'term' => $term,
209-
'data' => array_merge(
210-
json_decode( $term->description, true ),
211-
array(
212-
'sources' => $stored_validation_error['sources'],
213-
)
206+
207+
$errors[] = array_merge(
208+
array(
209+
'term' => $term,
210+
'data' => $stored_validation_error['data'],
214211
),
212+
$sanitization
215213
);
216214
}
217215
return $errors;
@@ -234,10 +232,8 @@ public static function get_invalid_url_validation_error_counts( $post ) {
234232
);
235233

236234
$validation_errors = self::get_invalid_url_validation_errors( $post );
237-
foreach ( wp_list_pluck( $validation_errors, 'term' ) as $term ) {
238-
if ( isset( $counts[ $term->term_group ] ) ) {
239-
$counts[ $term->term_group ]++;
240-
}
235+
foreach ( $validation_errors as $error ) {
236+
$counts[ $error['status'] ]++;
241237
}
242238
return $counts;
243239
}
@@ -318,15 +314,6 @@ public static function store_validation_errors( $validation_errors, $url ) {
318314

319315
$terms = array();
320316
foreach ( $validation_errors as $data ) {
321-
/*
322-
* Exclude sources from data since not available unless sources are being obtained,
323-
* and thus not able to be matched when hashed.
324-
*/
325-
$sources = null;
326-
if ( isset( $data['sources'] ) ) {
327-
$sources = $data['sources'];
328-
}
329-
330317
$term_data = AMP_Validation_Error_Taxonomy::prepare_validation_error_taxonomy_term( $data );
331318
$term_slug = $term_data['slug'];
332319
if ( ! isset( $terms[ $term_slug ] ) ) {
@@ -352,7 +339,7 @@ public static function store_validation_errors( $validation_errors, $url ) {
352339
$terms[ $term_slug ] = $term;
353340
}
354341

355-
$stored_validation_errors[] = compact( 'term_slug', 'sources' );
342+
$stored_validation_errors[] = compact( 'term_slug', 'data' );
356343
}
357344

358345
$post_content = wp_json_encode( $stored_validation_errors );
@@ -627,13 +614,11 @@ public static function filter_row_actions( $actions, $post ) {
627614
$view_url = add_query_arg( AMP_Validation_Manager::VALIDATE_QUERY_VAR, '', $url ); // Prevent redirection to non-AMP page.
628615
$actions['view'] = sprintf( '<a href="%s">%s</a>', esc_url( $view_url ), esc_html__( 'View', 'amp' ) );
629616

630-
if ( ! empty( $url ) ) {
631-
$actions[ self::RECHECK_ACTION ] = sprintf(
632-
'<a href="%s">%s</a>',
633-
self::get_recheck_url( $post, get_edit_post_link( $post->ID, 'raw' ), $url ),
634-
esc_html__( 'Re-check', 'amp' )
635-
);
636-
}
617+
$actions[ self::RECHECK_ACTION ] = sprintf(
618+
'<a href="%s">%s</a>',
619+
esc_url( self::get_recheck_url( $post, get_edit_post_link( $post->ID, 'raw' ) ) ),
620+
esc_html__( 'Re-check', 'amp' )
621+
);
637622

638623
return $actions;
639624
}
@@ -708,9 +693,9 @@ public static function print_admin_notice() {
708693
$errors_remain = ! empty( $_GET[ self::REMAINING_ERRORS ] ); // WPCS: CSRF ok.
709694
if ( $errors_remain ) {
710695
$class = 'notice-warning';
711-
$message = _n( 'The rechecked URL still has validation errors.', 'The rechecked URLs still have validation errors.', $count_urls_tested, 'amp' );
696+
$message = _n( 'The rechecked URL still has blocking validation errors.', 'The rechecked URLs still have validation errors.', $count_urls_tested, 'amp' );
712697
} else {
713-
$message = _n( 'The rechecked URL has no validation errors.', 'The rechecked URLs have no validation errors.', $count_urls_tested, 'amp' );
698+
$message = _n( 'The rechecked URL has no blocking validation errors.', 'The rechecked URLs have no validation errors.', $count_urls_tested, 'amp' );
714699
$class = 'updated';
715700
}
716701

@@ -722,8 +707,8 @@ public static function print_admin_notice() {
722707
);
723708
}
724709

725-
if ( isset( $_GET['amp_taxonomy_terms_updated'] ) ) { // WPCS: CSRF ok.
726-
$count = intval( $_GET['amp_taxonomy_terms_updated'] );
710+
$count = isset( $_GET['amp_taxonomy_terms_updated'] ) ? intval( $_GET['amp_taxonomy_terms_updated'] ) : 0; // WPCS: CSRF ok.
711+
if ( $count > 0 ) {
727712
$class = 'updated';
728713
printf(
729714
'<div class="notice is-dismissible %s"><p>%s</p><button type="button" class="notice-dismiss"><span class="screen-reader-text">%s</span></button></div>',
@@ -751,20 +736,14 @@ public static function print_admin_notice() {
751736
*/
752737
public static function handle_inline_recheck( $post_id ) {
753738
check_admin_referer( self::NONCE_ACTION . $post_id );
754-
$post = get_post( $post_id );
755-
$url = $post->post_title;
756-
if ( isset( $_GET['recheck_url'] ) ) {
757-
$url = wp_validate_redirect( wp_unslash( $_GET['recheck_url'] ) );
758-
}
759-
$validation_errors = AMP_Validation_Manager::validate_url( $url );
760-
$remaining_errors = true;
761-
if ( is_array( $validation_errors ) ) {
762-
self::store_validation_errors( $validation_errors, $url );
763-
$remaining_errors = ! empty( $validation_errors );
764-
}
739+
$validation_results = self::recheck_post( $post_id );
765740

766741
$redirect = wp_get_referer();
767-
if ( ! $redirect || empty( $validation_errors ) ) {
742+
if ( $redirect ) {
743+
$redirect = remove_query_arg( wp_removable_query_args(), $redirect );
744+
}
745+
746+
if ( ! $redirect || is_wp_error( $validation_results ) || empty( $validation_results ) ) {
768747
// If there are no remaining errors and the post was deleted, redirect to edit.php instead of post.php.
769748
$redirect = add_query_arg(
770749
'post_type',
@@ -773,13 +752,48 @@ public static function handle_inline_recheck( $post_id ) {
773752
);
774753
}
775754
$args = array(
776-
self::URLS_TESTED => '1',
777-
self::REMAINING_ERRORS => $remaining_errors ? '1' : '0',
755+
self::URLS_TESTED => '1',
778756
);
757+
758+
// @todo For WP_Error case, see <https://github.com/Automattic/amp-wp/issues/1166>.
759+
if ( ! is_wp_error( $validation_results ) ) {
760+
$args[ self::REMAINING_ERRORS ] = count( array_filter(
761+
$validation_results,
762+
function( $result ) {
763+
return ! $result['sanitized'];
764+
}
765+
) );
766+
}
767+
779768
wp_safe_redirect( add_query_arg( $args, $redirect ) );
780769
exit();
781770
}
782771

772+
/**
773+
* Re-check invalid URL post for whether it has blocking validation errors.
774+
*
775+
* @param int|WP_Post $post Post.
776+
* @return array|WP_Error List of blocking validation resukts, or a WP_Error in the case of failure.
777+
*/
778+
public static function recheck_post( $post ) {
779+
$post = get_post( $post );
780+
$url = $post->post_title;
781+
782+
$validation_errors = AMP_Validation_Manager::validate_url( $url );
783+
if ( is_wp_error( $validation_errors ) ) {
784+
return $validation_errors;
785+
}
786+
787+
$validation_results = array();
788+
self::store_validation_errors( $validation_errors, $url );
789+
foreach ( $validation_errors as $error ) {
790+
$sanitized = AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $error );
791+
792+
$validation_results[] = compact( 'error', 'sanitized' );
793+
}
794+
return $validation_results;
795+
}
796+
783797
/**
784798
* Handle validation error status update.
785799
*
@@ -792,6 +806,10 @@ public static function handle_validation_error_status_update() {
792806
if ( empty( $_POST[ AMP_Validation_Manager::VALIDATION_ERROR_TERM_STATUS_QUERY_VAR ] ) || ! is_array( $_POST[ AMP_Validation_Manager::VALIDATION_ERROR_TERM_STATUS_QUERY_VAR ] ) ) {
793807
return;
794808
}
809+
$post = get_post();
810+
if ( ! $post || self::POST_TYPE_SLUG !== $post->post_type ) {
811+
return;
812+
}
795813
$updated_count = 0;
796814

797815
$has_pre_term_description_filter = has_filter( 'pre_term_description', 'wp_filter_kses' );
@@ -819,7 +837,32 @@ public static function handle_validation_error_status_update() {
819837
$args = array(
820838
'amp_taxonomy_terms_updated' => $updated_count,
821839
);
822-
wp_safe_redirect( add_query_arg( $args, wp_get_referer() ) );
840+
841+
/*
842+
* Re-check the post after the validation status change. This is particularly important for validation errors like
843+
* 'removed_unused_css_rules' since whether it is accepted will determine whether other validation errors are triggered
844+
* such as in this case 'excessive_css'.
845+
*/
846+
if ( $updated_count > 0 ) {
847+
$validation_results = self::recheck_post( $post->ID );
848+
// @todo For WP_Error case, see <https://github.com/Automattic/amp-wp/issues/1166>.
849+
if ( ! is_wp_error( $validation_results ) ) {
850+
$args[ self::REMAINING_ERRORS ] = count( array_filter(
851+
$validation_results,
852+
function( $result ) {
853+
return ! $result['sanitized'];
854+
}
855+
) );
856+
}
857+
}
858+
859+
$redirect = wp_get_referer();
860+
if ( ! $redirect ) {
861+
$redirect = get_edit_post_link( $post->ID );
862+
}
863+
864+
$redirect = remove_query_arg( wp_removable_query_args(), $redirect );
865+
wp_safe_redirect( add_query_arg( $args, $redirect ) );
823866
exit();
824867
}
825868

@@ -921,7 +964,7 @@ public static function print_validation_errors_meta_box( $post ) {
921964
$validation_errors = self::get_invalid_url_validation_errors( $post );
922965

923966
$can_serve_amp = 0 === count( array_filter( $validation_errors, function( $validation_error ) {
924-
return AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS !== $validation_error['term']->term_group;
967+
return AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS !== $validation_error['status'];
925968
} ) );
926969
?>
927970
<style>
@@ -966,6 +1009,22 @@ public static function print_validation_errors_meta_box( $post ) {
9661009
} );
9671010
</script>
9681011

1012+
<?php
1013+
$has_forced_sanitized = false;
1014+
foreach ( $validation_errors as $validation_error ) {
1015+
if ( $validation_error['forced'] ) {
1016+
$has_forced_sanitized = true;
1017+
break;
1018+
}
1019+
}
1020+
?>
1021+
1022+
<?php if ( $has_forced_sanitized ) : ?>
1023+
<div class="notice notice-info notice-alt inline">
1024+
<p><?php esc_html_e( 'This site is configured to automatically decide whether or not some validation errors may be accepted. For these errors, you cannot change their status below.', 'amp' ); ?></p>
1025+
</div>
1026+
<?php endif; ?>
1027+
9691028
<div class="amp-validation-errors">
9701029
<ul>
9711030
<?php foreach ( $validation_errors as $error ) : ?>
@@ -975,23 +1034,23 @@ public static function print_validation_errors_meta_box( $post ) {
9751034
$select_name = sprintf( '%s[%s]', AMP_Validation_Manager::VALIDATION_ERROR_TERM_STATUS_QUERY_VAR, $term->slug );
9761035
?>
9771036
<li>
978-
<details <?php echo ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $term->term_group ) ? 'open' : ''; ?>>
1037+
<details <?php echo ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $error['status'] ) ? 'open' : ''; ?>>
9791038
<summary>
9801039
<label for="<?php echo esc_attr( $select_name ); ?>" class="screen-reader-text">
9811040
<?php esc_html_e( 'Status:', 'amp' ); ?>
9821041
</label>
983-
<select class="amp-validation-error-status" id="<?php echo esc_attr( $select_name ); ?>" name="<?php echo esc_attr( $select_name ); ?>">
984-
<?php if ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $term->term_group ) : ?>
1042+
<select class="amp-validation-error-status" id="<?php echo esc_attr( $select_name ); ?>" name="<?php echo esc_attr( $select_name ); ?>" <?php disabled( $error['forced'] ); ?>>
1043+
<?php if ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $error['status'] ) : ?>
9851044
<option value=""><?php esc_html_e( 'New', 'amp' ); ?></option>
9861045
<?php endif; ?>
987-
<option value="<?php echo esc_attr( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS ); ?>" <?php selected( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS, $term->term_group ); ?>><?php esc_html_e( 'Accepted', 'amp' ); ?></option>
988-
<option value="<?php echo esc_attr( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECTED_STATUS ); ?>" <?php selected( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECTED_STATUS, $term->term_group ); ?>><?php esc_html_e( 'Rejected', 'amp' ); ?></option>
1046+
<option value="<?php echo esc_attr( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS ); ?>" <?php selected( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS, $error['status'] ); ?>><?php esc_html_e( 'Accepted', 'amp' ); ?></option>
1047+
<option value="<?php echo esc_attr( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECTED_STATUS ); ?>" <?php selected( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECTED_STATUS, $error['status'] ); ?>><?php esc_html_e( 'Rejected', 'amp' ); ?></option>
9891048
</select>
990-
<?php if ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $term->term_group ) : ?>
1049+
<?php if ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $error['status'] ) : ?>
9911050
&#x2753;
992-
<?php elseif ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECTED_STATUS === $term->term_group ) : ?>
1051+
<?php elseif ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECTED_STATUS === $error['status'] ) : ?>
9931052
&#x274C;
994-
<?php elseif ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group ) : ?>
1053+
<?php elseif ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $error['status'] ) : ?>
9951054
&#x2705;
9961055
<?php endif; ?>
9971056
<code><?php echo esc_html( $error['data']['code'] ); ?></code>
@@ -1147,15 +1206,13 @@ public static function filter_the_title_in_post_list_table( $title, $post ) {
11471206
*
11481207
* @param WP_Post $post The post storing the validation error.
11491208
* @param string $redirect_url The URL of the redirect.
1150-
* @param string $recheck_url The URL to check. Optional.
11511209
* @return string The URL to recheck the post.
11521210
*/
1153-
public static function get_recheck_url( $post, $redirect_url, $recheck_url = null ) {
1211+
public static function get_recheck_url( $post, $redirect_url ) {
11541212
return wp_nonce_url(
11551213
add_query_arg(
11561214
array(
1157-
'action' => self::RECHECK_ACTION,
1158-
'recheck_url' => $recheck_url,
1215+
'action' => self::RECHECK_ACTION,
11591216
),
11601217
$redirect_url
11611218
),

0 commit comments

Comments
 (0)