-
Notifications
You must be signed in to change notification settings - Fork 152
Shortcut method for creating response from a string #52
Shortcut method for creating response from a string #52
Conversation
I always wonder, should convenience method like proposed one be a part of main object, or is it better to create factory class? |
I don't think there's a need for a factory class here - this is a factory method and the whole point is to make instantiation easier. I don't think a factory class would serve that purpose. ;) |
@franzliedke I do not want this on the response object, as it's ambiguous: does the method expect an entire response string (including status line and headers), or just the body? From the name, it's unclear. Additionally, it violates SRP; the message is a value object; what you're proposing is a builder. namespace Zend\Diactoros\Response;
use Zend\Diactoros\Response;
class HtmlResponse extends Response
{
public function __construct($html)
{
$this->body = new Stream();
$this->body->write($html);
}
} Or similar. (This may require that we make a property or two protected instead of private, however.) Alternately, a factory, as you suggested, would also be reasonable. But not a static factory method on the message itself. |
It's just an alternate constructor for the one use case that I encountered most often when using this library. A factory class would destroy any ease of use that this PR aims to introduce. |
I agree that the naming could be improved (any suggestions?), but I'd argue, or rather let Mathias Verraes argue, that named constructor methods are totally sensible when used in a use-case-driven way. |
No, it wouldn't. use Zend\Diactoros\Response\HtmlResponse;
// constructor version:
$response = new HtmlResponse($html);
// factory approach:
$response = HtmlResponse:: fromHtml($html); For the consumer, it replaces the import statement they'd otherwise use for importing the response class, and remains a single call. It's also semantically more clear, as the class name and/or factory indicates that html is expected. In both cases, you have a PSR -7 response instance in the end. Choose which syntax you prefer, update the PR, and I'll review. |
Oh, you want the extension class as part of this package? In that case, I'm all in. ;) Sorry, I thought you were suggesting I should add my own subclass whenever consuming this package. |
This allows easy creation of responses with string content. No need to mess with streams.
Okay, I created a subclass now. Please check it out. :) |
Creating another response class to create a stream from a string makes little sense. There's nothing different about the behavior of the I think what you're really looking for is a class MemoryStream extends Stream
{
public function __construct($contents)
{
parent::__construct('php://memory', 'w+');
$this->write($contents);
}
} This stream could be used when creating a $response = new Response(new MemoryStream($contents)); |
This is an example of string response.. IMO.. |
@trowski No, I don't want to create a stream to instantiate a response from a simple string. I agree that |
What about separate factory class then? I think that using inheritance to provide different way of creating object is not good OOP practice, as after instantiation process you end up with the very same class. Separate factory class can also be used to create different convenience methods, for different use cases. |
here other solution ;D |
*/ | ||
public function __construct($html, $status = 200, array $headers = []) | ||
{ | ||
parent::__construct('php://memory', $status, $headers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use php://temp
here. php://temp
will use memory, but, more importantly, if the memory limit is reached, it will dump to a temp file, freeing up memory, which is useful with large payloads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that this is an HTML response, perhaps the Content-Type
header should also be set?
$hasContentType = array_reduce(array_keys($headers), function ($carry, $item) {
return $carry ?: (strtolower($item) === 'content-type');
}, false);
if (! $hasContentType) {
$headers['content-type'] = ['text/html']; // or application/xhtml+xml, one of the two
}
parent::__construct('php://temp', $status, $headers);
(The array_reduce
is in there because we can't guarantee the casing of the header keys.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want this to be a HTML response, though, just a StringResponse
. We could, of course, add helpers for the most commonly used ones. Something like Response::html()
and Response::json()
would come to mind.
I posted a comment on a line that then became outdated, and now we're losing some comments in the UI, so.. I commented that since this was listed as an $hasContentType = array_reduce(array_keys($headers), function ($carry, $item) {
return $carry ?: (strtolower($item) === 'content-type');
}, false);
if (! $hasContentType) {
$headers['content-type'] = ['text/html']; // or application/xhtml+xml
}
parent::__construct('php://temp', $status, $headers); To which @franzliedke replied:
This sounds to me like we likely want a factory, that has some common logic. Here's a take on that: namespace Zend\Diactoros\Response;
use Zend\Diactoros\Response;
use Zend\Diactoros\Stream;
final class StringResponse
{
/**
* Do not allow instantiation
*/
private function __construct()
{
}
public static function html($html, $status = 200, array $headers = [])
{
return self::createResponse($html, 'text/html', $status, $headers);
}
public static function json($jsonOrData, $status = 200, array $headers = [])
{
if (! is_string($jsonOrData)) {
$jsonOrData = json_encode($jsonOrData, JSON_UNESCAPED_SLASHES);
}
return self::createResponse($jsonOrData, 'application/json', $status, $headers);
}
private static function createResponse($text, $type, $status, $headers)
{
$hasContentType = array_reduce(array_keys($headers), function ($carry, $item) {
return $carry ?: (strtolower($item) === 'content-type');
}, false);
if (! $hasContentType) {
$headers['content-type'] = [$type];
}
$body = new Stream();
$body->write($text);
return new Response($body, $status, $headers);
}
} Usage would be: use Zend\Diactoros\Response\StringResponse;
// HTML
$response = StringResponse::html($html);
$response = StringResponse::html($html, 200, ['content-type' => ['application/xhtml+xml']]);
// JSON
$response = StringResponse::json($json);
$response = StringResponse::json($objectToRepresentAsJson); Thoughts? |
I like where this is going. I'll have a go at this later tonight. |
@weierophinney Okay, I integrated your proposal. We have a |
public static function json($data, $status = 200, array $headers = []) | ||
{ | ||
if ($data === null) { | ||
$data = new \ArrayObject(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArrayObject
can be moved to use statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why do you want to encode an empty ArrayObject
instead of null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samsonasik Will fix in a second.
@slavcodev Because there's no null
in JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@franzliedke what do you mean?
JSON Schema primitive types: boolean, string, number, null, array, object
anyway, perhaps you may write
json_encode([], JSON_FORCE_OBJECT)
and not to create a temporary instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From http://php.net/json_encode:
RFC 4627 only supports these values when they are nested inside an array or an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@franzliedke Good to known. Thanks. 👍
|
||
private function __construct() | ||
{ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is for what ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It ensures that the factory class can not be instantiated directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To enforce that the factory class is non-instatiable.
On Jun 12, 2015 6:40 AM, "Abdul Malik Ikhsan" notifications@github.com
wrote:
In src/Response/StringResponse.php
#52 (comment)
:
\* @return Response
*/
- public static function json($data, $status = 200, array $headers = [])
- {
if ($data === null) {
$data = new ArrayObject();
}
$json = json_encode($data, JSON_UNESCAPED_SLASHES);
return static::createResponse($json, $status, $headers, 'application/json');
- }
- private function __construct()
- {
- }
this is for what ?
—
Reply to this email directly or view it on GitHub
https://github.com/zendframework/zend-diactoros/pull/52/files#r32308786.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, needs doc comment ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add comment then like :
/**
* This class is non-instantiable,
* You can use Zend\Diactoros\Response\StringResponse::staticMethod() directly
*/
Whoops, sorry. |
Shortcut method for creating response from a string
- Added method docblocks where missing. - Added behavior to `json()` method: cast scalars to array prior to attempts to serialize.
Merged to develop for release with 1.1. Thanks, @franzliedke ! |
Creating a response object from a string of HTML (etc.) seems to be a very common use-case, so we might as well make it very easy (without having to handle streams).