Dynamically allow AMP responses based on validation status

[westonruter]

The sanitizers are designed to force a page served as AMP to be valid, without any illegal markup or missing markup which would cause a document to be invalid and not able to be served from an AMP cache. While 1.0-alpha now has the ability to inform users of the validation errors that are occurring on a site, it doesn't currently use this information to help make decisions for the user. The sanitizer may very well remove something that completely breaks the page for the user. So what is needed is a way to detect when such a critical (deal breaker) validation error occurs, and when it does, prevent AMP from being served from a given URL. This is essentially building upon the enable/disable toggle that exists currently in the paired mode: there could in essence be an in-between behavior like “enable when possible” (which could make the whole AMP enable/disable toggle obsolete). This ties in with some ideas from #934 (Add theme support scenarios: allow paired mode to serve some content exclusively in AMP): even though a site may be Native AMP normally, when such a deal breaker validation error occurs, AMP should be disabled on a given response.

What determines what a critical validation error is? I think in general any validation error should by default be considered critical. Removing something from the page should never be done without the user approving it. So this is where #1003 comes in with the ability to acknowledge validation errors that occur on a site. If a user has acknowledged a given validation error by saying it should be ignored, then a response that includes such a validation error should continue to be served as AMP. Otherwise, by default if there is a validation error detected in a response and this validation error has not knowingly been ignored by the user, then AMP should be prevented in the response. (Likewise, if a user acknowledges a validation error as being critical, then AMP would be prevented. Acknowledging a validation error marks it as “read”.)

The default behavior for an unacknowledged validation error should be configurable so that instead of defaulting to unacknowledged errors being treated as critical, they can instead be treated as ignored. This could be accomplished by a filter.

The handling of a critical validation error would look something like this:

1. Theme enables amp support for a given request.
2. User makes first request to a given URL.
3. Sanitizer detects validation errors.
4. Validation errors are looked up to see if they have been acknowledged and ignored. A validation callback could also be provided to let a site dynamically judge whether something is critical, via #1063.
5. If all validation errors have been ignored, then the AMP response finalizes.
6. Otherwise, if there are critical validation errors then the given URL needs to be flagged as excluded from AMP and instead of the output-buffered response finalizing, the response needs to redirect to the non-AMP version of the page. (Special consideration is needed here for Native AMP sites, such as in #934.)

This whole process could be done en masse upon first activating the plugin through a sort of onboarding/wizard/setup flow when activating the plugin for the first time or switching to a new theme, per #1003 (comment):

1. I activate the AMP plugin.
2. The plugin sees that I don't have AMP theme support.
3. The plugin asks if you would like to try AMP theme support on your theme.
4. It runs validation report and lets you browse the site with AMP active just for yourself.
5. Then at the end you decide whether you want to enable AMP for the theme, or re-use the legacy post templates, or whether you want to use paired mode to selectively opt-in to certain templates being available in AMP.
6. AMP theme support is enabled by the plugin as desired.

[westonruter]

This is essentially building upon the enable/disable toggle that exists currently in the paired mode: there could in essence be an in-between behavior like “enable when possible” (which could make the whole AMP enable/disable toggle obsolete).

I think with this capability that the Enable setting would change to be Enable-when-possible. Similarly to when editing a post type today in legacy paired mode, of the post type does not have amp support, the publish metabox shows that AMP isn't available, with a link to go enable the post type. In the theme support context, however, it could rather show that AMP is not available due to validation errors. There could then be a link to review them so acknowledge the ones that should be ignored to allow amp to be enabled. In other words, we could essentially kill the manual toggle in favor of letting the toggle be automatic based on the presence of validation errors.

[westonruter]

When there are validation errors on a given URL, and the redirect happens from AMP to non-AMP, then there will need a way to force AMP so the user can still see what is going on and whether they are critical issues or not.

[kienstra]

Steps To Test

Hi @csossi,
Could you please test this?

1. Go to this post, which has an illegal <script> in the content, and other validation errors
2. Go to the front-end of that page: https://paired-ampconfdemo.pantheonsite.io/2018/06/06/test-illegal-script/?amp
3. Expected: the ?amp is removed from the URL (in a redirect), because it has validation errors
4. Return to the post in step 1.
5. You'll see something like: "There are 16 issues from AMP validation. But none are directly due to content here. Non-accepted validation errors prevent AMP from being served. Review issues"
6. Click the "Review issues" link
7. There will be <select> elements with options for "New," "Accepted," and "Rejected."
8. Choose "Accepted" for all of them, and click the "Update" button
9. Go to the front-end of the post again: https://paired-ampconfdemo.pantheonsite.io/2018/06/06/test-illegal-script/?amp
10. Expected: ?amp isn't removed from the URL (through a redirect), and the page is valid AMP

[csossi]

verified in QA