Merge pull request #253 from mnapoli/fix-216-better

Fix #216 with a proper fix by upgrading hoa/fastcgi

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"
     },

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);
     }

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

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);
     }