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

[FrameworkBundle] Add file helper to Controller #18502

Closed
wants to merge 24 commits into
base: master
from

Conversation

Projects
None yet
@dfridrich
Contributor

dfridrich commented Apr 10, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#6454

I think it would be more "sexy" to serve files from controller easier (like json() helper does).

This Controller helper allows user to serve files to Response in these ways:

  • pass Symfony\Component\HttpFoundation\File (or Symfony\Component\HttpFoundation\UploadedFile) instance
  • [REMOVED] provide content as string and specify file name (mime type will be auto recognized)
  • provide path to file (you are still able to specify other than original file name)

Examples

return $this->file($uploadedFile);
// ...or...
return $this->file('/path/to/my/picture.jpg');
@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Apr 10, 2016

Member

@dfridrich thanks for sending this contribution to improve Symfony (and its associated docs too).

Just a comment for your next contributions: it's always better to open an issue to propose new features without sending the PR to implement them. That allows the community to review your idea and propose changes to it. Besides, if the idea is ultimately rejected, creating a PR will make you waste your time whereas creating an issue is quick and easy. Thanks!

Member

javiereguiluz commented Apr 10, 2016

@dfridrich thanks for sending this contribution to improve Symfony (and its associated docs too).

Just a comment for your next contributions: it's always better to open an issue to propose new features without sending the PR to implement them. That allows the community to review your idea and propose changes to it. Besides, if the idea is ultimately rejected, creating a PR will make you waste your time whereas creating an issue is quick and easy. Thanks!

@dfridrich

This comment has been minimized.

Show comment
Hide comment
@dfridrich

dfridrich Apr 10, 2016

Contributor

@javiereguiluz thanks for information, I'll remember next time.

Contributor

dfridrich commented Apr 10, 2016

@javiereguiluz thanks for information, I'll remember next time.

@dfridrich

This comment has been minimized.

Show comment
Hide comment
@dfridrich

dfridrich May 2, 2016

Contributor

I think this my PR is really very useful, what do you think @symfony/deciders?

Contributor

dfridrich commented May 2, 2016

I think this my PR is really very useful, what do you think @symfony/deciders?

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude May 2, 2016

Member

Besides my minor comments, I think this is a nice addition. Thanks :)

Member

HeahDude commented May 2, 2016

Besides my minor comments, I think this is a nice addition. Thanks :)

@dfridrich

This comment has been minimized.

Show comment
Hide comment
@dfridrich

dfridrich May 2, 2016

Contributor

@HeahDude Thanks for your feedback, I really appreciate it! I just commited fixed version of this great feature 👻

Contributor

dfridrich commented May 2, 2016

@HeahDude Thanks for your feedback, I really appreciate it! I just commited fixed version of this great feature 👻

@jvasseur

This comment has been minimized.

Show comment
Hide comment
@jvasseur

jvasseur May 2, 2016

Contributor

I don't think allowing to pass a string containing the file contents is a good idea. If you already have the content of the response, why storing it in a temporary file to then read back from it ?

Plus this means passing a string as a first argument can mean two totally different things, which means if for example you make a typo in the name of the file you want to send you will get this name as a file instead of an error.

Contributor

jvasseur commented May 2, 2016

I don't think allowing to pass a string containing the file contents is a good idea. If you already have the content of the response, why storing it in a temporary file to then read back from it ?

Plus this means passing a string as a first argument can mean two totally different things, which means if for example you make a typo in the name of the file you want to send you will get this name as a file instead of an error.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh May 2, 2016

Member

I agree with @jvasseur. We had something similar in the YAML parser in the past which was not a good idea (we cleared things in 3.0). We should simply not support passing file contents here.

Member

xabbuh commented May 2, 2016

I agree with @jvasseur. We had something similar in the YAML parser in the past which was not a good idea (we cleared things in 3.0). We should simply not support passing file contents here.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 15, 2016

Member

The method should read as follows:

    protected function file($file, $fileName = null, $disposition = ResponseHeaderBag::DISPOSITION_ATTACHMENT)
    {
        $response = new BinaryFileResponse($file);
        $response->setContentDisposition($disposition, null === $fileName ? $file->getFileName() : $fileName);

        return $response;
    }
Member

fabpot commented Jun 15, 2016

The method should read as follows:

    protected function file($file, $fileName = null, $disposition = ResponseHeaderBag::DISPOSITION_ATTACHMENT)
    {
        $response = new BinaryFileResponse($file);
        $response->setContentDisposition($disposition, null === $fileName ? $file->getFileName() : $fileName);

        return $response;
    }
@jvasseur

This comment has been minimized.

Show comment
Hide comment
@jvasseur

jvasseur Jun 15, 2016

Contributor

@fabpot your version doesn't work if $file is a string and $fileName is null.

Maybe something like this:

    protected function file($file, $fileName = '', $disposition = ResponseHeaderBag::DISPOSITION_ATTACHMENT)
    {
        $response = new BinaryFileResponse($file);
        $response->setContentDisposition($disposition, $fileName);

        return $response;
    }
Contributor

jvasseur commented Jun 15, 2016

@fabpot your version doesn't work if $file is a string and $fileName is null.

Maybe something like this:

    protected function file($file, $fileName = '', $disposition = ResponseHeaderBag::DISPOSITION_ATTACHMENT)
    {
        $response = new BinaryFileResponse($file);
        $response->setContentDisposition($disposition, $fileName);

        return $response;
    }
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 15, 2016

Member

Thank you @dfridrich.

Member

fabpot commented Jun 15, 2016

Thank you @dfridrich.

@fabpot fabpot closed this in 1e263c0 Jun 15, 2016

@@ -124,6 +127,23 @@ protected function json($data, $status = 200, $headers = array(), $context = arr
}
/**
* Returns a BinaryFileResponse object with original or customized file name and disposition header.
*
* @param File|string $file File object or path to file to be sent as response

This comment has been minimized.

@apfelbox

apfelbox Jun 20, 2016

Contributor

Shouldn't this be \SplFileInfo|string to be equal to BinaryFileResponse?

@apfelbox

apfelbox Jun 20, 2016

Contributor

Shouldn't this be \SplFileInfo|string to be equal to BinaryFileResponse?

This comment has been minimized.

@xabbuh

xabbuh Jun 20, 2016

Member

good catch, see #19115

@xabbuh

xabbuh Jun 20, 2016

Member

good catch, see #19115

fabpot added a commit that referenced this pull request Jun 20, 2016

minor #19115 [FrameworkBundle] fix argument type docblock (xabbuh)
This PR was merged into the 3.2-dev branch.

Discussion
----------

[FrameworkBundle] fix argument type docblock

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #18502 (comment)
| License       | MIT
| Doc PR        |

Commits
-------

c4e440c [FrameworkBundle] fix argument type docblock
*
* @return BinaryFileResponse
*/
protected function file($file, $fileName = null, $disposition = ResponseHeaderBag::DISPOSITION_ATTACHMENT)

This comment has been minimized.

@staabm

staabm Jun 29, 2016

Contributor

doesnt this new method manifest as a BC break when the subclassing controller already contains a file method?

@staabm

staabm Jun 29, 2016

Contributor

doesnt this new method manifest as a BC break when the subclassing controller already contains a file method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment