Use mixed return type on controller stubs #46166
Conversation
|
Developers can do it on their own project, I think no need to change to |
|
@datlechin I'm well aware they can, the question is whether the default (as it stands currently) should be to error when a developer creates a new controller and tries to return a I could change the return type to |
|
Noob question Doesn't this kill the whole purpose of return types? What's the difference between this and no return types at all And cant we instead do something like this Response|View etc |
|
@yungifez one could definitely make that argument; however, we have added return types to all other parts of the skeleton so it may feel weird to not have them on controllers at all. Having the |
|
@taylorotwell Do views and Eloquent API resources both implement the |
|
@martinbean |
|
I agree 100% — the default return type should reflect what the framework is willing to accept here. And it accepts all the views, arrays, strings and everything else. @yungifez |
|
I think the return type Personally, I always return views for normal web controllers, and responses for API controllers (isn't this the "Laravel way"?). If I had to choose, I'd go with |
|
There could be an opportunity here to direct users to change the return types and educate them at the same time. Adding a note to the head of the stub could point them in the direction of what returns types mean. They can always be removed in the stubs for users who don't need direction. |
|
@LucaRed I return arrays from controllers pretty frequently. Wouldn't work with that type-hint you suggested. I personally lean @tontonsb's direction in that the |
|
Returning |
|
I don't know if this is possible, but can we extend |
How about changing that? Make Response and View implement Responsable, as they can both be used for returning from controllers. |
|
Yeah for me too 99% of the times a View type would be more appropriate so having the default being Response is a bit annoying, as much sense as it makes. I think mixed would be better though, this way we also don't need to delete imports etc. if we want to change it... |
Fair enough. Should the |
|
I think looking at all these comments |
|
Are there any differences between specifying If there are no differences, we could go with |
|
Using Hopefully a better solution could come out of this, specifically something along of what @martinbean suggests / a common interface as the return type. |
|
I think it should be a |
|
Food for thought, what about using the |
|
For the sake of further clarification, we can all refer to the Documentation on @LucaRed you’re correct. No return type and I would argue that to the good initiative to strict-type anything, this is that little exception to the rule where we’re actually typing something for the framework to process initially. We can always adjust typings as must of us would be already doing, or we can update our projects stubs manually to be more strict. Talking from a community standpoint, there’s no majority that will use X or Y; every developer will be using their own conventions, and reflecting that from a stubs seems really accurate as @jonathanmacgregor mentioned. |
|
I think that |
I don't see what is so bad about returning mixed as a return type. In my opinion, the framework has functioned without type hints for a long time. You are not accounting for cases like Inertia that does not make use of API controllers and I'm speculating is not of type Mixed in this case just acts as a decorator to prevent a partially typed application and follow consistent design |
|
This conversation is leading nowhere. The reality is: Laravel accepts any return types in controller handlers (strings, arrays, views, Response, etc.). The ideal return type, as for the current API is:
Edit: As @taylorotwell mentioned, this discussion is about a good default type for stubs, therefore, given the current API I conclude Perhaps in the future, if there's the need to, we could use mixed internally and leave the surface API fully typed. There's likely the possibility to do this: public function foo(): Responsable {
return response('foo');
}
public function foo(): Responsable {
return response()->json(['foo' => 'bar']);
}
public function foo(): Responsable {
return redirect()->route('foo');
}
public function foo(): Responsable {
return view('foo');
}
public function foo(User $user): Responsable {
return new UserResource($user);
}
public function foo(): Responsable {
return UserResource::collection(User::all());
}The In reality, the response helper will have mixed as it's parameter type, but on the surface we can be sure that the returned type is correct. With good enough internal testing for the helper, we shall be good. There seems to be no correct answer on this but rather multiple points of view. |
|
I personally don't find that return types are very useful on controllers actions, so my first choice would be to just leave them out of that stub (I'm in favor of return types in many other places—but only where they bring value). That said, |
|
Honestly, since Laravel offers the option to return almost anything from a People can still add a return type of their own if they wish, or just ignore it. |
|
Another option: #46167 Use |
|
Isn't there a common interface that both Response and View implements that can be used here as the return type? The main problem I see is that, even with a common interface the only acceptable type here is I agree with the sentiment of adding a |
|
Not to make this too academic but from a SOLID perspective Responsable makes more sense than mixed, where the latter would give more options to return different types of course but then on the other hand too many I feel. Not sure when e.g. I'd want to return an int here, it should not be allowed by means of the return types definition already though, otherwise (as others mentioned already) types don't make much sense here. Only valid argument against the Interface I've seen here was the array being returned, tbh I didn't even know that this is possible until now and should probably not be the case to start with. In this case I don't feel it's too much of a limitation though, this is not talking about a breaking change for existing classes but only a stub. Meaning, future controllers would be discouraged to return arrays, which again imo wouldn't be the worst idea. Having a typed return defined as an interface would make most sense therefore. |
|
make:controller NameController --response, --view, --responsable --mixed, --api One command to rule them all ~ Lord of the rings |
|
I think mixed makes the least friction out of the gate. Maybe laravel new can have a flag for changing the controller stubs for smoothing out the customization story. --controller=inertia |
|
Note that Having anything narrower than
Having a narrower return type that does not encompass other accepted responses in counterintuitive in both cases. As others have already said, a controller is fundamentally consumed by the framework, so its methods' return type should satisfy the framework expectations. IMHO, |
|
Just going to remove return types on controller method stubs. |
|
Wouldn't that make phpstan complain? |
|
@dillingham complain about what? the stubs have no code in them and thus no return type. |
|
@taylorotwell thanks for taking the time to consider various approaches, and sorry if my tweet caused the headache here. I agree that removing types is probably the best because historically it worked with almost whatever you would return:
|
|
I think the default code should be a good example from where anyone (junior or senior developers) can start from. If we use Sure getting an error when returning a If the "Response" causes an error I will learn to modify my return type based on my needs and not just use Again, I understand the willingness to avoid confusion but I always considered the default code of Laravel as "good practice" to follow or learn from. Using |
|
@madebydanda on his last commit, Taylor actually removed any return types, you can see on the changes tab. As the merge title didn't change, it can be misleading. |
|
@rodrigopedra Yes just saw that, well I guess my comment can also apply to the "no return type" solution. |
|
Well, I would say in a usual Laravel app those are more common than a plain Having Also, Laravel strives for DX. Narrowing the return type just because it feels "correct", while the framework can handle many other return types, is neither correct, in the sense it could imply the framework expect you to return a specific data type, but also goes against its philosophy, of moving out the way to let an app code shine, not to make a developer worry about complying to a specification, IMO. By the way, what is a response? The request object is a wrapper that flats out many PHP inconsistencies, but should a response be wrapped onto a specific class? Should we move away from model route binding, and have the request object retrieve the route parameters, as a controller method without a request wouldn't feel "correct"? I understand your concern, but I personally prefer not having a return type suggested than having something like: |
|
@rodrigopedra that wasn't really my point. In my opinion it's not an issue to have a specific return type that would throw an error when I return a different type. It "teaches" me something and makes me aware that I either need to update my return type or that I have an actual error in my code. I would have left everything as it was but I understand the reason why this was modified. |
That's exactly the purpose of an interface... You have to read it as, return me anything that can be turned into a response. However, as I pointed out in some of my comments, the issue will come with primitive types, as you can't implement an interface on them. |
I got that in the first time. We just happen to have different opinions on this matter. I don't think it is up to this fundamental part of the framework, to get in front of a developer and try to "teach" them anything. |
Having to read something differently from what it is written is counter-productive, IMO. Having an interface on the return type of method is a hint for whoever will consume that method. And on this particular part of the request/response cycle, a controller method is only consumed by the framework, which can handle many other different types than this interface. I would agree if Laravel allowed to swap the request dispatcher by something else, like a PSR-7, or PSR-15. In that case, using a contract would enforce compliance with unknown code that could consume that method. That is not the case here, though. A controller's method is not meant to be reused anywhere else, or consumed by any other part of the framework. It is not code meant to be interoperable, nor code written without knowing who will consume it. If there is a single consumer, requiring a narrow interface, or contract -- when this consumer does already accept a wider domain of values -- adds little to no value, but unneeded noise and overhead. I don't want to extend this discussion, as this seems to be a matter of opinion, or self-preference. But what would be the true benefit of enforcing any kind of contract on a controller's method return type, when the only consumer accepts anything from it? |
|
I have to say that I am somehow surprised, Laravel has always been a highly opinionated framework, with a very specific way of doing things. Finally, it has started to standardize things that are logical, like for example, a language based on request, response. End up by returning the obvious, a Response. I think it is a change that breaks the current way of working, but I see a logical change and, as I mentioned before, consistent with the programming language. Not only that, it's unnatural to fight against the very language that goes towards typed types. |
|
so how about a slightly different approach?
so you might do artisan make:controller --invokable --return=Illuminate\Http\JsonResponse for example? this feels like it might offer the best of both worlds without compromising the intended purpose of the original method |
Not sure about that. (see expanded details)<?php
// routes/web.php
use App\Events\AEvent;
use App\Http\Controllers\AController;
use App\Http\Controllers\AnotherController;
use App\Jobs\AJob;
use App\Models\User;
use App\Notifications\ANotification;
use Auth as AuthAlias;
use Bus as BusAlias;
use DB as DBAlias;
use Event as EventAlias;
use Facades\Illuminate\Http\Request as RequestRealTimeFacade;
use Hash as HashAlias;
use Illuminate\Contracts\Auth\Authenticatable;
use Illuminate\Contracts\Auth\Factory;
use Illuminate\Contracts\Bus\Dispatcher as BusDispatcher;
use Illuminate\Contracts\Events\Dispatcher as EventsDispatcher;
use Illuminate\Contracts\Foundation\Application;
use Illuminate\Contracts\Notifications\Dispatcher as NotificationsDispatcher;
use Illuminate\Contracts\Session\Session;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Bus;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Event;
use Illuminate\Support\Facades\Hash;
use Illuminate\Support\Facades\Notification;
use Illuminate\Support\Facades\Queue;
use Illuminate\Support\Facades\Request as RequestFacade;
use Illuminate\Support\Facades\Route;
use Illuminate\Support\Facades\Session as SessionFacade;
use Illuminate\Support\Str;
use Notification as NotificationAlias;
use Queue as QueueAlias;
use Request as RequestAlias;
use Session as SessionAlias;
use Str as StrAlias;
if (is_file(database_path('database.sqlite'))) {
User::query()->truncate(); // query() + method
User::truncate(); // auto delegate to query builder
DB::query()->from('users')->truncate();
DBAlias::table('users')->truncate();
$aUser = User::factory()->createOne([
'name' => Str::random(),
'email' => StrAlias::random() . '@example.com',
'password' => bcrypt('password'),
]);
Auth::login($aUser);
User::forceCreate([
'id' => 2,
'name' => Str::random(),
'email' => StrAlias::random() . '@example.com',
'password' => Hash::make('password'),
]);
}
Route::get('/foo', AController::class) // with preeciding forward slash
->middleware('web', 'auth'); // middleware as parameter list, auth without guard
// middleware can also be defined on a controller's constructor
// (not shown here)
Route::get('bar', [AnotherController::class, 'method']);
Route::get('baz', AnotherController::class . '@method')
->middleware(['web', 'auth:web']) // middleware as array, auth with guard
->name('a-name');
Route::get('qux', [
'prefix' => '/boo',
'uses' => AnotherController::class . '@method',
'as' => 'boo.',
'middleware' => ['auth:web'],
]); // classic, all the others get converted to something like this internally
// helper class, as ijecting Authenticatable
// does not work properly when also injecting User
class AUser extends Model
{
protected $table = 'users';
protected $guarded = [];
}
// $router variable is injected on web.php
$router->get('/user/{user}', function (
Application $app,
Request $injectedRequest,
Session $session,
Authenticatable $authUser,
BusDispatcher $bus,
EventsDispatcher $events,
NotificationsDispatcher $notifications,
Factory $authFactory,
AUser $user,
) {
$user->fill([
'password' => HashAlias::make('password'),
])->save(); // fill + save
$session->put('bar', 3);
dispatch(new AJob());
AJob::dispatch();
Queue::push(new AJob());
QueueAlias::push(new AJob());
Bus::dispatch(new AJob());
BusAlias::dispatch(new AJob());
event(new AEvent());
AEvent::dispatch();
Event::dispatch(new AEvent());
EventAlias::dispatch(new AEvent());
/** @var \App\Models\User $authUser */
$authUser->notify(new ANotification());
$notifications->send([$authUser], new ANotification());
Notification::send([$authUser], new ANotification());
NotificationAlias::send([$authUser], new ANotification());
return [
request('foo'),
request()->input('foo'),
app('request')->get('foo'),
$app['request']->input('foo'),
$injectedRequest->query->get('foo'),
RequestFacade::query('foo'),
RequestAlias::input('foo'),
RequestRealTimeFacade::get('foo'),
'###################################',
request()->user(),
$authUser,
auth()->user(),
Auth::user(),
AuthAlias::user(),
app('auth')->user(),
$authFactory->guard('web')->user(),
'###################################',
$user,
request('user'),
request()->user,
request()->route('user'),
request()->route()->parameter('user'),
$injectedRequest->route()->parameters['user'],
$injectedRequest->route()->user,
resolve('request')->route('user'), // resolve() is an alias to app()
'###################################',
$session->get('bar'),
$injectedRequest->session()->get('bar'),
SessionFacade::get('bar'),
SessionAlias::get('bar'),
session('bar'),
session()->get('bar'),
app('session')->get('bar'),
'###################################',
User::all(),
User::query()->get(),
DB::table('users')->get(),
];
})->middleware('auth:web');
// could keep on with how to define route groups with prefixes, aliases, ...
Agreed. But its strength is precisely getting out the way of the developer, so they have to worry mostly with their own business logic, and not with implementation details. It provides sensible defaults, while still allowing a developer to stick with any style they want. |
This is entirely sensible. You're stubbing out userland code, this isn't internal Laravel code, so adding useless return types that only have any meaning once the developer has changed them is arbitrary. |
|
I propose to add property to
If blade option is set then So if you're building project with Inertia, you just change property value to inertia. I also propose to add flags
|
|
One can already customize the stubs (templates) used by all php artisan stub:publishreference: https://laravel.com/docs/10.x/artisan#stub-customization |
|
I feel like the main argument against an interface like What about extending |
|
@Pluuk this is already possible with Although I am a heavy user of types I think no types by default as it is right now is the best option. // this is not an issue:
public function method()
{
return view('foo');
}
// this however is:
public function method(): Responsable
{
return [ 'bar' ];
}the second option simply includes more use cases by default. And if anyone want's to lock down the first method to views, that it can be done either in the controller itself or in published stubs. |
Thank you, you mentioned the obvious. Why hit zend_gc_collector with garbage it cannot even collect? The streets gonna stink like all the previous versions of php |
This pull request updates the controller stubs to use a
mixedreturn type to accommodate the various types of return values a controller can return in Laravel.This allows developers to return anything they want without an error by default, but they can easily tighten up the types as they decide which each controller endpoint will actually return.