From 005cafae5fee34ffde215fdabee79a45572c9d68 Mon Sep 17 00:00:00 2001 From: Arnold Almeida Date: Sun, 11 May 2014 18:12:26 +1000 Subject: [PATCH 1/7] Added jsonp support --- .../ResponseFormat/JsonpResponseFormat.php | 72 +++++++++++++++ .../ResponseFormat/RequestAwareInterface.php | 12 +++ src/Routing/Router.php | 89 ++++++++++--------- 3 files changed, 132 insertions(+), 41 deletions(-) create mode 100644 src/Http/ResponseFormat/JsonpResponseFormat.php create mode 100644 src/Http/ResponseFormat/RequestAwareInterface.php diff --git a/src/Http/ResponseFormat/JsonpResponseFormat.php b/src/Http/ResponseFormat/JsonpResponseFormat.php new file mode 100644 index 000000000..614e42c91 --- /dev/null +++ b/src/Http/ResponseFormat/JsonpResponseFormat.php @@ -0,0 +1,72 @@ +request = $request; + } + + /** + * Has a callback parameter been specified in the request ? + * @return bool + */ + protected function hasValidCallback() + { + $this->callback = $this->request->input($this->callbackName); + + if (!empty($this->callback)) { + return true; + } + + return false; + } + + /** + * Get the response content type. + * + * @return string + */ + public function getContentType() + { + if ($this->hasValidCallback()) { + return 'application/javascript'; + } + + return 'application/json'; + } + + /** + * Encode the content to its JSON representation. + * + * @param string $content + * @return string + */ + protected function encode($content) + { + if ($this->hasValidCallback()) { + return sprintf('%s(%s);', $this->callback, json_encode($content)); + } + + return json_encode($content); + } + +} diff --git a/src/Http/ResponseFormat/RequestAwareInterface.php b/src/Http/ResponseFormat/RequestAwareInterface.php new file mode 100644 index 000000000..87e5b409e --- /dev/null +++ b/src/Http/ResponseFormat/RequestAwareInterface.php @@ -0,0 +1,12 @@ +api[$version] = new ApiRouteCollection($version, array_except($options, 'version')); } } - + $this->group($options, $callback); } /** * Dispatch the request to the application and return either a regular response * or an API response. - * + * * @param \Illuminate\Http\Request $request * @return \Illuminate\Http\Response|\Dingo\Api\Http\Response */ @@ -170,6 +171,12 @@ public function dispatch(Request $request) if ($this->requestTargettingApi($request)) { + $formatter = Response::getFormatter($this->requestedFormat); + + if ($formatter instanceof \Dingo\Api\Http\ResponseFormat\RequestAwareInterface) { + $formatter->setRequest($request); + } + $response = Response::makeFromExisting($response)->morph($this->requestedFormat); } @@ -178,7 +185,7 @@ public function dispatch(Request $request) /** * Handle exception thrown when dispatching a request. - * + * * @param \Symfony\Component\HttpKernel\Exception\HttpExceptionInterface $exception * @return \Dingo\Api\Http\Response */ @@ -215,7 +222,7 @@ public function handleException(HttpExceptionInterface $exception) /** * Add a new route to either the routers collection or an API collection. - * + * * @param array|string $methods * @param string $uri * @param callable|array|string $action @@ -235,7 +242,7 @@ protected function addRoute($methods, $uri, $action) /** * Add a new route to an API collection. - * + * * @param \Illuminate\Routing\Route $route * @return \Illuminate\Routing\Route */ @@ -265,7 +272,7 @@ protected function addApiRoute($route) /** * Find a route either from the routers collection or the API collection. - * + * * @param \Illuminate\Http\Request $request * @return \Illuminate\Routing\Route */ @@ -285,7 +292,7 @@ protected function findRoute($request) /** * Determine if the current request is targetting an API. - * + * * @param \Illuminate\Http\Request $request * @return bool */ @@ -322,7 +329,7 @@ public function requestTargettingApi($request = null) /** * Parse a requests accept header. - * + * * @param \Illuminate\Http\Request $request * @return array */ @@ -338,7 +345,7 @@ public function parseAcceptHeader($request) /** * Get a matching API route collection from the request. - * + * * @param \Illuminate\Http\Request $request * @return null|\Dingo\Api\Routing\ApiRouteCollection */ @@ -362,7 +369,7 @@ public function getApiRouteCollectionFromRequest(Request $request) /** * Get an API route collection for a given version. - * + * * @param string $version * @return \Dingo\Api\Routing\ApiRouteCollection */ @@ -378,7 +385,7 @@ public function getApiRouteCollection($version) /** * Determine if a route is targetting the API. - * + * * @param \Illuminate\Routing\Route * @return bool */ @@ -391,7 +398,7 @@ public function routeTargettingApi($route) /** * Set the exception handler instance. - * + * * @param \Dingo\Api\ExceptionHandler * @return void */ @@ -402,7 +409,7 @@ public function setExceptionHandler(ExceptionHandler $exceptionHandler) /** * Get the exception handler instance. - * + * * @return \Dingo\Api\ExceptionHandler */ public function getExceptionHandler() @@ -412,7 +419,7 @@ public function getExceptionHandler() /** * Set the default API version. - * + * * @param string $defaultVersion * @return void */ @@ -423,7 +430,7 @@ public function setDefaultVersion($defaultVersion) /** * Get the default API version. - * + * * @return string */ public function getDefaultVersion() @@ -433,7 +440,7 @@ public function getDefaultVersion() /** * Set the default API prefix. - * + * * @param string $defaultPrefix * @return void */ @@ -444,7 +451,7 @@ public function setDefaultPrefix($defaultPrefix) /** * Get the default API prefix. - * + * * @return string */ public function getDefaultPrefix() @@ -454,7 +461,7 @@ public function getDefaultPrefix() /** * Set the default API domain. - * + * * @param string $defaultDomain * @return void */ @@ -465,7 +472,7 @@ public function setDefaultDomain($defaultDomain) /** * Get the default API domain. - * + * * @return string */ public function getDefaultDomain() @@ -475,7 +482,7 @@ public function getDefaultDomain() /** * Set the API vendor. - * + * * @param string $vendor * @return void */ @@ -486,7 +493,7 @@ public function setVendor($vendor) /** * Get the API vendor. - * + * * @return string */ public function getVendor() @@ -496,7 +503,7 @@ public function getVendor() /** * Set the default API format. - * + * * @param string $defaultformat * @return void */ @@ -507,7 +514,7 @@ public function setDefaultFormat($defaultFormat) /** * Get the default API format. - * + * * @return string */ public function getDefaultFormat() @@ -517,7 +524,7 @@ public function getDefaultFormat() /** * Get the requested version. - * + * * @return string */ public function getRequestedVersion() @@ -527,7 +534,7 @@ public function getRequestedVersion() /** * Get the requested format. - * + * * @return string */ public function getRequestedFormat() @@ -547,7 +554,7 @@ public function getInspector() /** * Set the controller reviser instance. - * + * * @param \Dingo\Api\Routing\ControllerReviser $reviser * @return void */ @@ -558,7 +565,7 @@ public function setControllerReviser(ControllerReviser $reviser) /** * Get the controller reviser instance. - * + * * @return \Dingo\Api\Routing\ControllerReviser */ public function getControllerReviser() @@ -568,7 +575,7 @@ public function getControllerReviser() /** * Set the current request. - * + * * @param \Illuminate\Http\Request $request * @return void */ @@ -579,7 +586,7 @@ public function setCurrentRequest(Request $request) /** * Set the current route. - * + * * @param \Illuminate\Routing\Route $route * @return void */ From bd05efe6c7f90f17023576b84cc3031837b35caa Mon Sep 17 00:00:00 2001 From: Mohit Mamoria Date: Mon, 12 May 2014 15:26:31 +0530 Subject: [PATCH 2/7] Determining transformable correctly for Collections --- src/Transformer/Factory.php | 5 +++++ tests/TransformerFactoryTest.php | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Transformer/Factory.php b/src/Transformer/Factory.php index 388039d81..c714e19b5 100644 --- a/src/Transformer/Factory.php +++ b/src/Transformer/Factory.php @@ -175,6 +175,11 @@ protected function hasBinding($class) */ protected function boundByContract($class) { + if ($this->isCollection($class)) + { + return is_object($class->first()) and $class->first() instanceof TransformableInterface; + } + return is_object($class) and $class instanceof TransformableInterface; } diff --git a/tests/TransformerFactoryTest.php b/tests/TransformerFactoryTest.php index 15b623d7a..2daf6614e 100644 --- a/tests/TransformerFactoryTest.php +++ b/tests/TransformerFactoryTest.php @@ -39,7 +39,7 @@ public function testDeterminingIfResponseIsTransformable() $this->transformerFactory->transform('Foo', 'FooTransformerStub'); $this->assertTrue($this->transformerFactory->transformableResponse('Foo')); $this->assertTrue($this->transformerFactory->transformableResponse(new Bar)); - $this->assertTrue($this->transformerFactory->transformableResponse(new Illuminate\Support\Collection([new Foo, new Foo]))); + $this->assertTrue($this->transformerFactory->transformableResponse(new Illuminate\Support\Collection([new Bar, new Bar]))); } From ed7fcaa4eb806dcf35a02fdf2893cc23a0b65bd8 Mon Sep 17 00:00:00 2001 From: Jason Lewis Date: Mon, 12 May 2014 22:32:54 +1000 Subject: [PATCH 3/7] Minor adjustments and clarifying some tests for future reference. Signed-off-by: Jason Lewis --- src/Transformer/Factory.php | 2 +- tests/TransformerFactoryTest.php | 23 ++++++++++++----------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/Transformer/Factory.php b/src/Transformer/Factory.php index c714e19b5..4b38a4b17 100644 --- a/src/Transformer/Factory.php +++ b/src/Transformer/Factory.php @@ -177,7 +177,7 @@ protected function boundByContract($class) { if ($this->isCollection($class)) { - return is_object($class->first()) and $class->first() instanceof TransformableInterface; + $class = $class->first(); } return is_object($class) and $class instanceof TransformableInterface; diff --git a/tests/TransformerFactoryTest.php b/tests/TransformerFactoryTest.php index 2daf6614e..af107da5e 100644 --- a/tests/TransformerFactoryTest.php +++ b/tests/TransformerFactoryTest.php @@ -28,18 +28,19 @@ public function testRegisterTransformer() public function testDeterminingIfResponseIsTransformable() { - $this->assertFalse($this->transformerFactory->transformableResponse(['foo' => 'bar'])); - $this->assertFalse($this->transformerFactory->transformableResponse(new stdClass)); - $this->assertFalse($this->transformerFactory->transformableResponse('Foo')); - $this->assertFalse($this->transformerFactory->transformableResponse(1)); - $this->assertFalse($this->transformerFactory->transformableResponse(true)); - $this->assertFalse($this->transformerFactory->transformableResponse(false)); - $this->assertFalse($this->transformerFactory->transformableResponse(31.1)); - $this->assertFalse($this->transformerFactory->transformableResponse(new Illuminate\Support\Collection([new Foo, new Foo]))); + $this->assertFalse($this->transformerFactory->transformableResponse(['foo' => 'bar']), 'Testing that an array is not transformable.'); + $this->assertFalse($this->transformerFactory->transformableResponse(new stdClass), 'Testing that a class with no binding is not transformable.'); + $this->assertFalse($this->transformerFactory->transformableResponse('Foo'), 'Testing that a string with no binding is not transformable.'); + $this->assertFalse($this->transformerFactory->transformableResponse(1), 'Testing that an integer is not transformable.'); + $this->assertFalse($this->transformerFactory->transformableResponse(true), 'Testing that a boolean is not transformable.'); + $this->assertFalse($this->transformerFactory->transformableResponse(false), 'Testing that a boolean is not transformable.'); + $this->assertFalse($this->transformerFactory->transformableResponse(31.1), 'Testing that a float is not transformable.'); + $this->assertFalse($this->transformerFactory->transformableResponse(new Illuminate\Support\Collection([new Foo, new Foo])), 'Testing that a collection with instances that have no binding are not transformable.'); $this->transformerFactory->transform('Foo', 'FooTransformerStub'); - $this->assertTrue($this->transformerFactory->transformableResponse('Foo')); - $this->assertTrue($this->transformerFactory->transformableResponse(new Bar)); - $this->assertTrue($this->transformerFactory->transformableResponse(new Illuminate\Support\Collection([new Bar, new Bar]))); + $this->assertTrue($this->transformerFactory->transformableResponse('Foo'), 'Testing that a string with a binding is transformable.'); + $this->assertTrue($this->transformerFactory->transformableResponse(new Bar), 'Testing that an instance that is bound by a contract is transformable.'); + $this->assertTrue($this->transformerFactory->transformableResponse(new Illuminate\Support\Collection([new Bar, new Bar])), 'Testing that a collection with instances bound by a contract are transformable.'); + $this->assertTrue($this->transformerFactory->transformableResponse(new Illuminate\Support\Collection([new Foo, new Foo])), 'Testing that a collection with instances that have a binding are transformable.'); } From 9f3e052874d2519144f18b873a5159491a05e272 Mon Sep 17 00:00:00 2001 From: Arnold Almeida Date: Wed, 14 May 2014 20:27:04 +1000 Subject: [PATCH 4/7] Refactored : ResponseFormatInterface -> AbstractResponseFormat - All response formats should now have the request accessible --- ...terface.php => AbstractResponseFormat.php} | 39 +++++++++++-------- .../ResponseFormat/JsonResponseFormat.php | 33 +++++++++++----- .../ResponseFormat/JsonpResponseFormat.php | 23 +++-------- .../ResponseFormat/RequestAwareInterface.php | 12 ------ 4 files changed, 52 insertions(+), 55 deletions(-) rename src/Http/ResponseFormat/{ResponseFormatInterface.php => AbstractResponseFormat.php} (59%) delete mode 100644 src/Http/ResponseFormat/RequestAwareInterface.php diff --git a/src/Http/ResponseFormat/ResponseFormatInterface.php b/src/Http/ResponseFormat/AbstractResponseFormat.php similarity index 59% rename from src/Http/ResponseFormat/ResponseFormatInterface.php rename to src/Http/ResponseFormat/AbstractResponseFormat.php index 1caea753d..f3e3a6c9b 100644 --- a/src/Http/ResponseFormat/ResponseFormatInterface.php +++ b/src/Http/ResponseFormat/AbstractResponseFormat.php @@ -1,60 +1,67 @@ request = $request; + } /** * Format an Eloquent model. - * + * * @param \Illuminate\Database\Eloquent\Model $model * @return string */ @@ -19,7 +32,7 @@ public function formatEloquentModel($model) /** * Format an Eloquent collection. - * + * * @param \Illuminate\Database\Eloquent\Collection $collection * @return string */ @@ -37,7 +50,7 @@ public function formatEloquentCollection($collection) /** * Format a string. - * + * * @param string $string * @return string */ @@ -48,7 +61,7 @@ public function formatString($string) /** * Format an array or instance implementing ArrayableInterface. - * + * * @param \Illuminate\Support\Contracts\ArrayableInterface $response * @return string */ @@ -66,7 +79,7 @@ public function formatArrayableInterface($response) /** * Format an instance implementing JsonableInterface. - * + * * @param \Illuminate\Support\Contracts\JsonableInterface $response * @return string */ @@ -77,7 +90,7 @@ public function formatJsonableInterface($response) /** * Format an unknown type. - * + * * @param mixed $response * @return string */ @@ -88,7 +101,7 @@ public function formatUnknown($response) /** * Get the response content type. - * + * * @return string */ public function getContentType() @@ -98,7 +111,7 @@ public function getContentType() /** * Morph a value to an array. - * + * * @param array|\Illuminate\Support\Contracts\ArrayableInterface * @return array */ @@ -109,7 +122,7 @@ protected function morphToArray($value) /** * Encode the content to its JSON representation. - * + * * @param string $content * @return string */ diff --git a/src/Http/ResponseFormat/JsonpResponseFormat.php b/src/Http/ResponseFormat/JsonpResponseFormat.php index 614e42c91..3861905d2 100644 --- a/src/Http/ResponseFormat/JsonpResponseFormat.php +++ b/src/Http/ResponseFormat/JsonpResponseFormat.php @@ -1,30 +1,19 @@ request = $request; - } - /** * Has a callback parameter been specified in the request ? * @return bool @@ -51,7 +40,7 @@ public function getContentType() return 'application/javascript'; } - return 'application/json'; + return parent::getContentType(); } /** @@ -66,7 +55,7 @@ protected function encode($content) return sprintf('%s(%s);', $this->callback, json_encode($content)); } - return json_encode($content); + return parent::encode($content); } } diff --git a/src/Http/ResponseFormat/RequestAwareInterface.php b/src/Http/ResponseFormat/RequestAwareInterface.php deleted file mode 100644 index 87e5b409e..000000000 --- a/src/Http/ResponseFormat/RequestAwareInterface.php +++ /dev/null @@ -1,12 +0,0 @@ - Date: Fri, 16 May 2014 13:46:06 +1000 Subject: [PATCH 5/7] Force response status code to 200 OK when requesting jsonp else body may be dropped --- src/Routing/Router.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Routing/Router.php b/src/Routing/Router.php index ec7a1594e..b89dc62e9 100644 --- a/src/Routing/Router.php +++ b/src/Routing/Router.php @@ -180,6 +180,13 @@ public function dispatch(Request $request) $response = Response::makeFromExisting($response)->morph($this->requestedFormat); } + // Force the response to always return "200 OK" + // Chances are if the client is requesting jsonp their browser + // wont implement 422 etc and will drop the body + if ($this->requestedFormat == 'jsonp') { + $response->setStatusCode(200); + } + return $response; } From 986f0358f839e58ebe035878ba213283683d8fc0 Mon Sep 17 00:00:00 2001 From: Arnold Almeida Date: Fri, 16 May 2014 13:55:29 +1000 Subject: [PATCH 6/7] 404 should be implemented --- src/Routing/Router.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Routing/Router.php b/src/Routing/Router.php index b89dc62e9..d0dbdc9ee 100644 --- a/src/Routing/Router.php +++ b/src/Routing/Router.php @@ -184,7 +184,9 @@ public function dispatch(Request $request) // Chances are if the client is requesting jsonp their browser // wont implement 422 etc and will drop the body if ($this->requestedFormat == 'jsonp') { - $response->setStatusCode(200); + if (!in_array($response->getStatusCode(), [404, 200])) { + $response->setStatusCode(200); + } } return $response; From 51538f7f5df7911e6b686ec5b0cd37de7b94ff90 Mon Sep 17 00:00:00 2001 From: Arnold Almeida Date: Thu, 29 May 2014 11:06:37 +1000 Subject: [PATCH 7/7] - Don't monkey patch status codes for jsonp - Use 400 instead of 422 for increased compatibility for clients. See comments in https://github.com/dingo/api/pull/35 for rational. --- src/Exception/ResourceException.php | 12 ++++++------ src/Routing/Router.php | 9 --------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/Exception/ResourceException.php b/src/Exception/ResourceException.php index a7862af5a..1aedb59ec 100644 --- a/src/Exception/ResourceException.php +++ b/src/Exception/ResourceException.php @@ -8,14 +8,14 @@ class ResourceException extends HttpException { /** * MessageBag errors. - * + * * @var \Illuminate\Support\MessageBag */ protected $errors; /** * Create a new resource exception instance. - * + * * @param int $statusCode * @param string $message * @param \Illuminate\Support\MessageBag|array $errors @@ -35,12 +35,12 @@ public function __construct($message = null, $errors = null, Exception $previous $this->errors = is_array($errors) ? new MessageBag($errors) : $errors; } - parent::__construct(422, $message, $previous, $headers, $code); + parent::__construct(400, $message, $previous, $headers, $code); } /** * Get the errors message bag. - * + * * @return \Illuminate\Support\MessageBag **/ public function errors() @@ -50,7 +50,7 @@ public function errors() /** * Get the errors message bag. - * + * * @return \Illuminate\Support\MessageBag **/ public function getErrors() @@ -60,7 +60,7 @@ public function getErrors() /** * Determine if message bag has any errors. - * + * * @return bool */ public function hasErrors() diff --git a/src/Routing/Router.php b/src/Routing/Router.php index d0dbdc9ee..ec7a1594e 100644 --- a/src/Routing/Router.php +++ b/src/Routing/Router.php @@ -180,15 +180,6 @@ public function dispatch(Request $request) $response = Response::makeFromExisting($response)->morph($this->requestedFormat); } - // Force the response to always return "200 OK" - // Chances are if the client is requesting jsonp their browser - // wont implement 422 etc and will drop the body - if ($this->requestedFormat == 'jsonp') { - if (!in_array($response->getStatusCode(), [404, 200])) { - $response->setStatusCode(200); - } - } - return $response; }