Merge branch 'hotfix/442'

Close #442
zendframework · Feb 13, 2017 · 36a0aa1 · 36a0aa1
2 parents 036e0d0 + b95338f
commit 36a0aa1
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -101,7 +101,12 @@ All notable changes to this project will be documented in this file, in reverse
 
 ### Fixed
 
-- Nothing.
+- [#442](https://github.com/zendframework/zend-expressive/pull/442) fixes how
+  the `WhoopsFactory` disables JSON output for whoops; previously, providing
+  boolean `false` values for either of the configuration flags
+  `json_exceptions.show_trace` or `json_exceptions.ajax_only` would result in
+  enabling the settings; these flags are now correctly evaluated by the
+  `WhoopsFactory`.
 
 ## 1.0.6 - 2017-01-09
 

diff --git a/src/Container/WhoopsFactory.php b/src/Container/WhoopsFactory.php
@@ -10,6 +10,7 @@
 use Interop\Container\ContainerInterface;
 use Whoops\Handler\JsonResponseHandler;
 use Whoops\Run as Whoops;
+use Whoops\Util\Misc as WhoopsUtil;
 
 /**
  * Create and return an instance of the Whoops runner.
@@ -75,14 +76,15 @@ private function registerJsonHandler(Whoops $whoops, $config)
 
         $handler = new JsonResponseHandler();
 
-        if (isset($config['json_exceptions']['show_trace'])) {
+        if (! empty($config['json_exceptions']['show_trace'])) {
             $handler->addTraceToOutput(true);
         }
 
-        if (isset($config['json_exceptions']['ajax_only'])) {
-            if (method_exists(\Whoops\Util\Misc::class, 'isAjaxRequest')) {
-                // Whoops 2.x
-                if (! \Whoops\Util\Misc::isAjaxRequest()) {
+        if (! empty($config['json_exceptions']['ajax_only'])) {
+            if (method_exists(WhoopsUtil::class, 'isAjaxRequest')) {
+                // Whoops 2.x; don't push handler on stack unless we are in
+                // an XHR request.
+                if (! WhoopsUtil::isAjaxRequest()) {
                     return;
                 }
             } elseif (method_exists($handler, 'onlyForAjaxRequests')) {

diff --git a/test/Container/WhoopsFactoryTest.php b/test/Container/WhoopsFactoryTest.php
@@ -12,6 +12,7 @@
 use PHPUnit_Framework_TestCase as TestCase;
 use Prophecy\Prophecy\ObjectProphecy;
 use ReflectionProperty;
+use Traversable;
 use Whoops\Handler\JsonResponseHandler;
 use Whoops\Handler\PrettyPageHandler;
 use Whoops\Run as Whoops;
@@ -76,33 +77,68 @@ public function testWillInjectJsonResponseHandlerIfConfigurationExpectsIt()
     }
 
     /**
-     * @depends testWillInjectJsonResponseHandlerIfConfigurationExpectsIt
+     * @depends      testWillInjectJsonResponseHandlerIfConfigurationExpectsIt
+     * @dataProvider provideConfig
+     *
+     * @param bool  $showsTrace
+     * @param bool  $isAjaxOnly
+     * @param bool  $requestIsAjax
      */
-    public function testJsonResponseHandlerCanBeConfigured()
+    public function testJsonResponseHandlerCanBeConfigured($showsTrace, $isAjaxOnly, $requestIsAjax)
     {
         // Set for Whoops 2.x json handler detection
-        $_SERVER['HTTP_X_REQUESTED_WITH'] = 'xmlhttprequest';
+        if ($requestIsAjax) {
+            $_SERVER['HTTP_X_REQUESTED_WITH'] = 'xmlhttprequest';
+        }
 
         $config = [
             'whoops' => [
                 'json_exceptions' => [
                     'display'    => true,
-                    'show_trace' => true,
-                    'ajax_only'  => true,
+                    'show_trace' => $showsTrace,
+                    'ajax_only'  => $isAjaxOnly,
                 ],
             ],
         ];
+
         $this->injectServiceInContainer($this->container, 'config', $config);
 
         $factory = $this->factory;
         $whoops  = $factory($this->container->reveal());
+        $handler = $whoops->popHandler();
+
+        // If ajax only, not ajax request and Whoops 2, it does not inject JsonResponseHandler
+        if ($isAjaxOnly
+            && ! $requestIsAjax
+            && method_exists(\Whoops\Util\Misc::class, 'isAjaxRequest')
+        ) {
+            $this->assertInstanceOf(PrettyPageHandler::class, $handler);
 
-        $jsonHandler = $whoops->popHandler();
-        $this->assertInstanceOf(JsonResponseHandler::class, $jsonHandler);
-        $this->assertAttributeSame(true, 'returnFrames', $jsonHandler);
+            // Skip remaining assertions
+            return;
+        }
+
+        $this->assertAttributeSame($showsTrace, 'returnFrames', $handler);
 
-        if (method_exists($jsonHandler, 'onlyForAjaxRequests')) {
-            $this->assertAttributeSame(true, 'onlyForAjaxRequests', $jsonHandler);
+        if (method_exists($handler, 'onlyForAjaxRequests')) {
+            $this->assertAttributeSame($isAjaxOnly, 'onlyForAjaxRequests', $handler);
         }
     }
+
+    /**
+     * @return Traversable
+     */
+    public function provideConfig()
+    {
+        // @codingStandardsIgnoreStart
+        //    test case                        => showsTrace, isAjaxOnly, requestIsAjax
+        yield 'Shows trace'                    => [true,      true,       true];
+        yield 'Does not show trace'            => [false,     true,       true];
+
+        yield 'Ajax only, request is ajax'     => [true,      true,       true];
+        yield 'Ajax only, request is not ajax' => [true,      true,       false];
+
+        yield 'Not ajax only'                  => [true,      false,      false];
+        // @codingStandardsIgnoreEnd
+    }
 }