-
Notifications
You must be signed in to change notification settings - Fork 24.4k
Add user(): helper function #6582
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
base: 12.x
Are you sure you want to change the base?
Conversation
I don't get the point of this. Seems like yet another worthless file I'll have to remove from every app I make like the console routes file. And seems directly contrary to th direction Laravel went with 11.x for slimming the skeleton. If you want it just add it yourself to your app. |
Yeah this should be either apart of the framework's existing helpers or added yourself to your own project. With the new |
We already have Laravel's default middleware |
throw new Exception('The current user is not authenticated.'); | ||
} | ||
|
||
if (! $currentUser instanceof User) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the function has a return type of App\Models\User
this check is unnecessary.
If an object which is not an instance of App\Models\User
is returned, PHP will error out automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would throw a TypeError you're right, but I made it explicit here so that static analysis tools can tell what's going on
Another option would be to docblock the $currentUser
variable for static analysis and allow the TypeError to be thrown at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have the user model defined in the config: auth.providers.users.model
.
It should use that.
I’m experiencing the same frustration with PHPStan. I’ll probably implement something like this in my projects, but I feel like a better version should be implemented at the framework level.
I usually add a <?php
namespace App;
function foo() {} It is still short enough to use in views ( I would vote to have the helpers file under I can see the benefit of having this file included on the likes to show users how easy, and useful, it is to have a helpers file. Much like what we have in |
It also has the additional benefit of an added helper file that will come with the project, most of the devs had to add that file to their project manually. Maybe |
throw new Exception('The current user is not authenticated.'); | ||
} | ||
|
||
if (! $currentUser instanceof User) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helpful idea, but I think using App\Models\User
directly could be limiting. In many cases, the authenticated user isn't always a User
instance — it can be any Authenticatable
implementation, defined via auth.providers.users.model
and possibly located in a different namespace.
Also, when calling auth()->user()
(or this helper if added), developers typically expect an Authenticatable
, not specifically a User
model. This might lead to confusion or unexpected type errors in custom setups.
|
||
if (! $currentUser instanceof User) { | ||
throw new Exception('The currently authenticated user is not an instance of '.User::class); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks in apps with multiple auth guards. It assumes only User gets authenticated.
In my opinion, it's not a necessary functionality. Even if it was, it does not consider all possible situations. For example, what if someone (like me) uses a custom-curated class for the user entity that does not extend the App\Models\User? |
$currentUser = auth()->user(); | ||
|
||
if ($currentUser === null) { | ||
throw new Exception('The current user is not authenticated.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, this could be a dedicated UnauthenticatedException
or the like 🤔
if ($currentUser === null) { | ||
throw new Exception('The current user is not authenticated.'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want, you could shorten this
if ($currentUser === null) { | |
throw new Exception('The current user is not authenticated.'); | |
} | |
throw_if($currentUser === null, Exception::class, 'The current user is not authenticated.'); |
I think this is an issue for the vscode extension honestly. There has got to be a good way somehow to configure static analysis to bind preferred, current or default implementations for contracts in general. I honestly feel the OP's pain. I do, but this is part of a larger problem. |
Agree that this is necessary. Also agree that improving the IDE extension is the way to go. Laravel needs a best-in-class IDE that works out of the box without having to install a bunch of things to get it to work. Now that Laravel Cloud exists, the IDE is the last painful point that sucks for literally every developer. PHPStorm wont do anything and defers everything to the IDEHelper (BLESS BARRYV) but it adds a ton of bloat. VSCode is the obvious choice and the official extension is really the future. We shouldn't be making framework choices based on IDEs being bad at what they do. Instead we should be improving the IDE extension. |
The Laravel IDEA plugin is worth every cent, and works incredible well with (most) type analysis needed on a Laravel project. PHP Storm offers the Laravel IDEA plugin at a discounted rate when licensing both the IDE and the plugin together. I am not affiliated to PHP Storm nor to the Laravel IDEA plugin, just a satisfied client of both tools |
I've paid for both and went to vscode anyway. Caleb's make vscode awesome already puts it ahead of phpstorm, and with intelephense (the paid version) and the Laravel vscode extension, it's miles ahead. If we can invest more in a 1st party extension, it solves all of these issues. Case in point, we have talented developers sending pull requests for functions that have been around from day 1 because IDEs are still painful even for veterans. |
I always create a method called class User {
public static function auth(): self {
$user = auth()->user();
throw_if(! $user, Exception::class, "Called from invalid auth context!");
return $user;
}
} |
This might be interesting to add to |
Ignoring other issues, this will need manual tweaking as soon as someone uses another Authenticatable class. namespace Illuminate\Contracts\Auth {
interface Guard
{
public function user(): \App\Models\User;
}
} I don't usually reach for |
I think your idea is excellent, but the way it’s been implemented doesn’t align with the framework’s current structure. Depending on the project, we might have multiple authentication models or models organized into separate folders in modular architectures. The helper should be flexible enough to handle these cases. If you can refine your approach, it would make a wonderful contribution ❤️🚀 |
This adds a
user()
helper function to the base application, which is type-hinted to return an instance of theApp\Models\User
class by default.Why?
auth()->user()
is handy, and widely used in Laravel projects, but its return typehint is for aIlluminate\Contracts\Auth\Authenticatable
instance.While this may be valid from a framework perspective, and there are many uses for Authenticatables that are not the User model, most Laravel applications use the defaults. Therefore, users expect an instance of the User model returned.
This means that, without the help of third-party tools like laravel-ide-helper, as developers we get a poor experience when getting the authenticated user and expecting the model:
/** @var \App\Models\User $user */
everywhere we use the helperBy adding this to the basic application scaffold, the new
user()
helper can get proper typehinting for the out-of-the-box User model that ships with a fresh application, supporting proper typehinting. As it is in the userland instead of the framework, it can be changed to support other Authenticatables if the developer wishes.Why should this be in the base application?
Yes, users could always add this helper manually to their app, but having a standard way the framework does it will eventually bring consistency to new applications—and I believe this utility is used commonly enough that it warrants being in the base.
There's clearly some interest in it, and people have a dozen of their own home-grown solutions for the same problem, something this could provide.
Why not in the framework?
App\Models\User
is defined in the user's application, so the framework has no knowledge of it or if it changes names/namespaces/etc.