Skip to content

[HttpFoundation] BinaryFileResponse untestable? #10608

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.

Sign up for GitHub

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

Jump to bottom
Closed
anlutro opened this issue Apr 1, 2014 · 3 comments
Closed

[HttpFoundation] BinaryFileResponse untestable? #10608

anlutro opened this issue Apr 1, 2014 · 3 comments
Labels
HttpFoundation

Comments

@anlutro
Copy link
Contributor

anlutro commented Apr 1, 2014

BinaryFileResponse calls $file = new File((string) $file); on the first parameter of its constructor, which means that it can't easily be mocked. In a web test using the BrowserKit client, you have to provide the path to a file that actually exists for it to not throw an exception. This seems less ideal for a controller action that, for example, writes a file to the disk before letting the user download it. In some cases you will want to mock away that file system read/write operation.

I had a quick search on google and looked through the issues but couldn't find anything about this. Am I missing something here or is this a problem?

https://github.com/symfony/symfony/blob/v2.4.4/src/Symfony/Component/HttpFoundation/BinaryFileResponse.php#L84
https://github.com/symfony/symfony/blob/v2.4.4/src/Symfony/Component/HttpFoundation/File/File.php#L40-L42
@anlutro
Copy link
Contributor Author

anlutro commented Apr 28, 2014

Could a solution to this be to not call new File(..) in BinaryFileResponse if $file is already an instance of File? That way we can define our mocked services to return a mocked or real File object and the BinaryFileResponse will work fine.

@jakzal
Copy link
Contributor

jakzal commented Apr 28, 2014

The BinaryFileResponse already accepts either string or SplFileInfo, there's no typehint, so supporting File would be relatively easy.

Have you tried using vfsStream to mock the filesystem?

@anlutro
Copy link
Contributor Author

anlutro commented Apr 28, 2014

I have not, though by the looks of it it should be able to manage it. However, in my case there are only a few file download routes in the application, so bringing in something that heavy seems like overkill. At that point I'd rather just create a file in a fixtures directory, but I think being able to mock it entirely would be useful.

@anlutro anlutro mentioned this issue Apr 28, 2014
Merged
fabpot added a commit that referenced this issue Apr 28, 2014
@fabpot
bug #10797 [HttpFoundation] Allow File instance to be passed to Binar…
9e2a834 
…yFileResponse (anlutro)

This PR was merged into the 2.3 branch.

Discussion
----------

[HttpFoundation] Allow File instance to be passed to BinaryFileResponse

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #10608
| License       | MIT
| Doc PR        |

See #10608 for discussion and reasoning.

The docblocks get rather long with this change. The File class does extend SplFileObject so technically it could be left out of the docblock if that is more desirable.

Commits
-------

fc04ad2 Allow File instance to be passed to BinaryFileResponse
@fabpot fabpot closed this as completed Apr 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
HttpFoundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@fabpot @anlutro @jakzal