-
-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DownloadResponse Class #6
Comments
Sorry that I let this slip. I'll get it finalised this week, all being well. Originally posted by @settermjd at zendframework/zend-diactoros#361 (comment) |
Hey @weierophinney, I've been having a think about this one. As I see it at the moment, keeping it part of Diactoros, to me, makes the most sense. I'll aim to get it implemented this week. Matthew Originally posted by @settermjd at zendframework/zend-diactoros#361 (comment) |
I've made a start on working the three methods, however, I think they're going to take a bit of time to create and test thoroughly enough to feel comfortable with. Originally posted by @settermjd at zendframework/zend-diactoros#361 (comment) |
No worries - we're pretty busy finalizing the Laminas migration tooling as it is. 😄 Originally posted by @weierophinney at zendframework/zend-diactoros#361 (comment) |
I can only imagine. Originally posted by @settermjd at zendframework/zend-diactoros#361 (comment) |
@settermjd Would you be able to resolve conflicts on this PR, please? Not sure what exactly happened but github is not showing proper diff on EDIT: Is it not WIP anymore, right? If so, please update PR subject. Originally posted by @michalbundyra at zendframework/zend-diactoros#361 (comment) |
@michalbundyra, sorry for disappearing. I'm working on it this week until it's completed. Originally posted by @settermjd at zendframework/zend-diactoros#361 (comment) |
I would like to reiterate here for the record my objections I voiced before in chat for approach introducing specialized response types as opposed to straight up factories. Functionality introduced here is a factory method pattern. I think what it tries to achieve here violates single responsibility principle in that psr message represents complex http response structure and Csv and Download responses here are building response. It does not introduce any new or changed behavior and as such does not call for an inheritance. Look at CsvResponse constructor from PR: public function __construct($text, int $status = 200, string $filename = '', array $headers = [])
{
if (is_string($filename) && $filename !== '') {
$headers = $this->prepareDownloadHeaders($filename, $headers);
}
parent::__construct(
$this->createBody($text),
$status,
$this->injectContentType('text/csv; charset=utf-8', $headers)
);
} Same code as factory method using no inheritance: class DownloadResponseFactory
{
public function prepareResponse($text, int $status = 200, string $filename = '', array $headers = []) : Response
{
if (is_string($filename) && $filename !== '') {
$headers = $this->prepareDownloadHeaders($filename, $headers);
}
$headers = $this->injectContentType('text/csv; charset=utf-8', $headers);
return new Responsex(
$this->createBody($text),
$status,
$headers
);
}
} Same functionality, same code, single purpose. No inherited behavior adding inherited complexity cost. Simpler. Makes it easier to test with high confidence. High confidence in general means better long term maintainability. As added bonus, if extracted as a factory/builder it gives extra flexibility: Convenience factory to create from file: class DownloadResponseFactory
{
public function prepareFromFile(string $file, int $status = 200, array $headers = []) : Response;
} May be partial downloads? class DownloadResponseFactory
{
public function preparePartialFromFile(int $rangeStart, int $rangeEnd, string $file, ...) : Response;
} Create response object from any provider: class CsvResponseFactory
{
public function __construct(PsrResponseFactory $responseFactory, PsrStreamFactory $streamFactory);
} Now, from library/framework standpoint: introduction of subclass entices users to make rigged design decisions.
What said above comes from the point of view of someone with heavy focus on long term maintainability and defensive programming practices. It is not an absolute and there are other aspects to be considered. This is my opinion as a developer, not as a ZF Community Review Team member Originally posted by @Xerkus at zendframework/zend-diactoros#361 (comment) |
Hey @Xerkus, to be honest, in the chat, I didn't fully appreciate what you were trying to say. However, after reading through such detailed feedback, what you're saying makes a lot of sense to me and, to be honest, I'm thinking that your approach is the correct one. What most stands out for me is the That said, the PR is still a WIP. The CSVResponse class doesn't reflect the presence of the DownloadResponse class. I should have removed it so that it didn't cause anyone any confusion. Originally posted by @settermjd at zendframework/zend-diactoros#361 (comment) |
I am going to say no to this approach. With the introduction of http-factory this should definitely be done as a convenience factory or builder configuring new generic response object that might not be from diactoros. This is more suitable for a separate package that does not depend on laminas-diactoros at all. |
Why is the new feature needed? What purpose does it serve?
After doing a bit of searching, I didn't find a class that would send a CSV response. There were the Text, JSON, HTML, Redirect, XML, and Empty response classes, but nothing specific to CSV. So I created this PR to add a CSV response class, which can send both plain CSV text as well as a response that will be interpreted by the client as a downloadable file.
How will users use the new feature?
Users can use the CSV response class very similarly to how they use the existing response classes. The only difference is that if they supply a file name as the third parameter to the constructor, then a download response will be sent, not a textual response.
Originally posted by @settermjd at zendframework/zend-diactoros#361
The text was updated successfully, but these errors were encountered: