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

[5.5] Possibility to specify default user provider and passing user provider in RequestGuard callback #18856

Merged
merged 1 commit into from
Apr 24, 2017
Merged

[5.5] Possibility to specify default user provider and passing user provider in RequestGuard callback #18856

merged 1 commit into from
Apr 24, 2017

Conversation

evsign
Copy link

@evsign evsign commented Apr 18, 2017

Some small refactoring.

  1. Move getters and setters for user provider from Illuminate\Auth\SessionGuard to Illuminate\Auth\GuardHelpers.
  2. Possibility to specify default user provider in config
  3. Pass default user provider in RequestGuard callback

$config = $this->app['config']['auth.providers.'.$provider];
$driver = array_get($config, 'driver');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind replacing all array_*() with with Arr::*()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. But would you mind to explain difference? In many places used global array helper functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Using Laravel's helper function makes this component (Auth) dependant on the framework and can't be used as a standalone component. It's a common practise across the framework's codebase.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@taylorotwell
Copy link
Member

Can you describe what the actual goal is here?

@evsign
Copy link
Author

evsign commented Apr 19, 2017

I just think this will be better.

  1. If property definition situated in trait, then accessers must be in trait too. And also this provide the way to get user provider from all guards, which have one.
  2. It for uniformity.
  3. If we want just quick implement specific session logic and access user in uniform for guards way from configured user provider.

@taylorotwell
Copy link
Member

Can you provide a before / after example of what this makes easier for end users?

@evsign
Copy link
Author

evsign commented Apr 19, 2017

Ill try)

  1. Before: In case of TokenGuard or RequestGuard (before this and third point of this list) if we call Auth::guard('token-or-request-guard')->getProvider() will be error.
    After: Return user provider as expected.

  2. Before: We always need to specify user provider for all guards and password brokers.
    Old config example:

      return [
       'defaults' => [
            'guard' => 'api',
            'passwords' => 'users',
        ],
        'guards' => [
            'api' => [
                'driver' => 'session',
                'provider' => 'eloquent-user-provider'
            ],
            'web' => [
                'driver' => 'token'
                'provider' => 'eloquent-user-provider'
            ]
        ],
        'providers' => [
            'eloquent-user-provider' => [
                'driver' => 'eloquent',
                'model' => App\Models\User::class,
            ]
        ],
        'passwords' => [
            'users' => [
                'provider' => 'eloquent-user-provider',
                'table' => 'password_resets',
                'expire' => 60,
            ],
        ],
      ];

    After: If we have one user provider for all guards and password brokers we can configure default user
    provider name which will be used. Like default guard and password broker.
    New config example:

      return [
       'defaults' => [
            'guard' => 'api',
            'passwords' => 'users',
            'provider' => 'eloquent-user-provider'
        ],
        'guards' => [
            'api' => [
                'driver' => 'session',
            ],
            'web' => [
                'driver' => 'token'
            ]
        ],
        'providers' => [
            'eloquent-user-provider' => [
                'driver' => 'eloquent',
                'model' => App\Models\User::class,
            ]
        ],
        'passwords' => [
            'users' => [
                'table' => 'password_resets',
                'expire' => 60,
            ],
        ],
      ];
  3. Before: In case when we need to retrieve user from database when used viaRequest way can occur
    that we have to duplicate code from user provider.

    Auth::viaRequest('my-guard', function($request) {
        $query = User::query();
        foreach ($request->only('needed', 'fields') as $key => $value) {
            if (! Str::contains($key, 'password')) {
                $query->where($key, $value);
            }
        }
        return $query->first();
    });

    After: Default user provider will be passed in callback as second argument. Also this provide the way
    to incapsulate (in user provider) another dependencies which may be needed for user resolving and
    another user operations like password checking in validateCredentials method.

    Auth::viaRequest('my-guard', function($request, $userProvider) {
      return $userProvider->retrieveByCredentials($request->only('needed', 'fields'));
    });

@taylorotwell taylorotwell merged commit 7503a9a into laravel:master Apr 24, 2017
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

Successfully merging this pull request may close these issues.

3 participants