barryvdh / laravel-ide-helper

IDE Helper for Laravel
MIT License
14.17k stars 1.16k forks source link

Add support for multiple or alternately named auth providers #1263

Closed miken32 closed 2 years ago

miken32 commented 2 years ago

Summary

When determining return value for functions like Auth::user(), only the hardcoded path auth.providers.users.model is checked in the configuration. In the case of multiple guards (or a guard not named "users") the resulting return type is incomplete. This fix checks every entry in the auth.providers array to build an intersection type (e.g. App\User|App\Admin) for use as return.

I considered adding more checks (e.g. make sure class_exists() and it implements Authenticatable) but if you have invalid class names as part of your application config, that's not this tool's problem.

Type of change

Checklist

miken32 commented 2 years ago

I wasn’t sure how to do a test based on the generator command, looks like I'd be starting from scratch. If you’ve got any suggestions where to start, I’m happy to create a test.

jonnywilliamson commented 2 years ago

I grabed this code change and did a very quick test with it. I got the following generated output in the ide_helper.php

         /**
         * Get the currently authenticated user.
         *
         * @return \App\Models\User|App\Models\IalpaMember|null
         * @static 
         */ 
        public static function user()
        {
                        /** @var \Illuminate\Auth\SessionGuard $instance */
                        return $instance->user();
        }

Note the missing \ at the start of the second User model. My IDE throws up an error.

CleanShot 2021-11-13 at 13 25 56@2x

Not sure if it's this PR or another section of the code that's causing the issue.

miken32 commented 2 years ago

@jonnywilliamson it just looks at what’s in your auth config file. Did you check that?

jonnywilliamson commented 2 years ago

@jonnywilliamson it just looks at what’s in your auth config file. Did you check that?

So the original auth.php file, fresh in a new install looks like this:

https://github.com/laravel/laravel/blob/8.x/config/auth.php#L62-L66

With no leading \,

So I had copied that like follows (unintentionally):

CleanShot 2021-11-15 at 14 51 32@2x

So both of them had missing \ at the start but it seems to add it for the first one when generating the ide_helper.php file.

I can of course add it myself manually, no biggie but i wonder why it's done automatically for the first provider?

miken32 commented 2 years ago

@jonnywilliamson I'd bet what's happening is somewhere the code is assuming this is a single class name, and it's sticking a leading backslash at the beginning to make it absolute. But of course with a union type only the first class in the list will be absolute. I just pushed out a change on the branch that will resolve the issue by making sure all the classes in the list are absolute.

jonnywilliamson commented 2 years ago

@miken32 Thank you michael for looking into it. I'd say you are spot on with that analysis.

Either way, I appreciate you addressing it so quickly. I hope this gets merged soon! Thanks.