Added Json support for POST and PUT operations in restful controller. #3028

Closed
wants to merge 6 commits into
from

2 participants

@jvandemo

Added Json support for POST and PUT operations in restful controllers.

This is particularly handy if you are using a javascript library (such as AngularJS by Google) that uses "Content-Type: application/json" for POST and PUT operations.

This PR detects if the content type is set to json (in request headers) and makes sure that the "create" and "update" methods of the restful controller receive the correct data.

Without this PR, the data is empty and one needs to decode the response body manually.

Change-Id: I11963778fa99d1dd1c523203c25d5a9f53958310

Jurgen Van de Moere Added Json support for POST and PUT operations in restful controller.
Change-Id: I11963778fa99d1dd1c523203c25d5a9f53958310
50564ab
@weierophinney weierophinney and 1 other commented on an outdated diff Nov 20, 2012
...ary/Zend/Mvc/Controller/AbstractRestfulController.php
@@ -11,6 +11,7 @@
namespace Zend\Mvc\Controller;
use Zend\Http\Request as HttpRequest;
+use Zend\Json\Decoder;
@weierophinney
Zend Framework member
weierophinney added a line comment Nov 20, 2012

Don't use Zend\Json\Decoder. Use Zend\Json\Json::decode() instead, as it uses json_decode() by default. Decoder should only be used in rare occasions, typically when the JSON is known to be in an encoding other than UTF-8, or where you know JS lambdas may be included in the content.

@jvandemo
jvandemo added a line comment Nov 20, 2012

Could you be more specific ?

Do you suggest to have Zend\Json\Decover replaced with json_decode ?

Or do you want to skip automatic decoding in the abstract class and leave decoding to the controller instance ?

@weierophinney
Zend Framework member
weierophinney added a line comment Nov 20, 2012

I mean:

  • import Zend\Json\Json instead of Zend\Json\Decoder
  • Use Json::decode() instead of Decoder::decode()

In other words, continue with automated decoding, just change the mechanism. :)

@jvandemo
jvandemo added a line comment Nov 20, 2012

Ok, got it, I will update the code and notify you when finished. Thanks !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney and 1 other commented on an outdated diff Nov 20, 2012
...ary/Zend/Mvc/Controller/AbstractRestfulController.php
{
+ $contentType = strtolower( $request->getHeaders()->get('Content-Type')->getFieldValue());
+
+ if (strpos($contentType, 'application/json') !== false) {
@weierophinney
Zend Framework member
weierophinney added a line comment Nov 20, 2012

I'm wondering if this check should be configurable. It's not uncommon to define custom media types such as application/vnd.my.app+json -- which would fail this check, but still be something you'd want to automatically decode. The configuration could be present in a protected class member.

@jvandemo
jvandemo added a line comment Nov 20, 2012

Very true, do you think it would be useful to handle other content types as well ?

If so, maybe we can create some kind of pluggable content type handler that formats data depending on content type...

@weierophinney
Zend Framework member
weierophinney added a line comment Nov 20, 2012

You might take a look at how the new acceptedViewModel plugin works -- you pass it a list of Accept media types you want to match. We could do similarly here, but instead have those in a protected member so that developers could specify them themselves:

class MyResource extends AbstractRestfulController
{
    protected $jsonContentTypes = array(
        'application/vnd.myapp.resource+json',
        'application/json',
    );
}

The code would then check the Content-Type header value against each of these; if any match, it does the decoding.

@jvandemo
jvandemo added a line comment Nov 20, 2012

Good idea, I will implement it like that and get back to you when ready. Thanks !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jurgen Van d... added some commits Nov 21, 2012
Jurgen Van de Moere Replaced Zend\Json\Decoder::decode with Zend\Json\Json::decode
Change-Id: Iac885dbf1511dfb8087d7f3586ae845e071b26c5
48addfa
Jurgen Van de Moere Added protected contentTypes array and requestHasContentType function
Change-Id: I222617c77b3942fdc5b8263b827c1c58dab570b7
9dfdefb
@jvandemo

Hi @weierophinney,

I updated the code with a generic content type checker that can later be extended for other content types as well (if necessary).

The protected contentTypes array can now easily be overridden to add more content type values to check against.

PS: I noticed some strange test file updates in the commit that were pushed by my zend studio...

@weierophinney weierophinney commented on an outdated diff Dec 10, 2012
...ary/Zend/Mvc/Controller/AbstractRestfulController.php
/**
* @var string
*/
protected $eventIdentifier = __CLASS__;
-
+
+ /**
+ * @var array
+ */
+ protected $contentTypes = array(
+ self::CONTENT_TYPE_JSON => array(
+ 'application/vnd.myapp.resource+json',
@weierophinney
Zend Framework member
weierophinney added a line comment Dec 10, 2012

I'd remove this one. However, I'd likely add "application/hal+json" to it (as that's becoming a common standard).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on an outdated diff Dec 10, 2012
...ary/Zend/Mvc/Controller/AbstractRestfulController.php
$content = $request->getContent();
parse_str($content, $parsedParams);
return $this->update($id, $parsedParams);
}
+
+ /**
+ * Check if request has certain content type
+ *
+ * @return boolean
+ */
+ public function requestHasContentType (Request $request, $contentType = '')
@weierophinney
Zend Framework member
weierophinney added a line comment Dec 10, 2012

Use the Accept header object's match() method instead of what you do in here; it's more robust, and won't return a false positive in situations where other content types have higher priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney
Zend Framework member

Looks like the various files under _files should not have been changed; can you remove those from the PR, by any chance? Also, please address the other feedback I provided.

Otherwise... looking good!

@weierophinney
Zend Framework member

@jvandemo Will you be able to get this ready in the next 1-2 weeks? If not, I need to change the milestone.

@jvandemo

@weierophinney Yes, absolutely. I just got back today from a 2 week absence due to the birth of my son, so I'll fix it asap. Thanks !!

Jurgen Van d... added some commits Jan 3, 2013
Jurgen Van de Moere Revert "Added protected contentTypes array and requestHasContentType …
…function"

This reverts commit 9dfdefbf4c9d695bc0d2125c30571ae37ff95a89.
66ffac0
Jurgen Van de Moere Updated requestHasContentType to use the match method of the Accept
header object.

Change-Id: I7e43851c88692b5ea675393719f91d3a4ef379dd
b69296a
Jurgen Van de Moere Format source code
Change-Id: I8035798710e17d3e0ec2fdf08fe2d0e1b8086732
3557b13
@jvandemo

@weierophinney The various files under _files were removed and the changes you suggested have been implemented.

Let me know if you need any other changes.

Thanks !

@weierophinney weierophinney added a commit that referenced this pull request Jan 4, 2013
@weierophinney weierophinney [#3028] Add check for existence of Accept header
- If we don't have an Accept header, we can't test for allowed content type, and
  must return false
e353a79
@weierophinney
Zend Framework member

Merged to develop branch for 2.1 release -- thanks!

@katalonec katalonec pushed a commit to katalonec/zf2 that referenced this pull request Jan 4, 2013
@weierophinney weierophinney Merge branch 'feature/3028' into develop
Close #3028
f3b4e2b
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney [#3028] Add check for existence of Accept header
- If we don't have an Accept header, we can't test for allowed content type, and
  must return false
4833554
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney Merge branch 'feature/3028' into develop
Close #3028
c0a33e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment