slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.97k stars 1.95k forks source link

[v3] Response::withJson #1276

Closed ahundiak closed 9 years ago

ahundiak commented 9 years ago

The existing slim response object already has a non-PSR7 method withRedirect.

I wonder if it would make sense to add some additional helper functions such as withJson, withPreflight and possibly a few more? Be glad to tackle the withJson method since I currently do a lot of:

$response->getBody()->write(json_encode($items));
$response = $response->withHeader('Content-Type','application/json');

// as opposed to
$response = $response->withJson($payload);

where $payload could either be a json string or an array.

Just seems like this would be a common enough use case to standardize on.

codeguy commented 9 years ago

+1

codeguy commented 9 years ago

Perhaps we want to replace the Response body rather than write to it.

ahundiak commented 9 years ago

Here is a proposed implementation. Figured it might be easier to discuss the design here instead of inside a PR:

public function withJson($data, $status = 200, $encodingOptions = 0)
{
    $body = $this->getBody();
    $body->rewind(); // Replace contents instead of trying to append
    $body->write(json_encode($data,$encodingOptions));
    return $this->withStatus($status)->withHeader('Content-Type','application/json;charset='utf-8');
}

The default encoding options are my main question. The smart folks at Symfony have kept their class RFC4627 compliant with:

$encodingOptions = JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT; 

while PHP targets the newly proposed RFC7159. My inclination is to just go with PHP. If someone needs to support old json processors then they can add the encoding options.

The S2 version also has a bunch of code to setup an error handler. I don't see this as being necessary.

I added charset to the content type because json_encode requires all strings to be utf-8 encoded.

Suggestions?

geggleto commented 9 years ago

@codeguy @ahundiak Is there a way we can have a 3rd party service do this, I would like the ability to keep using the PSR interface in my functions rather than the Slim object.

ahundiak commented 9 years ago

That is indeed the 64,536 dollar question. I confess I don't have a clear understanding of the Slim Framework's goal with respect to making inter-operable middleware. There are a bunch of non-PSR7 methods in the Slim request/response classes. I only suggested withJson because they already had withRedirect.

Even worse (in my opinion) than the extra methods is the notion of binding the callables to the application instance. So now the callables are not only dependent on the request/response objects but on the application object as well.

You could of course define withJson to be a service. But then you have a problem in that your route callable it now dependent on a specific service. Move to another framework and you still need the service to be available. So you really have just pushed the problem off to the side.

I think the Symfony folks have come up with the inter-operable missing piece of the puzzle by implementing the notion of a message bridge: http://dunglas.fr/2015/06/using-psr-7-in-symfony/ Basically, when you call $next the function should check the classes of the source request/response objects against the expected destination objects. If the classes are different then convert them. I think that will go a long way in solving the inter-operable question but have not really messed around with it.

So the bottom line is that I'm comfortable with the notion of having additional non-PSR7 methods.

geggleto commented 9 years ago

I dont see why withRedirect needs to be there... it is easy enough to accomplish via proper status codes...

$response->withStatusCode(301)->withHeader('Location', 'your new uri');

I would prefer having some sort of Helper that implements these helper functions that implements the functions via the PSR-7 methods so that the Helper is merely reducing the amount of code one has to write and not providing additional functionality over the PSR-7 interface.

If you create a helper object that operates only within PSR-7, then you merely have to include that object in another project and you maintain interop.

One could make a static helper class with functions that have the signature of either ($request) or ($response)...

in this case it might be something like...


class PSRHelper {

protected static function $encodingOptions = array();

public static function withJSON($request)
{
    $request = $request->withBody(json_encode($request->getBody(),$encodingOptions));
    return $request;
}

To use...
//Usage ?
$app->add(function ($request, $response, $next) {
    $next(PSRHelper::withJSON($request), $response);
});

Probably not valid code but I hope this illustrates my idea.

silentworks commented 9 years ago

Moving from Slim 2 to 3 we want to keep the ease of use, I don't think abstracting away withRedirect and withJson is a good approach, they should probably stay where they are.

codeguy commented 9 years ago

I'm not sure I really understand the aversion to native methods on our own PSR-7 implementation. You don't have to use those methods. And you don't even have to use our own PSR-7 implementation if you prefer something else entirely. These are merely there for convenience and do not affect Slim's support for PSr-7 interoperability.

akrabat commented 9 years ago

Me neither.

PSR-7 is interesting for writing middleware that works with multiple dispatchers. When you're writing the route callable, it's Slim specific anyway.

ahundiak commented 9 years ago

Getting back on topic: Implemented Response::withJson in PR #1286

ahundiak commented 9 years ago

Going back off topic and returning to the discussion of using non-Psr7 methods in the message objects:

I understand the perspective of framework developers in that you want your framework to be as fast and easy to develop in as possible. Hence implementing middleware against specific Slim classes is fine.

However, as an application developer, I would rather write code against interfaces. I disagree with the notion that you cannot share callables between different frameworks. You might need an adapter and you might take a slight performance hit but I have shown to my own satisfaction that I can indeed share code between Symfony 2 and Slim 3.

As long as Slim middleware type hints against Slim classes then framework Interoperability is a possibility. And wouldn't be nice to be able to market Slim as a direct competitor against Symfony without requiring Symfony developers to completely rewrite their code?

akrabat commented 9 years ago

FWiW, Zend-Stratigility solves this problem by wrapping the PSR-7 objects in its own versions which have the extended functionality.

e.g. their Response object implements write(), but getProtocolVersion() is simply a proxy to the underlying PSR-7 object.

codeguy commented 9 years ago

@akrabat That assumes the end user is providing a third-party implementation, correct? Slim would need to have its own implementation AND then more decorators. Seems to be getting complex.

akrabat commented 9 years ago

Yeah. I wasn't suggesting that Slim does this at all - just showing a different approach. I'm happy with the additional methods on our objects.

akrabat commented 9 years ago

Fixed.