Skip to content

Conversation

@JosephSilber
Copy link
Contributor

@JosephSilber JosephSilber commented Jan 28, 2022

Problem

When streaming a download using response()->streamDownload(...), if an exception is thrown in the middle of the stream, the HTML of the error ends up in the downloaded file.

To test it, paste this code block into your routes/web.php file:

Route::get('/', function () {
    return response()->streamDownload(function () {
        echo "Line Number, Value\n";

        collect()->times(1000)->each(function ($number) {
            echo "{$number}, Value {$number}\n";
        });

        throw new Exception('I do not want to see this in my CSV file');
    }, 'stream-error-test.csv');
});

When you visit that route, it'll download the CSV file. Open it, and you'll see the HTML of the message directly in the file:

image

This is clearly not what anyone wants. We want the error to be logged (AKA reported), but not rendered.

Solution

Wrap the provided callback in our own closure. Within that closure, we can wrap the original callback in a try/catch block, and return a custom exception that indicates that it was thrown from within a stream. Then we'll pass the wrapped closure to the StreamedResponse constructor:

public function streamDownload($callback, $name = null, array $headers = [], $disposition = 'attachment')
{
    $withWrappedException = function () use ($callback) {
        try {
            $callback();
        } catch (Throwable $e) {
            throw new StreamedResponseException($e);
        }
    };

    $response = new StreamedResponse($withWrappedException, 200, $headers);

    //...

    return $response;
}

Note about mapping inner exceptions

For logging purposes (AKA reporting) we're not interested in the special StreamedResponseException. We want to log the original exception as is.

In C# and in JS, there's an official way to wrap exceptions, and to then get back to the original exception. C# uses the InnerException property, and JS uses the cause property.

Alas, PHP does not have a built-in mechanism for this. Instead, what this PR does is add a new convention: just like we already respect render and report methods directly on the exception class, we'll now also respect a getInnerException method, and use that in the mapException logic in the Handler. This way, we can map the StreamedResponseException back to the original exception for logging purposes.

@JosephSilber JosephSilber force-pushed the swallow-stream-errors branch from 26c65bc to 954184e Compare January 28, 2022 18:21
@taylorotwell taylorotwell merged commit 306e7e4 into laravel:9.x Jan 28, 2022
@JosephSilber JosephSilber deleted the swallow-stream-errors branch January 30, 2022 14:57
taylorotwell pushed a commit that referenced this pull request Apr 13, 2022
Hey, this PR just fixes a small bug in the change not render exception inside a streamed download.  
Currently it accesses the `->message` property of a `Throwable`, which is not defined in the interface and is (in my case) protected, which results in a new error.
This PR just changes that to the defined `->getMessage()` of a `Throwable`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants