Cache response status and headers when fetching external stylesheets #4509

pierlon · 2020-04-02T10:22:27Z

Summary

When an external stylesheet URL is fetched for the first time, the body (stylesheet), headers, and status code are retrieved from the response and cached. A RemoteGetRequestResponse is then returned containing the stylesheet, headers and status code from the response.

On a subsequent request to that same URL, the cached stylesheet is retrieved and a RemoteGetRequestResponse object is again returned, but this time the headers and status code being returned are set to their default values:

amp-wp/src/RemoteRequest/CachedRemoteGetRequest.php

Lines 109 to 110 in 00036be

 $headers = []; 

 $status = null;

When the style sanitizer receives this response object it checks it throws an error due to the status code being null:

amp-wp/includes/sanitizers/class-amp-style-sanitizer.php

Lines 1446 to 1451 in 5718373

 $status = $response->getStatusCode(); 

 if ( $status < 200 || $status >= 300 ) { 

 /* translators: %s: the fetched URL */ 

 return new WP_Error( "http_{$status}", sprintf( __( 'Failed to fetch: %s', 'amp' ), $url ) ); 

 }

This is then displayed to the user as the stylesheet failing to be retrieved:

This PR fixes the issue by caching the headers and status code from the first request of the stylesheet so that when it is to be retrieved again the correct response data is returned.

Fixes #4503.

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).

…eets

lib/common/src/Exception/FailedToGetCachedResponseData.php

src/RemoteRequest/CachedRemoteGetRequest.php

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

pierlon · 2020-04-02T13:33:02Z

Last commit should have been "~~Rename~~ Remove exception not being thrown".

schlessera · 2020-04-02T13:27:03Z

src/RemoteRequest/CachedRemoteGetRequest.php

