Splitting StringResponse into many smaller classes #61
Conversation
|
I was thinking the same when we created the |
Per discussion started on zendframework#61, this pull request removes the `StringResponse` factory class in favor of two dedicated response types, one for each of HTML and JSON responses. In order to achieve that goal, it introduces the `InjectContentTypeTrait`, which will search for a Content-Type header in the provided headers, and, if none is found, inject the provided Content-Type. This also allowed extracting the logic for encoding data to JSON to another method.
|
@moufmouf I've already completed it, if you want to review. |
|
Ping @franzliedke - this is a proposed rewrite of the factories, for the reasons outlined in the issues description. |
|
Ho, damn, I was working on the same thing (in #62). I definitely like the I'll add comments directly in the commits. |
|
Awesome. 👍 |
| $data = (array) $data; | ||
| } | ||
|
|
||
| return json_encode($data, JSON_UNESCAPED_SLASHES); |
There was a problem hiding this comment.
Symfony is using by default the JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT options.
It seems it makes RFC4627-compliant JSON, which may also be embedded into HTML safely.
Also, in my implementation, I added a fourth parameter (https://github.com/moufmouf/zend-diactoros/blob/develop/src/Response/JsonResponse.php#L36) to allow passing any JSON encoding options. It might be helpful to some people I think.
|
@weierophinney PR is coming on your branch :) |
|
This is a BC break because StringResponse class is removed |
|
@Maks3w But that class was never part of any public release, was it? |
|
@Maks3w not a BC break, as this has only been on the develop branch, and not in a release so far. |
Adding error handling via exceptions and JSON options parameter
- Per @moufmouf on zendframework#62, this patch adds some basic error handling to the `HtmlResponse` to ensure we have a either a string, or a StreamInterface instance, raising an InvalidArgumentException if not.
|
@moufmouf Merged your pull request, and added error handling to the |
- PHP 5.4 is a bit more noisy about the inability to encode. Detect it early when possible.
- As there's no way to make that condition fail on that platform.
|
Okay, finally tests and coverage are looking good. :) |
Handle method return HtmlResponse instead of StringResponse which does not exist anymore in Zend/Diactoros : zendframework/zend-diactoros#61
I would like to suggest splitting the new
StringResponseinto 2 smaller classes:JsonResponseandHtmlResponse.Here is the rationale:
StringResponseis actually aStringResponseFactory. It is final, so it cannot be extended, and it provides only 2 static methods:htmlandjson. However, there are a bunch of other perfectly viable responses that are also "strings". For instance, why is there noxmlmethod? As time goes by, it is likely that this class will start growing and growing up to the point where it is not maintainable anymore.Also, yesterday, I tried to implement my own response. I ended up copy-pasting the
createResponsemethod in my own class. I think this method is very valuable and should be exposed.Would you be ok if I submit a pull request splitting
StringResponselike this:Both
JsonResponseandHtmlResponsemay extend an abstractStringResponsethat provides a protectedcreateResponsemethod.Does it make sense? Shall I work on a pull-request about this?