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

FileResponse: send response either as attachment or inline #11

Merged
merged 1 commit into from Jun 2, 2014

Conversation

@honzalilak
Copy link

honzalilak commented May 31, 2014

This pull requests allows files in response being sent directly into browser instead of forcing them to be downloaded - this is especially helpful when there is a need to show e.g. pdf files directly in the browser window. See related issue nette/nette#1410.

As proposed in the contribution guide http://nette.org/en/contributing, I am adding following information:

  • feature
  • issues - #1410
  • documentation - not needed
  • BC break - no

I am really a newbie. I will appreciate if you provide me with some explaining comments in case you find this pull request as not acceptable. Thanks a lot!

@@ -33,6 +33,9 @@ class FileResponse extends Nette\Object implements Nette\Application\IResponse
/** @var bool */
public $resuming = TRUE;

/** @var bool */
private $contentDispositionAttachment = TRUE;

This comment has been minimized.

Copy link
@Majkl578

Majkl578 May 31, 2014

Contributor

What an ugly name, $isAttachment? Or maybe better $forceDownload?

@Majkl578
Copy link
Contributor

Majkl578 commented May 31, 2014

I would add it as 4th constructor parameter as well, defaulting to TRUE and remove setter.

*/
public function setContentDispositionAttachment($attachment = TRUE)
{
return $this->contentDispositionAttachment = $attachment;

This comment has been minimized.

Copy link
@Majkl578

Majkl578 May 31, 2014

Contributor

Setters in Nette usually return $this so they provide fluent behavior.

This comment has been minimized.

Copy link
@honzalilak

honzalilak May 31, 2014

Author

Good point! Thanks!

* Sends response to output.
* @return void
*/
public function send(Nette\Http\IRequest $httpRequest, Nette\Http\IResponse $httpResponse)
{
$contentDisposition = $this->contentDispositionAttachment ? 'attachment' : 'inline';

This comment has been minimized.

Copy link
@Majkl578

Majkl578 May 31, 2014

Contributor

Unnecessary variable, see below.

$httpResponse->setContentType($this->contentType);
$httpResponse->setHeader('Content-Disposition', 'attachment; filename="' . $this->name . '"');
$httpResponse->setHeader('Content-Disposition', $contentDisposition . '; filename="' . $this->name . '"');

This comment has been minimized.

Copy link
@Majkl578

Majkl578 May 31, 2014

Contributor

You can write it directly e.g. like this:

$httpResponse->setHeader('Content-Disposition', ($this->forceDownload ? 'attachment' : 'inline') . '; filename="' . $this->name . '"');

Or split it to more lines if it gets too long.

This comment has been minimized.

Copy link
@honzalilak

honzalilak May 31, 2014

Author

Yeah, I was thinking about this, but I found your solution little bit less readable. But you are right that when the line is divided into two it wont be a problem. Will rewrite it.


Assert::same( $origData, ob_get_clean() );
Assert::same('inline; filename="' . $fileName . '"', $response->getHeader('Content-Disposition'));
});

This comment has been minimized.

Copy link
@hrach

hrach Jun 1, 2014

Contributor

missing empty line at the end of file.

FileResponse: send response either as attachment or inline

FileResponse: send response either as attachment or inline

FileResponse: send response as attachment or inline
@honzalilak
Copy link
Author

honzalilak commented Jun 1, 2014

Guys thanks a lot for your feedback! I followed the contribution guide, changed the code and forced-pushed changes here. The only problem I see is the missing line at the end of the file with tests. I can see it in my local version. I will check the settings of git, maybe it is cutting them? Or do you have any ideas?

dg added a commit that referenced this pull request Jun 2, 2014
FileResponse: send response either as attachment or inline
@dg dg merged commit dcde44d into nette:master Jun 2, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.