@@ -101,42 +102,48 @@ public function __construct(
 *
 * @param string $url URL to get.
 * @return Response Response for the executed request.
- * @throws FailedToGetFromRemoteUrl If retrieving the contents from the URL failed.
+ * @throws FailedToGetFromRemoteUrl|FailedToGetCachedResponseData If retrieving the contents from the reponse failed.


Actually, the FailedToGetFromRemoteUrl is being caught, so the only one that might be thrown here is FailedToGetCachedResponseData.

However, that actually contradicts the interface RemoteGetRequest... So we should either adapt the interface to consider both types of exceptions, or roll everything into one agaon.

schlessera · 2020-04-02T13:27:25Z

src/RemoteRequest/CachedRemoteGetRequest.php

@@ -101,42 +102,48 @@ public function __construct(
 *
 * @param string $url URL to get.
 * @return Response Response for the executed request.
- * @throws FailedToGetFromRemoteUrl If retrieving the contents from the URL failed.
+ * @throws FailedToGetFromRemoteUrl|FailedToGetCachedResponseData If retrieving the contents from the reponse failed.


Suggested change

* @throws FailedToGetFromRemoteUrl|FailedToGetCachedResponseData If retrieving the contents from the reponse failed.

* @throws FailedToGetFromRemoteUrl|FailedToGetCachedResponseData If retrieving the contents from the response failed.

Alright, I think what should happen here is that we provide an exception interface FailedRemoteRequest and have both FailedToGetFromRemoteUrl and FailedToGetCachedResponseData implement that.

That way, we can add a @throws FailedRemoteRequest ... to the RemoteGetRequest interface, and when we provide more specific exceptions in the implementations, we won't contradict the RemoteGetRequest interface.

Done in af0b450.

schlessera · 2020-04-02T13:32:22Z

src/RemoteRequest/CachedResponse.php

+ * @param int $status_code Cached status code.
+ * @param DateTimeInterface $expiry Expiry of the cached value.
+ */
+ public function __construct( $body, array $headers, $status_code, DateTimeInterface $expiry ) {


Don't use a type declaration with array, as that is broken in PHP. There's no possibility whatsoever to extend the array into an object with added logic and still pass that type declaration. Therefore, this is a hard lock-in into the exact array type, without any leeway.

Suggested change

public function __construct( $body, array $headers, $status_code, DateTimeInterface $expiry ) {

public function __construct( $body, $headers, $status_code, DateTimeInterface $expiry ) {

Done in e87ab88.

src/RemoteRequest/CachedResponse.php

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

…o remote requests

googlebot · 2020-04-02T19:24:49Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

pierlon · 2020-04-02T19:46:47Z

lib/common/phpstan.neon.dist

@@ -8,3 +8,5 @@ parameters:
 - %currentWorkingDirectory%/src/
 autoload_files:
 - %currentWorkingDirectory%/vendor/autoload.php
+ ignoreErrors:
+ - '#^PHPDoc tag @throws with type AmpProject\\Exception\\FailedRemoteRequest is not subtype of Throwable$#'


phpstan complains about FailedRemoteRequest not being a subtype of Throwable, but Throwable is not available in PHP 5.6 so I'm ignoring it for now.

pierlon · 2020-04-03T04:02:53Z

src/RemoteRequest/CachedRemoteGetRequest.php

@@ -28,7 +30,7 @@ final class CachedRemoteGetRequest implements RemoteGetRequest {
 *
 * @var string
 */
- const TRANSIENT_PREFIX = 'amp_remote_request_';
+ const TRANSIENT_PREFIX = 'amp_remote_request_v2_';


Updated the transient prefix to bust the cache.

Might not be necessary if we check the type of the class per my comment below.

westonruter · 2020-04-03T06:10:57Z

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

@schlessera This one's for you.

schlessera · 2020-04-03T06:11:27Z

@googlebot I consent.

googlebot · 2020-04-03T06:11:36Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

westonruter · 2020-04-03T16:28:52Z

Linting problems:

FILE: src/RemoteRequest/CachedRemoteGetRequest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 107 | ERROR | Missing @throws tag for "FailedToGetCachedResponse"
     |       | exception (Squiz.Commenting.FunctionCommentThrowTag.Missing)
--------------------------------------------------------------------------------
FILE: src/RemoteRequest/WpHttpRemoteGetRequest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 103 | ERROR | Missing @throws tag for "FailedToGetFromRemoteUrl" exception
     |       | (Squiz.Commenting.FunctionCommentThrowTag.Missing)
--------------------------------------------------------------------------------

src/RemoteRequest/WpHttpRemoteGetRequest.php

src/RemoteRequest/CachedRemoteGetRequest.php

Co-Authored-By: Weston Ruter <westonruter@google.com>

westonruter

A couple minor suggestions

westonruter · 2020-04-03T17:33:17Z

src/RemoteRequest/CachedRemoteGetRequest.php

 }
 }

- if ( false === $cached_data || $cached_data->is_expired() ) {
+ if ( false === $cached_response || $cached_response->is_expired() ) {


Suggested change

if ( false === $cached_response || $cached_response->is_expired() ) {

if ( ! $cached_response instanceof CachedResponse || $cached_response->is_expired() ) {

westonruter · 2020-04-03T17:35:10Z

src/RemoteRequest/CachedRemoteGetRequest.php

+
+ return $response;


Why is this return needed?

Suggested change

return $response;

Would removing it make the logic more consistent between cached and non-cached code paths?

westonruter · 2020-04-03T17:35:53Z

src/RemoteRequest/CachedRemoteGetRequest.php

@@ -28,7 +30,7 @@ final class CachedRemoteGetRequest implements RemoteGetRequest {
 *
 * @var string
 */
- const TRANSIENT_PREFIX = 'amp_remote_request_';
+ const TRANSIENT_PREFIX = 'amp_remote_request_v2_';


Might not be necessary if we check the type of the class per my comment below.

…stylesheet

westonruter

It does fix the issue but I want to defer to @schlessera for his final approval.

kienstra · 2020-04-03T18:51:48Z

Looks Good

An external stylesheet:

add_action( 'wp_enqueue_scripts', function () {
	wp_enqueue_style( 'bulma-cdn', 'https://cdn.jsdelivr.net/npm/bulma@0.8.0/css/bulma.min.css' );
} );

...still appears after clicking 'Recheck', or refreshing a front-end URL:

Before, validation resulted in the notice 'Unable to validate URL. A white screen of death was encountered which is likely due to a PHP fatal error. ' The error in the log didn't look to be related to the STYLESHEET_FETCH_ERROR reported in #4503 (comment).

But with this PR, the error didn't appear.

…4509) * Cache response status code and headers when fetching external stylesheets * Remove whitespace * Separate throw annotations Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com> * Rename CachedData to CacheResponse * Rename exception not being thrown * Apply suggestions from Alain Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com> * Add FailedRemoteRequest interface to mark exception classes related to remote requests * Use specific exception name in docblock Co-Authored-By: Weston Ruter <westonruter@google.com> * Use specific exception name in docblock Co-Authored-By: Weston Ruter <westonruter@google.com> * Check if cached response is an instance of CachedResponse::class * Remove unused imports Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com> Co-authored-by: Weston Ruter <westonruter@google.com>

* tag '1.5.2': Bump 1.5.2 Bump version to 1.5.1-RC1 Cache response status and headers when fetching external stylesheets (#4509) Fix securing multi-line mustache templates (#4521) Add CSS monitoring time series to Site Health debugging info (#4519) Update hostname used for WordPress TV embeds to fix external HTTP requests (#4524) Fix processing of element child sanitization loop when invalid elements are replaced with children (#4512) Account for more YouTube URL formats (#4508) Update selected featured image ID on select (#4453) Raise default threshold for disabling CSS caching (#4513) Cast i-amphtml-intrinsic-sizer dimensions to integers (#4506) Only move meta tags to the head when required and add processing for meta[http-equiv] (#4505) Fix failing tests (#4507) Bump 1.5.2-alpha

…aching-reenable-button * 'develop' of github.com:ampproject/amp-wp: Restore unification of multi-page post content in Reader mode (#4547) Prevent styles from being removed when in Customizer preview with Standard mode (#4553) Omit Jetpack from being activated during PHPUnit test runs Use title case for Paired Browsing link in edit post screen (#4540) Ensure that validation query vars persist through redirects (#4544) Update dependency babel-jest to v25.2.6 (#4510) Update dependency css-loader to v3.5.0 (#4537) Update dependency autoprefixer to v9.7.6 (#4539) Add requirements to plugin file header (#4543) Force status code of validation responses to be 200 (#4533) Update optimizer test specs (#4527) Bump stable tag to 1.5.2 Cache response status and headers when fetching external stylesheets (#4509) Fix securing multi-line mustache templates (#4521) Add CSS monitoring time series to Site Health debugging info (#4519) Update hostname used for WordPress TV embeds to fix external HTTP requests (#4524) Mock Imgur embed tests Mock Facebook embed tests Standardize file and class names for embed handlers

Cache response status code and headers when fetching external stylesh…

75a98d2

…eets

googlebot added the cla: yes Signed the Google CLA label Apr 2, 2020

pierlon requested review from westonruter and schlessera April 2, 2020 10:22

Remove whitespace

b42340c

schlessera requested changes Apr 2, 2020

View reviewed changes

pierlon and others added 2 commits April 2, 2020 06:55

Separate throw annotations

78b7bcd

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Rename CachedData to CacheResponse

83a14b3

pierlon requested a review from schlessera April 2, 2020 12:41

Rename exception not being thrown

a1203b5

schlessera requested changes Apr 2, 2020

View reviewed changes

westonruter added this to the v1.5.2 milestone Apr 2, 2020

pierlon and others added 2 commits April 2, 2020 13:42

Apply suggestions from Alain

e87ab88

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Add FailedRemoteRequest interface to mark exception classes related t…

af0b450

…o remote requests

googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Apr 2, 2020

pierlon commented Apr 2, 2020

View reviewed changes

pierlon requested a review from schlessera April 2, 2020 20:24

westonruter assigned schlessera Apr 3, 2020

pierlon commented Apr 3, 2020

View reviewed changes

googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Apr 3, 2020

westonruter requested changes Apr 3, 2020

View reviewed changes

src/RemoteRequest/WpHttpRemoteGetRequest.php Outdated Show resolved Hide resolved

src/RemoteRequest/CachedRemoteGetRequest.php Outdated Show resolved Hide resolved

Use specific exception name in docblock

d940b45

Co-Authored-By: Weston Ruter <westonruter@google.com>

Use specific exception name in docblock

2784270

Co-Authored-By: Weston Ruter <westonruter@google.com>

westonruter requested changes Apr 3, 2020

View reviewed changes

pierlon added 2 commits April 3, 2020 13:45

Check if cached response is an instance of CachedResponse::class

fab8ef3

Merge remote-tracking branch 'origin/develop' into fix/4503-external-…

fcc435c

…stylesheet

westonruter approved these changes Apr 3, 2020

View reviewed changes

Remove unused imports

be928c5

schlessera approved these changes Apr 3, 2020

View reviewed changes

kienstra approved these changes Apr 3, 2020

View reviewed changes

westonruter merged commit 0e8a50c into develop Apr 3, 2020

westonruter deleted the fix/4503-external-stylesheet branch April 3, 2020 18:53

schlessera added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache response status and headers when fetching external stylesheets #4509

Cache response status and headers when fetching external stylesheets #4509

pierlon commented Apr 2, 2020 •

edited

Loading

pierlon commented Apr 2, 2020

schlessera Apr 2, 2020

schlessera Apr 2, 2020

schlessera Apr 2, 2020

pierlon Apr 2, 2020

schlessera Apr 2, 2020

pierlon Apr 2, 2020

googlebot commented Apr 2, 2020

pierlon Apr 2, 2020

pierlon Apr 3, 2020

westonruter Apr 3, 2020

westonruter commented Apr 3, 2020

schlessera commented Apr 3, 2020

googlebot commented Apr 3, 2020

westonruter commented Apr 3, 2020

westonruter left a comment

westonruter Apr 3, 2020

westonruter Apr 3, 2020

westonruter Apr 3, 2020

westonruter left a comment

kienstra commented Apr 3, 2020

	$status = $response->getStatusCode();

	if ( $status < 200 \|\| $status >= 300 ) {
	/* translators: %s: the fetched URL */
	return new WP_Error( "http_{$status}", sprintf( __( 'Failed to fetch: %s', 'amp' ), $url ) );
	}

	* @throws FailedToGetFromRemoteUrl\|FailedToGetCachedResponseData If retrieving the contents from the reponse failed.
	* @throws FailedToGetFromRemoteUrl\|FailedToGetCachedResponseData If retrieving the contents from the response failed.

	public function __construct( $body, array $headers, $status_code, DateTimeInterface $expiry ) {
	public function __construct( $body, $headers, $status_code, DateTimeInterface $expiry ) {

	if ( false === $cached_response \|\| $cached_response->is_expired() ) {
	if ( ! $cached_response instanceof CachedResponse \|\| $cached_response->is_expired() ) {

Cache response status and headers when fetching external stylesheets #4509

Cache response status and headers when fetching external stylesheets #4509

Conversation

pierlon commented Apr 2, 2020 • edited Loading

Summary

Checklist

pierlon commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Apr 3, 2020

schlessera commented Apr 3, 2020

googlebot commented Apr 3, 2020

westonruter commented Apr 3, 2020

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

kienstra commented Apr 3, 2020

pierlon commented Apr 2, 2020 •

edited

Loading