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

Load default auth model from config #602

Merged
merged 7 commits into from
Jun 16, 2020

Conversation

0xb4lint
Copy link
Contributor

@0xb4lint 0xb4lint commented Jun 13, 2020

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

This PR adds support for dynamic auth model load from config.

Replaces

$userModel = $config->get('auth.providers.users.model');

with

if ($guard = $config->get('auth.defaults.guard')) {
    if ($provider = $config->get('auth.guards.'.$guard.'.provider')) {
        if ($authModel = $config->get('auth.providers.'.$provider.'.model')) {
            return $authModel;
        }
    }
}

(In the future maybe it can be extended with dynamic guard handling.)

@szepeviktor
Copy link
Collaborator

szepeviktor commented Jun 13, 2020

Thank you for your contribution! ❤️

Could you replace return+else-es×3?
And please see https://github.styleci.io/analyses/KZp2wV

@0xb4lint
Copy link
Contributor Author

Hey @szepeviktor!

I'd like to fix the style but there is a stylelint-PHPstan conflict:

stylelint wants this form:

/**
 * Returns the default auth model from config.
 *
 * @return string|null
 */
private function getAuthModel(ConfigRepository $config)
{
    if (
        ($guard = $config->get('auth.defaults.guard')) &&
        ($provider = $config->get('auth.guards.'.$guard.'.provider')) &&
        ($authModel = $config->get('auth.providers.'.$provider.'.model'))
    ) {
        return $authModel;
    } else {
        return;
    }
}

But PHPStan will gives me the following errors:

 ------ ---------------------------------------------------------------------------------------------------------------------------------------- 
  Line   ReturnTypes/AuthExtension.php                                                                                                           
 ------ ---------------------------------------------------------------------------------------------------------------------------------------- 
  65     Method NunoMaduro\Larastan\ReturnTypes\AuthExtension::getAuthModel() never returns null so it can be removed from the return typehint.  
  74     Method NunoMaduro\Larastan\ReturnTypes\AuthExtension::getAuthModel() should return string|null but empty return statement found.        
 ------ ---------------------------------------------------------------------------------------------------------------------------------------- 

If I change @return string|null to @return string|void and remove the else { return; } part, like this:

/**
 * Returns the default auth model from config.
 *
 * @return string|void
 */
private function getAuthModel(ConfigRepository $config)
{
    if (
        ($guard = $config->get('auth.defaults.guard')) &&
        ($provider = $config->get('auth.guards.'.$guard.'.provider')) &&
        ($authModel = $config->get('auth.providers.'.$provider.'.model'))
    ) {
        return $authModel;
    }
}

Then I think a PHPStan bug (or an unhandled case) occurs:

 ------ --------------------------------------------------------------------------------------------------------- 
  Line   ReturnTypes/AuthExtension.php                                                                            
 ------ --------------------------------------------------------------------------------------------------------- 
  54     Parameter #1 $className of class PHPStan\Type\ObjectType constructor expects string, string|void given.  
 ------ --------------------------------------------------------------------------------------------------------- 

There is a condition to handle false-ish values:

if ($authModel = $this->getAuthModel($config)) {
    return TypeCombinator::addNull(new ObjectType($authModel));
}

So I've commited a much uglier solution and stylelint still wants to remove return null;

@szepeviktor
Copy link
Collaborator

szepeviktor commented Jun 13, 2020

It is preferable to put errors in conditionals, leave normal/expected execution unindented.

        if (! ($guard = $config->get('auth.defaults.guard'))
            || ! ($provider = $config->get('auth.guards.'.$guard.'.provider'))
            || ! ($authModel = $config->get('auth.providers.'.$provider.'.model'))
        ) {
            return;
        }

        return $authModel;

@0xb4lint
Copy link
Contributor Author

Thank you!
I've changed the code, now styleci passes, but if I run PHPStan on the project the bug is still present:

 ------ --------------------------------------------------------------------------------------------------------- 
  Line   ReturnTypes/AuthExtension.php                                                                            
 ------ --------------------------------------------------------------------------------------------------------- 
  54     Parameter #1 $className of class PHPStan\Type\ObjectType constructor expects string, string|void given.  
 ------ --------------------------------------------------------------------------------------------------------- 

@szepeviktor
Copy link
Collaborator

Yes, we must pass a string only.
Could you do that in all cases?

@0xb4lint
Copy link
Contributor Author

Thank you @szepeviktor!
It looks like everything works.

@szepeviktor
Copy link
Collaborator

szepeviktor commented Jun 13, 2020

Thank you.
Actually we write return typehints: ?string

Please see https://github.com/nunomaduro/larastan/pull/603/files

@szepeviktor szepeviktor mentioned this pull request Jun 13, 2020
@0xb4lint
Copy link
Contributor Author

I see, all right.
Shall I merge your commit into this PR?

@szepeviktor
Copy link
Collaborator

As you wish, it is example code.

@szepeviktor szepeviktor requested a review from canvural June 13, 2020 20:54
Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

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

Thank you! It looks good.

@canvural canvural merged commit 3882bca into larastan:master Jun 16, 2020
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