From dca8f5841fc471866dd13401ffefe3e21ee453ea Mon Sep 17 00:00:00 2001 From: Thomas Ploch Date: Tue, 16 Dec 2014 13:23:04 +0100 Subject: [PATCH 1/3] Normalize frames for trace items since they can contain invalid data. Refs https://github.com/Seldaek/monolog/pull/474/files The fix in the previous PR did not take into account that there might be object wrapped resources that would break json_encode, so the best solution would be normalizing those frames again. @Seldaek Sorry for the inconvenience, but our graylog is still ramming up with those json_encode error messages. --- src/Monolog/Formatter/NormalizerFormatter.php | 29 +--------- .../Formatter/GelfMessageFormatterTest.php | 44 --------------- .../Formatter/NormalizerFormatterTest.php | 53 +++++++++++++++++++ 3 files changed, 55 insertions(+), 71 deletions(-) diff --git a/src/Monolog/Formatter/NormalizerFormatter.php b/src/Monolog/Formatter/NormalizerFormatter.php index 6717e40e..8a1fdab8 100644 --- a/src/Monolog/Formatter/NormalizerFormatter.php +++ b/src/Monolog/Formatter/NormalizerFormatter.php @@ -108,8 +108,8 @@ class NormalizerFormatter implements FormatterInterface if (isset($frame['file'])) { $data['trace'][] = $frame['file'].':'.$frame['line']; } else { - $frame = $this->convertResourceArgs($frame); - $data['trace'][] = json_encode($frame); + // We should again normalize the frames, because it might contain invalid items + $data['trace'][] = $this->toJson($this->normalize($frame), true); } } @@ -137,29 +137,4 @@ class NormalizerFormatter implements FormatterInterface return json_encode($data); } - - /** - * This method checks recursively for resource args inside the frame, since json_encode is choking on them. - * - * @param array $frame Reference to current frame - * @return array - */ - private function convertResourceArgs(array $frame) - { - foreach ($frame as $key => $item) { - if (is_scalar($item)) { - continue; - } - - if (is_resource($item)) { - $frame[$key] = (string) $item; - } - - if (is_array($item)) { - $frame[$key] = $this->convertResourceArgs($item); - } - } - - return $frame; - } } diff --git a/tests/Monolog/Formatter/GelfMessageFormatterTest.php b/tests/Monolog/Formatter/GelfMessageFormatterTest.php index 2cbba3fc..3f47a09a 100644 --- a/tests/Monolog/Formatter/GelfMessageFormatterTest.php +++ b/tests/Monolog/Formatter/GelfMessageFormatterTest.php @@ -182,50 +182,6 @@ class GelfMessageFormatterTest extends \PHPUnit_Framework_TestCase $this->assertEquals('pair', $message_array['_EXTkey']); } - /** - * @covers Monolog\Formatter\GelfMessageFormatter::format - */ - public function testExceptionObjectWithResourceTrace() - { - if (defined('HHVM_VERSION')) { - $this->markTestSkipped('Not supported in HHVM since it detects errors differently'); - } - - // This happens i.e. in React promises or Guzzle streams where stream wrappers are registered - // and no file or line are included in the trace because it's treated as internal function - set_error_handler(function ($errno, $errstr, $errfile, $errline ) { - throw new \ErrorException($errstr, 0, $errno, $errfile, $errline); - }); - - try { - $resource = fopen('php://memory', '+w'); - // Just do something stupid with a resource as argument - strpos($resource, 'FOO'); - } catch (\Exception $e) { - } - - // restore the error handler - restore_error_handler(); - - $formatter = new GelfMessageFormatter(); - $record = array( - 'level' => Logger::ERROR, - 'level_name' => 'ERROR', - 'channel' => 'meh', - 'context' => array('from' => 'logger', 'exception' => $e), - 'datetime' => new \DateTime("@0"), - 'extra' => array(), - 'message' => 'log' - ); - - $message = $formatter->format($record); - - $this->assertRegExp( - '%\\\\"resource\\\\":\\\\"Resource id #\d+\\\\"%', - $message->getAdditional('ctxt_exception') - ); - } - private function isLegacy() { return interface_exists('\Gelf\IMessagePublisher'); diff --git a/tests/Monolog/Formatter/NormalizerFormatterTest.php b/tests/Monolog/Formatter/NormalizerFormatterTest.php index 74778712..e5983f56 100644 --- a/tests/Monolog/Formatter/NormalizerFormatterTest.php +++ b/tests/Monolog/Formatter/NormalizerFormatterTest.php @@ -166,6 +166,41 @@ class NormalizerFormatterTest extends \PHPUnit_Framework_TestCase $this->assertEquals(@json_encode(array($resource)), $res); } + + public function testExceptionTraceWithArgs() + { + // This happens i.e. in React promises or Guzzle streams where stream wrappers are registered + // and no file or line are included in the trace because it's treated as internal function + set_error_handler(function ($errno, $errstr, $errfile, $errline) { + throw new \ErrorException($errstr, 0, $errno, $errfile, $errline); + }); + + try { + // This will contain $resource and $wrappedResource as arguments in the trace item + $resource = fopen('php://memory', 'rw+'); + fwrite($resource, 'test_resource'); + $wrappedResource = new TestStreamFoo($resource); + // Just do something stupid with a resource/wrapped resource as argument + array_keys($wrappedResource); + } catch (\Exception $e) { + restore_error_handler(); + } + + $formatter = new NormalizerFormatter(); + $record = ['context' => ['exception' => $e]]; + $result = $formatter->format($record); + + $this->assertRegExp( + '%"resource":"\[resource\]"%', + $result['context']['exception']['trace'][0] + ); + + // Tests that the wrapped resource is ignored while encoding + $this->assertRegExp( + '%\\\\"resource\\\\":null%', + $result['context']['exception']['trace'][0] + ); + } } class TestFooNorm @@ -180,3 +215,21 @@ class TestBarNorm return 'bar'; } } + +class TestStreamFoo +{ + public $foo; + public $resource; + + public function __construct($resource) + { + $this->resource = $resource; + $this->foo = 'BAR'; + } + + public function __toString() + { + fseek($this->resource, 0); + return $this->foo . ' - ' . (string) stream_get_contents($this->resource); + } +} From e66ba56cc02b619a2afe28c5d531ff4234bdde5b Mon Sep 17 00:00:00 2001 From: Thomas Ploch Date: Tue, 16 Dec 2014 13:35:22 +0100 Subject: [PATCH 2/3] Fixed `array()` usage and added `HHVM_VERSION` check --- tests/Monolog/Formatter/NormalizerFormatterTest.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/Monolog/Formatter/NormalizerFormatterTest.php b/tests/Monolog/Formatter/NormalizerFormatterTest.php index e5983f56..0da32ca7 100644 --- a/tests/Monolog/Formatter/NormalizerFormatterTest.php +++ b/tests/Monolog/Formatter/NormalizerFormatterTest.php @@ -169,6 +169,10 @@ class NormalizerFormatterTest extends \PHPUnit_Framework_TestCase public function testExceptionTraceWithArgs() { + if (defined('HHVM_VERSION')) { + $this->markTestSkipped('Not supported in HHVM since it detects errors differently'); + } + // This happens i.e. in React promises or Guzzle streams where stream wrappers are registered // and no file or line are included in the trace because it's treated as internal function set_error_handler(function ($errno, $errstr, $errfile, $errline) { @@ -187,7 +191,7 @@ class NormalizerFormatterTest extends \PHPUnit_Framework_TestCase } $formatter = new NormalizerFormatter(); - $record = ['context' => ['exception' => $e]]; + $record = array('context' => array('exception' => $e)); $result = $formatter->format($record); $this->assertRegExp( From 50b6bf45d1a5c17beaebd2aa95f552a8f2e7aec0 Mon Sep 17 00:00:00 2001 From: Thomas Ploch Date: Tue, 16 Dec 2014 13:46:06 +0100 Subject: [PATCH 3/3] Use different testing pattern for PHP 5.4 and PHP 5.5 --- tests/Monolog/Formatter/NormalizerFormatterTest.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/Monolog/Formatter/NormalizerFormatterTest.php b/tests/Monolog/Formatter/NormalizerFormatterTest.php index 0da32ca7..859fe25d 100644 --- a/tests/Monolog/Formatter/NormalizerFormatterTest.php +++ b/tests/Monolog/Formatter/NormalizerFormatterTest.php @@ -199,9 +199,15 @@ class NormalizerFormatterTest extends \PHPUnit_Framework_TestCase $result['context']['exception']['trace'][0] ); - // Tests that the wrapped resource is ignored while encoding + if (version_compare(PHP_VERSION, '5.5.0', '>=')) { + $pattern = '%"wrappedResource":"\[object\] \(Monolog\\\\\\\\Formatter\\\\\\\\TestStreamFoo: \)"%'; + } else { + $pattern = '%\\\\"resource\\\\":null%'; + } + + // Tests that the wrapped resource is ignored while encoding, only works for PHP <= 5.4 $this->assertRegExp( - '%\\\\"resource\\\\":null%', + $pattern, $result['context']['exception']['trace'][0] ); }