diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index da5a5a7d6..b97f4fb94 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -5,7 +5,7 @@ on: - pull_request permissions: - contents: read # to fetch code (actions/checkout) + contents: read jobs: tests: @@ -43,8 +43,7 @@ jobs: composer-options: "--ignore-platform-req=php+" steps: - - name: "Checkout" - uses: "actions/checkout@v4" + - uses: "actions/checkout@v4" - name: Run CouchDB timeout-minutes: 3 @@ -58,8 +57,7 @@ jobs: with: mongodb-version: 5.0 - - name: "Install PHP" - uses: "shivammathur/setup-php@v2" + - uses: "shivammathur/setup-php@v2" with: coverage: "none" php-version: "${{ matrix.php-version }}" @@ -75,8 +73,7 @@ jobs: composer require --no-update --no-interaction --dev elasticsearch/elasticsearch:^7 composer config --no-plugins allow-plugins.ocramius/package-versions true - - name: "Update dependencies with composer" - uses: "ramsey/composer-install@v2" + - uses: "ramsey/composer-install@v2" with: dependency-versions: "${{ matrix.dependencies }}" composer-options: "${{ matrix.composer-options }}" @@ -132,8 +129,7 @@ jobs: es-version: "7.0.0" steps: - - name: "Checkout" - uses: "actions/checkout@v4" + - uses: "actions/checkout@v4" # required for elasticsearch - name: Configure sysctl limits @@ -149,8 +145,7 @@ jobs: with: stack-version: "${{ matrix.es-version }}" - - name: "Install PHP" - uses: "shivammathur/setup-php@v2" + - uses: "shivammathur/setup-php@v2" with: coverage: "none" php-version: "${{ matrix.php-version }}" @@ -165,8 +160,7 @@ jobs: if: "matrix.php-version == '7.4' && matrix.dependencies == 'lowest'" run: "composer config allow-plugins.ocramius/package-versions true" - - name: "Update dependencies with composer" - uses: "ramsey/composer-install@v2" + - uses: "ramsey/composer-install@v2" with: dependency-versions: "${{ matrix.dependencies }}" @@ -177,9 +171,9 @@ jobs: if: "contains(matrix.dependencies, 'highest') && matrix.php-version >= '8.0'" run: | composer remove --no-update --dev graylog2/gelf-php ruflin/elastica elasticsearch/elasticsearch rollbar/rollbar - composer require --no-update --no-interaction --dev ruflin/elastica elasticsearch/elasticsearch:^7 + composer require --no-update --no-interaction --dev ruflin/elastica:^7 elasticsearch/elasticsearch:^7 composer require --no-update psr/log:^3 - composer update -W + composer update composer exec phpunit -- --group Elasticsearch,Elastica --verbose tests-es-8: @@ -211,8 +205,7 @@ jobs: - "8.2.0" steps: - - name: "Checkout" - uses: "actions/checkout@v4" + - uses: "actions/checkout@v4" # required for elasticsearch - name: Configure sysctl limits @@ -228,8 +221,7 @@ jobs: with: stack-version: "${{ matrix.es-version }}" - - name: "Install PHP" - uses: "shivammathur/setup-php@v2" + - uses: "shivammathur/setup-php@v2" with: coverage: "none" php-version: "${{ matrix.php-version }}" @@ -246,8 +238,7 @@ jobs: if: "matrix.php-version == '7.4' && matrix.dependencies == 'lowest'" run: "composer config allow-plugins.ocramius/package-versions true" - - name: "Update dependencies with composer" - uses: "ramsey/composer-install@v2" + - uses: "ramsey/composer-install@v2" with: dependency-versions: "${{ matrix.dependencies }}" diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 708e22997..f23abf17a 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -1,11 +1,11 @@ name: "PHP Lint" on: - push: - pull_request: + - push + - pull_request permissions: - contents: read # to fetch code (actions/checkout) + contents: read jobs: tests: @@ -17,19 +17,27 @@ jobs: matrix: php-version: - "7.2" - - "8.3" + - "nightly" steps: - - name: "Checkout" - uses: "actions/checkout@v4" + - uses: actions/checkout@v4 - - name: "Install PHP" - uses: "shivammathur/setup-php@v2" + - uses: shivammathur/setup-php@v2 with: - coverage: "none" - extensions: "intl" - ini-values: "memory_limit=-1" php-version: "${{ matrix.php-version }}" + coverage: none - name: "Lint PHP files" - run: "find src/ -type f -name '*.php' -print0 | xargs -0 -L1 -P4 -- php -l -f" + run: | + hasErrors=0 + for f in $(find src/ tests/ -type f -name '*.php' ! -path '*/vendor/*' ! -path '*/Fixtures/*') + do + { error="$(php -derror_reporting=-1 -ddisplay_errors=1 -l -f $f 2>&1 1>&3 3>&-)"; } 3>&1; + if [ "$error" != "" ]; then + while IFS= read -r line; do echo "::error file=$f::$line"; done <<< "$error" + hasErrors=1 + fi + done + if [ $hasErrors -eq 1 ]; then + exit 1 + fi diff --git a/.github/workflows/phpstan.yml b/.github/workflows/phpstan.yml index 2effa1eb8..82a85db74 100644 --- a/.github/workflows/phpstan.yml +++ b/.github/workflows/phpstan.yml @@ -4,11 +4,8 @@ on: - push - pull_request -env: - COMPOSER_FLAGS: "--ansi --no-interaction --no-progress --prefer-dist" - permissions: - contents: read # to fetch code (actions/checkout) + contents: read jobs: tests: @@ -22,33 +19,22 @@ jobs: - "8.0" steps: - - name: "Checkout" - uses: "actions/checkout@v4" + - uses: actions/checkout@v4 - - name: "Install PHP" - uses: "shivammathur/setup-php@v2" + - uses: shivammathur/setup-php@v2 with: - coverage: "none" php-version: "${{ matrix.php-version }}" + coverage: none extensions: mongodb, redis, amqp - - name: Get composer cache directory - id: composercache - run: echo "dir=$(composer config cache-files-dir)" >> "$GITHUB_OUTPUT" - - - name: Cache dependencies - uses: actions/cache@v4 - with: - path: ${{ steps.composercache.outputs.dir }} - key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.json') }} - restore-keys: ${{ runner.os }}-composer- - - name: Add require for mongodb/mongodb to make tests runnable run: "composer require ${{ env.COMPOSER_FLAGS }} mongodb/mongodb --dev --no-update" - - name: "Install latest dependencies" - # --ignore-platform-req=php here needed as long as elasticsearch/elasticsearch does not support php 8 - run: "composer update ${{ env.COMPOSER_FLAGS }} --ignore-platform-req=php" + - uses: ramsey/composer-install@v3 + with: + dependency-versions: highest + # --ignore-platform-req=php here needed as long as elasticsearch/elasticsearch does not support php 8 + composer-options: '--ignore-platform-req=php' - name: Run PHPStan run: composer phpstan diff --git a/CHANGELOG.md b/CHANGELOG.md index a9a31e713..122d85cde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +### 2.10.0 (2024-11-12) + + * Added `$fileOpenMode` to `StreamHandler` to define a custom fopen mode to open the log file (#1913) + * Fixed `StreamHandler` handling of write failures so that it now closes/reopens the stream and retries the write once before failing (#1882) + * Fixed `StreamHandler` error handler causing issues if a stream handler triggers an error (#1866) + * Fixed `JsonFormatter` handling of incomplete classes (#1834) + * Fixed `RotatingFileHandler` bug where rotation could sometimes not happen correctly (#1905) + ### 2.9.3 (2024-04-12) * Fixed PHP 8.4 deprecation warnings (#1874) diff --git a/src/Monolog/Formatter/JsonFormatter.php b/src/Monolog/Formatter/JsonFormatter.php index b737d82e3..753e6852c 100644 --- a/src/Monolog/Formatter/JsonFormatter.php +++ b/src/Monolog/Formatter/JsonFormatter.php @@ -192,6 +192,10 @@ protected function normalize($data, int $depth = 0) return $data; } + if (\get_class($data) === '__PHP_Incomplete_Class') { + return new \ArrayObject($data); + } + if (method_exists($data, '__toString')) { return $data->__toString(); } diff --git a/src/Monolog/Handler/ElasticsearchHandler.php b/src/Monolog/Handler/ElasticsearchHandler.php index e88375c0e..264b380d7 100644 --- a/src/Monolog/Handler/ElasticsearchHandler.php +++ b/src/Monolog/Handler/ElasticsearchHandler.php @@ -187,6 +187,7 @@ protected function bulkSend(array $records): void */ protected function createExceptionFromResponses($responses): Throwable { + // @phpstan-ignore offsetAccess.nonOffsetAccessible foreach ($responses['items'] ?? [] as $item) { if (isset($item['index']['error'])) { return $this->createExceptionFromError($item['index']['error']); diff --git a/src/Monolog/Handler/RotatingFileHandler.php b/src/Monolog/Handler/RotatingFileHandler.php index 17745d221..2d0c1a72d 100644 --- a/src/Monolog/Handler/RotatingFileHandler.php +++ b/src/Monolog/Handler/RotatingFileHandler.php @@ -112,17 +112,22 @@ public function setFilenameFormat(string $filenameFormat, string $dateFormat): s */ protected function write(array $record): void { - // on the first record written, if the log is new, we should rotate (once per day) + // on the first record written, if the log is new, we rotate (once per day) after the log has been written so that the new file exists if (null === $this->mustRotate) { $this->mustRotate = null === $this->url || !file_exists($this->url); } + // if the next rotation is expired, then we rotate immediately if ($this->nextRotation <= $record['datetime']) { $this->mustRotate = true; - $this->close(); + $this->close(); // triggers rotation } parent::write($record); + + if ($this->mustRotate) { + $this->close(); // triggers rotation + } } /** @@ -134,6 +139,8 @@ protected function rotate(): void $this->url = $this->getTimedFilename(); $this->nextRotation = new \DateTimeImmutable('tomorrow'); + $this->mustRotate = false; + // skip GC of old logs if files are unlimited if (0 === $this->maxFiles) { return; @@ -166,8 +173,6 @@ protected function rotate(): void restore_error_handler(); } } - - $this->mustRotate = false; } protected function getTimedFilename(): string diff --git a/src/Monolog/Handler/StreamHandler.php b/src/Monolog/Handler/StreamHandler.php index 82c048e1c..218d43847 100644 --- a/src/Monolog/Handler/StreamHandler.php +++ b/src/Monolog/Handler/StreamHandler.php @@ -41,17 +41,22 @@ class StreamHandler extends AbstractProcessingHandler protected $filePermission; /** @var bool */ protected $useLocking; + /** @var string */ + protected $fileOpenMode; /** @var true|null */ private $dirCreated = null; + /** @var bool */ + private $retrying = false; /** * @param resource|string $stream If a missing path can't be created, an UnexpectedValueException will be thrown on first write * @param int|null $filePermission Optional file permissions (default (0644) are only for owner read/write) * @param bool $useLocking Try to lock log file before doing any writes + * @param string $fileOpenMode The fopen() mode used when opening a file, if $stream is a file path * * @throws \InvalidArgumentException If stream is not a resource or string */ - public function __construct($stream, $level = Logger::DEBUG, bool $bubble = true, ?int $filePermission = null, bool $useLocking = false) + public function __construct($stream, $level = Logger::DEBUG, bool $bubble = true, ?int $filePermission = null, bool $useLocking = false, $fileOpenMode = 'a') { parent::__construct($level, $bubble); @@ -78,6 +83,7 @@ public function __construct($stream, $level = Logger::DEBUG, bool $bubble = true throw new \InvalidArgumentException('A stream must either be a resource or a string.'); } + $this->fileOpenMode = $fileOpenMode; $this->filePermission = $filePermission; $this->useLocking = $useLocking; } @@ -134,9 +140,11 @@ protected function write(array $record): void } $this->createDir($url); $this->errorMessage = null; - set_error_handler([$this, 'customErrorHandler']); + set_error_handler(function (...$args) { + return $this->customErrorHandler(...$args); + }); try { - $stream = fopen($url, 'a'); + $stream = fopen($url, $this->fileOpenMode); if ($this->filePermission !== null) { @chmod($url, $this->filePermission); } @@ -162,8 +170,30 @@ protected function write(array $record): void flock($stream, LOCK_EX); } - $this->streamWrite($stream, $record); + $this->errorMessage = null; + set_error_handler(function (...$args) { + return $this->customErrorHandler(...$args); + }); + try { + $this->streamWrite($stream, $record); + } finally { + restore_error_handler(); + } + if ($this->errorMessage !== null) { + $error = $this->errorMessage; + // close the resource if possible to reopen it, and retry the failed write + if (!$this->retrying && $this->url !== null && $this->url !== 'php://memory') { + $this->retrying = true; + $this->close(); + $this->write($record); + + return; + } + + throw new \UnexpectedValueException('Writing to the log file failed: '.$error . Utils::getRecordMessageForException($record)); + } + $this->retrying = false; if ($this->useLocking) { flock($stream, LOCK_UN); } @@ -183,7 +213,7 @@ protected function streamWrite($stream, array $record): void private function customErrorHandler(int $code, string $msg): bool { - $this->errorMessage = preg_replace('{^(fopen|mkdir)\(.*?\): }', '', $msg); + $this->errorMessage = preg_replace('{^(fopen|mkdir|fwrite)\(.*?\): }', '', $msg); return true; } @@ -212,7 +242,9 @@ private function createDir(string $url): void $dir = $this->getDirFromStream($url); if (null !== $dir && !is_dir($dir)) { $this->errorMessage = null; - set_error_handler([$this, 'customErrorHandler']); + set_error_handler(function (...$args) { + return $this->customErrorHandler(...$args); + }); $status = mkdir($dir, 0777, true); restore_error_handler(); if (false === $status && !is_dir($dir) && strpos((string) $this->errorMessage, 'File exists') === false) { diff --git a/src/Monolog/Logger.php b/src/Monolog/Logger.php index 3c588a707..bf65d3c53 100644 --- a/src/Monolog/Logger.php +++ b/src/Monolog/Logger.php @@ -169,7 +169,7 @@ class Logger implements LoggerInterface, ResettableInterface private $logDepth = 0; /** - * @var \WeakMap<\Fiber, int>|null Keeps track of depth inside fibers to prevent infinite logging loops + * @var \WeakMap<\Fiber, int> Keeps track of depth inside fibers to prevent infinite logging loops */ private $fiberLogDepth; @@ -197,7 +197,7 @@ public function __construct(string $name, array $handlers = [], array $processor if (\PHP_VERSION_ID >= 80100) { // Local variable for phpstan, see https://github.com/phpstan/phpstan/issues/6732#issuecomment-1111118412 - /** @var \WeakMap<\Fiber, int> $fiberLogDepth */ + /** @var \WeakMap<\Fiber, int> $fiberLogDepth */ $fiberLogDepth = new \WeakMap(); $this->fiberLogDepth = $fiberLogDepth; } @@ -345,6 +345,7 @@ public function addRecord(int $level, string $message, array $context = [], ?Dat if ($this->detectCycles) { if (\PHP_VERSION_ID >= 80100 && $fiber = \Fiber::getCurrent()) { + // @phpstan-ignore offsetAssign.dimType $this->fiberLogDepth[$fiber] = $this->fiberLogDepth[$fiber] ?? 0; $logDepth = ++$this->fiberLogDepth[$fiber]; } else { @@ -753,7 +754,7 @@ public function __unserialize(array $data): void if (\PHP_VERSION_ID >= 80100) { // Local variable for phpstan, see https://github.com/phpstan/phpstan/issues/6732#issuecomment-1111118412 - /** @var \WeakMap<\Fiber, int> $fiberLogDepth */ + /** @var \WeakMap<\Fiber, int> $fiberLogDepth */ $fiberLogDepth = new \WeakMap(); $this->fiberLogDepth = $fiberLogDepth; } diff --git a/src/Monolog/Utils.php b/src/Monolog/Utils.php index 360c42199..d4ff4c040 100644 --- a/src/Monolog/Utils.php +++ b/src/Monolog/Utils.php @@ -249,7 +249,7 @@ public static function expandIniShorthandBytes($val) } $val = (int) $match['val']; - switch (strtolower($match['unit'] ?? '')) { + switch (strtolower($match['unit'])) { case 'g': $val *= 1024; case 'm': diff --git a/tests/Monolog/Formatter/JsonFormatterTest.php b/tests/Monolog/Formatter/JsonFormatterTest.php index 147021029..33f6b4d7d 100644 --- a/tests/Monolog/Formatter/JsonFormatterTest.php +++ b/tests/Monolog/Formatter/JsonFormatterTest.php @@ -297,6 +297,18 @@ public function testNormalizeHandleLargeArrays() $this->assertEquals('Over 1000 items (2000 total), aborting normalization', $res['context'][0]['...']); } + public function testCanNormalizeIncompleteObject(): void + { + $serialized = "O:17:\"Monolog\TestClass\":1:{s:23:\"\x00Monolog\TestClass\x00name\";s:4:\"test\";}"; + $object = unserialize($serialized); + + $formatter = new JsonFormatter(); + $record = ['context' => ['object' => $object]]; + $result = $formatter->format($record); + + self::assertSame('{"context":{"object":{"__PHP_Incomplete_Class_Name":"Monolog\\\\TestClass"}}}'."\n", $result); + } + public function testEmptyContextAndExtraFieldsCanBeIgnored() { $formatter = new JsonFormatter(JsonFormatter::BATCH_MODE_JSON, true, true); diff --git a/tests/Monolog/Handler/StreamHandlerTest.php b/tests/Monolog/Handler/StreamHandlerTest.php index 8c69cc58e..4a339d5f7 100644 --- a/tests/Monolog/Handler/StreamHandlerTest.php +++ b/tests/Monolog/Handler/StreamHandlerTest.php @@ -17,6 +17,13 @@ class StreamHandlerTest extends TestCase { + public function tearDown(): void + { + parent::tearDown(); + + @unlink(__DIR__.'/test.log'); + } + /** * @covers Monolog\Handler\StreamHandler::__construct * @covers Monolog\Handler\StreamHandler::write @@ -54,9 +61,9 @@ public function testClose() $handler->handle($this->getRecord(Logger::WARNING, 'test')); $stream = $handler->getStream(); - $this->assertTrue(is_resource($stream)); + $this->assertTrue(\is_resource($stream)); $handler->close(); - $this->assertFalse(is_resource($stream)); + $this->assertFalse(\is_resource($stream)); } /** @@ -204,6 +211,63 @@ public function testWriteNonExistingFileResource() $handler->handle($this->getRecord()); } + /** + * @covers Monolog\Handler\StreamHandler::write + */ + public function testWriteErrorDuringWriteRetriesWithClose() + { + $handler = $this->getMockBuilder(StreamHandler::class) + ->onlyMethods(['streamWrite']) + ->setConstructorArgs(['file://'.sys_get_temp_dir().'/bar/'.rand(0, 10000).DIRECTORY_SEPARATOR.rand(0, 10000)]) + ->getMock(); + + $refs = []; + $handler->expects($this->exactly(2)) + ->method('streamWrite') + ->willReturnCallback(function ($stream) use (&$refs) { + $refs[] = $stream; + if (\count($refs) === 2) { + self::assertNotSame($stream, $refs[0]); + } + if (\count($refs) === 1) { + trigger_error('fwrite(): Write of 378 bytes failed with errno=32 Broken pipe', E_USER_ERROR); + } + }); + + $handler->handle($this->getRecord()); + if (method_exists($this, 'assertIsClosedResource')) { + self::assertIsClosedResource($refs[0]); + self::assertIsResource($refs[1]); + } + } + + /** + * @covers Monolog\Handler\StreamHandler::write + */ + public function testWriteErrorDuringWriteRetriesButThrowsIfStillFails() + { + $handler = $this->getMockBuilder(StreamHandler::class) + ->onlyMethods(['streamWrite']) + ->setConstructorArgs(['file://'.sys_get_temp_dir().'/bar/'.rand(0, 10000).DIRECTORY_SEPARATOR.rand(0, 10000)]) + ->getMock(); + + $refs = []; + $handler->expects($this->exactly(2)) + ->method('streamWrite') + ->willReturnCallback(function ($stream) use (&$refs) { + $refs[] = $stream; + if (\count($refs) === 2) { + self::assertNotSame($stream, $refs[0]); + } + trigger_error('fwrite(): Write of 378 bytes failed with errno=32 Broken pipe', E_USER_ERROR); + }); + + self::expectException(\UnexpectedValueException::class); + self::expectExceptionMessage('Writing to the log file failed: Write of 378 bytes failed with errno=32 Broken pipe +The exception occurred while attempting to log: test'); + $handler->handle($this->getRecord()); + } + /** * @covers Monolog\Handler\StreamHandler::__construct * @covers Monolog\Handler\StreamHandler::write diff --git a/tests/Monolog/SignalHandlerTest.php b/tests/Monolog/SignalHandlerTest.php index bc511d08b..b0cf772bb 100644 --- a/tests/Monolog/SignalHandlerTest.php +++ b/tests/Monolog/SignalHandlerTest.php @@ -185,7 +185,7 @@ public function testRegisterCallablePreviousSignalHandler($callPrevious) $logger = new Logger('test', array($handler = new TestHandler)); $errHandler = new SignalHandler($logger); $previousCalled = 0; - pcntl_signal(SIGURG, function ($signo, array $siginfo = null) use (&$previousCalled) { + pcntl_signal(SIGURG, function ($signo, ?array $siginfo = null) use (&$previousCalled) { ++$previousCalled; }); $errHandler->registerSignalHandler(SIGURG, LogLevel::INFO, $callPrevious, false, false);