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

Don't assume model App\User #21

Closed
jonagoldman opened this issue Dec 25, 2015 · 23 comments
Closed

Don't assume model App\User #21

jonagoldman opened this issue Dec 25, 2015 · 23 comments

Comments

@jonagoldman
Copy link

Model App\User is hard-coded in various classes:

Silber\Bouncer\Database\Models
Silber\Bouncer\Database\Ability
Silber\Bouncer\Database\Role

It is possible to make it configurable?

@Arcesilas
Copy link
Contributor

Actually, it already takes care of the configuration. The auth config file has changed as of Laravel 5.2 and Bouncer seems to not have been updated since.

It happens in BouncerServiceProvider on line 130:

$model = $this->app->make('config')->get('auth.model');

In Laravel 5.2 you should use:

$model = $this->app->make('config')->get('auth.providers.users.model');

I could propose a pull request, but I'm not sure the best way to do it :

  • check Laravel version and use the appropriate config
  • or consider package is for Laravel 5.2 and just change the configuration path ?

@Arcesilas
Copy link
Contributor

Anyway, I understand that the use of use App\User in the files you mention can be confusing. In fact, it's just a key used for the Models class to store the model name. It could be whatever we like, such as a simple string 'User'. Maybe this way it would not lead to confusion.

@jonagoldman
Copy link
Author

I'm using Laravel 5.1 for this so get('auth.model') is working for me. Plan to update in about a month.
Regarding use App\User, what do you mean that is just a key? If it doesn't exists, like in my case, then it breaks.

Regarding the pull request, the way most packages handle this is by having a different version for each supported Laravel version... So for example Bouncer 0.1 works with 5.1 and 0.2 works with 5.2

@Arcesilas
Copy link
Contributor

Was using 5.1 last week, 5.2 now (project still in development) and never had the problem you mention.
I spent quite some time how it was possible as I did not understand a first glance.
The Models class stores the different models names to use in static $models array. The user model name is retrieved by passing the name to Models::classname() method. Whatever its value, it's just the key of that table to find out what is the actual User model name in the application (which is initialized in BouncerServiceProvider depending on the auth config file (which is updated with command app:name).
You could use MyPrettyAwesomeUserModelName everywhere in replacement of User::class it would work the same way.

I've done exactly what @andradedev described in #25 on a fresh install:

  • install laravel: composer create-project --prefer-dist laravel/laravel laravel5
  • change application name: artisan app:name laravel52
  • install package: composer require silber/bouncer
  • edit app.php file to load BouncerServiceProvider and use BouncerFacade
  • publish bouncer migration: artisan vendor:publish --provider="Silber\Bouncer\BouncerServiceProvider" --tag="migrations"
  • run migration: artisan migrate

Then, I have the following error:

[Symfony\Component\Debug\Exception\FatalErrorException]  
  Class 'App\User' not found 

Ok. Edit BouncerServiceProvider file to change the auth config key to user model as I mentioned, remove tables creating before failing migrations and run artisan migrate again:

laravel5$ artisan migrate
Migration table created successfully.
Migrated: 2014_10_12_000000_create_users_table
Migrated: 2014_10_12_100000_create_password_resets_table
Migrated: 2015_12_26_164210_create_bouncer_tables

No error and use App\user is still present in the source code. Did I miss something ?

@jonagoldman
Copy link
Author

I forked the repo and replaced App\User with my User entity for my project, so I can't go back and check it from scratch right now. But anyways App\User must be removed or taken from the app itself, it cant be hard-coded inside the package.
I will dig into the code later and check why it might be working for you.

@Arcesilas
Copy link
Contributor

Even if it's working for me, it's still not very difficult to remove App\User to make sure there is no error at any time in any configuration. It would make it less confusing too.

@jonagoldman
Copy link
Author

@JosephSilber I don't think the PR solves the issue. Model App\User still hard-coded in classes.

@JosephSilber
Copy link
Owner

Have you tried it? Did you get an error?

If so, can you please post the stack trace here?

@jonagoldman
Copy link
Author

The error is that App\User doesn't exist. I changed my app namespace and moved the User model to a different location, so it can't find it.
If you read trough the comments you will see that @Arcesilas made a very detailed explanation of the issue:

[Symfony\Component\Debug\Exception\FatalErrorException]
Class 'App\User' not found

