From 75a98d218c98b7763dadd4ccb47c622284980c0d Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 2 Apr 2020 06:20:18 -0400 Subject: [PATCH 01/11] Cache response status code and headers when fetching external stylesheets --- .../class-amp-validation-error-taxonomy.php | 4 + .../FailedToGetCachedResponseData.php | 29 ++++ src/RemoteRequest/CachedRemoteGetRequest.php | 23 +++- tests/php/test-amp-style-sanitizer.php | 124 +++++++++++++----- 4 files changed, 143 insertions(+), 37 deletions(-) create mode 100644 lib/common/src/Exception/FailedToGetCachedResponseData.php diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 29fccdaaf69..8a6501b67cd 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -3344,6 +3344,10 @@ public static function get_source_key_label( $key, $validation_error ) { return __( 'Invalid attribute', 'amp' ); case 'required_attr_value': return __( 'Required attribute value', 'amp' ); + case 'url': + return __( 'URL', 'amp' ); + case 'message': + return __( 'Message', 'amp' ); default: return $key; } diff --git a/lib/common/src/Exception/FailedToGetCachedResponseData.php b/lib/common/src/Exception/FailedToGetCachedResponseData.php new file mode 100644 index 00000000000..fb6db56019c --- /dev/null +++ b/lib/common/src/Exception/FailedToGetCachedResponseData.php @@ -0,0 +1,29 @@ += 7 ) { @@ -126,17 +126,26 @@ public function get( $url ) { $headers = $response->getHeaders(); $body = $response->getBody(); } catch ( FailedToGetFromRemoteUrl $exception ) { - $status = $exception->getStatusCode(); - $expiry = new DateTimeImmutable( "+ {$this->min_expiry} seconds" ); - $body = $exception->getMessage(); + $status = $exception->getStatusCode(); + $expiry = new DateTimeImmutable( "+ {$this->min_expiry} seconds" ); + $body = $exception->getMessage(); + $response = new RemoteGetRequestResponse( $body, $headers, $status ); } - $cached_data = new CachedData( $body, $expiry ); + $cached_data = new CachedData( compact( 'body', 'status', 'headers' ), $expiry ); set_transient( $cache_key, serialize( $cached_data ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + + return $response; + } + + $cached_value = $cached_data->get_value(); + + if ( ! isset( $cached_value['body'], $cached_value['headers'], $cached_value['status'] ) ) { + throw new FailedToGetCachedResponseData( $url ); } - return new RemoteGetRequestResponse( $cached_data->get_value(), $headers, $status ); + return new RemoteGetRequestResponse( $cached_value['body'], $cached_value['headers'], $cached_value['status'] ); } /** diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index be7a6ade443..d2011e923d2 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -1587,22 +1587,24 @@ public function test_external_stylesheet_cache_control() { $request_count = 0; $href = 'https://www.example.com/styles.css'; $response_body = 'body{color:red}'; + $headers = [ + 'content-type' => 'text/css', + 'cache-control' => 'max-age=' . ( YEAR_IN_SECONDS + MONTH_IN_SECONDS ), + ]; + $status_code = 200; add_filter( 'pre_http_request', - function( $preempt, $request, $url ) use ( $href, &$request_count, $response_body ) { + function( $preempt, $request, $url ) use ( $href, &$request_count, $response_body, $headers, $status_code ) { $this->assertRegExp( '#^https?://#', $url ); if ( set_url_scheme( $url, 'https' ) === set_url_scheme( $href, 'https' ) ) { $request_count++; $preempt = [ 'response' => [ - 'code' => 200, - ], - 'headers' => [ - 'content-type' => 'text/css', - 'cache-control' => 'max-age=' . ( YEAR_IN_SECONDS + MONTH_IN_SECONDS ), + 'code' => $status_code, ], - 'body' => $response_body, + 'headers' => $headers, + 'body' => $response_body, ]; } return $preempt; @@ -1652,7 +1654,14 @@ function( $preempt, $request, $url ) use ( $href, &$request_count, $response_bod $cached_data = unserialize( $transient ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize $this->assertInstanceOf( CachedData::class, $cached_data ); - $this->assertEquals( $response_body, $cached_data->get_value() ); + $this->assertEquals( + [ + 'body' => $response_body, + 'headers' => $headers, + 'status' => $status_code, + ], + $cached_data->get_value() + ); $expiry = $cached_data->get_expiry(); $this->assertGreaterThan( ( new DateTimeImmutable( '+ 1 year' ) )->getTimestamp(), $expiry->getTimestamp() ); @@ -1662,28 +1671,86 @@ function( $preempt, $request, $url ) use ( $href, &$request_count, $response_bod } /** - * Test that external stylesheets fetches are cached when failures occur. + * Data for test_external_stylesheet() * + * @return array + */ + public function get_external_stylesheet_data() { + return [ + 'successful' => [ + 'style_url' => 'https://www.example.com/styles.css', + 'http_response' => [ + 'body' => 'body { color: #fff }', + 'code' => 200, + 'headers' => [ + 'cache-control' => 'max-age=1441', + 'content-type' => 'text/css', + ], + ], + 'expected_styles' => [ 'body{color:#fff}' ], + 'expected_errors' => [], + 'cached_data' => new CachedData( + [ + 'body' => 'body { color: #fff }', + 'headers' => [ + 'cache-control' => 'max-age=1441', + 'content-type' => 'text/css', + ], + 'status' => 200, + ], + new DateTimeImmutable( '+ 1441 seconds' ) + ), + ], + 'failed' => [ + 'style_url' => 'https://www.example.com/not-found/styles.css', + 'http_response' => [ + 'body' => 'Not Found!', + 'code' => 404, + 'headers' => [ + 'content-type' => 'text/html', + ], + ], + 'expected_styles' => [], + 'expected_errors' => [ AMP_Style_Sanitizer::STYLESHEET_FETCH_ERROR ], + 'cached_data' => new CachedData( + [ + 'body' => FailedToGetFromRemoteUrl::withHttpStatus( 'https://www.example.com/not-found/styles.css', 404 )->getMessage(), + 'headers' => [], + 'status' => 404, + ], + new DateTimeImmutable( '+ ' . DAY_IN_SECONDS . ' seconds' ) + ), + ], + ]; + } + + /** + * Test that external stylesheets fetches are cached. + * + * @dataProvider get_external_stylesheet_data * @covers AMP_Style_Sanitizer::process_link_element() + * + * @param string $style_url Stylesheet URL. + * @param array $http_response Mocked HTTP response. + * @param array $expected_styles Expected minified stylesheets. + * @param array $expected_errors Expected error codes. + * @param CachedData $expected_cached_data Expected cache data. */ - public function test_external_stylesheet_not_found() { + public function test_external_stylesheet( $style_url, $http_response, $expected_styles, $expected_errors, $expected_cached_data ) { $request_count = 0; - $href = 'https://www.example.com/styles.css'; add_filter( 'pre_http_request', - function( $preempt, $request, $url ) use ( $href, &$request_count ) { + function( $preempt, $request, $url ) use ( $style_url, $http_response, &$request_count ) { $this->assertRegExp( '#^https?://#', $url ); - if ( set_url_scheme( $url, 'https' ) === set_url_scheme( $href, 'https' ) ) { + if ( set_url_scheme( $url, 'https' ) === set_url_scheme( $style_url, 'https' ) ) { $request_count++; $preempt = [ 'response' => [ - 'code' => 404, - ], - 'headers' => [ - 'content-type' => 'text/html', + 'code' => $http_response['code'], ], - 'body' => 'Not Found!', + 'headers' => $http_response['headers'], + 'body' => $http_response['body'], ]; } return $preempt; @@ -1692,8 +1759,8 @@ function( $preempt, $request, $url ) use ( $href, &$request_count ) { 3 ); - $sanitize_and_get_stylesheets = static function() use ( $href ) { - $html = sprintf( '', esc_url( $href ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + $sanitize_and_get_stylesheets = static function( $css_url ) { + $html = sprintf( '', esc_url( $css_url ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet $dom = Document::fromHtml( $html ); $found_error_codes = []; @@ -1714,13 +1781,13 @@ function( $preempt, $request, $url ) use ( $href, &$request_count ) { $this->assertEquals( 0, $request_count ); - list( $found_error_codes, $actual_stylesheets ) = $sanitize_and_get_stylesheets(); + list( $found_error_codes, $actual_stylesheets ) = $sanitize_and_get_stylesheets( $style_url ); - $this->assertEquals( [ AMP_Style_Sanitizer::STYLESHEET_FETCH_ERROR ], $found_error_codes ); - $this->assertEmpty( $actual_stylesheets ); + $this->assertEquals( $expected_errors, $found_error_codes ); + $this->assertEquals( $expected_styles, $actual_stylesheets ); $this->assertEquals( 1, $request_count, 'Expected HTTP request.' ); - $cache_key = CachedRemoteGetRequest::TRANSIENT_PREFIX . md5( CachedRemoteGetRequest::class . $href ); + $cache_key = CachedRemoteGetRequest::TRANSIENT_PREFIX . md5( CachedRemoteGetRequest::class . $style_url ); $transient = get_transient( $cache_key ); $this->assertNotFalse( $transient ); @@ -1732,15 +1799,12 @@ function( $preempt, $request, $url ) use ( $href, &$request_count ) { $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() ); + $this->assertEquals( $expected_cached_data->get_value(), $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() ); + $this->assertEquals( $cached_data->get_expiry()->getTimestamp(), $expiry->getTimestamp() ); - $sanitize_and_get_stylesheets(); + $sanitize_and_get_stylesheets( $style_url ); $this->assertEquals( 1, $request_count, 'Expected HTTP request to be cached.' ); } From b42340c894aea9bf531b1d458e5bbc9e6dad7bc4 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 2 Apr 2020 06:42:45 -0400 Subject: [PATCH 02/11] Remove whitespace --- lib/common/src/Exception/FailedToGetCachedResponseData.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/common/src/Exception/FailedToGetCachedResponseData.php b/lib/common/src/Exception/FailedToGetCachedResponseData.php index fb6db56019c..f1fde591bdc 100644 --- a/lib/common/src/Exception/FailedToGetCachedResponseData.php +++ b/lib/common/src/Exception/FailedToGetCachedResponseData.php @@ -1,9 +1,7 @@ Date: Thu, 2 Apr 2020 06:55:08 -0400 Subject: [PATCH 03/11] Separate throw annotations Co-Authored-By: Alain Schlesser --- src/RemoteRequest/CachedRemoteGetRequest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/RemoteRequest/CachedRemoteGetRequest.php b/src/RemoteRequest/CachedRemoteGetRequest.php index 2e1db808899..98bde26f8d9 100644 --- a/src/RemoteRequest/CachedRemoteGetRequest.php +++ b/src/RemoteRequest/CachedRemoteGetRequest.php @@ -102,7 +102,8 @@ public function __construct( * * @param string $url URL to get. * @return Response Response for the executed request. - * @throws FailedToGetFromRemoteUrl|FailedToGetCachedResponseData If retrieving the contents from the URL failed. + * @throws FailedToGetFromRemoteUrl If retrieving the contents from the URL failed. + * @throws FailedToGetCachedResponseData If retrieving the contents from the cache failed. */ public function get( $url ) { $cache_key = self::TRANSIENT_PREFIX . md5( __CLASS__ . $url ); From 83a14b318d375744169d7d0e5a8551f40666c762 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 2 Apr 2020 08:05:49 -0400 Subject: [PATCH 04/11] Rename CachedData to CacheResponse --- src/RemoteRequest/CachedData.php | 71 ----------- src/RemoteRequest/CachedRemoteGetRequest.php | 29 +++-- src/RemoteRequest/CachedResponse.php | 117 +++++++++++++++++++ tests/php/test-amp-style-sanitizer.php | 73 ++++++------ 4 files changed, 163 insertions(+), 127 deletions(-) delete mode 100644 src/RemoteRequest/CachedData.php create mode 100644 src/RemoteRequest/CachedResponse.php diff --git a/src/RemoteRequest/CachedData.php b/src/RemoteRequest/CachedData.php deleted file mode 100644 index df7ec8957dd..00000000000 --- a/src/RemoteRequest/CachedData.php +++ /dev/null @@ -1,71 +0,0 @@ -value = $value; - $this->expiry = $expiry; - } - - /** - * Get the cache value. - * - * @return mixed Cached value. - */ - public function get_value() { - return $this->value; - } - - /** - * Get the expiry of the cached value. - * - * @return DateTimeInterface Expiry of the cached value. - */ - public function get_expiry() { - return $this->expiry; - } - - /** - * Check whether the cached value is expired. - * - * @return bool Whether the cached value is expired. - */ - public function is_expired() { - return new DateTimeImmutable( 'now' ) > $this->expiry; - } -} diff --git a/src/RemoteRequest/CachedRemoteGetRequest.php b/src/RemoteRequest/CachedRemoteGetRequest.php index 98bde26f8d9..65cee8976bf 100644 --- a/src/RemoteRequest/CachedRemoteGetRequest.php +++ b/src/RemoteRequest/CachedRemoteGetRequest.php @@ -29,7 +29,7 @@ final class CachedRemoteGetRequest implements RemoteGetRequest { * * @var string */ - const TRANSIENT_PREFIX = 'amp_remote_request_'; + const TRANSIENT_PREFIX = 'amp_remote_request_v2_'; /** * Cache control header directive name. @@ -102,24 +102,23 @@ 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 FailedToGetCachedResponseData If retrieving the contents from the cache failed. + * @throws FailedToGetFromRemoteUrl|FailedToGetCachedResponseData If retrieving the contents from the reponse failed. */ public function get( $url ) { - $cache_key = self::TRANSIENT_PREFIX . md5( __CLASS__ . $url ); - $cached_data = get_transient( $cache_key ); - $headers = []; + $cache_key = self::TRANSIENT_PREFIX . md5( __CLASS__ . $url ); + $cached_response = get_transient( $cache_key ); + $headers = []; - if ( false !== $cached_data ) { + if ( false !== $cached_response ) { if ( PHP_MAJOR_VERSION >= 7 ) { - $cached_data = unserialize( $cached_data, [ CachedData::class, DateTimeImmutable::class ] ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize,PHPCompatibility.FunctionUse.NewFunctionParameters.unserialize_optionsFound + $cached_response = unserialize( $cached_response, [ CachedResponse::class, DateTimeImmutable::class ] ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize,PHPCompatibility.FunctionUse.NewFunctionParameters.unserialize_optionsFound } else { // PHP 5.6 does not provide the second $options argument yet. - $cached_data = unserialize( $cached_data ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize + $cached_response = unserialize( $cached_response ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize } } - if ( false === $cached_data || $cached_data->is_expired() ) { + if ( false === $cached_response || $cached_response->is_expired() ) { try { $response = $this->remote_request->get( $url ); $status = $response->getStatusCode(); @@ -133,20 +132,18 @@ public function get( $url ) { $response = new RemoteGetRequestResponse( $body, $headers, $status ); } - $cached_data = new CachedData( compact( 'body', 'status', 'headers' ), $expiry ); + $cached_response = new CachedResponse( $body, $headers, $status, $expiry ); - set_transient( $cache_key, serialize( $cached_data ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + set_transient( $cache_key, serialize( $cached_response ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize return $response; } - $cached_value = $cached_data->get_value(); - - if ( ! isset( $cached_value['body'], $cached_value['headers'], $cached_value['status'] ) ) { + if ( ! $cached_response->is_valid() ) { throw new FailedToGetCachedResponseData( $url ); } - return new RemoteGetRequestResponse( $cached_value['body'], $cached_value['headers'], $cached_value['status'] ); + return new RemoteGetRequestResponse( $cached_response->get_body(), $cached_response->get_headers(), $cached_response->get_status_code() ); } /** diff --git a/src/RemoteRequest/CachedResponse.php b/src/RemoteRequest/CachedResponse.php new file mode 100644 index 00000000000..1298849aa93 --- /dev/null +++ b/src/RemoteRequest/CachedResponse.php @@ -0,0 +1,117 @@ +body = $body; + $this->headers = $headers; + $this->status_code = $status_code; + $this->expiry = $expiry; + } + + /** + * Get the cached body. + * + * @return string Cached body. + */ + public function get_body() { + return $this->body; + } + + /** + * Get the cached headers. + * + * @return array Cached headers. + */ + public function get_headers() { + return $this->headers; + } + + /** + * Get the cached status code. + * + * @return int Cached status code. + */ + public function get_status_code() { + return $this->status_code; + } + + /** + * Determine the validity of the cached response. + * + * @return bool Whether the cached response if valid. + */ + public function is_valid() { + // $this->headers is typed so no need for sanity check. + return null !== $this->body && is_int( $this->status_code ); + } + + /** + * Get the expiry of the cached value. + * + * @return DateTimeInterface Expiry of the cached value. + */ + public function get_expiry() { + return $this->expiry; + } + + /** + * Check whether the cached value is expired. + * + * @return bool Whether the cached value is expired. + */ + public function is_expired() { + return new DateTimeImmutable( 'now' ) > $this->expiry; + } +} diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index d2011e923d2..7f912e32a10 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -7,7 +7,7 @@ // phpcs:disable WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned -use AmpProject\AmpWP\RemoteRequest\CachedData; +use AmpProject\AmpWP\RemoteRequest\CachedResponse; use AmpProject\AmpWP\RemoteRequest\CachedRemoteGetRequest; use AmpProject\AmpWP\Tests\AssertContainsCompatibility; use AmpProject\Dom\Document; @@ -1647,23 +1647,18 @@ function( $preempt, $request, $url ) use ( $href, &$request_count, $response_bod $this->assertNotFalse( $transient ); /** - * Cached data. + * Cached response. * - * @var CachedData + * @var CachedResponse */ - $cached_data = unserialize( $transient ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize - $this->assertInstanceOf( CachedData::class, $cached_data ); + $cached_response = unserialize( $transient ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize + $this->assertInstanceOf( CachedResponse::class, $cached_response ); - $this->assertEquals( - [ - 'body' => $response_body, - 'headers' => $headers, - 'status' => $status_code, - ], - $cached_data->get_value() - ); + $this->assertEquals( $response_body, $cached_response->get_body() ); + $this->assertEquals( $headers, $cached_response->get_headers() ); + $this->assertEquals( $status_code, $cached_response->get_status_code() ); - $expiry = $cached_data->get_expiry(); + $expiry = $cached_response->get_expiry(); $this->assertGreaterThan( ( new DateTimeImmutable( '+ 1 year' ) )->getTimestamp(), $expiry->getTimestamp() ); $sanitize_and_get_stylesheets(); @@ -1689,15 +1684,13 @@ public function get_external_stylesheet_data() { ], 'expected_styles' => [ 'body{color:#fff}' ], 'expected_errors' => [], - 'cached_data' => new CachedData( + 'cached_data' => new CachedResponse( + 'body { color: #fff }', [ - 'body' => 'body { color: #fff }', - 'headers' => [ - 'cache-control' => 'max-age=1441', - 'content-type' => 'text/css', - ], - 'status' => 200, + 'cache-control' => 'max-age=1441', + 'content-type' => 'text/css', ], + 200, new DateTimeImmutable( '+ 1441 seconds' ) ), ], @@ -1712,12 +1705,10 @@ public function get_external_stylesheet_data() { ], 'expected_styles' => [], 'expected_errors' => [ AMP_Style_Sanitizer::STYLESHEET_FETCH_ERROR ], - 'cached_data' => new CachedData( - [ - 'body' => FailedToGetFromRemoteUrl::withHttpStatus( 'https://www.example.com/not-found/styles.css', 404 )->getMessage(), - 'headers' => [], - 'status' => 404, - ], + 'cached_data' => new CachedResponse( + FailedToGetFromRemoteUrl::withHttpStatus( 'https://www.example.com/not-found/styles.css', 404 )->getMessage(), + [], + 404, new DateTimeImmutable( '+ ' . DAY_IN_SECONDS . ' seconds' ) ), ], @@ -1730,13 +1721,13 @@ public function get_external_stylesheet_data() { * @dataProvider get_external_stylesheet_data * @covers AMP_Style_Sanitizer::process_link_element() * - * @param string $style_url Stylesheet URL. - * @param array $http_response Mocked HTTP response. - * @param array $expected_styles Expected minified stylesheets. - * @param array $expected_errors Expected error codes. - * @param CachedData $expected_cached_data Expected cache data. + * @param string $style_url Stylesheet URL. + * @param array $http_response Mocked HTTP response. + * @param array $expected_styles Expected minified stylesheets. + * @param array $expected_errors Expected error codes. + * @param CachedResponse $expected_cached_response Expected cache response. */ - public function test_external_stylesheet( $style_url, $http_response, $expected_styles, $expected_errors, $expected_cached_data ) { + public function test_external_stylesheet( $style_url, $http_response, $expected_styles, $expected_errors, $expected_cached_response ) { $request_count = 0; add_filter( @@ -1792,17 +1783,19 @@ function( $preempt, $request, $url ) use ( $style_url, $http_response, &$request $this->assertNotFalse( $transient ); /** - * Cached data. + * Cached response. * - * @var CachedData + * @var CachedResponse */ - $cached_data = unserialize( $transient ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize - $this->assertInstanceOf( CachedData::class, $cached_data ); + $cached_response = unserialize( $transient ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize + $this->assertInstanceOf( CachedResponse::class, $cached_response ); - $this->assertEquals( $expected_cached_data->get_value(), $cached_data->get_value() ); + $this->assertEquals( $expected_cached_response->get_body(), $cached_response->get_body() ); + $this->assertEquals( $expected_cached_response->get_headers(), $cached_response->get_headers() ); + $this->assertEquals( $expected_cached_response->get_status_code(), $cached_response->get_status_code() ); - $expiry = $cached_data->get_expiry(); - $this->assertEquals( $cached_data->get_expiry()->getTimestamp(), $expiry->getTimestamp() ); + $expiry = $cached_response->get_expiry(); + $this->assertEquals( $cached_response->get_expiry()->getTimestamp(), $expiry->getTimestamp() ); $sanitize_and_get_stylesheets( $style_url ); $this->assertEquals( 1, $request_count, 'Expected HTTP request to be cached.' ); From a1203b59a14736e580a19dda2d405cbf201dd67f Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 2 Apr 2020 09:26:51 -0400 Subject: [PATCH 05/11] Rename exception not being thrown --- src/RemoteRequest/CachedRemoteGetRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/RemoteRequest/CachedRemoteGetRequest.php b/src/RemoteRequest/CachedRemoteGetRequest.php index 65cee8976bf..d2d7d54d4bc 100644 --- a/src/RemoteRequest/CachedRemoteGetRequest.php +++ b/src/RemoteRequest/CachedRemoteGetRequest.php @@ -102,7 +102,7 @@ public function __construct( * * @param string $url URL to get. * @return Response Response for the executed request. - * @throws FailedToGetFromRemoteUrl|FailedToGetCachedResponseData If retrieving the contents from the reponse failed. + * @throws FailedToGetCachedResponseData If retrieving the contents from the cache failed. */ public function get( $url ) { $cache_key = self::TRANSIENT_PREFIX . md5( __CLASS__ . $url ); From e87ab886e321bb181adb3668db9fdf01c77bcee3 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 2 Apr 2020 13:42:32 -0400 Subject: [PATCH 06/11] Apply suggestions from Alain Co-Authored-By: Alain Schlesser --- src/RemoteRequest/CachedResponse.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/RemoteRequest/CachedResponse.php b/src/RemoteRequest/CachedResponse.php index 1298849aa93..194cfadd91a 100644 --- a/src/RemoteRequest/CachedResponse.php +++ b/src/RemoteRequest/CachedResponse.php @@ -49,14 +49,14 @@ final class CachedResponse { * Instantiate a CachedResponse object. * * @param string $body Cached body. - * @param array $headers Cached headers. + * @param string[] $headers Associative array of cached headers. * @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 ) { - $this->body = $body; - $this->headers = $headers; - $this->status_code = $status_code; + public function __construct( $body, $headers, $status_code, DateTimeInterface $expiry ) { + $this->body = (string) $body; + $this->headers = (array) $headers; + $this->status_code = (int) $status_code; $this->expiry = $expiry; } @@ -72,7 +72,7 @@ public function get_body() { /** * Get the cached headers. * - * @return array Cached headers. + * @return string[] Cached headers. */ public function get_headers() { return $this->headers; @@ -90,11 +90,11 @@ public function get_status_code() { /** * Determine the validity of the cached response. * - * @return bool Whether the cached response if valid. + * @return bool Whether the cached response is valid. */ public function is_valid() { - // $this->headers is typed so no need for sanity check. - return null !== $this->body && is_int( $this->status_code ); + // Values are already typed, so we just control the status code for validity. + return $this->status_code > 100 && $this->status_code <= 599; } /** From af0b45013ee29a6704cf4b71504fde718ef7299d Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 2 Apr 2020 14:39:32 -0400 Subject: [PATCH 07/11] Add FailedRemoteRequest interface to mark exception classes related to remote requests --- lib/common/phpstan.neon.dist | 2 ++ lib/common/src/Exception/FailedRemoteRequest.php | 13 +++++++++++++ ...sponseData.php => FailedToGetCachedResponse.php} | 4 ++-- .../src/Exception/FailedToGetFromRemoteUrl.php | 2 +- lib/common/src/RemoteGetRequest.php | 4 ++-- .../src/RemoteRequest/CurlRemoteGetRequest.php | 3 ++- .../src/RemoteRequest/FallbackRemoteGetRequest.php | 4 ++-- .../RemoteRequest/FilesystemRemoteGetRequest.php | 3 ++- .../src/RemoteRequest/StubbedRemoteGetRequest.php | 4 ++-- src/RemoteRequest/CachedRemoteGetRequest.php | 7 ++++--- src/RemoteRequest/WpHttpRemoteGetRequest.php | 3 ++- 11 files changed, 34 insertions(+), 15 deletions(-) create mode 100644 lib/common/src/Exception/FailedRemoteRequest.php rename lib/common/src/Exception/{FailedToGetCachedResponseData.php => FailedToGetCachedResponse.php} (73%) diff --git a/lib/common/phpstan.neon.dist b/lib/common/phpstan.neon.dist index 1857a8b4809..fb2b62bf871 100644 --- a/lib/common/phpstan.neon.dist +++ b/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$#' diff --git a/lib/common/src/Exception/FailedRemoteRequest.php b/lib/common/src/Exception/FailedRemoteRequest.php new file mode 100644 index 00000000000..0a03765943b --- /dev/null +++ b/lib/common/src/Exception/FailedRemoteRequest.php @@ -0,0 +1,13 @@ +is_valid() ) { - throw new FailedToGetCachedResponseData( $url ); + throw new FailedToGetCachedResponse( $url ); } return new RemoteGetRequestResponse( $cached_response->get_body(), $cached_response->get_headers(), $cached_response->get_status_code() ); diff --git a/src/RemoteRequest/WpHttpRemoteGetRequest.php b/src/RemoteRequest/WpHttpRemoteGetRequest.php index 2f08364456c..f1e5d73768e 100644 --- a/src/RemoteRequest/WpHttpRemoteGetRequest.php +++ b/src/RemoteRequest/WpHttpRemoteGetRequest.php @@ -7,6 +7,7 @@ namespace AmpProject\AmpWP\RemoteRequest; +use AmpProject\Exception\FailedRemoteRequest; use AmpProject\Exception\FailedToGetFromRemoteUrl; use AmpProject\RemoteGetRequest; use AmpProject\RemoteRequest\RemoteGetRequestResponse; @@ -98,7 +99,7 @@ public function __construct( $ssl_verify = true, $timeout = self::DEFAULT_TIMEOU * * @param string $url URL to get. * @return Response Response for the executed request. - * @throws FailedToGetFromRemoteUrl If retrieving the contents from the URL failed. + * @throws FailedRemoteRequest If retrieving the contents from the URL failed. */ public function get( $url ) { $retries_left = $this->retries; From d940b4561aa703c72c73bed8baaf40eda155bf32 Mon Sep 17 00:00:00 2001 From: Pierre Gordon <16200219+pierlon@users.noreply.github.com> Date: Fri, 3 Apr 2020 13:21:50 -0400 Subject: [PATCH 08/11] Use specific exception name in docblock Co-Authored-By: Weston Ruter --- src/RemoteRequest/CachedRemoteGetRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/RemoteRequest/CachedRemoteGetRequest.php b/src/RemoteRequest/CachedRemoteGetRequest.php index b1316dfb346..9da62aeb73e 100644 --- a/src/RemoteRequest/CachedRemoteGetRequest.php +++ b/src/RemoteRequest/CachedRemoteGetRequest.php @@ -103,7 +103,7 @@ public function __construct( * * @param string $url URL to get. * @return Response Response for the executed request. - * @throws FailedRemoteRequest If retrieving the contents from the URL failed. + * @throws FailedToGetCachedResponse If retrieving the contents from the URL failed. */ public function get( $url ) { $cache_key = self::TRANSIENT_PREFIX . md5( __CLASS__ . $url ); From 27842701e8677575db1212cb5f34892d8c00ddba Mon Sep 17 00:00:00 2001 From: Pierre Gordon <16200219+pierlon@users.noreply.github.com> Date: Fri, 3 Apr 2020 13:24:49 -0400 Subject: [PATCH 09/11] Use specific exception name in docblock Co-Authored-By: Weston Ruter --- src/RemoteRequest/WpHttpRemoteGetRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/RemoteRequest/WpHttpRemoteGetRequest.php b/src/RemoteRequest/WpHttpRemoteGetRequest.php index f1e5d73768e..cc2ee1f7b28 100644 --- a/src/RemoteRequest/WpHttpRemoteGetRequest.php +++ b/src/RemoteRequest/WpHttpRemoteGetRequest.php @@ -99,7 +99,7 @@ public function __construct( $ssl_verify = true, $timeout = self::DEFAULT_TIMEOU * * @param string $url URL to get. * @return Response Response for the executed request. - * @throws FailedRemoteRequest If retrieving the contents from the URL failed. + * @throws FailedToGetFromRemoteUrl If retrieving the contents from the URL failed. */ public function get( $url ) { $retries_left = $this->retries; From fab8ef32e732cf35e982276d5ec8ca7b11630beb Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Fri, 3 Apr 2020 13:45:58 -0400 Subject: [PATCH 10/11] Check if cached response is an instance of CachedResponse::class --- src/RemoteRequest/CachedRemoteGetRequest.php | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/RemoteRequest/CachedRemoteGetRequest.php b/src/RemoteRequest/CachedRemoteGetRequest.php index 9da62aeb73e..4c0fd23d793 100644 --- a/src/RemoteRequest/CachedRemoteGetRequest.php +++ b/src/RemoteRequest/CachedRemoteGetRequest.php @@ -30,7 +30,7 @@ final class CachedRemoteGetRequest implements RemoteGetRequest { * * @var string */ - const TRANSIENT_PREFIX = 'amp_remote_request_v2_'; + const TRANSIENT_PREFIX = 'amp_remote_request_'; /** * Cache control header directive name. @@ -119,7 +119,7 @@ public function get( $url ) { } } - if ( false === $cached_response || $cached_response->is_expired() ) { + if ( ! $cached_response instanceof CachedResponse || $cached_response->is_expired() ) { try { $response = $this->remote_request->get( $url ); $status = $response->getStatusCode(); @@ -127,17 +127,14 @@ public function get( $url ) { $headers = $response->getHeaders(); $body = $response->getBody(); } catch ( FailedToGetFromRemoteUrl $exception ) { - $status = $exception->getStatusCode(); - $expiry = new DateTimeImmutable( "+ {$this->min_expiry} seconds" ); - $body = $exception->getMessage(); - $response = new RemoteGetRequestResponse( $body, $headers, $status ); + $status = $exception->getStatusCode(); + $expiry = new DateTimeImmutable( "+ {$this->min_expiry} seconds" ); + $body = $exception->getMessage(); } $cached_response = new CachedResponse( $body, $headers, $status, $expiry ); set_transient( $cache_key, serialize( $cached_response ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize - - return $response; } if ( ! $cached_response->is_valid() ) { From be928c54b9f2158648c8523e9fe7d7c325f9f55b Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Fri, 3 Apr 2020 20:37:20 +0200 Subject: [PATCH 11/11] Remove unused imports --- src/RemoteRequest/CachedRemoteGetRequest.php | 1 - src/RemoteRequest/WpHttpRemoteGetRequest.php | 1 - 2 files changed, 2 deletions(-) diff --git a/src/RemoteRequest/CachedRemoteGetRequest.php b/src/RemoteRequest/CachedRemoteGetRequest.php index 4c0fd23d793..fcf349a7899 100644 --- a/src/RemoteRequest/CachedRemoteGetRequest.php +++ b/src/RemoteRequest/CachedRemoteGetRequest.php @@ -7,7 +7,6 @@ namespace AmpProject\AmpWP\RemoteRequest; -use AmpProject\Exception\FailedRemoteRequest; use AmpProject\Exception\FailedToGetCachedResponse; use AmpProject\Exception\FailedToGetFromRemoteUrl; use AmpProject\RemoteGetRequest; diff --git a/src/RemoteRequest/WpHttpRemoteGetRequest.php b/src/RemoteRequest/WpHttpRemoteGetRequest.php index cc2ee1f7b28..2f08364456c 100644 --- a/src/RemoteRequest/WpHttpRemoteGetRequest.php +++ b/src/RemoteRequest/WpHttpRemoteGetRequest.php @@ -7,7 +7,6 @@ namespace AmpProject\AmpWP\RemoteRequest; -use AmpProject\Exception\FailedRemoteRequest; use AmpProject\Exception\FailedToGetFromRemoteUrl; use AmpProject\RemoteGetRequest; use AmpProject\RemoteRequest\RemoteGetRequestResponse;