Skip to content

Commit

Permalink
Improve error messages when validation requests fail (#3793)
Browse files Browse the repository at this point in the history 
* Remove broken removal of query args during URL normalization

* Add missing encoding of validate URL query args in admin bar

* Improve error messages shown after failing to perform validation requests

* Fix PHP comments

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>
  • Loading branch information
@westonruter @schlessera
westonruter and schlessera authored Nov 21, 2019
1 parent 35b414a commit be67818
Show file tree
Hide file tree
Showing 4 changed files with 169 additions and 121 deletions.
86 changes: 32 additions & 54 deletions includes/options/class-amp-options-manager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -718,23 +718,9 @@ public static function handle_updated_theme_support_option() {
$validation = AMP_Validation_Manager::validate_url( $url );

if ( is_wp_error( $validation ) ) {
$review_messages[] = esc_html(
sprintf(
/* translators: 1: error message. 2: error code. */
__( 'However, there was an error when checking the AMP validity for your site.', 'amp' ),
$validation->get_error_message(),
$validation->get_error_code()
)
);

$error_message = $validation->get_error_message();
if ( $error_message ) {
$review_messages[] = $error_message;
} else {
/* translators: %s is the error code */
$review_messages[] = esc_html( sprintf( __( 'Error code: %s.', 'amp' ), $validation->get_error_code() ) );
}
$notice_type = 'error';
$review_messages[] = esc_html__( 'However, there was an error when checking the AMP validity for your site.', 'amp' );
$review_messages[] = AMP_Validation_Manager::get_validate_url_error_message( $validation->get_error_code(), $validation->get_error_message() );
$notice_type = 'error';
} elseif ( is_array( $validation ) ) {
$new_errors = 0;
$rejected_errors = 0;
Expand All @@ -757,7 +743,7 @@ public static function handle_updated_theme_support_option() {
if ( $rejected_errors > 0 ) {
$notice_type = 'error';

$message = wp_kses_post(
$message = esc_html(
sprintf(
/* translators: %s is count of rejected errors */
_n(
Expand All @@ -772,43 +758,37 @@ public static function handle_updated_theme_support_option() {
);

if ( $invalid_url_screen_url ) {
$message .= ' ' . wp_kses_post(
sprintf(
/* translators: %s is URL to review issues */
_n(
'<a href="%s">Review Issue</a>.',
'<a href="%s">Review Issues</a>.',
$rejected_errors,
'amp'
),
esc_url( $invalid_url_screen_url )
)
$message .= ' ' . sprintf(
/* translators: %s is URL to review issues */
_n(
'<a href="%s">Review Issue</a>.',
'<a href="%s">Review Issues</a>.',
$rejected_errors,
'amp'
),
esc_url( $invalid_url_screen_url )
);
}

$review_messages[] = $message;
} else {
$message = wp_kses_post(
sprintf(
/* translators: %s is an AMP URL */
__( 'View an <a href="%s">AMP version of your site</a>.', 'amp' ),
esc_url( $url )
)
$message = sprintf(
/* translators: %s is an AMP URL */
__( 'View an <a href="%s">AMP version of your site</a>.', 'amp' ),
esc_url( $url )
);

if ( $new_errors > 0 && $invalid_url_screen_url ) {
$message .= ' ' . wp_kses_post(
sprintf(
/* translators: 1: URL to review issues. 2: count of new errors. */
_n(
'Please also <a href="%1$s">review %2$s issue</a> which may need to be fixed (for one URL at least).',
'Please also <a href="%1$s">review %2$s issues</a> which may need to be fixed (for one URL at least).',
$new_errors,
'amp'
),
esc_url( $invalid_url_screen_url ),
number_format_i18n( $new_errors )
)
$message .= ' ' . sprintf(
/* translators: 1: URL to review issues. 2: count of new errors. */
_n(
'Please also <a href="%1$s">review %2$s issue</a> which may need to be fixed (for one URL at least).',
'Please also <a href="%1$s">review %2$s issues</a> which may need to be fixed (for one URL at least).',
$new_errors,
'amp'
),
esc_url( $invalid_url_screen_url ),
number_format_i18n( $new_errors )
);
}

Expand All @@ -831,18 +811,16 @@ public static function handle_updated_theme_support_option() {
}
break;
case AMP_Theme_Support::READER_MODE_SLUG:
$message = wp_kses_post(
sprintf(
/* translators: %s is an AMP URL */
__( 'Reader mode activated! View the <a href="%s">AMP version of a recent post</a>. It is recommended that you upgrade to Standard or Transitional mode.', 'amp' ),
esc_url( $url )
)
$message = sprintf(
/* translators: %s is an AMP URL */
__( 'Reader mode activated! View the <a href="%s">AMP version of a recent post</a>. It is recommended that you upgrade to Standard or Transitional mode.', 'amp' ),
esc_url( $url )
);
break;
}

if ( isset( $message ) ) {
add_settings_error( self::OPTION_NAME, 'template_mode_updated', $message, $notice_type );
add_settings_error( self::OPTION_NAME, 'template_mode_updated', wp_kses_post( $message ), $notice_type );
}
}
}
52 changes: 29 additions & 23 deletions includes/validation/class-amp-validated-url-post-type.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,17 +631,13 @@ public static function get_url_from_post( $post ) {
/**
* Normalize a URL for storage.
*
* This ensures that query vars like utm_* and the like will not cause duplicates.
* The AMP query param is removed to facilitate switching between standard and transitional.
* The URL scheme is also normalized to HTTPS to help with transition from HTTP to HTTPS.
*
* @param string $url URL.
* @return string Normalized URL.
* @global WP $wp
*/
protected static function normalize_url_for_storage( $url ) {
global $wp;

// Only ever store the canonical version.
$url = amp_remove_endpoint( $url );

Expand All @@ -651,12 +647,11 @@ protected static function normalize_url_for_storage( $url ) {
// Normalize query args, removing all that are not recognized or which are removable.
$url_parts = explode( '?', $url, 2 );
if ( 2 === count( $url_parts ) ) {
parse_str( $url_parts[1], $args );
$args = wp_parse_args( $url_parts[1] );
foreach ( wp_removable_query_args() as $removable_query_arg ) {
unset( $args[ $removable_query_arg ] );
}
$args = wp_array_slice_assoc( $args, $wp->public_query_vars );
$url = $url_parts[0];
$url = $url_parts[0];
if ( ! empty( $args ) ) {
$url = $url_parts[0] . '?' . build_query( $args );
}
Expand Down Expand Up @@ -1231,7 +1226,7 @@ public static function handle_bulk_action( $redirect, $action, $items ) {

$validity = AMP_Validation_Manager::validate_url( $url );
if ( is_wp_error( $validity ) ) {
$errors[] = $validity->get_error_code();
$errors[] = AMP_Validation_Manager::get_validate_url_error_message( $validity->get_error_code(), $validity->get_error_message() );
continue;
}

Expand Down Expand Up @@ -1259,13 +1254,13 @@ static function( $error ) {
self::URLS_TESTED => count( $items ),
];
if ( ! empty( $errors ) ) {
$args['amp_validate_error'] = $errors;
$args['amp_validate_error'] = AMP_Validation_Manager::serialize_validation_error_messages( $errors );
} else {
$args[ self::REMAINING_ERRORS ] = count( $remaining_invalid_urls );
}

$redirect = remove_query_arg( wp_removable_query_args(), $redirect );
return add_query_arg( $args, $redirect );
return add_query_arg( rawurlencode_deep( $args ), $redirect );
}

/**
Expand All @@ -1278,14 +1273,23 @@ public static function print_admin_notice() {
return;
}

if ( isset( $_GET['amp_validate_error'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
$error_codes = array_unique( array_map( 'sanitize_key', (array) $_GET['amp_validate_error'] ) ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
foreach ( $error_codes as $error_code ) {
printf(
'<div class="notice is-dismissible error"><p>%s</p><button type="button" class="notice-dismiss"><span class="screen-reader-text">%s</span></button></div>',
esc_html( AMP_Validation_Manager::get_validate_url_error_message( $error_code ) ),
esc_html__( 'Dismiss this notice.', 'amp' )
);
if ( isset( $_GET['amp_validate_error'] ) && is_string( $_GET['amp_validate_error'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
// Note: The input var is validated by the unserialize_validation_error_messages method.
$errors = AMP_Validation_Manager::unserialize_validation_error_messages( wp_unslash( $_GET['amp_validate_error'] ) ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
if ( $errors ) {
foreach ( array_unique( $errors ) as $error_message ) {
printf(
'<div class="notice is-dismissible error"><p>%s</p><button type="button" class="notice-dismiss"><span class="screen-reader-text">%s</span></button></div>',
wp_kses(
$error_message,
[
'a' => array_fill_keys( [ 'href', 'target' ], true ),
'code' => [],
]
),
esc_html__( 'Dismiss this notice.', 'amp' )
);
}
}
}

Expand Down Expand Up @@ -1487,7 +1491,7 @@ public static function handle_validate_request() {

$validity = AMP_Validation_Manager::validate_url( $url );
if ( is_wp_error( $validity ) ) {
throw new Exception( esc_html( $validity->get_error_code() ) );
throw new Exception( AMP_Validation_Manager::get_validate_url_error_message( $validity->get_error_code(), $validity->get_error_message() ) );
}

$errors = wp_list_pluck( $validity['results'], 'error' );
Expand All @@ -1502,7 +1506,7 @@ public static function handle_validate_request() {
)
);
if ( is_wp_error( $stored ) ) {
throw new Exception( esc_html( $stored->get_error_code() ) );
throw new Exception( AMP_Validation_Manager::get_validate_url_error_message( $stored->get_error_code(), $stored->get_error_message() ) );
}
$redirect = get_edit_post_link( $stored, 'raw' );

Expand All @@ -1518,7 +1522,9 @@ static function ( $error ) {
$args[ self::URLS_TESTED ] = '1';
$args[ self::REMAINING_ERRORS ] = $error_count;
} catch ( Exception $e ) {
$args['amp_validate_error'] = $e->getMessage();
$args['amp_validate_error'] = AMP_Validation_Manager::serialize_validation_error_messages(
[ $e->getMessage() ]
);
$args[ self::URLS_TESTED ] = '0';

if ( $post && self::POST_TYPE_SLUG === $post->post_type ) {
Expand All @@ -1533,7 +1539,7 @@ static function ( $error ) {
}
}

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

Expand Down Expand Up @@ -2073,7 +2079,7 @@ public static function get_recheck_url( $url_or_post ) {
}

return wp_nonce_url(
add_query_arg( $args, admin_url() ),
add_query_arg( rawurlencode_deep( $args ), admin_url() ),
self::NONCE_ACTION
);
}
Expand Down
Loading

0 comments on commit be67818

Please sign in to comment.