Add optional $path argument for AccessLogHandler

clue-labs · Feb 28, 2024 · 1be48fe · 1be48fe
1 parent fe0df4e
commit 1be48fe
Show file tree

Hide file tree

Showing 5 changed files with 173 additions and 45 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/tests/AccessLogHandlerTest.php b/tests/AccessLogHandlerTest.php
@@ -12,6 +12,101 @@
 
 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));
+        unlink($path);
+
+        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);
+        }
+    }
+
+    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));
+        unlink($path);
+
+        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);
+        }
+    }
+
+    /**
+     * @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
+        );
     }
 }