Repository: ampproject/amp-toolbox-php (php)

Issue:
i-amphtml-version=0 is invalid
If the metadata endpoint does not respond successfully, the current version ist set to '0':
https://github.com/ampproject/amp-toolbox-php/blame/main/src/RuntimeVersion.php#L80-L82

This is used by  the [AmpRuntimeCss.php](https://github.com/ampproject/amp-toolbox-php/blob/main/src/Optimizer/Transformer/AmpRuntimeCss.php#L162).

Atm I cannot provide a reproduction, but this occurs from time to time.

Desired behaviour:
Instead of setting the version to '0' it would be nice to throw an error/exception.



Pull request description:
Cache only valid responses
Hi!

Recently we discovered that some requests to fetch the metadata failed, but were cached regardless of the status code. This PR adds a guard, that only valid responses are cached.

This fixes #530 

Patch:
diff --git a/src/RemoteRequest/CurlRemoteGetRequest.php b/src/RemoteRequest/CurlRemoteGetRequest.php
index d4f7c1cbc..dbe6683f8 100644
--- a/src/RemoteRequest/CurlRemoteGetRequest.php
+++ b/src/RemoteRequest/CurlRemoteGetRequest.php
@@ -132,7 +132,7 @@ static function ($curl, $header) use (&$headers) {
             $curlErrno = curl_errno($curlHandle);
             curl_close($curlHandle);
 
-            if ($body === false) {
+            if ($body === false || $status < 200 || $status >= 300) {
                 if (! $retriesLeft || in_array($curlErrno, self::RETRYABLE_ERROR_CODES, true) === false) {
                     if (! empty($status) && is_numeric($status)) {
                         throw FailedToGetFromRemoteUrl::withHttpStatus($url, (int) $status);
diff --git a/src/RemoteRequest/StubbedRemoteGetRequest.php b/src/RemoteRequest/StubbedRemoteGetRequest.php
index b8acd2e45..23d60ceed 100644
--- a/src/RemoteRequest/StubbedRemoteGetRequest.php
+++ b/src/RemoteRequest/StubbedRemoteGetRequest.php
@@ -45,6 +45,7 @@ public function get($url, $headers = [])
             throw new LogicException("Trying to stub a remote request for an unknown URL: {$url}.");
         }
 
-        return new RemoteGetRequestResponse($this->argumentMap[$url]);
+        $args = $this->argumentMap[$url];
+        return new RemoteGetRequestResponse($args['body'], [], isset($args['status']) ? $args['status'] : 200);
     }
 }
diff --git a/src/RemoteRequest/TemporaryFileCachedRemoteGetRequest.php b/src/RemoteRequest/TemporaryFileCachedRemoteGetRequest.php
index 105f4e2be..55ffa4a5e 100644
--- a/src/RemoteRequest/TemporaryFileCachedRemoteGetRequest.php
+++ b/src/RemoteRequest/TemporaryFileCachedRemoteGetRequest.php
@@ -141,6 +141,11 @@ private function getRemoteResponse($url, $headers)
      */
     private function cacheResponse($url, Response $response)
     {
+        $statusCode = $response->getStatusCode();
+        if ($statusCode < 200 || $statusCode >= 300) {
+            return;
+        }
+
         file_put_contents($this->getTemporaryFilePath($url), serialize($response));
     }
 


Test patch:
diff --git a/tests/RemoteRequest/StubbedRemoteGetRequestTest.php b/tests/RemoteRequest/StubbedRemoteGetRequestTest.php
index 539c24b09..16c26d7ac 100644
--- a/tests/RemoteRequest/StubbedRemoteGetRequestTest.php
+++ b/tests/RemoteRequest/StubbedRemoteGetRequestTest.php
@@ -17,7 +17,7 @@ public function testGetReturnsStubbedUrl()
     {
         $stubbedRequest = new StubbedRemoteGetRequest(
             [
-                'https://example.com/stubbed-file' => 'stubbed response',
+                'https://example.com/stubbed-file' => ['body' => 'stubbed response'],
             ]
         );
 
@@ -32,7 +32,7 @@ public function testGetThrowsExceptionOnInvalidUrl()
     {
         $stubbedRequest = new StubbedRemoteGetRequest(
             [
-                'https://example.com/stubbed-file' => 'stubbed response',
+                'https://example.com/stubbed-file' => ['body' => 'stubbed response'],
             ]
         );
 
diff --git a/tests/RemoteRequest/TemporaryFileCachedRemoteGetRequestTest.php b/tests/RemoteRequest/TemporaryFileCachedRemoteGetRequestTest.php
index 198650b30..bd333a8a3 100644
--- a/tests/RemoteRequest/TemporaryFileCachedRemoteGetRequestTest.php
+++ b/tests/RemoteRequest/TemporaryFileCachedRemoteGetRequestTest.php
@@ -56,7 +56,7 @@ public function testCachingWorkflow()
 
         $urls            = array_keys(TestMarkup::STUBBED_REMOTE_REQUESTS);
         $url             = $urls[0];
-        $stubbedResponse = TestMarkup::STUBBED_REMOTE_REQUESTS[$url];
+        $stubbedResponse = TestMarkup::STUBBED_REMOTE_REQUESTS[$url]['body'];
 
         $filename = TemporaryFileCachedRemoteGetRequest::CACHED_FILE_PREFIX . md5($url);
         $file     = vfsStream::url(self::DIRECTORY_NAME . "/{$filename}");
@@ -112,6 +112,24 @@ public function testCacheException()
         $cachedRequest->get('http://example.com');
     }
 
+    /**
+     * Test that errors do not get cached.
+     */
+    public function testCacheOnlyValidRepsonses()
+    {
+        $unknownUrl = 'https://cdn.ampproject.com/non-existant.really.not.html';
+
+        $tmpDirectory = vfsStream::url(self::DIRECTORY_NAME);
+        $cachedRequest = new TemporaryFileCachedRemoteGetRequest(
+            new StubbedRemoteGetRequest([$unknownUrl => ['body' => '404 content', 'status' => 404]]),
+            $tmpDirectory
+        );
+
+        $cachedRequest->get($unknownUrl);
+
+        $this->assertFileDoesNotExist($tmpDirectory . DIRECTORY_SEPARATOR . TemporaryFileCachedRemoteGetRequest::CACHED_FILE_PREFIX . md5($unknownUrl));
+    }
+
     /**
      * Get the request class instance to test against.
      *
diff --git a/tests/RuntimeVersionTest.php b/tests/RuntimeVersionTest.php
index bd801243d..551245f7d 100644
--- a/tests/RuntimeVersionTest.php
+++ b/tests/RuntimeVersionTest.php
@@ -20,8 +20,8 @@ class RuntimeVersionTest extends TestCase
      * @var array
      */
     const STUBBED_REMOTE_REQUESTS = [
-        'https://cdn.ampproject.org/rtv/metadata' => '{"ampRuntimeVersion":"012345678900000","ampCssUrl":"https://cdn.ampproject.org/rtv/012345678900000/v0.css","canaryPercentage":"0.1","diversions":["023456789000000","034567890100000","045678901200000"]}',
-        'https://cdn.ampproject.org/v0.css'       => '/* v0.css */',
+        'https://cdn.ampproject.org/rtv/metadata' => ['body' => '{"ampRuntimeVersion":"012345678900000","ampCssUrl":"https://cdn.ampproject.org/rtv/012345678900000/v0.css","canaryPercentage":"0.1","diversions":["023456789000000","034567890100000","045678901200000"]}'],
+        'https://cdn.ampproject.org/v0.css'       => ['body' => '/* v0.css */'],
     ];
 
     /**
diff --git a/tests/src/TestMarkup.php b/tests/src/TestMarkup.php
index 703eb9ef7..1d2661216 100644
--- a/tests/src/TestMarkup.php
+++ b/tests/src/TestMarkup.php
@@ -21,10 +21,10 @@ final class TestMarkup
      * @var array
      */
     const STUBBED_REMOTE_REQUESTS = [
-        'https://cdn.ampproject.org/rtv/metadata'               => '{"ampRuntimeVersion":"012345678900000","ampCssUrl":"https://cdn.ampproject.org/rtv/012345678900000/v0.css","canaryPercentage":"0.1","diversions":["023456789000000","034567890100000","045678901200000"]}',
-        'https://cdn.ampproject.org/v0.css'                     => '/* v0.css */',
-        'https://cdn.ampproject.org/rtv/012345678900000/v0.css' => '/* 012345678900000/v0.css */',
-        'https://cdn.ampproject.org/rtv/012345678901234/v0.css' => '/* 012345678901234/v0.css */',
+        'https://cdn.ampproject.org/rtv/metadata'               => ['body' => '{"ampRuntimeVersion":"012345678900000","ampCssUrl":"https://cdn.ampproject.org/rtv/012345678900000/v0.css","canaryPercentage":"0.1","diversions":["023456789000000","034567890100000","045678901200000"]}'],
+        'https://cdn.ampproject.org/v0.css'                     => ['body' => '/* v0.css */'],
+        'https://cdn.ampproject.org/rtv/012345678900000/v0.css' => ['body' => '/* 012345678900000/v0.css */'],
+        'https://cdn.ampproject.org/rtv/012345678901234/v0.css' => ['body' => '/* 012345678901234/v0.css */'],
     ];
 
     // Doctype is the doctype expected for AMP documents.