Coverage decreased (-7.1%) to 92.145% when pulling 9b8d9fc on adriansuter:Response-File into 36c06df on slimphp:master.

I wonder if we should split it into two methods. withFileDownload and withFileStream. They're both pretty similar but I think that would be better. What do you think?

Also, I think we should take in a StreamInterface as the first parameter and an optional filename as second parameter.

What is the difference between file download and file stream? attachment vs. inline?

A StreamInterface is good. Although I am not sure whether this is user friendly. Most users would expect to pass in a string, i.e. the path to the file, don't they? I wonder, how you get the stream factory in a route callable? By default we would only have the Request, the Response and an array of arguments in the callable.

What about the other configs (ETag, Last-Modified)? Should that be left to the user? I suggest to add these as optional params (boolean) to the method(s).

Do you think that the content type(s) should automatically be detected?

What about other headers like Content-Length, Cache-Control or Expires? Should we add them in the method(s).

I have found a lot of information about the filename part of the Content-Disposition header. Apparently, only US-ASCII chars are allowed by default rfc2183, section 2.3. Should we clean that for the user?

It is possible not to give a filename at all. For example simply

Content-Disposition: attachment

Then the browser would use the last part of the path of the url. So we need to cover that possibility. I suggest, that if the filename is an empty string, then the filename would not be attached to the Content-Disposition header. If is is null, then the filename would be taken from the real file.

A lot of interesting tests could be found at http://test.greenbytes.de/tech/tc2231/. For example the use of non US-ASCII chars http://test.greenbytes.de/tech/tc2231/#attwithfn2231utf8comp.

What is the difference between file download and file stream? attachment vs. inline?

The file download would trigger en client side to download the contents while the stream would make the client read the contents, e.g. viewing a PDF versus downloading it.

What about the other configs (ETag, Last-Modified)? Should that be left to the user? I suggest to add these as optional params (boolean) to the method(s).

I do think that should be optional for sure. and yes we can add it as a boolean.

Do you think that the content type(s) should automatically be detected?

This is possible via finfo() all you would need to do is:

$finfo = new \finfo(FILEINFO_MIME_TYPE);
$contentType = $finfo->buffer($this->body->getContents());

We should probably have a fallback in case we cannot detect or an optional user parameter perhaps?

A StreamInterface is good. Although I am not sure whether this is user friendly. Most users would expect to pass in a string, i.e. the path to the file, don't they? I wonder, how you get the stream factory in a route callable? By default we would only have the Request, the Response and an array of arguments in the callable.

I think that we might need to create a FileInterface then in the implementation have static methods to create them from either a path or a stream. Then our withFileDownload() and withFileStream() methods become standardized and it provides an easy way for the user to create files without having to go through too many hoops.

Here is the FileInterface

<?php
use Psr\Http\Message\StreamInterface;
use RuntimeException;

interface FileInterface
{
    /**
     * @return string
     */
    public function getFileName(): string;

    /**
     * @return string|null
     */
    public function getPath(): ?string;

    /**
     * @return string|null
     */
    public function getContentType(): ?string;

    /**
     * @return string
     */
    public function getContents(): string;
}

Here is the implementation

<?php
use Psr\Http\Message\StreamInterface;
use RuntimeException;

class File implements FileInterface
{
    /**
     * @var string
     */
    private $fileName;

    /**
     * @var string|null
     */
    private $path;

    /**
     * @var string|null
     */
    private $contentType;

    /**
     * @var string
     */
    private $contents;

    /**
     * @param string          $fileName
     * @param string|null     $path
     * @param string|null     $contentType
     * @param string          $contents
     */
    public function __construct(
        string $fileName,
        ?string $path,
        ?string $contentType,
        string $contents
    ) {
        $this->fileName = $fileName;
        $this->path = $path;
        $this->contentType = $contentType;
        $this->contents = $contents;
    }

