merged branch havvg/feature/jsonp-response (PR #3639)

Commits
-------

4a43453 remove callback from constructor and create method
601b87c add basic validation of callback name
266f76d rename jsonp to callback, defaults to null
38b79a7 add data and callback setter to JsonResponse
6788224 add JSONP support to JsonResponse

Discussion
----------

add JSONP support to JsonResponse

---------------------------------------------------------------------------

by schmittjoh at 2012-03-19T17:56:24Z

I think a ``setCallback()`` method would be more expressive, and easier to use:

```php
<?php

return JsonResponse::create($myData)->setCallback('foo');
// vs
return new JsonResponse($myData, 200, array(), 'foo');
```

What do you think?

---------------------------------------------------------------------------

by havvg at 2012-03-19T18:07:38Z

Looks good to me, I'll add it.

---------------------------------------------------------------------------

by dlsniper at 2012-03-19T19:38:45Z

+1 for this one :)

---------------------------------------------------------------------------

by vicb at 2012-03-19T23:09:50Z

Sorry for nitpicking but what about:

* some validation on the function name ?
* renamming `jsonp` -> `callback` ?

---------------------------------------------------------------------------

by havvg at 2012-03-20T09:16:32Z

@vicb What do you mean with "some validation on the function name"? I can't follow you there.

---------------------------------------------------------------------------

by vicb at 2012-03-20T09:22:49Z

I mean a valid JS function name

---------------------------------------------------------------------------

by havvg at 2012-03-20T09:34:59Z

Ah I see, I searched for it, and ended up with those results:

* The most complete: http://stackoverflow.com/questions/2008279/validate-a-javascript-function-name#answer-9392578
* and a less accurate one: http://www.geekality.net/2011/08/03/valid-javascript-identifier/

I'm not sure whether to put this into the `JsonResponse` itself, or to add somewhere else (where, if so?).

---------------------------------------------------------------------------

by vicb at 2012-03-20T09:45:20Z

I would go for a regexp only (ignoring reserved words); The idea would be not to use this to run arbitrary JS code.

---------------------------------------------------------------------------

by fabpot at 2012-03-21T21:33:36Z

Now that you have added the `setCallback` method, I would remove the constructor argument as it makes the signature quite long. As we have the `create` method anyway, it's more explicit and clearer to use the `setCallback` .

---------------------------------------------------------------------------

by havvg at 2012-03-21T21:37:51Z

So remove the callback argument from both, the constructor and the `create` method or only from the constructor?

---------------------------------------------------------------------------

by havvg at 2012-03-21T21:38:30Z

Ehr.. never mind :-)

src/Symfony/Component/HttpFoundation/JsonResponse.php

@@ -18,25 +18,21 @@ namespace Symfony\Component\HttpFoundation;
  */
 class JsonResponse extends Response
 {
+    protected $data;
+    protected $callback;
+
     /**
      * Constructor.
      *
-     * @param mixed   $data    The response data
-     * @param integer $status  The response status code
-     * @param array   $headers An array of response headers
+     * @param mixed   $data     The response data
+     * @param integer $status   The response status code
+     * @param array   $headers  An array of response headers
      */
     public function __construct($data = array(), $status = 200, $headers = array())
     {
-        // root should be JSON object, not array
-        if (is_array($data) && 0 === count($data)) {
-            $data = new \ArrayObject();
-        }
+        parent::__construct('', $status, $headers);
 
-        parent::__construct(
-            json_encode($data),
-            $status,
-            array_merge(array('Content-Type' => 'application/json'), $headers)
-        );
+        $this->setData($data);
     }
 
     /**
@@ -46,4 +42,64 @@ class JsonResponse extends Response
     {
         return new static($data, $status, $headers);
     }
+
+    /**
+     * Sets the JSONP callback.
+     *
+     * @param string $callback
+     *
+     * @return JsonResponse
+     */
+    public function setCallback($callback = null)
+    {
+        if ($callback) {
+            // taken from http://www.geekality.net/2011/08/03/valid-javascript-identifier/
+            $pattern = '/^[$_\p{L}][$_\p{L}\p{Mn}\p{Mc}\p{Nd}\p{Pc}\x{200C}\x{200D}]*+$/u';
+            if (!preg_match($pattern, $callback)) {
+                throw new \InvalidArgumentException('The callback name is not valid.');
+            }
+        }
+
+        $this->callback = $callback;
+
+        return $this->update();
+    }
+
+    /**
+     * Sets the data to be sent as json.
+     *
+     * @param mixed $data
+     *
+     * @return JsonResponse
+     */
+    public function setData($data = array())
+    {
+        // root should be JSON object, not array
+        if (is_array($data) && 0 === count($data)) {
+            $data = new \ArrayObject();
+        }
+
+        $this->data = json_encode($data);
+
+        return $this->update();
+    }
+
+    /**
+     * Updates the content and headers according to the json data and callback.
+     *
+     * @return JsonResponse
+     */
+    protected function update()
+    {
+        $content = $this->data;
+        $this->headers->set('Content-Type', 'application/json', false);
+
+        if ($this->callback) {
+            $content = sprintf('%s(%s);', $this->callback, $content);
+            // Not using application/javascript for compatibility reasons with older browsers.
+            $this->headers->set('Content-Type', 'text/javascript', true);
+        }
+
+        return $this->setContent($content);
+    }
 }

tests/Symfony/Tests/Component/HttpFoundation/JsonResponseTest.php

@@ -15,6 +15,8 @@ use Symfony\Component\HttpFoundation\JsonResponse;
 
 /**
  * @covers Symfony\Component\HttpFoundation\JsonResponse::__construct
+ * @covers Symfony\Component\HttpFoundation\JsonResponse::setData
+ * @covers Symfony\Component\HttpFoundation\JsonResponse::setCallback
  */
 class JsonResponseTest extends \PHPUnit_Framework_TestCase
 {
@@ -86,4 +88,20 @@ class JsonResponseTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('{"foo":"bar"}', $response->getContent());
         $this->assertEquals(204, $response->getStatusCode());
     }
+
+    public function testSetCallback()
+    {
+        $response = JsonResponse::create(array('foo' => 'bar'))->setCallback('callback');
+
+        $this->assertEquals('callback({"foo":"bar"});', $response->getContent());
+        $this->assertEquals('text/javascript', $response->headers->get('Content-Type'));
+    }
+
+    public function testSetCallbackInvalidIdentifier()
+    {
+        $response = new JsonResponse('foo');
+
+        $this->setExpectedException('InvalidArgumentException');
+        $response->setCallback('+invalid');
+    }
 }