-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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.
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.
I think so
|
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 🤔
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.
I think you can add the guard as a parameter to the function so we can get the user for a specific guard
| 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 ❤️🚀 |
If you add |
I guess there's nothing wrong with that. |
|
With the Laravel Idea Plugin being free now in PhpStorm, the vscode extension should fix this if it doesn't already, then this helper shouldn't be needed. I also think having a |
|
please not.. |
|
|
|
I think this is an overall good idea, I usually implement aliases for user(), and use inertia(), auth() everywhere I can, it's simply shorter to type than Auth::user() or Inertia::render()... if people want to customize the "driver" for it, introduce a config variable for it, or an optional param? |
|
Why not make an improvement to the VSCode plugin instead of making changes? Most of the helper functions yield IDE warnings like url()->full() auth()->user() etc. No need to make user() or full() separate things. |
|
Plenty of comments here about using the PHPStorm/VSCode plugin, but that has 2 problems:
The issue this aims to resolve is to get the right type hinted for the user returned, which those functions don't do for the scenario described. |
I agree, but I don't think a global helper is the solution, but that requires a return type that handles the multiple possible implementations of the A solution could be a trait that adds a static function on the |
|
“You shouldn’t have to rely on a very specific IDE to get a good experience for what can be supported with a fundamental PHP feature” That’s true in theory — but in practice, this proposal introduces more assumptions and magic than it solves. You’re enforcing a single, hardcoded model (App\Models\User) in a helper that pretends to simplify things, but in reality: If this was really about improving static analysis, you’d push for better support in IDEs and tools like PHPStan — not inject app-specific logic into framework-level conventions. “You shouldn’t need a specific IDE” You also shouldn’t need to overwrite framework defaults with brittle shortcuts just to avoid typing a docblock. This isn’t developer experience — it’s developer laziness disguised as convenience. |
|
@noxsii it makes an assumption but it does not enforce anything. The function explicitly resides in the user's application and allows them to change it if they use a different Authenticatable than the default.
I'd argue the framework defaults are the problem here - PHP can already support what needs to be done here |
|
If the framework defaults are the problem, then the solution should be to improve the defaults — not introduce yet another userland workaround that: Calling this “not enforcing anything” is disingenuous — because the moment you ship this in a skeleton, you’re implicitly enforcing a convention, and every beginner will take it as canonical. "PHP can already support what needs to be done here" Yes, and Laravel already provides the flexibility for devs to override contracts, bind implementations, and configure their app however they want. Why is a hardcoded helper the answer to a tooling gap? This isn’t solving the problem — it’s papering over it in a way that breaks as soon as you leave the happy path. If the goal is better static analysis, IDE support, and DX: |
💯
It's not that people suddenly started implementing the |
|
@shaedrich Sure, Laravel ships with App\Models\User as the default model — but it never guarantees or enforces that you’ll keep using it. That’s the entire point of the “So you can’t blame that intermixing on the individual devs but on the framework” Except we can — when individual devs start hardcoding assumptions (like App\Models\User) in global helpers without checking for configurability, that’s not Laravel’s fault. The framework provides sensible defaults — not mandates. This PR introduces a global helper that pretends to simplify things, while: Let’s not blame Laravel for flexible defaults just because we choose to misuse them. |
This adds a
user()helper function to the base application, which is type-hinted to return an instance of theApp\Models\Userclass by default.Why?
auth()->user()is handy, and widely used in Laravel projects, but its return typehint is for aIlluminate\Contracts\Auth\Authenticatableinstance.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\Useris defined in the user's application, so the framework has no knowledge of it or if it changes names/namespaces/etc.