NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[FrameworkBundle] Add file helper to Controller#18502

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

dfridrich wants to merge24 commits intosymfony:masterfromdfridrich:file_helper

Closed

[FrameworkBundle] Add file helper to Controller#18502

dfridrich wants to merge24 commits intosymfony:masterfromdfridrich:file_helper

Conversation

Copy link

Contributor

dfridrich commentedApr 10, 2016•
edited
Loading

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 (likejson() helper does).

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

passSymfony\Component\HttpFoundation\File (orSymfony\Component\HttpFoundation\UploadedFile) instance
[REMOVED] provide content asstring 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');

[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

Copy link

Member

javiereguiluz commentedApr 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!

javiereguiluz added Feature FrameworkBundle labels

Apr 10, 2016

[FrameworkBundle] Fix coding standard in file helper tests

2da9399

Copy link

ContributorAuthor

dfridrich commentedApr 10, 2016

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

Nicofuma reviewed

Apr 11, 2016

View reviewed changes

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

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

Copy link

Contributor

NicofumaApr 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

you could use the MimeTypeGuesser here

Copy link

ContributorAuthor

dfridrichApr 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

dfridrich added3 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 helpersymfony/symfony-docs#6454

Closed

Copy link

ContributorAuthor

dfridrich commentedMay 2, 2016•
edited
Loading

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

HeahDude reviewed

May 2, 2016

View reviewed changes

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

		/**
		* 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

Copy link

Contributor

HeahDudeMay 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should be@param string|null

Copy link

ContributorAuthor

dfridrichMay 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@HeahDude you are right, thx

Copy link

Contributor

HeahDude commentedMay 2, 2016

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

dfridrich added2 commits

May 2, 2016 11:24

[FrameworkBundle] Enhance file helper code

1a58eb9

[FrameworkBundle] Enhance file helper code (fix line)

b647e70

Copy link

ContributorAuthor

dfridrich commentedMay 2, 2016

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

HeahDude reviewed

May 2, 2016

View reviewed changes

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

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

Copy link

Contributor

HeahDudeMay 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 aFile 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 notstring orFile at the very beginning of the method.

Copy link

Contributor

jvasseur commentedMay 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.

Copy link

Member

xabbuh commentedMay 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.

Remove file helper throws annotation

3823cfa

fabpot reviewed

Jun 15, 2016

View reviewed changes

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

		$mimeType =$file->getMimeType();

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

Copy link

Member

fabpotJun 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It would be better to use thesetContentDisposition() ofBinaryFileResponse:

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

Add mime type to BinaryFileResponse contructor in file helper

bc040ac

fabpot reviewed

Jun 15, 2016

View reviewed changes

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

		if (!$fileinstanceof File) {
		$file =newFile($file);
		}

Copy link

Member

fabpotJun 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

dfridrich added3 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

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

		}

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

Copy link

Member

fabpotJun 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This line should be removed.

Copy link

Member

fabpot commentedJun 15, 2016

The method should read as follows:

protectedfunctionfile($file,$fileName =null,$disposition = ResponseHeaderBag::DISPOSITION_ATTACHMENT)    {$response =newBinaryFileResponse($file);$response->setContentDisposition($disposition,null ===$fileName ?$file->getFileName() :$fileName);return$response;    }

Use setContentDisposition method in file helper

3757cb9

Copy link

Contributor

jvasseur commentedJun 15, 2016

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

Maybe something like this:

protectedfunctionfile($file,$fileName ='',$disposition = ResponseHeaderBag::DISPOSITION_ATTACHMENT)    {$response =newBinaryFileResponse($file);$response->setContentDisposition($disposition,$fileName);return$response;    }

dfridrich added2 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

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

		$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'));

Copy link

Member

GromNaNJun 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The functionbasename is simpler thanpathinfofor such thing.

Use basename function instead of pathinfo

85dcb96

Copy link

Member

fabpot commentedJun 15, 2016

Thank you@dfridrich.

fabpot closed this in1e263c0

Jun 15, 2016

apfelbox reviewed

Jun 20, 2016

View reviewed changes

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


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

Copy link

Contributor

apfelboxJun 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

xabbuhJun 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

good catch, see#19115

xabbuh mentioned this pull request

Jun 20, 2016

[FrameworkBundle] fix argument type docblock#19115

Merged

fabpot added a commit that referenced this pull request

Jun 20, 2016

minor#19115[FrameworkBundle] fix argument type docblock (xabbuh)

84d31fa

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