Merge pull request #475 from tPl0ch/hotfix-normalizer-formatter

Normalize frames for trace items since they can contain invalid data.

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);
+                // We should again normalize the frames, because it might contain invalid items
-                $data['trace'][] = json_encode($frame);
+                $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;
-    }
 }

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');

tests/Monolog/Formatter/NormalizerFormatterTest.php

@@ -166,6 +166,51 @@ class NormalizerFormatterTest extends \PHPUnit_Framework_TestCase
 
         $this->assertEquals(@json_encode(array($resource)), $res);
     }
+
+    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) {
+            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 = array('context' => array('exception' => $e));
+        $result = $formatter->format($record);
+
+        $this->assertRegExp(
+            '%"resource":"\[resource\]"%',
+            $result['context']['exception']['trace'][0]
+        );
+
+        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(
+            $pattern,
+            $result['context']['exception']['trace'][0]
+        );
+    }
 }
 
 class TestFooNorm
@@ -180,3 +225,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);
+    }
+}