Refactor to move all SAPI related logic to new SapiHandler and improve test coverage

.github/workflows/ci.yml

@@ -26,9 +26,9 @@ jobs:
           php-version: ${{ matrix.php }}
           coverage: xdebug
       - run: composer install
-      - run: vendor/bin/phpunit --coverage-text
+      - run: vendor/bin/phpunit --coverage-text --stderr
         if: ${{ matrix.php >= 7.3 }}
-      - run: vendor/bin/phpunit --coverage-text -c phpunit.xml.legacy
+      - run: vendor/bin/phpunit --coverage-text --stderr -c phpunit.xml.legacy
         if: ${{ matrix.php < 7.3 }}
 
   Built-in-webserver:

README.md

@@ -102,7 +102,7 @@ $ composer install
 To run the test suite, go to the project root and run:
 
 ```bash
-$ php vendor/bin/phpunit
+$ vendor/bin/phpunit --stderr
 ```
 
 Additionally, you can run some simple acceptance tests to verify the framework

src/App.php

@@ -7,11 +7,9 @@
 use React\EventLoop\Loop;
 use React\EventLoop\LoopInterface;
 use React\Http\HttpServer;
-use React\Http\Message\ServerRequest;
 use React\Promise\Deferred;
 use React\Promise\PromiseInterface;
 use React\Socket\SocketServer;
