Use Cache-Control directive for external stylesheet caching

[schlessera]

Summary

Fixes #2092

This PR does the following:

• Moves CachedRemoteGetRequest and CachedData into a subnamespace AmpProject\AmpWP\RemoteRequest
• Adds a WpHttpRemoteGetRequest implementation using the WordPress WP_Http abstraction to implement RemoteGetRequest
• Makes the CachedRemoteGetRequest implementation more robust
• Uses WpHttpRemoteGetRequest for remote requests in the optimizer
• Uses WpHttpRemoteGetRequest to fetch and cache external stylesheets

The WpHttpRemoteGetRequest implementation will respect the max-age directive of the Cache-Control header and will automatically retry failed remote requests for the following status codes:

• WP_Http::REQUEST_TIMEOUT (408)
• WP_Http::LOCKED (423)
• WP_Http::TOO_MANY_REQUESTS (429)
• WP_Http::INTERNAL_SERVER_ERROR (500)
• WP_Http::SERVICE_UNAVAILABLE (503)
• WP_Http::GATEWAY_TIMEOUT (504)

Fixes #4293

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

[westonruter]

@schlessera I'm getting warnings still, but different than before:

[18-Mar-2020 16:55:25 UTC] PHP Warning:  array_keys() expects parameter 1 to be array, object given in /app/public/content/plugins/amp/lib/common/src/RemoteRequest/RemoteGetRequestResponse.php on line 178
[18-Mar-2020 16:55:25 UTC] PHP Warning:  array_combine() expects parameter 1 to be array, null given in /app/public/content/plugins/amp/lib/common/src/RemoteRequest/RemoteGetRequestResponse.php on line 178
[18-Mar-2020 16:55:25 UTC] PHP Warning:  array_change_key_case() expects parameter 1 to be array, null given in /app/public/content/plugins/amp/lib/common/src/RemoteRequest/RemoteGetRequestResponse.php on line 179
[18-Mar-2020 16:55:25 UTC] PHP Warning:  array_key_exists() expects parameter 2 to be array, null given in /app/public/content/plugins/amp/lib/common/src/RemoteRequest/RemoteGetRequestResponse.php on line 98

I can reproduce this each time after I wp transient delete --all and then access an AMP page.

[westonruter]

I've determined the cause to be attempted passing headers as Requests_Utility_CaseInsensitiveDictionary instead of as an array. Fixed in 2f0652a 24c295f by converting to an array. You may have a better way to do that however.

[schlessera]

I'll add a regression test and fix it.

[westonruter]

Maybe check first whether it actually is an iterator object, as it could also still be an array instead.

Good point. Since is_iterable() is only available in PHP 7, what about:

$headers = is_object( $response['headers'] ) ? iterator_to_array( $response['headers'] ) : (array) $response['headers'];

Anyway, I'll leave you to it 😄

[schlessera]

The correct interface to check would be https://www.php.net/manual/en/class.traversable.php

[westonruter]

Build for testing: amp.zip (v1.5.0-alpha-20200318T200637Z-f7034c1c7)

[kienstra]

Testing looks good

Hi @westonruter and @schlessera,
This looks good!

1. An external URL that does exist (200 response) is cached as expected. It stores an expiration, and successive calls to fetch_external_stylesheet() return the cached value.

2. An external URL that doesn't exist (404 response) is cached as expected, and this doesn't retry the request again.

Here's a snippet I used to test this:

add_action(
	'wp_enqueue_scripts',
	static function() {
		wp_enqueue_style( 'example-valid-external-style', 'https://cdnjs.cloudflare.com/ajax/libs/modern-normalize/0.6.0/modern-normalize.min.css' );
		wp_enqueue_style( 'non-existent-external-style', 'https://example.com/non-existent.css' );
	}
);

Sorry for the delay on this.