Skip to content

Commit

Permalink
Merge pull request #1190 from Automattic/fix/response-cache-varying
Browse files Browse the repository at this point in the history 
Improve handling of validation errors in response cache and admin display
  • Loading branch information
@westonruter
westonruter authored Jun 1, 2018
2 parents 70b81ae + decebc2 commit 90acca4
Show file tree
Hide file tree
Showing 5 changed files with 327 additions and 160 deletions.
37 changes: 29 additions & 8 deletions includes/class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1058,7 +1058,7 @@ public static function prepare_response( $response, $args = array() ) {
'allow_dirty_styles' => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes).
'allow_dirty_scripts' => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
'enable_response_caching' => (
( ! defined( 'WP_DEBUG' ) || true !== WP_DEBUG )
! ( defined( 'WP_DEBUG' ) && WP_DEBUG )
&&
! AMP_Validation_Manager::should_validate_response()
),
Expand All @@ -1080,8 +1080,20 @@ public static function prepare_response( $response, $args = array() ) {

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

if ( ! empty( $response_cache ) ) {
return $response_cache;
// Make sure that all of the validation errors should be sanitized in the same way; if not, then the cached body should be discarded.
if ( isset( $response_cache['validation_results'] ) ) {
foreach ( $response_cache['validation_results'] as $validation_result ) {
$should_sanitize = AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $validation_result['error'] );
if ( $should_sanitize !== $validation_result['sanitized'] ) {
unset( $response_cache['body'] );
break;
}
}
}

// Short-circuit response with cached body.
if ( isset( $response_cache['body'] ) ) {
return $response_cache['body'];
}
}

Expand All @@ -1102,14 +1114,13 @@ public static function prepare_response( $response, $args = array() ) {
1
);
}
$dom = AMP_DOM_Utils::get_dom( $response );

$xpath = new DOMXPath( $dom );

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

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

// Cache response if enabled.
if ( true === $args['enable_response_caching'] ) {
wp_cache_set( $response_cache_key, $response, self::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS );
$response_cache = array(
'body' => $response,
'validation_results' => array_map(
function( $result ) {
unset( $result['error']['sources'] );
return $result;
},
AMP_Validation_Manager::$validation_results
),
);
wp_cache_set( $response_cache_key, $response_cache, self::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS );
}

