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/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 @@ +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 205caab11c3..fcf349a7899 100644 --- a/src/RemoteRequest/CachedRemoteGetRequest.php +++ b/src/RemoteRequest/CachedRemoteGetRequest.php @@ -7,6 +7,7 @@ namespace AmpProject\AmpWP\RemoteRequest; +use AmpProject\Exception\FailedToGetCachedResponse; use AmpProject\Exception\FailedToGetFromRemoteUrl; use AmpProject\RemoteGetRequest; use AmpProject\RemoteRequest\RemoteGetRequestResponse; @@ -101,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 FailedToGetCachedResponse If retrieving the contents from the URL failed. */ public function get( $url ) { - $cache_key = self::TRANSIENT_PREFIX . md5( __CLASS__ . $url ); - $cached_data = get_transient( $cache_key ); - $headers = []; - $status = null; + $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 ( ! $cached_response instanceof CachedResponse || $cached_response->is_expired() ) { try { $response = $this->remote_request->get( $url ); $status = $response->getStatusCode(); @@ -131,12 +131,16 @@ public function get( $url ) { $body = $exception->getMessage(); } - $cached_data = new CachedData( $body, $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 new RemoteGetRequestResponse( $cached_data->get_value(), $headers, $status ); + if ( ! $cached_response->is_valid() ) { + 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/CachedResponse.php b/src/RemoteRequest/CachedResponse.php new file mode 100644 index 00000000000..194cfadd91a --- /dev/null +++ b/src/RemoteRequest/CachedResponse.php @@ -0,0 +1,117 @@ +body = (string) $body; + $this->headers = (array) $headers; + $this->status_code = (int) $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 string[] 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 is valid. + */ + public function is_valid() { + // Values are already typed, so we just control the status code for validity. + return $this->status_code > 100 && $this->status_code <= 599; + } + + /** + * 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 be7a6ade443..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; @@ -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, + 'code' => $status_code, ], - 'headers' => [ - 'content-type' => 'text/css', - 'cache-control' => 'max-age=' . ( YEAR_IN_SECONDS + MONTH_IN_SECONDS ), - ], - 'body' => $response_body, + 'headers' => $headers, + 'body' => $response_body, ]; } return $preempt; @@ -1645,16 +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( $response_body, $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(); @@ -1662,28 +1666,82 @@ 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 CachedResponse( + 'body { color: #fff }', + [ + 'cache-control' => 'max-age=1441', + 'content-type' => 'text/css', + ], + 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 CachedResponse( + FailedToGetFromRemoteUrl::withHttpStatus( 'https://www.example.com/not-found/styles.css', 404 )->getMessage(), + [], + 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 CachedResponse $expected_cached_response Expected cache response. */ - public function test_external_stylesheet_not_found() { + public function test_external_stylesheet( $style_url, $http_response, $expected_styles, $expected_errors, $expected_cached_response ) { $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 +1750,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,33 +1772,32 @@ 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 ); /** - * 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 ); - $expected_exception = FailedToGetFromRemoteUrl::withHttpStatus( $href, 404 ); - $this->assertEquals( $expected_exception->getMessage(), $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() ); - // 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() ); + $expiry = $cached_response->get_expiry(); + $this->assertEquals( $cached_response->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.' ); }