[HttpFoundation] Fix memory limit problems in BinaryFileResponse

[glady]

see #48692

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #48692
License	MIT
Doc PR	-

[carsonbot]

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 6.3 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[nicolas-grekas]

Thanks for the PR. Can you please add a test case?

[glady]

I tried to have a look on a reproduction and a test case. I can add a test, that chunks are used with my changes, but I failed in reproducing hitting memory_limit.

I analyzed, why it happened in our application and found that there were two levels of output buffering. Closing both/all (instead of just one) did work well for us.

So my findings are:

• without output_buffering active (ob_get_level() === 0), the download works with -1 (as it is implemented)
• with one level of output_buffering active (ob_get_level() === 1), the download fails with -1 and works when I do stream_copy_to_stream in chunks (this PR), but I not fully understand why.
• with two levels of output_buffering, my patch does not work too.

So my PR is not valid for all cases, as developer I would have expected either closing all output buffers internally via

            while (ob_get_level()) {
                ob_end_clean();
            }

or just throw an exception, if there is any output buffer active. For me it would help to see this dependency somehow, when using BinaryFileResponse. Its just not visible, that the file fully is loaded to memory instead of sent until you hit a file that does not fit in your memory limit.

[glady]

I thought about this some days now. Closing all output-buffers (via while) is possible in our specific case, so we need not to wait for this PR, but indeed especially for end-to-end functional tests it would be nice to be able to keep at least one buffer open in order to be able to check the content of the binary file response. In the test case, it will not exceed the memory limit. To change the code from -1 to $filesize would support this. In this case we would be able to close our one "known" buffer that catches all for some other reasons, but still would allow to put any code around it with separate handling like a test that checks a buffer afterwards.

I will check, if I can add testcases in order to demonstrate this.

[nicolas-grekas]

Thank you @glady.