[FrameworkBundle] Add file helper to Controller #18502

dfridrich · 2016-04-10T19:49:15Z

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 · 2016-04-10T19:59:03Z

@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 · 2016-04-10T20:12:36Z

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

Nicofuma · 2016-04-11T08:18:40Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+            $response = new BinaryFileResponse($file);
+            $mimeType = $file->getMimeType();
+        } elseif (is_string($file)) {
+            if ($mimeType === null) {


you could use the MimeTypeGuesser here

@Nicofuma thx, MimeTypeGuesser is now implemented (using BinaryFileResponse class)

dfridrich · 2016-05-02T07:29:15Z

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

HeahDude · 2016-05-02T08:56:47Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+    /**
+     * Returns a BinaryFileResponse object with original or customized file name and disposition header.
+     *
+     * @param File|string $file        File object, path to file or string with content to be sent as response


Should be @param string|null

@HeahDude you are right, thx

HeahDude · 2016-05-02T09:06:40Z

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

dfridrich · 2016-05-02T09:27:15Z

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

HeahDude · 2016-05-02T09:29:48Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+        } else {
+            throw new \InvalidArgumentException(
+                sprintf(
+                    '"%s" can\'t be passed as parameter to "%s" method of "%s" class.',


I've no strong opinion about this either, it just seems to me (IIUC) that at this point $file as string has been transformed to a File object, so it means if you got something else and you cannot be sure it's a scalar.
One other way would be to throw an \InvalidArgumentException when not string or File at the very beginning of the method.

jvasseur · 2016-05-02T09:37:13Z

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 · 2016-05-02T09:59:28Z

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 · 2016-06-15T10:16:15Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+        $mimeType = $file->getMimeType();
+
+        $response->headers->set('Content-Type', $mimeType);
+        $disposition = $response->headers->makeDisposition($disposition, $fileName === null ? $file->getFileName() : $fileName);


It would be better to use the setContentDisposition() of BinaryFileResponse:

$response->setContentDisposition($disposition, $fileName === null ? $file->getFileName() : $fileName);

fabpot · 2016-06-15T10:26:42Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+        if (!$file instanceof File) {
+            $file = new File($file);
+        }
+


As I commented after your changes, the content-type logic is already done in BinaryFileResponse, so this can be removed.

fabpot · 2016-06-15T10:50:56Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+        }
+
+        $response = new BinaryFileResponse($file, 200, ['Content-type' => $file->getMimeType()]);
+        $response->setContentDisposition($disposition, $fileName === null ? $file->getFileName() : $fileName);


This line should be removed.

fabpot · 2016-06-15T10:52:04Z

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 · 2016-06-15T10:57:35Z

@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;
    }

…eader

GromNaN · 2016-06-15T11:16:16Z

src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerTest.php

+            $this->assertSame('text/x-php', $response->headers->get('content-type'));
+        }
+        $this->assertContains(ResponseHeaderBag::DISPOSITION_ATTACHMENT, $response->headers->get('content-disposition'));
+        $this->assertContains(pathinfo(__FILE__, PATHINFO_BASENAME), $response->headers->get('content-disposition'));


The function basename is simpler than pathinfofor such thing.

fabpot · 2016-06-15T15:32:03Z

Thank you @dfridrich.

apfelbox · 2016-06-20T09:27:02Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php


+    /**
+     * Returns a BinaryFileResponse object with original or customized file name and disposition header.
+     *


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

good catch, see #19115

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

staabm · 2016-06-29T13:53:22Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+     * @param string      $disposition Disposition of response ("attachment" is default, other type is "inline")
+     *
+     * @return BinaryFileResponse
+     */


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

[FrameworkBundle] Add file helper to Controller

c2e7917

carsonbot added the Status: Needs Review label Apr 10, 2016

[FrameworkBundle] Fix coding standard in file helper

fb665d2

javiereguiluz added Feature FrameworkBundle labels Apr 10, 2016

[FrameworkBundle] Fix coding standard in file helper tests

2da9399

Nicofuma reviewed Apr 11, 2016
View reviewed changes

dfridrich added 3 commits April 11, 2016 12:15

[FrameworkBundle] Add auto-detect mime type feature in file helper

ab19f5a

[FrameworkBundle] Add ability to pass path to file helper

8dde06e

[FrameworkBundle] Fix typo in file helper

9bbecbc

dfridrich mentioned this pull request Apr 11, 2016

Added docs for file helper symfony/symfony-docs#6454

Closed

HeahDude reviewed May 2, 2016
View reviewed changes

dfridrich added 2 commits May 2, 2016 11:24

[FrameworkBundle] Enhance file helper code

1a58eb9

[FrameworkBundle] Enhance file helper code (fix line)

b647e70

HeahDude reviewed May 2, 2016
View reviewed changes

Remove file helper throws annotation

3823cfa

fabpot reviewed Jun 15, 2016
View reviewed changes

Add mime type to BinaryFileResponse contructor in file helper

bc040ac

fabpot reviewed Jun 15, 2016
View reviewed changes

dfridrich added 3 commits June 15, 2016 12:44

Fix sprint and add test for non existing files

03be5a9

Remove unnecessary check in file helper

211241d

Use setContentDisposition method in file helper

05495e9

fabpot reviewed Jun 15, 2016
View reviewed changes

Use setContentDisposition method in file helper

3757cb9

dfridrich added 2 commits June 15, 2016 12:59

Simplify file helper

63c831d

Simplify file helper and remove mimetype test for null content-type h…

cc7315b

…eader

GromNaN reviewed Jun 15, 2016
View reviewed changes

Use basename function instead of pathinfo

85dcb96

fabpot closed this in 1e263c0 Jun 15, 2016

apfelbox reviewed Jun 20, 2016
View reviewed changes

xabbuh mentioned this pull request Jun 20, 2016

[FrameworkBundle] fix argument type docblock #19115

Merged

staabm reviewed Jun 29, 2016
View reviewed changes

wouterj mentioned this pull request Jul 8, 2016

Document controller file() helper symfony/symfony-docs#6732

Closed

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

TomasVotruba mentioned this pull request Jan 1, 2019

[post/talk] Random cool PHP tips TomasVotruba/tomasvotruba.com#226

Closed

5 tasks

Uh oh!

[FrameworkBundle] Add file helper to Controller #18502

[FrameworkBundle] Add file helper to Controller #18502

Uh oh!

Conversation

dfridrich commented Apr 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javiereguiluz commented Apr 10, 2016

Uh oh!

dfridrich commented Apr 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfridrich commented May 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeahDude commented May 2, 2016

Uh oh!

dfridrich commented May 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvasseur commented May 2, 2016

Uh oh!

xabbuh commented May 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jun 15, 2016

Uh oh!

jvasseur commented Jun 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jun 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

dfridrich commented Apr 10, 2016 •

edited

Loading

dfridrich commented May 2, 2016 •

edited

Loading