From e34c361e8e2f889b5f1a723e8864b49817caae5b Mon Sep 17 00:00:00 2001
From: Matthieu Napoli <matthieu@mnapoli.fr>
Date: Sun, 24 Feb 2019 16:38:32 +0100
Subject: [PATCH] Fix #216 with a proper fix by upgrading hoa/fastcgi

This is roughly a revert of the original quickfix of #216 + an upgrade of hoa/fastcgi. This upgrade fixes the root cause of the bug (in https://github.com/hoaproject/Fastcgi/pull/19), which was then released (https://github.com/hoaproject/Fastcgi/issues/23).
---
 composer.json                   |  2 +-
 src/Runtime/PhpFpm.php          | 18 +-----------------
 tests/Runtime/PhpFpmTest.php    |  8 ++++++--
 tests/Sam/PhpFpmRuntimeTest.php |  9 ++++-----
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/composer.json b/composer.json
index ad25fa71b..2c20c267d 100644
--- a/composer.json
+++ b/composer.json
@@ -29,7 +29,7 @@
         "symfony/filesystem": "^3.1|^4.0",
         "symfony/process": "^3.3|^4.0",
         "aws/aws-sdk-php": "^3.44",
-        "hoa/fastcgi": "3.17.01.10",
+        "hoa/fastcgi": "^3.19.02.19",
         "hoa/socket": "1.17.05.16",
         "psr/http-message": "^1.0"
     },
diff --git a/src/Runtime/PhpFpm.php b/src/Runtime/PhpFpm.php
index 7ce1fd9b4..e87a342aa 100644
--- a/src/Runtime/PhpFpm.php
+++ b/src/Runtime/PhpFpm.php
@@ -105,22 +105,6 @@ public function proxy($event): LambdaResponse
 
         $responseHeaders = array_change_key_case($responseHeaders, CASE_LOWER);
 
-        /**
-         * In some cases PHP-FPM sends logs into the FastCGI connection. That isn't valid and isn't supported
-         * by our FastCGI client library, so those logs end up mangled in the HTTP headers.
-         * We can't return them to the client as:
-         * - the response is broken
-         * - it contains internal logs
-         * Until we find a better solution we simply return an empty 500 response, because since there were logs sent
-         * through FastCGI something wrong must have happened anyway. Those logs will also be sent to CloudWatch
-         * so debugging is possible.
-         *
-         * @see https://github.com/mnapoli/bref/pull/216
-         */
-        if (isset($responseHeaders['php message'])) {
-            return new LambdaResponse(500, [], '');
-        }
-
         // Extract the status code
         if (isset($responseHeaders['status'])) {
             [$status] = explode(' ', $responseHeaders['status']);
@@ -129,7 +113,7 @@ public function proxy($event): LambdaResponse
         }
         unset($responseHeaders['status']);
 
-        $responseBody = $responder->getResponseContent();
+        $responseBody = (string) $responder->getResponseContent();
 
         return new LambdaResponse((int) $status, $responseHeaders, $responseBody);
     }
diff --git a/tests/Runtime/PhpFpmTest.php b/tests/Runtime/PhpFpmTest.php
index 0f2ec1ab6..7067a8768 100644
--- a/tests/Runtime/PhpFpmTest.php
+++ b/tests/Runtime/PhpFpmTest.php
@@ -691,9 +691,13 @@ public function test response with cookies()
 
     public function test response with error_log()
     {
-        $headers = $this->get('error.php')->toApiGatewayFormat()['headers'];
+        $response = $this->get('error.php')->toApiGatewayFormat();
 
-        self::assertEquals([], (array) $headers);
+        self::assertStringStartsWith('PHP/', $response['headers']['x-powered-by'] ?? '');
+        unset($response['headers']['x-powered-by']);
+        self::assertEquals([
+            'content-type' => 'text/html; charset=UTF-8',
+        ], $response['headers']);
     }
 
     private function assertGlobalVariables(array $event, array $expectedGlobalVariables): void
diff --git a/tests/Sam/PhpFpmRuntimeTest.php b/tests/Sam/PhpFpmRuntimeTest.php
index 489235031..6e50ea5b9 100644
--- a/tests/Sam/PhpFpmRuntimeTest.php
+++ b/tests/Sam/PhpFpmRuntimeTest.php
@@ -47,7 +47,7 @@ public function test error_log function()
     {
         $response = $this->invoke('/?error_log=1');
 
-        self::assertSame(500, $response->getStatusCode(), $this->logs);
+        $this->assertResponseSuccessful($response);
         self::assertNotContains('This is a test log from error_log', $this->responseAsString($response));
         self::assertContains('This is a test log from error_log', $this->logs);
     }
@@ -84,10 +84,9 @@ public function test warnings are logged()
     {
         $response = $this->invoke('/?warning=1');
 
-        self::assertSame(500, $response->getStatusCode(), $this->logs);
-        // Unfortunately with the temporary fix in https://github.com/mnapoli/bref/pull/216
-        // we must settle for an empty 500 response for now
-//        self::assertEquals('Hello world!', $this->getBody($response), $this->logs);
+        $this->assertResponseSuccessful($response);
+        self::assertEquals('Hello world!', $this->getBody($response), $this->logs);
+        self::assertNotContains('This is a test warning', $this->responseAsString($response));
         self::assertContains('Warning:  This is a test warning in /var/task/tests/Sam', $this->logs);
     }