Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Use Symfony Response in exception handler #48226

Merged
merged 2 commits into from Aug 29, 2023

Conversation

thomasschiet
Copy link
Contributor

This change partially reverts #47328.

The Symfony Response is more accurate because it is the return type of the render method in the contract. Plus, Illuminate\Http\JsonResponse is a subtype of \Symfony\Component\HttpFoundation\Response but not ofIlluminate\Http\Response.

I did not revert the return type of toIlluminateResponse.

@GrahamCampbell
Copy link
Member

What does this achieve?

@thomasschiet
Copy link
Contributor Author

thomasschiet commented Aug 29, 2023

Improved typing, since the current implementation is incorrect.

Take this line for example. It returns a Illuminate\Http\JsonResponse, which is not allowed by its current return type.

If I were to extend a method in the current handler and return JsonReponse, this would yield the following PHPStan error:

Return type (Illuminate\Http\JsonResponse|Illuminate\Http\RedirectResponse) of method App\Exceptions\Handler::unauthenticated() should be covariant with return type (Illuminate\Http\RedirectResponse|Illuminate\Http\Response) of method Illuminate\Foundation\Exceptions\Handler::unauthenticated()

Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

Probably the right fix is to add in JsonResponse to the union, like we did for redirect response.

@X-Coder264
Copy link
Contributor

What is the point of listing all these child classes? IMO just the Symfony Response class should be there, all other responses extend that class anyway.

@GrahamCampbell
Copy link
Member

The point is that the laravel responses have additional methods available, that are not in the symfony response class. if we wanted to just not properly type things with some parent, we could just put object everywhere - see my point?

@X-Coder264
Copy link
Contributor

X-Coder264 commented Aug 29, 2023

@GrahamCampbell This is just an API contract between the framework and the framework users, methods that are explicitly protected that users can override in their own apps, like the renderExceptionResponse method. The return type was made narrower in #47328 for absolutely no reason as the framework can work with any Symfony Response class (https://github.com/laravel/framework/blob/v10.20.0/src/Illuminate/Foundation/Exceptions/Handler.php#L383) so users should be allowed to be able to return any Symfony Response child class they want (and if they manually call these methods directly in their codebase which they shouldn't then it's up to them to check the instanceof of the response before calling some potentially non existing method).

@mbabker
Copy link
Contributor

mbabker commented Aug 29, 2023

The point is that the laravel responses have additional methods available, that are not in the symfony response class. if we wanted to just not properly type things with some parent, we could just put object everywhere - see my point?

Accurate type contracts are just as important. As is, the type contract is saying that only Laravel's subclasses of the Symfony response classes are valid. Is that really the intended contract here?

Truthfully, #47328 should have been flagged as a B/C break and not accepted in 10.x; narrowing the return types on a contract should be considered a breaking change.

@taylorotwell taylorotwell merged commit 0ccc2d9 into laravel:10.x Aug 29, 2023
19 checks passed
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.

None yet

5 participants