From 9737cd72f9f1545fb2e8905da57fe5239f752385 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 10 Jun 2019 20:14:00 -0700 Subject: [PATCH] Ensure auto-accepting sanitization is always performed for stories; simplify warning notice --- assets/src/block-validation/helpers/index.js | 91 ++++++++++--------- includes/class-amp-theme-support.php | 6 +- .../class-amp-validated-url-post-type.php | 10 +- .../class-amp-validation-manager.php | 2 +- 4 files changed, 60 insertions(+), 49 deletions(-) diff --git a/assets/src/block-validation/helpers/index.js b/assets/src/block-validation/helpers/index.js index 20c207c1ff6..86fd1ac6f19 100644 --- a/assets/src/block-validation/helpers/index.js +++ b/assets/src/block-validation/helpers/index.js @@ -154,6 +154,7 @@ export const updateValidationErrors = () => { export const maybeDisplayNotice = () => { const { getValidationErrors, isSanitizationAutoAccepted, getReviewLink } = select( 'amp/block-validation' ); const { createWarningNotice } = dispatch( 'core/notices' ); + const { getCurrentPost } = select( 'core/editor' ); const validationErrors = getValidationErrors(); const validationErrorCount = validationErrors.length; @@ -174,54 +175,56 @@ export const maybeDisplayNotice = () => { const blockValidationErrors = validationErrors.filter( ( { clientId } ) => clientId ); const blockValidationErrorCount = blockValidationErrors.length; - if ( blockValidationErrorCount > 0 ) { - noticeMessage += ' ' + sprintf( - /* translators: %s: number of block errors. */ - _n( - '%s issue is directly due to content here.', - '%s issues are directly due to content here.', - blockValidationErrorCount, - 'amp' - ), - blockValidationErrorCount - ); - } else if ( validationErrors.length === 1 ) { - noticeMessage += ' ' + __( 'The issue is not directly due to content here.', 'amp' ); - } else { - noticeMessage += ' ' + __( 'The issues are not directly due to content here.', 'amp' ); - } - - noticeMessage += ' '; - - if ( isSanitizationAutoAccepted() ) { - const rejectedBlockValidationErrors = blockValidationErrors.filter( ( error ) => { - return ( - VALIDATION_ERROR_NEW_REJECTED_STATUS === error.status || - VALIDATION_ERROR_ACK_REJECTED_STATUS === error.status - ); - } ); - - const rejectedValidationErrors = validationErrors.filter( ( error ) => { - return ( - VALIDATION_ERROR_NEW_REJECTED_STATUS === error.status || - VALIDATION_ERROR_ACK_REJECTED_STATUS === error.status + if ( 'amp_story' !== getCurrentPost().type ) { + if ( blockValidationErrorCount > 0 ) { + noticeMessage += ' ' + sprintf( + /* translators: %s: number of block errors. */ + _n( + '%s issue is directly due to content here.', + '%s issues are directly due to content here.', + blockValidationErrorCount, + 'amp' + ), + blockValidationErrorCount ); - } ); - - const totalRejectedErrorsCount = rejectedBlockValidationErrors.length + rejectedValidationErrors.length; + } else if ( validationErrors.length === 1 ) { + noticeMessage += ' ' + __( 'The issue is not directly due to content here.', 'amp' ); + } else { + noticeMessage += ' ' + __( 'The issues are not directly due to content here.', 'amp' ); + } - if ( totalRejectedErrorsCount === 0 ) { - noticeMessage += __( 'However, your site is configured to automatically accept sanitization of the offending markup.', 'amp' ); + noticeMessage += ' '; + + if ( isSanitizationAutoAccepted() ) { + const rejectedBlockValidationErrors = blockValidationErrors.filter( ( error ) => { + return ( + VALIDATION_ERROR_NEW_REJECTED_STATUS === error.status || + VALIDATION_ERROR_ACK_REJECTED_STATUS === error.status + ); + } ); + + const rejectedValidationErrors = validationErrors.filter( ( error ) => { + return ( + VALIDATION_ERROR_NEW_REJECTED_STATUS === error.status || + VALIDATION_ERROR_ACK_REJECTED_STATUS === error.status + ); + } ); + + const totalRejectedErrorsCount = rejectedBlockValidationErrors.length + rejectedValidationErrors.length; + + if ( totalRejectedErrorsCount === 0 ) { + noticeMessage += __( 'However, your site is configured to automatically accept sanitization of the offending markup.', 'amp' ); + } else { + noticeMessage += _n( + 'Your site is configured to automatically accept sanitization errors, but this error could be from when auto-acceptance was not selected, or from manually rejecting an error.', + 'Your site is configured to automatically accept sanitization errors, but these errors could be from when auto-acceptance was not selected, or from manually rejecting an error.', + validationErrors.length, + 'amp' + ); + } } else { - noticeMessage += _n( - 'Your site is configured to automatically accept sanitization errors, but this error could be from when auto-acceptance was not selected, or from manually rejecting an error.', - 'Your site is configured to automatically accept sanitization errors, but these errors could be from when auto-acceptance was not selected, or from manually rejecting an error.', - validationErrors.length, - 'amp' - ); + noticeMessage += __( 'Non-accepted validation errors prevent AMP from being served, and the user will be redirected to the non-AMP version.', 'amp' ); } - } else { - noticeMessage += __( 'Non-accepted validation errors prevent AMP from being served, and the user will be redirected to the non-AMP version.', 'amp' ); } const options = { diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 17fcb5d7742..63fb3cfcde0 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -306,7 +306,7 @@ public static function read_theme_support() { self::$support_added_via_theme = $is_paired ? self::TRANSITIONAL_MODE_SLUG : self::STANDARD_MODE_SLUG; /* - * If the theme has transitional support, allow the user to opt for AMP first mode via an option, since a theme + * If the theme has transitional support, allow the user to opt for AMP-first mode via an option, since a theme * in transitional mode entails that it supports serving templates as both AMP and non-AMP, and this it is * able to serve AMP-first pages just as well as paired pages. Otherwise, consider that the the mode was * not set at all via option. @@ -411,7 +411,7 @@ public static function ensure_proper_amp_location( $exit = true ) { if ( amp_is_canonical() || is_singular( AMP_Story_Post_Type::POST_TYPE_SLUG ) ) { /* - * When AMP first/canonical, then when there is an /amp/ endpoint or ?amp URL param, + * When AMP-first/canonical, then when there is an /amp/ endpoint or ?amp URL param, * then a redirect needs to be done to the URL without any AMP indicator in the URL. * Permanent redirect is used for unauthenticated users since switching between modes * should happen infrequently. For admin users, this is kept temporary to allow them @@ -2119,7 +2119,7 @@ public static function prepare_response( $response, $args = array() ) { if ( $blocking_error_count > 0 && ! AMP_Validation_Manager::should_validate_response() ) { /* - * In AMP first, strip html@amp attribute to prevent GSC from complaining about a validation error + * In AMP-first, strip html@amp attribute to prevent GSC from complaining about a validation error * already surfaced inside of WordPress. This is intended to not serve dirty AMP, but rather a * non-AMP document (intentionally not valid AMP) that contains the AMP runtime and AMP components. */ diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index 99372bf2bb2..c0c59fa40ca 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -661,6 +661,14 @@ public static function store_validation_errors( $validation_errors, $url, $args ); } + $is_story = ( + isset( $args['queried_object'], $args['queried_object']['type'], $args['queried_object']['id'] ) + && + 'post' === $args['queried_object']['type'] + && + AMP_Story_Post_Type::POST_TYPE_SLUG === get_post_type( $args['queried_object']['id'] ) + ); + /* * The details for individual validation errors is stored in the amp_validation_error taxonomy terms. * The post content just contains the slugs for these terms and the sources for the given instance of @@ -714,7 +722,7 @@ public static function store_validation_errors( $validation_errors, $url, $args 'term_group' => $sanitization['status'], ) ); - } elseif ( AMP_Validation_Manager::is_sanitization_auto_accepted() ) { + } elseif ( AMP_Validation_Manager::is_sanitization_auto_accepted() || $is_story ) { $term_data['term_group'] = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS; wp_update_term( $term_id, diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index cd0c40d6812..dbb26d9f181 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -2029,7 +2029,7 @@ public static function enqueue_block_validation() { $enabled_status = $status_and_errors['status']; $data = array( - 'isSanitizationAutoAccepted' => self::is_sanitization_auto_accepted(), + 'isSanitizationAutoAccepted' => self::is_sanitization_auto_accepted() || AMP_Story_Post_Type::POST_TYPE_SLUG === get_post_type(), 'possibleStatuses' => array( AMP_Post_Meta_Box::ENABLED_STATUS, AMP_Post_Meta_Box::DISABLED_STATUS ), 'defaultStatus' => $enabled_status, );