    /**
 * @return string
 */
    public function getFileName(): string
    {
        return $this->fileName;
    }

    /**
     * @return string|null
     */
    public function getPath(): ?string
    {
        return $this->path;
    }

    /**
     * @return string|null
     */
    public function getContentType(): ?string
    {
        $contentType = $this->contentType;

        if (empty($contentType) && $this->contents) {
            $finfo = new \finfo(FILEINFO_MIME_TYPE);
            $contentType = $finfo->buffer($this->contents);
        }

        if (empty($contentType) && $this->path) {
            $finfo = new \finfo(FILEINFO_MIME_TYPE);
            $contentType = $finfo->file($this->path);
        }

        return $contentType;
    }

    /**
     * @return string
     */
    public function getContents(): string
    {
        return $this->contents;
    }

    /**
     * @param string $path
     * @return File
     */
    public static function fromPath(string $path): File
    {
        if (!file_exists($path)) {
            throw new RuntimeException(sprintf('`%s` does not exist.', $path));
        }

        $fileName = basename($path);
        $contentType = mime_content_type($path);
        $contents = file_get_contents($path);

        return new File($fileName, $path, $contentType, $contents);
    }

    /**
     * @param StreamInterface $stream
     * @param string          $fileName
     * @return File
     */
    public static function fromStream(StreamInterface $stream, string $fileName): File
    {
        $contents = (string) $stream;

        $finfo = new \finfo(FILEINFO_MIME_TYPE);
        $contentType = $finfo->buffer($contents);

        return new File($fileName, null, $contentType, $contents);
    }
}

Proposed method signatures

• Response::withFileDownload(FileInterface $file, ...options)
• Response::withFileStream(FileInterface $file, ...options)

Let’s make sure we update the README as well to reflect the addition of the two new methods.

Currently the Content-Type header is always application/force-download, application/octet-stream, application/download. I will need to update that, such that the real mime type would be used. Also, I am not sure about the other default headers. Pragma should actually only be used in requests, not in reponses. What should we do with the cache control and the expires.

And the current behaviour is such that if the user provides headers, then the default headers would never be added. But what if the user wanted to add other headers than the ones defined in the defaults? I guess we should check, if the user provided a header key, and if not, then the default would be applied.

(tomorrow)

We should set default to not cache I suppose. Let's get rid of Pragma and use Cache-Control: no-cache. The users can override them to fit their needs.

As for Content-Type set it to the detected value of File::getContentType() and if null then default to application/force-download for the withFileDownload method and application/octet-stream in case of the withFileStream.

Now there are lots of if conditions for the headers. Probably I could also use array_merge and a defaults array?

The problem with array_merge is that we offer the possibility of extending and overwriting default headers but not overriding them entirely. I’d rather just stick with the current logic.

We are loading the whole contents of the file in \Slim\Http\File::fromPath() and \Slim\Http\File::fromStream(). Couldn't that lead to "memory exhausted"-errors in case of a big file?

The function mime_content_type() in \Slim\Http\File::fromPath() returns text/plain for a json file (on my system). Do we have to add special cases depending on the file extension?

        $extension = mb_strtolower(pathinfo($path, PATHINFO_EXTENSION));
        switch($extension) {
            case 'json':
                $contentType = 'application/json';
                break;
            default:
                $contentType = mime_content_type($path);
        }

The same holds for the finfo mime type used in \Slim\Http\File::fromStream(). Special extension cases?

        $extension = mb_strtolower(pathinfo($fileName, PATHINFO_EXTENSION));
        switch ($extension) {
            case 'json':
                $contentType = 'application/json';
                break;
            default:
                $finfo = new finfo(FILEINFO_MIME_TYPE);
                $contentType = $finfo->buffer($contents);
        }

It is possible that the file name of the file stream might be extracted from the metadata uri. That way the second param of \Slim\Http\File::fromStream() could be made optional. But it is possible, that the uri does not point to a "real file". Would it be helpful to implement some sort of detection for the file name, or is this too error prone?

For the memory exhaustion issue there’s no real way to avoid that as we would need to defer the reading of the file to the moment it’s being output and buffer read it as its being emitted. I think this can’t be handled from our end.

As for content type detection there’s too many to parse manually. Maybe we could add an optional content type parameter to the static creator methods so it can be user defined.

For the memory exhaustion issue there’s no real way to avoid that as we would need to defer the reading of the file to the moment it’s being output and buffer read it as its being emitted. I think this can’t be handled from our end.

I think that the wrong way to resolve the issue was initially chosen. As a user, i would like to use code like this:

return $response->withFileDownload('bigBackup.gz');
# or
$handler = fopen('bigBackup.gz', 'r');
return $response->withFileDownload($handler);

And i not want to create FileInterface instance. Or if i have this instance then it will not use file_get_contents on initialization

A StreamInterface is good. Although I am not sure whether this is user friendly. Most users would expect to pass in a string, i.e. the path to the file, don't they? I wonder, how you get the stream factory in a route callable? By default we would only have the Request, the Response and an array of arguments in the callable.

$this->get(\Psr\Http\Message\StreamFactoryInterface::class);

If you don't want to follow the path of Symfony/Zend and others where we just create an instance like FileResponse then i would suggest this simple code:

/**
 * Note: This method is not part of the PSR-7 standard.
 *
 * @param string|resource|StreamInterface $file
 * @param string|null                     $name
 *
 * @return static
 */
public function withFileAttach($file, ?string $name = null): ResponseInterface
{
    $disposition = 'attachment';
    $filename = $name ?? (is_string($file) ? basename($file) : '');
    # filter filename
    # TODO: test on unicode
    $filename = preg_replace(['/[\x00-\x1F\x7F\"\*<>\?|;]/', '/[\\\:\/]/'], ['', '-'], $filename);
    if (strlen($filename)) {
        $disposition .= '; filename="' . $filename . '"';
    }

    return $this->withFile($file, $name)
                ->withHeader('Content-Disposition', $disposition);
}

/**
 * Note: This method is not part of the PSR-7 standard.
 *
 * This method prepares the response object to return a file response to the
 * client without 'Content-Disposition' header which default 'inline'
 *
 * @param string|resource|StreamInterface $file
 * @param string|null                     $name
 *
 * @return ResponseInterface
 * @throws RuntimeException If the file cannot be opened.
 * @throws InvalidArgumentException If the mode is invalid.
 */
public function withFile($file = null, ?string $name = null): ResponseInterface
{
    $response = $this->response;

    if (is_resource($file)) {
        $response = $response->withBody($this->streamFactory->createStreamFromResource($file));
    } elseif (is_string($file)) {
        # thrown exception if the file is not exists/readable
        $response = $response->withBody($this->streamFactory->createStreamFromFile($file));
    } elseif ($file instanceof StreamInterface) {
        $response = $response->withBody($file);
    }

    if (!$response->hasHeader('Content-Type')) {
        # TODO: get MIME type on file extension
        $contentType = is_string($file) ? mime_content_type($file) : 'application/octet-stream';
        $response = $response->withHeader('Content-Type', $contentType);
    }

    return new static($response, $this->streamFactory);
}

usage:

return $response->withFileAttach("../data/{$filePath}", 'backup.zip');

return $response->withFile('inline.pdf');

return $response->withFile('fresh_log')
                ->withHeader('Content-Type', 'text/plain-text');
# or:
return $response->withFile($handler, 'log.txt');

@roxblnfk those are all fair points. I think that I always get caught up in type hinting method parameters for such methods when they don't theoretically need to. I like the implementation you proposed, I suppose you're right, the user probably doesn't want to have to create a file then pass it to the method.. It needs to be more user friendly.

@adriansuter I'm going to close this PR and re-open a new one with the new direction.

Closing this. Follow discussion in #91