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.

Sign up for GitHub

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

Improve error messages when validation requests fail #3793

Merged
merged 4 commits into from
Nov 21, 2019
Merged

Improve error messages when validation requests fail #3793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5106e74
Remove broken removal of query args during URL normalization
westonruter Nov 18, 2019
39062b7
Add missing encoding of validate URL query args in admin bar
westonruter Nov 18, 2019
aff8140
Improve error messages shown after failing to perform validation requ…
westonruter Nov 19, 2019
63d4be1
Fix PHP comments
westonruter Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 32 additions & 54 deletions includes/options/class-amp-options-manager.php
View file
Open in desktop
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' ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, love the sprintf() here... 😄

$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
Open in desktop
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 ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of array_fill_keys().

'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