Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixes to $response->json() #43

Merged
merged 2 commits into from

3 participants

@nickl-

Thank you for this awesome hardcore minimalist one size fits all page router, knap gedaan!

Here is my small contribution to the amazing effort you've already done.

  • Added the Content-Type: application/json to the $request->json() function to reflect the json content we are responding with.
  • The quotes surrounding the $callback when doing the echo and passing a closure got php's knickers in a twist but by removing them it now works perfectly when doing something like;
<?php
    $response->json($data, function ($json) {
      return "<pre>". $json ."</pre>";
    });

...not sure if this was the intended purpose or not?

PHP's complaint otherwise:
Catchable fatal error: Object of class Closure could not be converted to string in /[snip]/klein.php on line 474

Keep up the great work...

nico Fixes to ->json(); added Content-Type header to reflect the applicati…
…on/json content we are sending, removed quotes surrounding function call
4bfbfa7
@abackstrom

According to the README.md, $callback is the JSONP callback rather than the PHP callback, so it's valid to output $callback as a simple string. You could try to identify if a Callable was passed as $callback, but there would still be ambiguity for simple strings.

@nickl- nickl- Disambiguate reference to the jsonp padding prefix misleadingly refer…
…red to as callback. Supply correct Content-Type response header for json but fall back on Content-Type text/javascript for jsonp as the proposed Content-Type application/json-p has not been adopted as yet. Documentation elaborated to further attempt to avoid confusion while keeping the implementation minimalist and klein.
634774f
@nickl-

Ahh indeed indeed, thought it didn't make sense to be a callback and due to the marginal documentation and ambiguous reference to a $callback function it results in a PLBKAC error instead of blaming PHP for being a whiney girl I have to first remove my end-user cap and replace it with the propeller-head I keep for emergencies.

Now that I am all clever with the propeller spinning to keep my engorged brain matter safely under a 100 I start knowing better than everyone and won't stop until you at least hear some of it:

  • I completely disagree with the notion that it is a callback! It is obvious what happened here again, some bloody idiot read the proposal with their limited capacity could only deduce that uhm duh uhm function oh yes I get it uhm uhm whatshamacallit that other thingy ah callback yes thats it, its a callback. Theres no bloody callback in JSON you moron its a serialized string. Why do you think its JSONP and not... lets not even go there. They called it JSON with padding for a reason you fool. No insult to injury would you even believe me if I told you these idiots standardized the request parameter for exchanging the function name as "callback", they should friggin pass their 515s I would love to "call them back" ...morons. I guess they would also expect us to refer to a "JSON callback" on the server-side and we would all stay ignorant to the actual callbacks, ahh bliss. I'm always telling you: it should hurt when you're stupid else you'll never stop.

Sound familiar? =)

Well at least a few crazy people like ourselves who probably also went off like that for a few hours managed to preserve JSON with padding so we can refer to the function definition as a padding prefix and its perfectly ok to use jsonp as the request parameter as well, just in case, you'll never know.

The intuitive approach would probably be to separate them into 2 functions function json($object) and function jsonp($prefix, $object) but I thought we would prefer to keep things klein.

I am also leaning heavily towards an automated approach handling the request checking for the presence of $request->callback or $request->jsonp but you know how it goes, someone would go swim upstream and sooner or later we would have to provide them with a function $request->setJSONPCallback($callback); *shivers* which would also require a function $request->getJSONPCallback(); and before you know it we change the name to vet.php... don't even go there.

My proposal:

<?php
    //Sends an object as json or jsonp by providing the padding prefix
    public function json($object, $jsonp_prefix = null) {
        $this->discard();
        $this->noCache();
        set_time_limit(1200);
        $json = json_encode($object);
        if (null !== $jsonp_prefix) {
            header('Content-Type: text/javascript'); // should ideally be application/json-p once adopted
            echo "$jsonp_prefix($json);";
        } else {
            header('Content-Type: application/json');
            echo $json;
        }
    }

Updating the documentation to reflect the same:

   json($object, $jsonp_prefix = null)             // Send an object as JSON or JSONP by providing padding prefix

Tested Content-Type application/json-p but Chrome throws a fit so I gave her the assumed text/javascript so she can chill. Works as expected with jQuery $.getJSON(), do you think I missed anything else?

Taking off the propeller-head now before... well you know.

@chriso
Owner

Thanks. You typically pass &callback= to a JSONP endpoint hence my original approach. I agree that it could be ambiguous in the context of the library so merging now.

@chriso chriso merged commit 59b17ab into chriso:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 11, 2012
  1. Fixes to ->json(); added Content-Type header to reflect the applicati…

    nico authored
    …on/json content we are sending, removed quotes surrounding function call
Commits on May 12, 2012
  1. @nickl-

    Disambiguate reference to the jsonp padding prefix misleadingly refer…

    nickl- authored
    …red to as callback. Supply correct Content-Type response header for json but fall back on Content-Type text/javascript for jsonp as the proposed Content-Type application/json-p has not been adopted as yet. Documentation elaborated to further attempt to avoid confusion while keeping the implementation minimalist and klein.
This page is out of date. Refresh to see the latest.
Showing with 7 additions and 5 deletions.
  1. +1 −1  README.md
  2. +6 −4 klein.php
View
2  README.md
@@ -261,7 +261,7 @@ $response->
flash($msg, $type = 'info', $params = array() // Set a flash message
file($path, $filename = null) // Send a file
noCache() // Tell the browser not to cache the response
- json($object, $callback = null) // Send an object as JSON(p)
+ json($object, $jsonp_prefix = null) // Send an object as JSON or JSONP by providing padding prefix
markdown($str, $args, ...) // Return a string formatted with markdown
code($code = null) // Return the HTTP response code, or send a new code
redirect($url, $code = 302) // Redirect to the specified URL
View
10 klein.php
@@ -438,15 +438,17 @@ public function file($path, $filename = null, $mimetype = null) {
readfile($path);
}
- //Sends an object as json
- public function json($object, $callback = null) {
+ //Sends an object as json or jsonp by providing the padding prefix
+ public function json($object, $jsonp_prefix = null) {
$this->discard();
$this->noCache();
set_time_limit(1200);
$json = json_encode($object);
- if (null !== $callback) {
- echo "$callback($json)";
+ if (null !== $jsonp_prefix) {
+ header('Content-Type: text/javascript'); // should ideally be application/json-p once adopted
+ echo "$jsonp_prefix($json);";
} else {
+ header('Content-Type: application/json');
echo $json;
}
}
Something went wrong with that request. Please try again.