Merge pull request clue#247 from clue-labs/accesslog-path

Add optional `$path` argument for `AccessLogHandler`
clue-labs · Feb 29, 2024 · 88d23bc · 88d23bc
2 parents 0068933 + d435ed2
commit 88d23bc
Show file tree

Hide file tree

Showing 7 changed files with 435 additions and 66 deletions.
diff --git a/docs/api/app.md b/docs/api/app.md
@@ -362,6 +362,62 @@ $app = new FrameworkX\App($container);
 // …
 ```
 
+If you do not want to log to the console, you can configure an absolute log file
+path by passing an argument to the [`AccessLogHandler`](middleware.md#accessloghandler)
+like this:
+
+=== "Using DI container"
+
+    ```php title="public/index.php"
+    <?php
+
+    require __DIR__ . '/../vendor/autoload.php';
+
+    $container = new FrameworkX\Container([
+        'accesslog' => __DIR__ . '/../logs/access.log',
+        FrameworkX\AccessLogHandler::class => fn(string $accesslog) => new FrameworkX\AccessLogHandler($accesslog)
+    ]);
+
+    $app = new FrameworkX\App($container);
+
+    // …
+    ```
+
+=== "Using middleware instances"
+
+    ```php title="public/index.php"
+    <?php
+
+    require __DIR__ . '/../vendor/autoload.php';
+
+    $app = new FrameworkX\App(
+        new FrameworkX\AccessLogHandler(__DIR__ . '/../logs/access.log'),
+        new FrameworkX\ErrorHandler()
+    );
+
+
+
+    // …
+    ```
+
+Likewise, you can disable writing an access log by passing an absolute path to
+`/dev/null` (Unix) or `nul` (Windows) like this:
+
+```php title="public/index.php"
+<?php
+
+require __DIR__ . '/../vendor/autoload.php';
+
+$container = new FrameworkX\Container([
+    'accesslog' => DIRECTORY_SEPARATOR !== '\\' ? '/dev/null' : __DIR__ . '\\nul'
+    FrameworkX\AccessLogHandler::class => fn(string $accesslog) => new FrameworkX\AccessLogHandler($accesslog),
+]);
+
+$app = new FrameworkX\App($container);
+
+// …
+```
+
 X supports running behind reverse proxies just fine. However, by default it will
 see the IP address of the last proxy server as the client IP address (this will
 often be `127.0.0.1`). You can get the original client IP address if you configure
@@ -385,8 +441,6 @@ it to the [`AccessLogHandler`](middleware.md#accessloghandler) like this:
         new FrameworkX\ErrorHandler()
     );
 
-    $app = new FrameworkX\App($container);
-
     // …
     ```
 
@@ -405,8 +459,6 @@ it to the [`AccessLogHandler`](middleware.md#accessloghandler) like this:
         FrameworkX\ErrorHandler::class
     );
 