return $response;
Expand Down
187 changes: 122 additions & 65 deletions includes/validation/class-amp-invalid-url-post-type.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ public static function add_admin_menu_new_invalid_url_count() {
*
* @type bool $ignore_accepted Exclude validation errors that are accepted. Default false.
* }
* @return array List of errors.
* @return array List of errors, with keys for term, data, status, and (sanitization) forced.
*/
public static function get_invalid_url_validation_errors( $post, $args = array() ) {
$args = array_merge(
Expand All @@ -198,20 +198,18 @@ public static function get_invalid_url_validation_errors( $post, $args = array()
continue;
}
$term = get_term_by( 'slug', $stored_validation_error['term_slug'], AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG );
if ( ! $term ) {
continue;
}
if ( $args['ignore_accepted'] && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group ) {

$sanitization = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $stored_validation_error['data'] );
if ( $args['ignore_accepted'] && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $sanitization['status'] ) {
continue;
}
$errors[] = array(
'term' => $term,
'data' => array_merge(
json_decode( $term->description, true ),
array(
'sources' => $stored_validation_error['sources'],
)

$errors[] = array_merge(
array(
'term' => $term,
'data' => $stored_validation_error['data'],
),
$sanitization
);
}
return $errors;
Expand All @@ -234,10 +232,8 @@ public static function get_invalid_url_validation_error_counts( $post ) {
);

$validation_errors = self::get_invalid_url_validation_errors( $post );
foreach ( wp_list_pluck( $validation_errors, 'term' ) as $term ) {
if ( isset( $counts[ $term->term_group ] ) ) {
$counts[ $term->term_group ]++;
}
foreach ( $validation_errors as $error ) {
$counts[ $error['status'] ]++;
}
return $counts;
}
Expand Down Expand Up @@ -318,15 +314,6 @@ public static function store_validation_errors( $validation_errors, $url ) {

$terms = array();
foreach ( $validation_errors as $data ) {
/*
* Exclude sources from data since not available unless sources are being obtained,
* and thus not able to be matched when hashed.
*/
$sources = null;
if ( isset( $data['sources'] ) ) {
$sources = $data['sources'];
}

$term_data = AMP_Validation_Error_Taxonomy::prepare_validation_error_taxonomy_term( $data );
$term_slug = $term_data['slug'];
if ( ! isset( $terms[ $term_slug ] ) ) {
Expand All @@ -352,7 +339,7 @@ public static function store_validation_errors( $validation_errors, $url ) {
$terms[ $term_slug ] = $term;
}

$stored_validation_errors[] = compact( 'term_slug', 'sources' );
$stored_validation_errors[] = compact( 'term_slug', 'data' );
}

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

if ( ! empty( $url ) ) {
$actions[ self::RECHECK_ACTION ] = sprintf(
'<a href="%s">%s</a>',
self::get_recheck_url( $post, get_edit_post_link( $post->ID, 'raw' ), $url ),
esc_html__( 'Re-check', 'amp' )
);
}
$actions[ self::RECHECK_ACTION ] = sprintf(
'<a href="%s">%s</a>',
esc_url( self::get_recheck_url( $post, get_edit_post_link( $post->ID, 'raw' ) ) ),
esc_html__( 'Re-check', 'amp' )
);

return $actions;
}
Expand Down Expand Up @@ -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' );
} else {
$message = _n( 'The rechecked URL has no validation errors.', 'The rechecked URLs have no validation errors.', $count_urls_tested, 'amp' );
$message = _n( 'The rechecked URL has no blocking validation errors.', 'The rechecked URLs have no validation errors.', $count_urls_tested, 'amp' );
$class = 'updated';
}

Expand All @@ -722,8 +707,8 @@ public static function print_admin_notice() {
);
}

if ( isset( $_GET['amp_taxonomy_terms_updated'] ) ) { // WPCS: CSRF ok.
$count = intval( $_GET['amp_taxonomy_terms_updated'] );
$count = isset( $_GET['amp_taxonomy_terms_updated'] ) ? intval( $_GET['amp_taxonomy_terms_updated'] ) : 0; // WPCS: CSRF ok.
if ( $count > 0 ) {
$class = 'updated';
printf(
'<div class="notice is-dismissible %s"><p>%s</p><button type="button" class="notice-dismiss"><span class="screen-reader-text">%s</span></button></div>',
Expand Down Expand Up @@ -751,20 +736,14 @@ public static function print_admin_notice() {
*/
public static function handle_inline_recheck( $post_id ) {
check_admin_referer( self::NONCE_ACTION . $post_id );
$post = get_post( $post_id );
$url = $post->post_title;
if ( isset( $_GET['recheck_url'] ) ) {
$url = wp_validate_redirect( wp_unslash( $_GET['recheck_url'] ) );
}
$validation_errors = AMP_Validation_Manager::validate_url( $url );
$remaining_errors = true;
if ( is_array( $validation_errors ) ) {
self::store_validation_errors( $validation_errors, $url );
$remaining_errors = ! empty( $validation_errors );
}
$validation_results = self::recheck_post( $post_id );

$redirect = wp_get_referer();
if ( ! $redirect || empty( $validation_errors ) ) {
if ( $redirect ) {
$redirect = remove_query_arg( wp_removable_query_args(), $redirect );
}

if ( ! $redirect || is_wp_error( $validation_results ) || empty( $validation_results ) ) {
// If there are no remaining errors and the post was deleted, redirect to edit.php instead of post.php.
$redirect = add_query_arg(
'post_type',
Expand All @@ -773,13 +752,48 @@ public static function handle_inline_recheck( $post_id ) {
);
}
$args = array(
self::URLS_TESTED => '1',
self::REMAINING_ERRORS => $remaining_errors ? '1' : '0',
self::URLS_TESTED => '1',
);

// @todo For WP_Error case, see <https://github.com/Automattic/amp-wp/issues/1166>.
if ( ! is_wp_error( $validation_results ) ) {
$args[ self::REMAINING_ERRORS ] = count( array_filter(
$validation_results,
function( $result ) {
return ! $result['sanitized'];
}
) );
}

wp_safe_redirect( add_query_arg( $args, $redirect ) );
exit();
}

/**
* Re-check invalid URL post for whether it has blocking validation errors.
*
* @param int|WP_Post $post Post.
* @return array|WP_Error List of blocking validation resukts, or a WP_Error in the case of failure.
*/
public static function recheck_post( $post ) {
$post = get_post( $post );
$url = $post->post_title;

$validation_errors = AMP_Validation_Manager::validate_url( $url );
if ( is_wp_error( $validation_errors ) ) {
return $validation_errors;
}

$validation_results = array();
self::store_validation_errors( $validation_errors, $url );
foreach ( $validation_errors as $error ) {
$sanitized = AMP_Validation_Error_Taxonomy::is_validation_error_sanitized( $error );

$validation_results[] = compact( 'error', 'sanitized' );
}
return $validation_results;
}

/**
* Handle validation error status update.
*
Expand All @@ -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 !== $post->post_type ) {
return;
}
$updated_count = 0;

$has_pre_term_description_filter = has_filter( 'pre_term_description', 'wp_filter_kses' );
Expand Down Expand Up @@ -819,7 +837,32 @@ public static function handle_validation_error_status_update() {
$args = array(
'amp_taxonomy_terms_updated' => $updated_count,
);
wp_safe_redirect( add_query_arg( $args, wp_get_referer() ) );

/*
* Re-check the post after the validation status change. This is particularly important for validation errors like
* 'removed_unused_css_rules' since whether it is accepted will determine whether other validation errors are triggered
* such as in this case 'excessive_css'.
*/
if ( $updated_count > 0 ) {
$validation_results = self::recheck_post( $post->ID );
// @todo For WP_Error case, see <https://github.com/Automattic/amp-wp/issues/1166>.
if ( ! is_wp_error( $validation_results ) ) {
$args[ self::REMAINING_ERRORS ] = count( array_filter(
$validation_results,
function( $result ) {
return ! $result['sanitized'];
}
) );
}
}

$redirect = wp_get_referer();
if ( ! $redirect ) {
$redirect = get_edit_post_link( $post->ID );
}

$redirect = remove_query_arg( wp_removable_query_args(), $redirect );
wp_safe_redirect( add_query_arg( $args, $redirect ) );
exit();
}

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

$can_serve_amp = 0 === count( array_filter( $validation_errors, function( $validation_error ) {
return AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS !== $validation_error['term']->term_group;
return AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS !== $validation_error['status'];
} ) );
?>
<style>
Expand Down Expand Up @@ -966,6 +1009,22 @@ public static function print_validation_errors_meta_box( $post ) {
} );
</script>

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

<?php if ( $has_forced_sanitized ) : ?>
<div class="notice notice-info notice-alt inline">
<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>
</div>
<?php endif; ?>

<div class="amp-validation-errors">
<ul>
<?php foreach ( $validation_errors as $error ) : ?>
Expand All @@ -975,23 +1034,23 @@ public static function print_validation_errors_meta_box( $post ) {
$select_name = sprintf( '%s[%s]', AMP_Validation_Manager::VALIDATION_ERROR_TERM_STATUS_QUERY_VAR, $term->slug );
?>
<li>
<details <?php echo ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $term->term_group ) ? 'open' : ''; ?>>
<details <?php echo ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $error['status'] ) ? 'open' : ''; ?>>
<summary>
<label for="<?php echo esc_attr( $select_name ); ?>" class="screen-reader-text">
<?php esc_html_e( 'Status:', 'amp' ); ?>
</label>
<select class="amp-validation-error-status" id="<?php echo esc_attr( $select_name ); ?>" name="<?php echo esc_attr( $select_name ); ?>">
<?php if ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $term->term_group ) : ?>
<select class="amp-validation-error-status" id="<?php echo esc_attr( $select_name ); ?>" name="<?php echo esc_attr( $select_name ); ?>" <?php disabled( $error['forced'] ); ?>>
<?php if ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $error['status'] ) : ?>
<option value=""><?php esc_html_e( 'New', 'amp' ); ?></option>
<?php endif; ?>
<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>
<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>
<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>
<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>
</select>
<?php if ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $term->term_group ) : ?>
<?php if ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_STATUS === $error['status'] ) : ?>
&#x2753;
<?php elseif ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECTED_STATUS === $term->term_group ) : ?>
<?php elseif ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECTED_STATUS === $error['status'] ) : ?>
&#x274C;
<?php elseif ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group ) : ?>
<?php elseif ( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $error['status'] ) : ?>
&#x2705;
<?php endif; ?>
<code><?php echo esc_html( $error['data']['code'] ); ?></code>
Expand Down Expand Up @@ -1147,15 +1206,13 @@ public static function filter_the_title_in_post_list_table( $title, $post ) {
*
* @param WP_Post $post The post storing the validation error.
* @param string $redirect_url The URL of the redirect.
* @param string $recheck_url The URL to check. Optional.
* @return string The URL to recheck the post.
*/
public static function get_recheck_url( $post, $redirect_url, $recheck_url = null ) {
public static function get_recheck_url( $post, $redirect_url ) {
return wp_nonce_url(
add_query_arg(
array(
'action' => self::RECHECK_ACTION,
'recheck_url' => $recheck_url,
'action' => self::RECHECK_ACTION,
),
$redirect_url
),
Expand Down
Loading

0 comments on commit 90acca4

Please sign in to comment.