Added JSONP support for JsonView #1111

Merged
merged 1 commit into from Feb 4, 2013

Projects

None yet

3 participants

@ADmad
Member
ADmad commented Feb 2, 2013

No description provided.

@markstory markstory commented on an outdated diff Feb 2, 2013
lib/Cake/View/JsonView.php
@@ -71,22 +71,38 @@ public function __construct(Controller $controller = null) {
/**
* Render a JSON view.
*
- * Uses the special '_serialize' parameter to convert a set of
- * view variables into a JSON response. Makes generating simple
- * JSON responses very easy. You can omit the '_serialize' parameter,
- * and use a normal view + layout as well.
+ * ### Special parameters
+ * `_serialize` To convert a set of view variables into a JSON response.
+ * It's value can be a string for single variable name or array for multiple names.
+ * You can omit the`_serialize` parameter, and use a normal view + layout as well.
+ * `_callback` Query string parameter which holds function name. If `_callback` is specified
+ * and query string parameter of same name is also set the response will be wrapped
+ * in specified function. Parameter name defaults to "callback" if `_callback` is set to true.
*
@markstory
markstory Feb 2, 2013 Member

Is it worth while to have a default expected callback?

@markstory markstory and 2 others commented on an outdated diff Feb 2, 2013
lib/Cake/View/JsonView.php
}
- if ($view !== false && $this->_getViewFileName($view)) {
- return parent::render($view, false);
+
+ if (!empty($this->viewVars['_callback'])) {
+ $callback = $this->viewVars['_callback'];
+ if ($this->viewVars['_callback'] === true) {
+ $callback = 'callback';
+ }
+ if (isset($this->request->query[$callback])) {
+ $return = $this->request->query[$callback] . '(' . $return . ')';
+ $this->response->type('js');
+ }
}
@markstory
markstory Feb 2, 2013 Member

This would be simpler if there was no true case. I don't know if it provides much value as the responses will always be jsonp with a generic callback name. This seems like a not overly useful situation to me.

@lorenzo
lorenzo Feb 2, 2013 Member

Yeah, I don't see much value in having a default callback either

@ADmad
ADmad Feb 2, 2013 Member

Ok i will remove it.

@ADmad
ADmad Feb 2, 2013 Member

Just to be clear it's not the default callback function name. It's the default query string param name which will holds the js function name.

@markstory
markstory Feb 2, 2013 Member

Oh, I misread the code. Having a default querystring parameter name of callback makes sense to me. That's what jQuery uses as well. Perhaps the name of _callback needs to be different to better describe that. Perhaps _callbackParam or _callbackArg or _jsonpCallback? In any case I'm not for removing a simple way to enable jsonp that uses a default that jQuery also uses.

@ADmad
ADmad Feb 3, 2013 Member

How about just _jsonp or _jsonpParam? For setting name "jsonp" the jquery api states:

Override the callback function name in a jsonp request. This value will be used instead of 'callback' in the 'callback=?' part of the query string in the url. So {jsonp:'onJSONPLoad'} would result in 'onJSONPLoad=?' passed to the server ....

So using _jsonp would be inline with the setting name used in jquery.

For setting name jsonpCallback the jquery API says:

Specify the callback function name for a JSONP request. This value will be used instead of the random name automatically generated by jQuery...

So using param name _jsonCallback in our code would in fact cause the confusion we are actually trying to avoid.

@markstory
markstory Feb 3, 2013 Member

I think _jsonp makes sense.

  • Setting it to true enables the default callback query string parameter.
  • Setting it to a string value, uses the provided query string parameter for finding the jsonp callback name.
@markstory
Member

Nice work. I like the general concept as jsonp can be quite useful.

@lorenzo
Member
lorenzo commented Feb 2, 2013

I have an slight concern about this, perhaps we should use h() for the callback function? I know it is not possible to full page cache this types of views, but what if someone managed to do a naive version of what we have for html views? An attacker could send a php executable string in the query as the callback and that would end up being executed in the server.

@ADmad
Member
ADmad commented Feb 2, 2013

@lorenzo Guess there is no harm in being extra careful.

@ADmad
Member
ADmad commented Feb 3, 2013

@markstory Updated param name.
@lorenzo Wrapped callback function in h().

@lorenzo
Member
lorenzo commented Feb 4, 2013

This looks good to me now

@markstory markstory commented on the diff Feb 4, 2013
lib/Cake/View/JsonView.php
}
- if ($view !== false && $this->_getViewFileName($view)) {
- return parent::render($view, false);
+
+ if (!empty($this->viewVars['_jsonp'])) {
+ $jsonpParam = $this->viewVars['_jsonp'];
+ if ($this->viewVars['_jsonp'] === true) {
+ $jsonpParam = 'callback';
+ }
+ if (isset($this->request->query[$jsonpParam])) {
@markstory
markstory Feb 4, 2013 Member

You could use $this->request->query($jsonParam) to save some code, but what you have is fine and will be ~1microsecond faster as its has fewer function calls.

@ADmad
ADmad Feb 4, 2013 Member

Yes i knowingly avoided the function call and chose micro optimization over typing few extra chars less 😄

@markstory
Member

Looks good to me as well.

@ADmad ADmad merged commit afb6295 into cakephp:2.4 Feb 4, 2013
@ADmad ADmad deleted the ADmad:2.4-jsonview branch Feb 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment