From 00036be3a5c05978c798b7e2519a21d97faa50f0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 20 Mar 2020 11:43:46 -0700 Subject: [PATCH] Fix CachedRemoteGetRequest handling for failures --- src/RemoteRequest/CachedData.php | 1 - src/RemoteRequest/CachedRemoteGetRequest.php | 21 +++-- src/RemoteRequest/WpHttpRemoteGetRequest.php | 2 +- tests/php/test-amp-style-sanitizer.php | 84 +++++++++++++++++++ tests/php/test-wp-http-remote-get-request.php | 1 - 5 files changed, 97 insertions(+), 12 deletions(-) diff --git a/src/RemoteRequest/CachedData.php b/src/RemoteRequest/CachedData.php index bfc0b7bb80f..df7ec8957dd 100644 --- a/src/RemoteRequest/CachedData.php +++ b/src/RemoteRequest/CachedData.php @@ -17,7 +17,6 @@ */ final class CachedData { - /** * Cached value. * diff --git a/src/RemoteRequest/CachedRemoteGetRequest.php b/src/RemoteRequest/CachedRemoteGetRequest.php index 367522d082d..205caab11c3 100644 --- a/src/RemoteRequest/CachedRemoteGetRequest.php +++ b/src/RemoteRequest/CachedRemoteGetRequest.php @@ -119,16 +119,19 @@ public function get( $url ) { } if ( false === $cached_data || $cached_data->is_expired() ) { - $response = $this->remote_request->get( $url ); - $status = $response->getStatusCode(); - $success = $status >= 200 && $status < 300; - - $expiry = $success - ? $this->get_expiry_time( $response ) - : new DateTimeImmutable( "+ {$this->min_expiry} seconds" ); + try { + $response = $this->remote_request->get( $url ); + $status = $response->getStatusCode(); + $expiry = $this->get_expiry_time( $response ); + $headers = $response->getHeaders(); + $body = $response->getBody(); + } catch ( FailedToGetFromRemoteUrl $exception ) { + $status = $exception->getStatusCode(); + $expiry = new DateTimeImmutable( "+ {$this->min_expiry} seconds" ); + $body = $exception->getMessage(); + } - $cached_data = new CachedData( $response->getBody(), $expiry ); - $headers = $response->getHeaders(); + $cached_data = new CachedData( $body, $expiry ); set_transient( $cache_key, serialize( $cached_data ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize } diff --git a/src/RemoteRequest/WpHttpRemoteGetRequest.php b/src/RemoteRequest/WpHttpRemoteGetRequest.php index fc21c6246a0..2f08364456c 100644 --- a/src/RemoteRequest/WpHttpRemoteGetRequest.php +++ b/src/RemoteRequest/WpHttpRemoteGetRequest.php @@ -77,7 +77,7 @@ final class WpHttpRemoteGetRequest implements RemoteGetRequest { * @param bool $ssl_verify Optional. Whether to verify SSL certificates. Defaults to true. * @param int $timeout Optional. Timeout value to use in seconds. Defaults to 10. * @param int $retries Optional. Number of retry attempts to do if a status code was thrown that is worth - * retrying. Defaults to 2. + * retrying. Defaults to 2. */ public function __construct( $ssl_verify = true, $timeout = self::DEFAULT_TIMEOUT, $retries = self::DEFAULT_RETRIES ) { if ( ! is_int( $timeout ) || $timeout < 0 ) { diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 850738855d7..0469dff905f 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -11,6 +11,7 @@ use AmpProject\AmpWP\RemoteRequest\CachedRemoteGetRequest; use AmpProject\Dom\Document; use AmpProject\AmpWP\Tests\PrivateAccess; +use AmpProject\Exception\FailedToGetFromRemoteUrl; /** * Test AMP_Style_Sanitizer. @@ -1612,6 +1613,89 @@ function( $preempt, $request, $url ) use ( $href, &$request_count, $response_bod $this->assertEquals( 1, $request_count, 'Expected HTTP request to be cached.' ); } + /** + * Test that external stylesheets fetches are cached when failures occur. + * + * @covers AMP_Style_Sanitizer::process_link_element() + */ + public function test_external_stylesheet_not_found() { + $request_count = 0; + $href = 'https://www.example.com/styles.css'; + + add_filter( + 'pre_http_request', + function( $preempt, $request, $url ) use ( $href, &$request_count ) { + $this->assertRegExp( '#^https?://#', $url ); + if ( set_url_scheme( $url, 'https' ) === set_url_scheme( $href, 'https' ) ) { + $request_count++; + $preempt = [ + 'response' => [ + 'code' => 404, + ], + 'headers' => [ + 'content-type' => 'text/html', + ], + 'body' => 'Not Found!', + ]; + } + return $preempt; + }, + 10, + 3 + ); + + $sanitize_and_get_stylesheets = static function() use ( $href ) { + $html = sprintf( '', esc_url( $href ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + $dom = Document::fromHtml( $html ); + + $found_error_codes = []; + + $sanitizer = new AMP_Style_Sanitizer( + $dom, + [ + 'use_document_element' => true, + 'validation_error_callback' => static function( $error ) use ( &$found_error_codes ) { + $found_error_codes[] = $error['code']; + }, + ] + ); + $sanitizer->sanitize(); + $dom->saveHTML( $dom->documentElement ); + return [ $found_error_codes, array_values( $sanitizer->get_stylesheets() ) ]; + }; + + $this->assertEquals( 0, $request_count ); + + list( $found_error_codes, $actual_stylesheets ) = $sanitize_and_get_stylesheets(); + + $this->assertEquals( [ AMP_Style_Sanitizer::STYLESHEET_FETCH_ERROR ], $found_error_codes ); + $this->assertEmpty( $actual_stylesheets ); + $this->assertEquals( 1, $request_count, 'Expected HTTP request.' ); + + $cache_key = CachedRemoteGetRequest::TRANSIENT_PREFIX . md5( CachedRemoteGetRequest::class . $href ); + $transient = get_transient( $cache_key ); + $this->assertNotFalse( $transient ); + + /** + * Cached data. + * + * @var CachedData + */ + $cached_data = unserialize( $transient ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize + $this->assertInstanceOf( CachedData::class, $cached_data ); + + $expected_exception = FailedToGetFromRemoteUrl::withHttpStatus( $href, 404 ); + $this->assertEquals( $expected_exception->getMessage(), $cached_data->get_value() ); + + // Verify that CachedRemoteGetRequest::$min_expiry is used for HTTP failures. It is set to 1 day. + $expiry = $cached_data->get_expiry(); + $this->assertLessThanOrEqual( ( new DateTimeImmutable( '+ 1 day' ) )->getTimestamp(), $expiry->getTimestamp() ); + $this->assertGreaterThan( ( new DateTimeImmutable( '+ 12 hours' ) )->getTimestamp(), $expiry->getTimestamp() ); + + $sanitize_and_get_stylesheets(); + $this->assertEquals( 1, $request_count, 'Expected HTTP request to be cached.' ); + } + /** * Get amp-keyframe styles. * diff --git a/tests/php/test-wp-http-remote-get-request.php b/tests/php/test-wp-http-remote-get-request.php index 93ce83b8b85..41cd6742f77 100644 --- a/tests/php/test-wp-http-remote-get-request.php +++ b/tests/php/test-wp-http-remote-get-request.php @@ -14,7 +14,6 @@ */ class Test_WP_Http_Remote_Get_Request extends \WP_UnitTestCase { - /** * Provide the data to test the processing of headers. *