@Arcesilas
Copy link
Contributor

As I explained, this error happened because auth configuration file changed as of Laravel 5.2. Therefore, default was to use App\User which actually did not exist.
Using the correct auth config key to get the model solved the problem, as I said in my very detailed explanation ;) Even with use App\User hardcoded, no error was raised.
You may consider try with a fresh install of Laravel 5.1 and 5.2, just as I did, to see if the problem is still present or not. The fact you encounter the problem on your application does not mean it's not solved for a fresh install : only you know what changes you made and where and which consequences they may have on the application behaviour.

@JosephSilber
Copy link
Owner

@jonagoldman without a stack trace, I can't tell what's wrong.

Please provide a full stack trace so that we can figure this out together.

Thank you.

@JosephSilber JosephSilber reopened this Dec 27, 2015
@jonagoldman
Copy link
Author

I could not get the error so I went trough the code.
Looks like you are using App\User as some kind of key, and even the class itself doesn't exists, User::class returns the string 'App\User', which you save for later use.

Not sure why PHP internals it's ok with this, but it just don't look right to me.
I just don't now how to explain myself correctly, but the class doesn't exists! It can't be right to do this! Even my IDE feels bad about it:

1
2

Man it's your package and your call. Maybe I don't understand something. I'm just pointing it out.

@Arcesilas
Copy link
Contributor

That's exactly what I meant when I said it was a key. Yes, for PHP it's ok, even if the class does not exist...

I would have proposed to remove App\User everywhere and also remove all references to User::class and replace them by a simple string : 'User' to make it more clear. It was very confusing to me first time I tryed to understand what happened and why there was no error...

@JosephSilber
Copy link
Owner

JosephSilber commented Dec 27, 2015

To summarize:

  1. There's currently no error. Everything works as expected.
  2. PHP is OK with using class constants on a fully qualified class name that doesn't exist.
  3. Bouncer uses this as a sensible key to locate the user model.
  4. You would prefer to use a magic key instead of a sensible key which your IDE doesn't like.

If I've summarized it all correctly, I think there's no point for future discussion here.

Have I missed anything?

@jonagoldman
Copy link
Author

👍

@Arcesilas
Copy link
Contributor

About PHP being OK: http://php.net/manual/en/language.oop5.changelog.php#117688

ClassName::class magically evaluates as "ClassName", without triggering autoloading of that class.

@JosephSilber
Copy link
Owner

@jonagoldman thanks for following through.

I appreciate all the feedback, even if I don't always agree with everything.

❤️

@jonagoldman
Copy link
Author

@Arcesilas I actually looked for that but could not find it, thanks!
@JosephSilber I'm the one that thanks you for this package, will be using it in my next project! :)

@dhcmega
Copy link

dhcmega commented Aug 24, 2017

Hi, this is pretty old, but the problem persists.
My User's model is in \App\Models\User, but Bouncer classes have App\User hardcoded as required class.

For example in IsRole.php

use App\User;

If I modify the package, an update will overwrite the fix, overriding all the methods to use the custom path will be too complex.

How should I overcome this?

edit:
I think a solution would be to use
config('auth.providers.users.model')
instead of the use App\User and the User::class reference.

edit2:
I think the better solution will be to have a config file for Bouncer in which to set up things like this.

Thanks!

@JosephSilber
Copy link
Owner

JosephSilber commented Aug 24, 2017

@dhcmega what exactly is the problem? Bouncer actually does use the user specified in the auth config. Have you tried it?

@dhcmega
Copy link

dhcmega commented Aug 24, 2017

In role for example:

https://github.com/JosephSilber/bouncer/blob/master/src/Database/Concerns/IsRole.php

It has line9: use App\User;
and line 26: Models::classname(User::class),

May be I'm wrong, I have just started using this package.
When I try to validate abilities for users, the query has 'App\User', but the data base model is 'App\Models\User'.

@Arcesilas
Copy link
Contributor

Been a long time, but I remember it has already been answered: App\User is registered as an alias. When requesting for App\User the container resolves it to the actual User model class and returns the appropriate instance according to your configuration.

@dhcmega
Copy link

dhcmega commented Aug 24, 2017

I will do some tests and get back if I can't work it out.. I'm using App\User for auth, but App\Models\User for CRUD. Thanks!

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

No branches or pull requests

4 participants