-use React\Stream\ReadableStreamInterface;
 
 class App
 {
@@ -23,6 +21,9 @@ class App
     /** @var RouteHandler */
     private $router;
 
+    /** @var SapiHandler */
+    private $sapi;
+
     /**
      * Instantiate new X application
      *
@@ -65,6 +66,7 @@ public function __construct($loop = null, callable ...$middleware)
         \array_unshift($middleware, $errorHandler);
         $middleware[] = $this->router;
         $this->handler = new MiddlewareHandler($middleware);
+        $this->sapi = new SapiHandler();
     }
 
     public function get(string $route, callable $handler, callable ...$handlers): void
@@ -119,10 +121,10 @@ public function redirect(string $route, string $target, int $code = 302): void
 
     public function run()
     {
-        if (\php_sapi_name() === 'cli') {
+        if (\PHP_SAPI === 'cli') {
             $this->runLoop();
         } else {
-            $this->runOnce();
+            $this->runOnce(); // @codeCoverageIgnore
         }
 
         $this->loop->run();
@@ -134,10 +136,10 @@ private function runLoop()
             $response = $this->handleRequest($request);
 
             if ($response instanceof ResponseInterface) {
-                $this->logRequestResponse($request, $response);
+                $this->sapi->logRequestResponse($request, $response);
             } elseif ($response instanceof PromiseInterface) {
                 $response->then(function (ResponseInterface $response) use ($request) {
-                    $this->logRequestResponse($request, $response);
+                    $this->sapi->logRequestResponse($request, $response);
                 });
             }
 
@@ -152,7 +154,7 @@ private function runLoop()
         $socket = new SocketServer($listen, [], $this->loop);
         $http->listen($socket);
 
-        $this->log('Listening on ' . \str_replace('tcp:', 'http:', $socket->getAddress()));
+        $this->sapi->log('Listening on ' . \str_replace('tcp:', 'http:', $socket->getAddress()));
 
         $http->on('error', function (\Exception $e) {
             $orig = $e;
@@ -161,123 +163,29 @@ private function runLoop()
                 $message .= '. Previous: ' . $e->getMessage();
             }
 
-            $this->log($message);
+            $this->sapi->log($message);
 
             \fwrite(STDERR, (string)$orig);
         });
     }
 
-    private function requestFromGlobals(): ServerRequestInterface
-    {
-        $host = null;
-        $headers = array();
-        if (\function_exists('getallheaders')) {
-            $headers = \getallheaders();
-            $host = \array_change_key_case($headers, \CASE_LOWER)['host'] ?? null;
-        } else {
-            foreach ($_SERVER as $key => $value) {
-                if (\strpos($key, 'HTTP_') === 0) {
-                    $key = str_replace(' ', '-', \ucwords(\strtolower(\str_replace('_', ' ', \substr($key, 5)))));
-                    $headers[$key] = $value;
-
-                    if ($host === null && $key === 'Host') {
-                        $host = $value;
-                    }
-                }
-            }
-        }
-
-        $body = file_get_contents('php://input');
-
-        $request = new ServerRequest(
-            $_SERVER['REQUEST_METHOD'] ?? 'GET',
-            ($_SERVER['HTTPS'] ?? null === 'on' ? 'https://' : 'http://') . ($host ?? 'localhost') . ($_SERVER['REQUEST_URI'] ?? '/'),
-            $headers,
-            $body,
-            substr($_SERVER['SERVER_PROTOCOL'] ?? 'http/1.1', 5),
-            $_SERVER
-        );
-        if ($host === null) {
-            $request = $request->withoutHeader('Host');
-        }
-        $request = $request->withParsedBody($_POST);
-
-        // Content-Length / Content-Type are special <3
-        if ($request->getHeaderLine('Content-Length') === '') {
-            $request = $request->withoutHeader('Content-Length');
-        }
-        if ($request->getHeaderLine('Content-Type') === '' && !isset($_SERVER['HTTP_CONTENT_TYPE'])) {
-            $request = $request->withoutHeader('Content-Type');
-        }
-
-        return $request;
-    }
-
     private function runOnce()
     {
-        $request = $this->requestFromGlobals();
+        $request = $this->sapi->requestFromGlobals();
 
         $response = $this->handleRequest($request);
 
         if ($response instanceof ResponseInterface) {
-            $this->sendResponse($request, $response);
+            $this->sapi->logRequestResponse($request, $response);
+            $this->sapi->sendResponse($response);
         } elseif ($response instanceof PromiseInterface) {
             $response->then(function (ResponseInterface $response) use ($request) {
-                $this->sendResponse($request, $response);
+                $this->sapi->logRequestResponse($request, $response);
+                $this->sapi->sendResponse($response);
             });
         }
     }
 
-    private function sendResponse(ServerRequestInterface $request, ResponseInterface $response): void
-    {
-        $this->logRequestResponse($request, $response);
-
-        header($_SERVER['SERVER_PROTOCOL'] . ' ' . $response->getStatusCode() . ' ' . $response->getReasonPhrase());
-
-        // automatically assign "Content-Length" response header if known and not already present
-        if (!$response->hasHeader('Content-Length') && $response->getBody()->getSize() !== null) {
-            $response = $response->withHeader('Content-Length', (string)$response->getBody()->getSize());
-        }
-
-        // remove default "Content-Type" header set by PHP (default_mimetype)
-        if (!$response->hasHeader('Content-Type')) {
-            header('Content-Type: foo');
-            header_remove('Content-Type');
-        }
-
-        // send all headers without applying default "; charset=utf-8" set by PHP (default_charset)
-        $old = ini_get('default_charset');
-        ini_set('default_charset', '');
-        foreach ($response->getHeaders() as $name => $values) {
-            foreach ($values as $value) {
-                header($name . ': ' . $value);
-            }
-        }
-        ini_set('default_charset', $old);
-
-        $body = $response->getBody();
-
-        if ($body instanceof ReadableStreamInterface) {
-            // clear all output buffers (default in cli-server)
-            while (ob_get_level()) {
-                ob_end_clean();
-            }
-
-            // try to disable nginx buffering (http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffering)
-            if (isset($_SERVER['SERVER_SOFTWARE']) && strpos($_SERVER['SERVER_SOFTWARE'], 'nginx') === 0) {
-                header('X-Accel-Buffering: no');
-            }
-
-            // flush data whenever stream reports one data chunk
-            $body->on('data', function ($chunk) {
-                echo $chunk;
-                flush();
-            });
-        } else {
-            echo $response->getBody();
-        }
-    }
-
     /**
      * @param ServerRequestInterface $request
      * @return ResponseInterface|PromiseInterface<ResponseInterface,void>
@@ -324,31 +232,4 @@ private function coroutine(\Generator $generator): PromiseInterface
 
         return $deferred->promise();
     }
-
-    private function logRequestResponse(ServerRequestInterface $request, ResponseInterface $response): void
-    {
-        // only log for built-in webserver and PHP development webserver, others have their own access log
-        if (PHP_SAPI !== 'cli' && PHP_SAPI !== 'cli-server') {
-            return; // @codeCoverageIgnore
-        }
-
-        $this->log(
-            ($request->getServerParams()['REMOTE_ADDR'] ?? '-') . ' ' .
-            '"' . $request->getMethod() . ' ' . $request->getUri()->getPath() . ' HTTP/' . $request->getProtocolVersion() . '" ' .
-            $response->getStatusCode() . ' ' . $response->getBody()->getSize()
-        );
-    }
-
-    private function log(string $message): void
-    {
-        $time = microtime(true);
-
-        $log = date('Y-m-d H:i:s', (int)$time) . sprintf('.%03d ', (int)(($time - (int)$time) * 1e3)) . $message . PHP_EOL;
-
-        if (\PHP_SAPI === 'cli') {
-            echo $log;
-        } else {
-            fwrite(defined('STDERR') ? STDERR : fopen('php://stderr', 'a'), $log);
-        }
-    }
 }

src/SapiHandler.php

@@ -0,0 +1,149 @@
+<?php
+
+namespace FrameworkX;
+
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+use React\Http\Message\ServerRequest;
+use React\Stream\ReadableStreamInterface;
+
+/**
+ * @internal
+ */
+class SapiHandler
+{
+    /** @var resource */
+    private $logStream;
+
+    /** @var bool */
+    private $shouldLogRequest;
+
+    public function __construct()
+    {
+        $this->logStream = PHP_SAPI === 'cli' ? \fopen('php://output', 'a') : (\defined('STDERR') ? \STDERR : \fopen('php://stderr', 'a'));
+
+        // Only log for built-in webserver and PHP development webserver, others have their own access log.
+        // Yes, this should be moved out of this class.
+        $this->shouldLogRequest = PHP_SAPI === 'cli' || PHP_SAPI === 'cli-server';
+    }
+
+    public function requestFromGlobals(): ServerRequestInterface
+    {
+        $host = null;
+        $headers = array();
+        // @codeCoverageIgnoreStart
+        if (\function_exists('getallheaders')) {
+            $headers = \getallheaders();
+            $host = \array_change_key_case($headers, \CASE_LOWER)['host'] ?? null;
+        } else {
+            foreach ($_SERVER as $key => $value) {
+                if (\strpos($key, 'HTTP_') === 0) {
+                    $key = str_replace(' ', '-', \ucwords(\strtolower(\str_replace('_', ' ', \substr($key, 5)))));
+                    $headers[$key] = $value;
+
+                    if ($host === null && $key === 'Host') {
+                        $host = $value;
+                    }
+                }
+            }
+        }
+        // @codeCoverageIgnoreEnd
+
+        $body = file_get_contents('php://input');
+
+        $request = new ServerRequest(
+            $_SERVER['REQUEST_METHOD'] ?? 'GET',
+            ($_SERVER['HTTPS'] ?? null === 'on' ? 'https://' : 'http://') . ($host ?? 'localhost') . ($_SERVER['REQUEST_URI'] ?? '/'),
+            $headers,
+            $body,
+            substr($_SERVER['SERVER_PROTOCOL'] ?? 'http/1.1', 5),
+            $_SERVER
+        );
+        if ($host === null) {
+            $request = $request->withoutHeader('Host');
+        }
+        $request = $request->withParsedBody($_POST);
+
+        // Content-Length / Content-Type are special <3
+        if ($request->getHeaderLine('Content-Length') === '') {
+            $request = $request->withoutHeader('Content-Length');
+        }
+        if ($request->getHeaderLine('Content-Type') === '' && !isset($_SERVER['HTTP_CONTENT_TYPE'])) {
+            $request = $request->withoutHeader('Content-Type');
+        }
+
+        return $request;
+    }
+
+    /**
+     * @param ResponseInterface $response
+     */
+    public function sendResponse(ResponseInterface $response): void
+    {
+        header($_SERVER['SERVER_PROTOCOL'] . ' ' . $response->getStatusCode() . ' ' . $response->getReasonPhrase());
+
+        // automatically assign "Content-Length" response header if known and not already present
+        if (!$response->hasHeader('Content-Length') && $response->getBody()->getSize() !== null) {
+            $response = $response->withHeader('Content-Length', (string)$response->getBody()->getSize());
+        }
+
+        // remove default "Content-Type" header set by PHP (default_mimetype)
+        if (!$response->hasHeader('Content-Type')) {
+            header('Content-Type:');
+            header_remove('Content-Type');
+        }
+
+        // send all headers without applying default "; charset=utf-8" set by PHP (default_charset)
+        $old = ini_get('default_charset');
+        ini_set('default_charset', '');
+        foreach ($response->getHeaders() as $name => $values) {
+            foreach ($values as $value) {
+                header($name . ': ' . $value);
+            }
+        }
+        ini_set('default_charset', $old);
+
+        $body = $response->getBody();
+
+        if ($body instanceof ReadableStreamInterface) {
+            // try to disable nginx buffering (http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffering)
+            if (isset($_SERVER['SERVER_SOFTWARE']) && strpos($_SERVER['SERVER_SOFTWARE'], 'nginx') === 0) {
+                header('X-Accel-Buffering: no');
+            }
+
+            // clear output buffer to show streaming output (default in cli-server)
+            if (\PHP_SAPI === 'cli-server') {
+                \ob_end_flush(); // @codeCoverageIgnore
+            }
+
+            // flush data whenever stream reports one data chunk
+            $body->on('data', function ($chunk) {
+                echo $chunk;
+                flush();
+            });
+        } else {
+            echo $body;
+        }
+    }
+
+    public function logRequestResponse(ServerRequestInterface $request, ResponseInterface $response): void
+    {
+        if (!$this->shouldLogRequest) {
+            return;
+        }
+
+        $this->log(
+            ($request->getServerParams()['REMOTE_ADDR'] ?? '-') . ' ' .
+            '"' . $request->getMethod() . ' ' . $request->getUri()->getPath() . ' HTTP/' . $request->getProtocolVersion() . '" ' .
+            $response->getStatusCode() . ' ' . $response->getBody()->getSize()
+        );
+    }
+
+    public function log(string $message): void
+    {
+        $time = microtime(true);
+        $log = date('Y-m-d H:i:s', (int)$time) . sprintf('.%03d ', (int)(($time - (int)$time) * 1e3)) . $message . PHP_EOL;
+
+        fwrite($this->logStream, $log);
+    }
+}