diff --git a/src/AccessLogHandler.php b/src/AccessLogHandler.php index 7a5bbee..a5d87f4 100644 --- a/src/AccessLogHandler.php +++ b/src/AccessLogHandler.php @@ -14,7 +14,7 @@ */ class AccessLogHandler { - /** @var LogStreamHandler */ + /** @var ?LogStreamHandler */ private $logger; /** @var bool */ @@ -31,15 +31,38 @@ public function __construct(?string $path = null) $path = \PHP_SAPI === 'cli' ? 'php://output' : 'php://stderr'; } - $this->logger = new LogStreamHandler($path); + $logger = new LogStreamHandler($path); + if (!$logger->isDevNull()) { + // only assign logger if we're not logging to /dev/null (which would discard any logs) + $this->logger = $logger; + } + $this->hasHighResolution = \function_exists('hrtime'); // PHP 7.3+ } + /** + * [Internal] Returns whether we're writing to /dev/null (which will discard any logs) + * + * @internal + * @return bool + */ + public function isDevNull(): bool + { + return $this->logger === null; + } + /** * @return ResponseInterface|PromiseInterface|\Generator */ public function __invoke(ServerRequestInterface $request, callable $next) { + if ($this->logger === null) { + // Skip if we're logging to /dev/null (which will discard any logs). + // As an additional optimization, the `App` will automatically + // detect we no longer need to invoke this instance at all. + return $next($request); // @codeCoverageIgnore + } + $now = $this->now(); $response = $next($request); @@ -95,6 +118,7 @@ private function log(ServerRequestInterface $request, ResponseInterface $respons $responseSize = 0; } + \assert($this->logger instanceof LogStreamHandler); $this->logger->log( ($request->getAttribute('remote_addr') ?? $request->getServerParams()['REMOTE_ADDR'] ?? '-') . ' ' . '"' . $this->escape($method) . ' ' . $this->escape($request->getRequestTarget()) . ' HTTP/' . $request->getProtocolVersion() . '" ' . diff --git a/src/App.php b/src/App.php index c746ceb..2e000b2 100644 --- a/src/App.php +++ b/src/App.php @@ -80,7 +80,12 @@ public function __construct(...$middleware) if ($needsErrorHandler && ($handler instanceof ErrorHandler || $handler instanceof AccessLogHandler) && !$handlers) { $needsErrorHandler = null; } - $handlers[] = $handler; + + // only add to list of handlers if this is not a NOOP + if (!$handler instanceof AccessLogHandler || !$handler->isDevNull()) { + $handlers[] = $handler; + } + if ($handler instanceof AccessLogHandler) { $needsAccessLog = null; $needsErrorHandlerNext = true; @@ -99,7 +104,10 @@ public function __construct(...$middleware) // only log for built-in webserver and PHP development webserver by default, others have their own access log if ($needsAccessLog instanceof Container) { - \array_unshift($handlers, $needsAccessLog->getAccessLogHandler()); + $handler = $needsAccessLog->getAccessLogHandler(); + if (!$handler->isDevNull()) { + \array_unshift($handlers, $handler); + } } $this->router = new RouteHandler($container); diff --git a/src/Io/LogStreamHandler.php b/src/Io/LogStreamHandler.php index 39ccb93..8c93349 100644 --- a/src/Io/LogStreamHandler.php +++ b/src/Io/LogStreamHandler.php @@ -7,7 +7,7 @@ */ class LogStreamHandler { - /** @var resource */ + /** @var ?resource */ private $stream; /** @@ -33,6 +33,31 @@ public function __construct(string $path) }); $stream = \fopen($path, 'ae'); + + // try to fstat($stream) to see if this points to /dev/null (skip on Windows) + // @codeCoverageIgnoreStart + $stat = false; + if ($stream !== false && \DIRECTORY_SEPARATOR !== '\\') { + if (\strtolower($path) === 'php://output') { + // php://output doesn't support stat, so assume php://output will go to php://stdout + $stdout = \defined('STDOUT') ? \STDOUT : \fopen('php://stdout', 'w'); + if (\is_resource($stdout)) { + $stat = \fstat($stdout); + } else { + // STDOUT can not be opened => assume piping to /dev/null + $stream = null; + } + } else { + $stat = \fstat($stream); + } + + // close stream if it points to /dev/null + if ($stat !== false && $stat === \stat('/dev/null')) { + $stream = null; + } + } + // @codeCoverageIgnoreEnd + \restore_error_handler(); if ($stream === false) { @@ -44,8 +69,18 @@ public function __construct(string $path) $this->stream = $stream; } + public function isDevNull(): bool + { + return $this->stream === null; + } + public function log(string $message): void { + // nothing to do if we're writing to /dev/null + if ($this->stream === null) { + return; // @codeCoverageIgnore + } + $time = \microtime(true); $prefix = \date('Y-m-d H:i:s', (int) $time) . \sprintf('.%03d ', (int) (($time - (int) $time) * 1e3)); diff --git a/tests/AccessLogHandlerTest.php b/tests/AccessLogHandlerTest.php index 07563fb..11d714d 100644 --- a/tests/AccessLogHandlerTest.php +++ b/tests/AccessLogHandlerTest.php @@ -36,6 +36,24 @@ public function testCtorWithPathToNewFileWillCreateNewFile(): void unlink($path); } + public function testIsDevNullReturnsFalseForDefaultPath(): void + { + $handler = new AccessLogHandler(); + + $this->assertFalse($handler->isDevNull()); + } + + public function testIsDevNullReturnsTrueForDevNull(): void + { + if (DIRECTORY_SEPARATOR === '\\') { + $this->markTestSkipped('Not supported on Windows'); + } + + $handler = new AccessLogHandler('/dev/null'); + + $this->assertTrue($handler->isDevNull()); + } + public function testInvokeWithDefaultPathWillLogMessageToConsole(): void { $handler = new AccessLogHandler(); diff --git a/tests/AppTest.php b/tests/AppTest.php index 3875e4b..baf861c 100644 --- a/tests/AppTest.php +++ b/tests/AppTest.php @@ -362,6 +362,31 @@ public function testConstructWithAccessLogHandlerAndErrorHandlerAssignsHandlersA $this->assertInstanceOf(RouteHandler::class, $handlers[2]); } + public function testConstructWithAccessLogHandlerToDevNullAndErrorHandlerWillRemoveAccessLogHandler(): void + { + $accessLogHandler = $this->createMock(AccessLogHandler::class); + $accessLogHandler->expects($this->once())->method('isDevNull')->willReturn(true); + assert(is_callable($accessLogHandler)); + + $errorHandler = new ErrorHandler(); + + $app = new App($accessLogHandler, $errorHandler); + + $ref = new ReflectionProperty($app, 'handler'); + $ref->setAccessible(true); + $handler = $ref->getValue($app); + assert($handler instanceof MiddlewareHandler); + + $ref = new ReflectionProperty($handler, 'handlers'); + $ref->setAccessible(true); + $handlers = $ref->getValue($handler); + assert(is_array($handlers)); + + $this->assertCount(2, $handlers); + $this->assertSame($errorHandler, $handlers[0]); + $this->assertInstanceOf(RouteHandler::class, $handlers[1]); + } + public function testConstructWithAccessLogHandlerClassAndErrorHandlerClassAssignsDefaultHandlers(): void { $app = new App(AccessLogHandler::class, ErrorHandler::class); @@ -410,6 +435,35 @@ public function testConstructWithContainerAndAccessLogHandlerClassAndErrorHandle $this->assertInstanceOf(RouteHandler::class, $handlers[2]); } + public function testConstructWithContainerAndAccessLogHandlerClassAndErrorHandlerClassWillUseContainerToGetAccessLogHandlerAndWillSkipAccessLogHandlerToDevNull(): void + { + $accessLogHandler = $this->createMock(AccessLogHandler::class); + $accessLogHandler->expects($this->once())->method('isDevNull')->willReturn(true); + + $errorHandler = new ErrorHandler(); + + $container = $this->createMock(Container::class); + $container->expects($this->once())->method('getAccessLogHandler')->willReturn($accessLogHandler); + $container->expects($this->once())->method('getErrorHandler')->willReturn($errorHandler); + + assert($container instanceof Container); + $app = new App($container, AccessLogHandler::class, ErrorHandler::class); + + $ref = new ReflectionProperty($app, 'handler'); + $ref->setAccessible(true); + $handler = $ref->getValue($app); + assert($handler instanceof MiddlewareHandler); + + $ref = new ReflectionProperty($handler, 'handlers'); + $ref->setAccessible(true); + $handlers = $ref->getValue($handler); + assert(is_array($handlers)); + + $this->assertCount(2, $handlers); + $this->assertSame($errorHandler, $handlers[0]); + $this->assertInstanceOf(RouteHandler::class, $handlers[1]); + } + public function testConstructWithMiddlewareBeforeAccessLogHandlerAndErrorHandlerAssignsDefaultErrorHandlerAsFirstHandlerFollowedByGivenHandlers(): void { $middleware = static function (ServerRequestInterface $request, callable $next) { }; diff --git a/tests/Io/LogStreamHandlerCliIsDevNull.php b/tests/Io/LogStreamHandlerCliIsDevNull.php new file mode 100644 index 0000000..aadc732 --- /dev/null +++ b/tests/Io/LogStreamHandlerCliIsDevNull.php @@ -0,0 +1,11 @@ +isDevNull(), true) . PHP_EOL; + +file_put_contents($argv[2] ?? 'php://stdout', $buffer); diff --git a/tests/Io/LogStreamHandlerCliIsDevNullAfterClose.php b/tests/Io/LogStreamHandlerCliIsDevNullAfterClose.php new file mode 100644 index 0000000..31df6c3 --- /dev/null +++ b/tests/Io/LogStreamHandlerCliIsDevNullAfterClose.php @@ -0,0 +1,18 @@ +getMessage() . PHP_EOL); + exit(1); +} + +$buffer = var_export($log->isDevNull(), true) . PHP_EOL; + +file_put_contents($argv[2] ?? 'php://stdout', $buffer); diff --git a/tests/Io/LogStreamHandlerTest.php b/tests/Io/LogStreamHandlerTest.php index 1f28240..f4a77ad 100644 --- a/tests/Io/LogStreamHandlerTest.php +++ b/tests/Io/LogStreamHandlerTest.php @@ -287,4 +287,187 @@ public function testLogWithDevNullWritesNothing(): void $logger->log('Hello'); } + + public function testIsDevNullReturnsFalseForCurrentFile(): void + { + $logger = new LogStreamHandler(__FILE__); + + $this->assertFalse($logger->isDevNull()); + } + + public function testIsDevNullReturnsTrueForDevNull(): void + { + if (DIRECTORY_SEPARATOR === '\\') { + $this->markTestSkipped('Not supported on Windows'); + } + + $logger = new LogStreamHandler('/dev/null'); + + $this->assertTrue($logger->isDevNull()); + } + + public function testIsDevNullReturnsTrueForSymlinkToDevNull(): void + { + if (DIRECTORY_SEPARATOR === '\\') { + $this->markTestSkipped('Not supported on Windows'); + } + + $path = tempnam(sys_get_temp_dir(), 'null'); + assert(is_string($path)); + unlink($path); + symlink('/dev/null', $path); + + $logger = new LogStreamHandler($path); + + $this->assertTrue($logger->isDevNull()); + + unlink($path); + } + + public function testIsDevNullReturnsFalseForStdoutInChildProcess(): void + { + $pipes = []; + $process = proc_open( + escapeshellarg(PHP_BINARY) . ' ' . escapeshellarg(__DIR__ . DIRECTORY_SEPARATOR . 'LogStreamHandlerCliIsDevNull.php'), + [ + 0 => STDIN, + 1 => ['pipe', 'w'], + 2 => STDERR + ], + $pipes, + null, + null, + [ + 'bypass_shell' => true + ] + ); + assert(is_resource($process)); + + assert(is_resource($pipes[1])); + $output = stream_get_contents($pipes[1]); + assert(is_string($output)); + proc_close($process); + + $this->assertEquals('false' . PHP_EOL, $output); + } + + public function testIsDevNullReturnsTrueForStdoutInChildProcessRedirectedToDevNull(): void + { + if (DIRECTORY_SEPARATOR === '\\') { + $this->markTestSkipped('Not supported on Windows'); + } + + $pipes = []; + $process = proc_open( + escapeshellarg(PHP_BINARY) . ' ' . escapeshellarg(__DIR__ . DIRECTORY_SEPARATOR . 'LogStreamHandlerCliIsDevNull.php') . ' php://stdout php://stderr', + [ + 0 => STDIN, + 1 => ['file', '/dev/null', 'r'], + 2 => ['pipe', 'w'] + ], + $pipes, + null, + null, + [ + 'bypass_shell' => true + ] + ); + assert(is_resource($process)); + + assert(is_resource($pipes[2])); + $output = stream_get_contents($pipes[2]); + assert(is_string($output)); + proc_close($process); + + $this->assertEquals('true' . PHP_EOL, $output); + } + + public function testIsDevNullReturnsTrueForOutputStreamInChildProcessRedirectedToDevNull(): void + { + if (DIRECTORY_SEPARATOR === '\\') { + $this->markTestSkipped('Not supported on Windows'); + } + + $pipes = []; + $process = proc_open( + escapeshellarg(PHP_BINARY) . ' ' . escapeshellarg(__DIR__ . '/LogStreamHandlerCliIsDevNull.php') . ' php://output php://stderr', + [ + 0 => STDIN, + 1 => ['file', '/dev/null', 'r'], + 2 => ['pipe', 'w'] + ], + $pipes, + null, + null, + [ + 'bypass_shell' => true + ] + ); + assert(is_resource($process)); + + assert(is_resource($pipes[2])); + $output = stream_get_contents($pipes[2]); + assert(is_string($output)); + proc_close($process); + + $this->assertEquals('true' . PHP_EOL, $output); + } + + public function testIsDevNullReturnsTrueForOutputStreamReferencingClosedStdoutInChildProcess(): void + { + if (DIRECTORY_SEPARATOR === '\\') { + $this->markTestSkipped('Not supported on Windows'); + } + + $pipes = []; + $process = proc_open( + escapeshellarg(PHP_BINARY) . ' ' . escapeshellarg(__DIR__ . '/LogStreamHandlerCliIsDevNullAfterClose.php') . ' php://output php://stderr', + [ + 0 => STDIN, + 1 => STDOUT, + 2 => ['pipe', 'w'] + ], + $pipes, + null, + null, + [ + 'bypass_shell' => true + ] + ); + assert(is_resource($process)); + + assert(is_resource($pipes[2])); + $output = stream_get_contents($pipes[2]); + assert(is_string($output)); + proc_close($process); + + $this->assertEquals('true' . PHP_EOL, $output); + } + + public function testCtorWithStdoutStreamThrowsIfStdoutIsAlreadyClosed(): void + { + $pipes = []; + $process = proc_open( + escapeshellarg(PHP_BINARY) . ' ' . escapeshellarg(__DIR__ . '/LogStreamHandlerCliIsDevNullAfterClose.php') . ' php://stdout php://stderr', + [ + 0 => STDIN, + 1 => STDOUT, + 2 => ['pipe', 'w'] + ], + $pipes, + null, + null, + [ + 'bypass_shell' => true + ] + ); + assert(is_resource($process)); + + assert(is_resource($pipes[2])); + $output = stream_get_contents($pipes[2]); + assert(is_string($output)); + proc_close($process); + + $this->assertEquals('RuntimeException: Unable to open log file "php://stdout": operation failed' . PHP_EOL, $output); + } }