Add DownloadResponse Class

[settermjd]

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.

[michalbundyra]

Hi @settermjd !

I am not maintainer so I can't approve this PR, but please see my comments. As I said I don't know if this feature is gonna be accepted.

I would consider passing the array to CsvResponse and build the body inside from the array. Similar we have with JSON response - we are passing array and json is build inside.

[michalbundyra]

wouldn't be better to accept also/only array with data and process it here?

[settermjd]

Honestly, hadn't considered that. Thanks.

[michalbundyra]

we can have RTH here

[settermjd]

Sorry, what's RTH?

[michalbundyra]

Return Type Hint declaration:

public function invalidHeadersWhenDownloading() : array
                                                ^^^^^^^^

[settermjd]

My bad.

[weierophinney]

I like the idea of this a lot. However, I would argue we need two things here:

• A general DownloadResponse for specifying file downloads. This could build on your provided DownloadResponseTrait. This should allow for either specifying string content as the download OR a filename; the latter is useful, as it can prevent the need for having the full file contents in memory at once.
• Three different constructors for the CsvResponse:
  • One which accepts a CSV-formatted string to use.
  • One which accepts a CSV file to use.
  • One which accepts an array of arrays to use, as well as (optionally) a delimiter character and enclosure character. This would build the CSV string.

You can do multiple constructors by using static constuctor methods:

• CsvResponse::fromString()
• CsvResponse::fromFile()
• CsvResponse::fromArray() (or fromTraversable())

Finally, this could be interesting either as part of Diactoros, or as a stand-alone package that uses PSR-17 to create the initial stream and response instances. If you want to push it here, we can definitely maintain it, though.

[settermjd]

Sorry that I let this slip. I'll get it finalised this week, all being well.

[settermjd]

I like the idea of this a lot. However, I would argue we need two things here:

* A general `DownloadResponse` for specifying file downloads. This could build on your provided `DownloadResponseTrait`. This should allow for either specifying string content as the download OR a filename; the latter is useful, as it can prevent the need for having the full file contents in memory at once.

* Three different constructors for the `CsvResponse`:
  
  * One which accepts a CSV-formatted string to use.
  * One which accepts a CSV file to use.
  * One which accepts an array of arrays to use, as well as (optionally) a delimiter character and enclosure character. This would build the CSV string.

You can do multiple constructors by using static constuctor methods:

* `CsvResponse::fromString()`

* `CsvResponse::fromFile()`

* `CsvResponse::fromArray()` (or `fromTraversable()`)

Finally, this could be interesting either as part of Diactoros, or as a stand-alone package that uses PSR-17 to create the initial stream and response instances. If you want to push it here, we can definitely maintain it, though.

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

[settermjd]

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.

[weierophinney]

I think they're going to take a bit of time to create and test thoroughly enough to feel comfortable with.

No worries - we're pretty busy finalizing the Laminas migration tooling as it is. 😄

[settermjd]

I think they're going to take a bit of time to create and test thoroughly enough to feel comfortable with.

No worries - we're pretty busy finalizing the Laminas migration tooling as it is. smile

I can only imagine.

[michalbundyra]

@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 custom-responses.md file. Thanks !

EDIT: Is it not WIP anymore, right? If so, please update PR subject.

[settermjd]

@michalbundyra, sorry for disappearing. I'm working on it this week until it's completed.

[Xerkus]

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. if ($response instanceof CsvResponse) { doLogicHere(); } appear as a valid and very appealing choice but it is a trap with implicit issues.

• response that is not CsvResponse instance can still be csv.
• CsvResponse can get modified and no longer contain csv data despite instance type.
• Code looking for specific instance introduces hidden dependency that increases complexity and reduces reliability from long term maintenance standpoint.
• Long living code gets messy over time, little details are forgotten and incorrect assumptions made at a glance that lead to subtle inconsistencies. "We get CsvResponse instances, therefore all csv responses will be of that type" and it can be true until third party middleware is used. So, see points above.

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

[settermjd]

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 preparePartialFromFile. That looks like a great idea.

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.

[weierophinney]

This repository has been closed and moved to laminas/laminas-diactoros; a new issue has been opened at laminas/laminas-diactoros#6.

[weierophinney]

This repository has been moved to laminas/laminas-diactoros. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

• Squash all commits in your branch (git rebase -i origin/{branch})
• Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
• Run the laminas/laminas-migration tool on the code.
• Clone laminas/laminas-diactoros to another directory.
• Copy the files from the second bullet point to the clone of laminas/laminas-diactoros.
• In your clone of laminas/laminas-diactoros, commit the files, push to your fork, and open the new PR.
  We will be providing tooling via laminas/laminas-migration soon to help automate the process.