Remove validation handling option by pierlon · Pull Request #3545 · ampproject/amp-wp

pierlon · 2019-10-17T04:41:46Z

Summary

Remove validation handling setting field
Remove relevant auto-sanitization notices
Remove auto_accept_sanitization option
Make AMP_Validation_Manage::is_sanitization_auto_accepted() filterable via amp_validation_error_default_sanitized. See https://gist.github.com/westonruter/c1496d668b2a73a44aa423e6547a59b7.
Update tests to use new filter to set sanitization auto acceptance

Fixes #3350.

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

includes/validation/class-amp-validation-manager.php

schlessera · 2019-10-17T06:01:13Z

includes/options/class-amp-options-menu.php

-
-			<?php if ( $forced_sanitization ) : ?>
-				<div class="notice notice-info notice-alt inline">
-					<p><?php esc_html_e( 'Your install is configured via a theme or plugin to automatically sanitize any AMP validation error that is encountered.', 'amp' ); ?></p>


@westonruter Is this information still needed? This comes from a different filter: 'amp_validation_error_sanitized'.

Good question. No, this is not needed anymore because it was only needed to explain why the checkbox is not displayed. Now that we are getting rid of the checkbox, there is no need for it.

schlessera · 2019-10-17T06:04:08Z

includes/validation/class-amp-validated-url-post-type.php

-			} elseif ( AMP_Theme_Support::TRANSITIONAL_MODE_SLUG === $template_mode ) {
-				$message = sprintf(
-					/* translators: %s is a link to the AMP settings screen */
-					__( 'The site is using transitional AMP mode without auto-sanitization, the validation errors found require action and influence which pages are shown in AMP. For automatically handling the errors turn on auto-sanitization from <a href="%s">Validation Handling settings</a>.', 'amp' ),


@westonruter Do we still need a notice/description to explain how transitional mode works?

Not here I think. I was never super happy with how we ended up adding permanent notices to the compatibility tool screens. I think the mode description should be restricted to the settings screen, or else explain contextually for each validated URL that AMP is not going to be available on it. Although we do have this to some degree currently:

schlessera · 2019-10-17T06:04:24Z

includes/validation/class-amp-validated-url-post-type.php

-					esc_url( admin_url( 'admin.php?page=' . AMP_Options_Manager::OPTION_NAME ) )
-				);
-			} else {
-				$message = __( 'The site is using AMP reader mode, your theme templates are not used and the errors below are irrelevant.', 'amp' );


@westonruter Do we still need a notice/description to explain how reader mode works?

The compatibility tool isn't directly accessible in Reader mode, so such users most likely won't even land here. Otherwise, the information about the modes should be fleshed out on the settings screen, IMO.

includes/validation/class-amp-validation-manager.php

tests/php/validation/trait-handle-validation.php

* Supply boolean as first arg to filter, unconditional based on mode. * Remove now-unnecessary special cade auto-acceptance logic for stories. * Update plugin version references from 1.3.1 to 1.4. * Add phpdoc.

includes/options/class-amp-options-manager.php

westonruter · 2019-10-17T20:59:11Z

includes/options/class-amp-options-menu.php

-
-			<?php if ( $forced_sanitization ) : ?>
-				<div class="notice notice-info notice-alt inline">
-					<p><?php esc_html_e( 'Your install is configured via a theme or plugin to automatically sanitize any AMP validation error that is encountered.', 'amp' ); ?></p>


Good question. No, this is not needed anymore because it was only needed to explain why the checkbox is not displayed. Now that we are getting rid of the checkbox, there is no need for it.

includes/validation/class-amp-validation-manager.php

westonruter · 2019-10-17T21:35:32Z

includes/validation/class-amp-validated-url-post-type.php

-			} elseif ( AMP_Theme_Support::TRANSITIONAL_MODE_SLUG === $template_mode ) {
-				$message = sprintf(
-					/* translators: %s is a link to the AMP settings screen */
-					__( 'The site is using transitional AMP mode without auto-sanitization, the validation errors found require action and influence which pages are shown in AMP. For automatically handling the errors turn on auto-sanitization from <a href="%s">Validation Handling settings</a>.', 'amp' ),


Not here I think. I was never super happy with how we ended up adding permanent notices to the compatibility tool screens. I think the mode description should be restricted to the settings screen, or else explain contextually for each validated URL that AMP is not going to be available on it. Although we do have this to some degree currently:

westonruter · 2019-10-17T21:39:00Z

includes/validation/class-amp-validated-url-post-type.php

-					esc_url( admin_url( 'admin.php?page=' . AMP_Options_Manager::OPTION_NAME ) )
-				);
-			} else {
-				$message = __( 'The site is using AMP reader mode, your theme templates are not used and the errors below are irrelevant.', 'amp' );


The compatibility tool isn't directly accessible in Reader mode, so such users most likely won't even land here. Otherwise, the information about the modes should be fleshed out on the settings screen, IMO.

Changes addressed

westonruter · 2019-10-18T00:18:06Z

Nice work. This is a great and sweeping change.

pierlon added 5 commits October 16, 2019 20:01

Remove validation handling setting field

95bec3c

Remove relevant auto-sanitization notices

6bbe197

Remove 'auto_accept_sanitization' option

f521568

Make AMP_Validation_Manage::is_sanitization_auto_accepted() filterable

8f05832

Update tests to use filter to set sanitization auto acceptance

bcb81d3

googlebot added the cla: yes Signed the Google CLA label Oct 17, 2019

pierlon requested review from schlessera and westonruter October 17, 2019 04:43

Align arrows

84f92b2

pierlon changed the title ~~Enhancement/3350 remove validation handling option~~ Remove validation handling option Oct 17, 2019

schlessera previously requested changes Oct 17, 2019

View reviewed changes

schlessera added Sanitizers Enhancement New feature or improvement of an existing one labels Oct 17, 2019

pierlon added 2 commits October 17, 2019 01:38

Add docblock for amp_is_sanitization_auto_accepted filter

3a5180b

Fix minor grammar issues

38b7b06

pierlon requested a review from schlessera October 17, 2019 06:40

Rename filter to amp_validation_error_auto_sanitized

c8384cd

* Supply boolean as first arg to filter, unconditional based on mode. * Remove now-unnecessary special cade auto-acceptance logic for stories. * Update plugin version references from 1.3.1 to 1.4. * Add phpdoc.

westonruter reviewed Oct 17, 2019

View reviewed changes

westonruter added this to the v1.4 milestone Oct 17, 2019

Rename auto-accept to accept-by-default

3ea1769

Undo over-eager rename of auto_accept_sanitization

080b20a

westonruter approved these changes Oct 17, 2019

View reviewed changes

westonruter merged commit 1f26b90 into develop Oct 18, 2019

westonruter deleted the enhancement/3350-remove-validation-handling-option branch October 18, 2019 00:17

westonruter mentioned this pull request Oct 30, 2019

Fix displaying warning in editor when there is kept invalid markup #3670

Merged

3 tasks

Conversation

pierlon commented Oct 17, 2019 • edited by westonruter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Oct 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pierlon commented Oct 17, 2019 •

edited by westonruter

Loading