Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache response status and headers when fetching external stylesheets #4509

Merged
merged 12 commits into from
Apr 3, 2020
4 changes: 4 additions & 0 deletions includes/validation/class-amp-validation-error-taxonomy.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 2 additions & 0 deletions lib/common/phpstan.neon.dist
Original file line number Diff line number Diff line change
Expand Up @@ -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$#'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpstan complains about FailedRemoteRequest not being a subtype of Throwable, but Throwable is not available in PHP 5.6 so I'm ignoring it for now.

13 changes: 13 additions & 0 deletions lib/common/src/Exception/FailedRemoteRequest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace AmpProject\Exception;

/**
* Marker interface to enable consumers to catch all exceptions for failed remote requests.
*
* @package ampproject/common
*/
interface FailedRemoteRequest extends AmpException
{

}
27 changes: 27 additions & 0 deletions lib/common/src/Exception/FailedToGetCachedResponse.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

namespace AmpProject\Exception;

use RuntimeException;

/**
* Exception thrown when a cached remote response could not be retrieved.
*
* @package ampproject/common
*/
final class FailedToGetCachedResponse extends RuntimeException implements FailedRemoteRequest
{

/**
* Instantiate a FailedToGetCachedResponseData exception for a URL if the cached response data could not be retrieved.
*
* @param string $url URL that failed to be fetched.
* @return self
*/
public static function withUrl($url)
{
$message = "Failed to retrieve the cached response for the URL '{$url}'.";

return new self($message);
}
}
2 changes: 1 addition & 1 deletion lib/common/src/Exception/FailedToGetFromRemoteUrl.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*
* @package ampproject/common
*/
final class FailedToGetFromRemoteUrl extends RuntimeException implements AmpException
final class FailedToGetFromRemoteUrl extends RuntimeException implements FailedRemoteRequest
{

/**
Expand Down
4 changes: 2 additions & 2 deletions lib/common/src/RemoteGetRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

namespace AmpProject;

use AmpProject\Exception\FailedToGetFromRemoteUrl;
use AmpProject\Exception\FailedRemoteRequest;

/**
* Interface for abstracting away the transport that is being used for making remote requests.
Expand All @@ -19,7 +19,7 @@ interface RemoteGetRequest
*
* @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);
}
3 changes: 2 additions & 1 deletion lib/common/src/RemoteRequest/CurlRemoteGetRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace AmpProject\RemoteRequest;

use AmpProject\Exception\FailedRemoteRequest;
use AmpProject\Exception\FailedToGetFromRemoteUrl;
use AmpProject\RemoteGetRequest;
use AmpProject\Response;
Expand Down Expand Up @@ -92,7 +93,7 @@ public function __construct($sslVerify = true, $timeout = self::DEFAULT_TIMEOUT,
*
* @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)
{
Expand Down
4 changes: 2 additions & 2 deletions lib/common/src/RemoteRequest/FallbackRemoteGetRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

namespace AmpProject\RemoteRequest;

use AmpProject\Exception\FailedToGetFromRemoteUrl;
use AmpProject\Exception\FailedRemoteRequest;
use AmpProject\RemoteGetRequest;
use AmpProject\Response;
use Exception;
Expand Down Expand Up @@ -55,7 +55,7 @@ private function addRemoteGetRequestInstance(RemoteGetRequest $remoteGetRequest)
*
* @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)
{
Expand Down
3 changes: 2 additions & 1 deletion lib/common/src/RemoteRequest/FilesystemRemoteGetRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace AmpProject\RemoteRequest;

use AmpProject\Exception\FailedRemoteRequest;
use AmpProject\Exception\FailedToGetFromRemoteUrl;
use AmpProject\RemoteGetRequest;
use AmpProject\Response;
Expand Down Expand Up @@ -40,7 +41,7 @@ public function __construct($argumentMap)
*
* @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)
{
Expand Down
4 changes: 2 additions & 2 deletions lib/common/src/RemoteRequest/StubbedRemoteGetRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

namespace AmpProject\RemoteRequest;

use AmpProject\Exception\FailedToGetFromRemoteUrl;
use AmpProject\Exception\FailedRemoteRequest;
use AmpProject\RemoteGetRequest;
use AmpProject\Response;
use LogicException;
Expand Down Expand Up @@ -37,7 +37,7 @@ public function __construct($argumentMap)
*
* @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)
{
Expand Down
71 changes: 0 additions & 71 deletions src/RemoteRequest/CachedData.php

This file was deleted.

40 changes: 24 additions & 16 deletions src/RemoteRequest/CachedRemoteGetRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

namespace AmpProject\AmpWP\RemoteRequest;

use AmpProject\Exception\FailedRemoteRequest;
use AmpProject\Exception\FailedToGetCachedResponse;
use AmpProject\Exception\FailedToGetFromRemoteUrl;
use AmpProject\RemoteGetRequest;
use AmpProject\RemoteRequest\RemoteGetRequestResponse;
Expand All @@ -28,7 +30,7 @@ final class CachedRemoteGetRequest implements RemoteGetRequest {
*
* @var string
*/
const TRANSIENT_PREFIX = 'amp_remote_request_';
const TRANSIENT_PREFIX = 'amp_remote_request_v2_';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the transient prefix to bust the cache.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not be necessary if we check the type of the class per my comment below.


/**
* Cache control header directive name.
Expand Down Expand Up @@ -101,42 +103,48 @@ 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 FailedRemoteRequest If retrieving the contents from the URL failed.
pierlon marked this conversation as resolved.
Show resolved Hide resolved
*/
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 ( false === $cached_response || $cached_response->is_expired() ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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();
$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();
$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_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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this return needed?

Suggested change
return $response;

Would removing it make the logic more consistent between cached and non-cached code paths?

}

if ( ! $cached_response->is_valid() ) {
throw new FailedToGetCachedResponse( $url );
}

return new RemoteGetRequestResponse( $cached_data->get_value(), $headers, $status );
return new RemoteGetRequestResponse( $cached_response->get_body(), $cached_response->get_headers(), $cached_response->get_status_code() );
}

/**
Expand Down
Loading