Fix CachedRemoteGetRequest handling for failures

ampproject · Mar 20, 2020 · 00036be · 00036be
1 parent 8b214f8
commit 00036be
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 12 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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( '<html amp><head><meta charset="utf-8"><link rel="stylesheet" href="%s"></head><body></body></html>', 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
@@ -14,7 +14,6 @@
  */
 class Test_WP_Http_Remote_Get_Request extends \WP_UnitTestCase {
 
-
  /**
  * Provide the data to test the processing of headers.
  *