Add caching of redirect to non-AMP URL when validation errors present

[westonruter]

When an AMP response in paired mode has unsanitized validation errors, the behavior is to redirect to the non-AMP version. I realized that the response cache was not applying in this case when it could be. This PR explores what that can look like.

[westonruter]

I don't think $ampless_url should be cached here, but rather a flag for whether to redirect to the non-AMP URL. The URL should be computed at runtime.

[kienstra]

Approved

Hi @westonruter,
This looks good, and it's nice how it caches in case of a redirect due to blocking errors.

[kienstra]

Good idea to cache the response when there are blocking errors and it has to redirect to a non-AMP URL.

[westonruter]

@kienstra I just made some additional changes to harden this.

Note that to test you have to make sure that your WP_DEBUG is false and you have a persistent object cache enabled. You can then confirm the caching is working for the redirect by verifying that no Server-Timing headers are in the response, like so:

HTTP/1.1 302 Found
Server: nginx
Date: Mon, 02 Jul 2018 21:58:42 GMT
Content-Type: text/html; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
Expires: Wed, 11 Jan 1984 05:00:00 GMT
Cache-Control: no-cache, must-revalidate, max-age=0
Link: <https://src.wordpress-develop.test/wp-json/>; rel="https://api.w.org/"
Link: <https://src.wordpress-develop.test/>; rel=shortlink
Location: https://src.wordpress-develop.test/?amp_validation_errors=2

Redirecting to non-AMP version.

As opposed to:

HTTP/1.1 302 Found
Server: nginx
Date: Mon, 02 Jul 2018 21:59:17 GMT
Content-Type: text/html; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
Expires: Wed, 11 Jan 1984 05:00:00 GMT
Cache-Control: no-cache, must-revalidate, max-age=0
Link: <https://src.wordpress-develop.test/wp-json/>; rel="https://api.w.org/"
Link: <https://src.wordpress-develop.test/>; rel=shortlink
Server-Timing: amp_output_buffer;desc="AMP Output Buffer";dur=276.745081
Server-Timing: amp_dom_parse;desc="AMP DOM Parse";dur=3.575087
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.284910
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=5.506992
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.113964
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.077009
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.087976
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.097036
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.088930
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.188828
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.228882
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.020027
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.241041
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.030041
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.021935
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=48.586845
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=42.685986
Location: https://src.wordpress-develop.test/?amp_validation_errors=2

Redirecting to non-AMP version.

[westonruter]

Actually, this is used to vary the cache.

[kienstra]

Server-Timing Values Not Present, As Expected

Hi @westonruter,
Sorry for the delay. Just as you described above, the Server-Timing values aren't present in the response with this PR:

Like you mentioned, before this PR, there were Server-Timing values:

[kienstra]

Moving To "Ready For Merging"

If it's alright, I'm moving this to "Ready For Merging." If you think this could use functional testing, feel free to move it back.