[13.x] Switch to Illuminate\Http\Response #1214
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Technically this will be a breaking change because people are currently returning a Symfony response if they overwrite the method in their own apps. I don't immediately see this being a big issue. Most likely things will continue to work as before and it does make sense to use the Illuminate one instead of the Symfony one. I also don't immediately know what the historical reasons are for choosing the Symfony one. Probably @taylorotwell knows. |
|
Sorry, I am not too experienced with this but how does one go about make a PR for a breaking change, i.e. I'm guessing 14.x? |
|
Yes (to master) but let's see what @taylorotwell says |
|
If it's a breaking change we can't merge it. |
|
@chasenyc honestly, I'd just leave this one be. You can always wrap it in an illuminate response if you want to. |
darynmitchell
added a commit
to darynmitchell/docs
that referenced
this pull request
Oct 23, 2022
The instructions for how to attach headers, found in "Attaching Headers To Responses", `$response->header(...)`, cause an error when called on response types returned by other code examples in this same doc page, e.g.
- `return response()->download($pathToFile);` returns a `\Symfony\Component\HttpFoundation\BinaryFileResponse`
- `return response()->file($pathToFile);` returns a `\Symfony\Component\HttpFoundation\BinaryFileResponse`
- `return response()->streamDownload(...) ` returns a `\Symfony\Component\HttpFoundation\StreamedResponse`
Attempting to add a header to such a response, in middleware, in the suggested way,
```php
$response = $next($request);
$response->header(...)
```
... results in an error "Call to undefined method Symfony\Component\HttpFoundation\BinaryFileResponse::header()"
## Appears to be common
It appears that lots of devs have encountered this problem and been confused by it.
e.g.
- Stackoverflow [BinaryFileResponse in Laravel undefined](https://stackoverflow.com/questions/29289177/binaryfileresponse-in-laravel-undefined), 32k views, 33 upvotes, answers with 74 upvotes
- Caused by `return response()->download(...);`
- Stackoverflow [Call to undefined method Symfony\Component\HttpFoundation\Response::header()](https://stackoverflow.com/questions/43593351/call-to-undefined-method-symfony-component-httpfoundation-responseheader), 26k views, 22 upvotes, answer with 52 upvotes
- Caused when started using Laravel Passport
- Various issues raised on Laravel projects, e.g. [bug fix in laravel/vapor-core](laravel/vapor-core#10), [confusion about cors middleware error when attempting to export a file in laravel/nova-issues](laravel/nova-issues#4036), [confusion about this error, asked in laravel/framework](laravel/framework#21922), [issue raised in laravel/cashier-stripe laravel#1214](laravel/cashier-stripe#1214),
## Confused by the documentation = Clarify in the docs
1. The above questions are being asked because devs are "following the docs" yet encountering an error, leading to confusion
2. Users of Laravel write Middleware a lot
3. The framework's own middleware that add headers to responses, all use `$response->headers->set()` and related `ResponseHeaderBag` methods. But the docs don't currently say that's what we should do too.
Therefore, I think the docs would be improved with a small clarification to avoid the chainable methods in middleware.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Switching the WebhookController to use Laravel's standard http response. This came up for me after adding a new middleware based on recent changes in Laravel 8.49:
I noticed in my QA environment that all of my webhooks were failing because of the following error:
I'm not sure if there is a historical or current reason to opt for Symfony response over Illuminate response which extends Symfony response.