Skip to content
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

Response::withFile() and Response::withFileDownload() #91

Merged
merged 16 commits into from
May 24, 2019

Conversation

l0gicgate
Copy link
Member

Resuming to adopt the proposed direction in #88

New Response Methods:

  • Response::withFile($file, $contentType)
  • Response::withFileDownload($file, $name, $contentType)

Behavior
The $file parameter can be a string which points to a file, an existing resource handle or a StreamInterface as proposed by @roxblnfk

The $name parameter overrides the default attachment value for the Content-Disposition header. The given $name is filtered through urlencode() to ensure it is a valid header value.

The $contentType parameter accepts a string to override the Content-Type header to a user defined value. Defaults to true which attempts to detect the mime type via mime_content_type() and falls back to application/octet-stream otherwise. If set to false the Content-Type header is not appended at all.

Closes #82

@l0gicgate l0gicgate added this to the 0.8 milestone May 22, 2019
@coveralls
Copy link

coveralls commented May 22, 2019

Coverage Status

Coverage increased (+0.07%) to 99.349% when pulling b187cc0 on l0gicgate:RespondWithFile into 36c06df on slimphp:master.

@adriansuter
Copy link
Contributor

In Firefox 67.0 (64-Bit) the file name in the "Save as" dialog would be wrong if we use urlencode() for a file name like sale$.json. The presented file name is sale%24.json. In Chrome 74.0.3729.169 (64-Bit), Opera 60.0.3255.95 and MS Edge 42.17134.1.0 the url encode would be decoded. Not sure what the rfc says about that.

If instead we write

            $disposition .= '; filename*=UTF-8\'\'' . urlencode($fileName);

then all the browsers mentioned above display the correct file name. See http://test.greenbytes.de/tech/tc2231/#encoding-2231-char for more information.

Note that this works only, if the $fileName really is in UTF-8.

@adriansuter
Copy link
Contributor

If we pass a StreamInterface as $file but no $name to \Slim\Http\Response::withFileDownload, then Firefox presents a "Save as" dialog with no file name at all. If then saved, the file would be saved with a random file name like uM3wqZYH.

public function __invoke(ServerRequest $request, Response $response, array $args = []): Response
    $stream = $psr17Factory->createStreamFromFile('data.json');
    return $response->withFileDownload($stream);
}

Chrome and Opera defaults to the filename Download, Edge could not save the file at all - it tried to save a file named localhost/.

Probably we should raise an exception if a user tries to do that? Or is that the responsibility of a user?

src/Response.php Show resolved Hide resolved
src/Response.php Show resolved Hide resolved
src/Response.php Outdated Show resolved Hide resolved
@roxblnfk
Copy link

Probably we should raise an exception if a user tries to do that? Or is that the responsibility of a user?

The filename is always optional

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition

@adriansuter
Copy link
Contributor

The filename is always optional
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition

Of course, if the url contains a path from which the user agent can parse the filename, then this is okay. But obviously, if the download would be triggered by calling https://domain.com/ directly ("no path part"), then the user agents would have no information regarding the file name (Edge fails in this case completely).

By the way, the same holds for any paths ending with a slash (of course that should be avoided). So if we define a route

$app->get('/hello/', function (\Slim\Http\ServerRequest $request, \Slim\Http\Response $response, array $args) {
    $psr17Factory = new Psr17Factory();
    $stream = $psr17Factory->createStream('1234');

    return $response->withFileDownload($stream);
});

then a request to http://localhost/hello/ would start a file download with an empty file name (in Firefox). At least, in Edge the file name would be hello. In Opera and Chrome, the file name would be Download.

So I guess this is a problem that the user agents should solve for themselves. It is not our duty to prevent that, right?

@roxblnfk
Copy link

So I guess this is a problem that the user agents should solve for themselves. It is not our duty to prevent that, right?

I think this is not our problem. But we can also set default filename to "Download" (don't throw error)

@l0gicgate
Copy link
Member Author

l0gicgate commented May 22, 2019

Probably we should raise an exception if a user tries to do that? Or is that the responsibility of a user?

Let's not throw an error but append a default filename as suggested like "download" or "attachment" when a StreamInterface is passed but no name is specified.

@adriansuter
Copy link
Contributor

Let's not throw an error but append a default filename as suggested like "download" or "attachment" when a StreamInterface is passed but no name is specified.

Would it make sense to try to get the file name from the metadata uri of the stream?

@l0gicgate
Copy link
Member Author

l0gicgate commented May 22, 2019

Would it make sense to try to get the file name from the metadata uri of the stream?

Yea for sure why not.

@l0gicgate l0gicgate closed this May 24, 2019
@l0gicgate l0gicgate deleted the RespondWithFile branch May 24, 2019 00:07
@l0gicgate l0gicgate restored the RespondWithFile branch May 24, 2019 00:07
@l0gicgate l0gicgate reopened this May 24, 2019
@l0gicgate l0gicgate merged commit 1c96f39 into slimphp:master May 24, 2019
@l0gicgate l0gicgate changed the title [WIP] Response::withFile() and Response::withFileDownload() Response::withFile() and Response::withFileDownload() May 24, 2019
@l0gicgate l0gicgate deleted the RespondWithFile branch May 24, 2019 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add helper to simplify streaming a file download
4 participants