-    $app = new FrameworkX\App($container);
-
     // …
     ```
 

diff --git a/src/AccessLogHandler.php b/src/AccessLogHandler.php
@@ -20,11 +20,18 @@ class AccessLogHandler
     /** @var bool */
     private $hasHighResolution;
 
-    /** @throws void */
-    public function __construct()
+    /**
+     * @param ?string $path (optional) absolute log file path or will log to console output by default
+     * @throws \InvalidArgumentException if given `$path` is not an absolute file path
+     * @throws \RuntimeException if given `$path` can not be opened in append mode
+     */
+    public function __construct(?string $path = null)
     {
-        /** @throws void because `fopen()` is known to always return a `resource` for built-in wrappers */
-        $this->logger = new LogStreamHandler(\PHP_SAPI === 'cli' ? 'php://output' : 'php://stderr');
+        if ($path === null) {
+            $path = \PHP_SAPI === 'cli' ? 'php://output' : 'php://stderr';
+        }
+
+        $this->logger = new LogStreamHandler($path);
         $this->hasHighResolution = \function_exists('hrtime'); // PHP 7.3+
     }
 

diff --git a/src/Io/LogStreamHandler.php b/src/Io/LogStreamHandler.php
@@ -10,9 +10,19 @@ class LogStreamHandler
     /** @var resource */
     private $stream;
 
-    /** @throws \RuntimeException if given `$path` can not be opened in append mode */
+    /**
+     * @param string $path absolute log file path
+     * @throws \InvalidArgumentException if given `$path` is not an absolute file path
+     * @throws \RuntimeException if given `$path` can not be opened in append mode
+     */
     public function __construct(string $path)
     {
+        if (\strpos($path, "\0") !== false || (\stripos($path, 'php://') !== 0 && !$this->isAbsolutePath($path))) {
+            throw new \InvalidArgumentException(
+                'Unable to open log file "' . \addslashes($path) . '": Invalid path given'
+            );
+        }
+
         $errstr = '';
         \set_error_handler(function (int $_, string $error) use (&$errstr): bool {
             // Match errstr from PHP's warning message.
@@ -42,4 +52,9 @@ public function log(string $message): void
         $ret = \fwrite($this->stream, $prefix . $message . \PHP_EOL);
         assert(\is_int($ret));
     }
+
+    private function isAbsolutePath(string $path): bool
+    {
+        return \DIRECTORY_SEPARATOR !== '\\' ? \substr($path, 0, 1) === '/' : (bool) \preg_match('#^[A-Z]:[/\\\\]#i', $path);
+    }
 }
diff --git a/tests/AccessLogHandlerTest.php b/tests/AccessLogHandlerTest.php
@@ -12,6 +12,105 @@
 
 class AccessLogHandlerTest extends TestCase
 {
+    public function testCtorWithRelativePathThrows(): void
+    {
+        $this->expectException(\InvalidArgumentException::class);
+        new AccessLogHandler('../access.log');
+    }
+
+    public function testCtorWithPathToDirectoryThrows(): void
+    {
+        $this->expectException(\RuntimeException::class);
+        new AccessLogHandler(__DIR__);
+    }
+
+    public function testCtorWithPathToNewFileWillCreateNewFile(): void
+    {
+        $path = tempnam(sys_get_temp_dir(), 'log');
+        assert(is_string($path));
+        unlink($path);
+
+        new AccessLogHandler($path);
+
+        $this->assertFileExists($path);
+        unlink($path);
+    }
+
+    public function testInvokeWithDefaultPathWillLogMessageToConsole(): void
+    {
+        $handler = new AccessLogHandler();
+
+        $request = new ServerRequest('GET', 'http://localhost:8080/users', [], '', '1.1', ['REMOTE_ADDR' => '127.0.0.1']);
+        $response = new Response(200, [], "Hello\n");
+
+        $this->expectOutputRegex('#^\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d\.\d\d\d 127\.0\.0\.1 "GET /users HTTP/1\.1" 200 6 0\.0\d\d' . PHP_EOL . '$#');
+        $handler($request, function () use ($response) { return $response; });
+    }
+
+    public function testInvokeWithPathToNewFileWillCreateNewFileWithLogMessage(): void
+    {
+        $path = tempnam(sys_get_temp_dir(), 'log');
+        assert(is_string($path));
+        unlink($path);
+
+        $handler = new AccessLogHandler($path);
+
+        $request = new ServerRequest('GET', 'http://localhost:8080/users', [], '', '1.1', ['REMOTE_ADDR' => '127.0.0.1']);
+        $response = new Response(200, [], "Hello\n");
+        $handler($request, function () use ($response) { return $response; });
+
+        $log = file_get_contents($path);
+        assert(is_string($log));
+
+        if (method_exists($this, 'assertMatchesRegularExpression')) {
+            $this->assertMatchesRegularExpression('#^\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d\.\d\d\d 127\.0\.0\.1 "GET /users HTTP/1\.1" 200 6 0\.0\d\d' . PHP_EOL . '$#', $log);
+        } else {
+            // legacy PHPUnit < 9.1
+            $this->assertRegExp('#^\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d\.\d\d\d 127\.0\.0\.1 "GET /users HTTP/1\.1" 200 6 0\.0\d\d' . PHP_EOL . '$#', $log);
+        }
+
+        unset($handler);
+        unlink($path);
+    }
+
+    public function testInvokeWithPathToExistingFileWillAppendLogMessage(): void
+    {
+        $path = tempnam(sys_get_temp_dir(), 'log');
+        assert(is_string($path));
+        file_put_contents($path, 'first' . PHP_EOL);
+
+        $handler = new AccessLogHandler($path);
+
+        $request = new ServerRequest('GET', 'http://localhost:8080/users', [], '', '1.1', ['REMOTE_ADDR' => '127.0.0.1']);
+        $response = new Response(200, [], "Hello\n");
+        $handler($request, function () use ($response) { return $response; });
+
+        $log = file_get_contents($path);
+        assert(is_string($log));
+
+        if (method_exists($this, 'assertMatchesRegularExpression')) {
+            $this->assertMatchesRegularExpression('#^first' . PHP_EOL . '\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d\.\d\d\d 127\.0\.0\.1 "GET /users HTTP/1\.1" 200 6 0\.0\d\d' . PHP_EOL . '$#', $log);
+        } else {
+            // legacy PHPUnit < 9.1
+            $this->assertRegExp('#^first' . PHP_EOL . '\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d\.\d\d\d 127\.0\.0\.1 "GET /users HTTP/1\.1" 200 6 0\.0\d\d' . PHP_EOL . '$#', $log);
+        }
+
+        unset($handler);
+        unlink($path);
+    }
+
+    /**
+     * @doesNotPerformAssertions
+     */
+    public function testInvokeWithDevNullWritesNothing(): void
+    {
+        $handler = new AccessLogHandler(DIRECTORY_SEPARATOR !== '\\' ? '/dev/null' : __DIR__ . '\\nul');
+
+        $request = new ServerRequest('GET', 'http://localhost:8080/users', [], '', '1.1', ['REMOTE_ADDR' => '127.0.0.1']);
+        $response = new Response(200, [], "Hello\n");
+        $handler($request, function () use ($response) { return $response; });
+    }
+
     public function testInvokeLogsRequest(): void
     {
         $handler = new AccessLogHandler();

diff --git a/tests/AppMiddlewareTest.php b/tests/AppMiddlewareTest.php
@@ -4,7 +4,7 @@
 
 use FrameworkX\AccessLogHandler;
 use FrameworkX\App;
-use FrameworkX\Io\MiddlewareHandler;
+use FrameworkX\ErrorHandler;
 use FrameworkX\Io\RouteHandler;
 use PHPUnit\Framework\TestCase;
 use Psr\Http\Message\ResponseInterface;
@@ -674,23 +674,10 @@ public function testInvokeWithGlobalMiddlewareReturnsResponseWhenGlobalMiddlewar
     /** @param callable|class-string ...$middleware */
     private function createAppWithoutLogger(...$middleware): App
     {
-        $app = new App(...$middleware);
-
-        $ref = new \ReflectionProperty($app, 'handler');
-        $ref->setAccessible(true);
-        $middleware = $ref->getValue($app);
-        assert($middleware instanceof MiddlewareHandler);
-
-        $ref = new \ReflectionProperty($middleware, 'handlers');
-        $ref->setAccessible(true);
-        $handlers = $ref->getValue($middleware);
-        assert(is_array($handlers));
-
-        $first = array_shift($handlers);
-        $this->assertInstanceOf(AccessLogHandler::class, $first);
-
-        $ref->setValue($middleware, $handlers);
-
-        return $app;
+        return new App(
+            new AccessLogHandler(DIRECTORY_SEPARATOR !== '\\' ? '/dev/null' : __DIR__ . '\\nul'),
+            new ErrorHandler(),
+            ...$middleware
+        );
     }
 }
diff --git a/tests/AppTest.php b/tests/AppTest.php
@@ -1651,23 +1651,10 @@ public function testInvokeWithMatchingRouteReturnsInternalServerErrorResponseWhe
 
     private function createAppWithoutLogger(callable ...$middleware): App
     {
-        $app = new App(...$middleware);
-
-        $ref = new \ReflectionProperty($app, 'handler');
-        $ref->setAccessible(true);
-        $middleware = $ref->getValue($app);
-        assert($middleware instanceof MiddlewareHandler);
-
-        $ref = new \ReflectionProperty($middleware, 'handlers');
-        $ref->setAccessible(true);
-        $handlers = $ref->getValue($middleware);
-        assert(is_array($handlers));
-
-        $first = array_shift($handlers);
-        $this->assertInstanceOf(AccessLogHandler::class, $first);
-
-        $ref->setValue($middleware, $handlers);
-
-        return $app;
+        return new App(
+            new AccessLogHandler(DIRECTORY_SEPARATOR !== '\\' ? '/dev/null' : __DIR__ . '\\nul'),
+            new ErrorHandler(),
+            ...$middleware
+        );
     }
 }