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.

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

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

Merged
merged 2 commits into from Jul 2, 2018

Conversation

2 participants
@westonruter
Member

westonruter commented Jun 10, 2018

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 westonruter added this to the v1.0 milestone Jun 10, 2018

@westonruter westonruter requested a review from ThierryA Jun 10, 2018

@westonruter westonruter added the beta label Jun 20, 2018

@kevincoleman kevincoleman added this to To do in v1.0 Jun 25, 2018

@westonruter westonruter moved this from To do to In progress in v1.0 Jun 26, 2018

@westonruter westonruter changed the title from [WIP] Add caching of redirect to non-AMP URL when validation errors present to Add caching of redirect to non-AMP URL when validation errors present Jul 2, 2018

@westonruter westonruter requested a review from kienstra Jul 2, 2018

@westonruter westonruter force-pushed the add/redirect-cache branch from 9053ca8 to a0c0859 Jul 2, 2018

$response = esc_html__( 'Redirecting to non-AMP version.', 'amp' );
if ( $cache_response ) {
$cache_response( $response, $validation_results, $ampless_url );

This comment has been minimized.

@westonruter

westonruter Jul 2, 2018

Member

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.

@@ -1732,12 +1756,18 @@ public static function prepare_response( $response, $args = array() ) {
);
}
$response = esc_html__( 'Redirecting to non-AMP version.', 'amp' );
if ( $cache_response ) {

This comment has been minimized.

@kienstra

kienstra Jul 2, 2018

Collaborator

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

@westonruter

This comment has been minimized.

Member

westonruter commented Jul 2, 2018

@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 westonruter force-pushed the add/redirect-cache branch from 79d2d13 to f4f6604 Jul 2, 2018

@@ -1613,12 +1613,16 @@ public static function prepare_response( $response, $args = array() ) {
&&
! AMP_Validation_Manager::should_validate_response()
),
'user_can_validate' => AMP_Validation_Manager::has_cap(),

This comment has been minimized.

@westonruter

westonruter Jul 2, 2018

Member

Actually, this is used to vary the cache.

@westonruter westonruter merged commit d9b2017 into develop Jul 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/redirect-cache branch Jul 2, 2018

@westonruter westonruter moved this from In progress to Ready for QA in v1.0 Jul 2, 2018

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Jul 3, 2018

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:

with-pr-applied

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

amp-url-redirected

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Aug 3, 2018

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.

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Aug 3, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment