Split IdentityInterface about yii-core HOT 17 CLOSED

@dynasource, @cebe, @klimov-paul, @SilverFire thoughts?

from yii-core.

Such change seems to be logical, but it make implementation more complicated.

Keep in mind that identity is bound to yii\web\User. There is a single property yii\web\User::$identityClass at the moment. Spliting IdentityInterface into several ones will also make yii\web\User to be more complex.

Current Identity finding methods findIdentity() and findIdentityByAccessToken() are static. Thier notation was inspired by ActiveRecord paradigm.
Extracting IndentityRepositoryInterface with static methods looks odd - those methods should not be static then: identity repository should be an instance, which finds instances of identity. Such change eliminates the ability of usage same ActiveRecord class for IdentityInterface and IndentityRepositoryInterface.

Keeping 'find' methods as static already allows you to create separated implementation for finding - it just need to be encapsulated in those static methods of Identity class.

I am against such change.

from yii-core.

Keeping 'find' methods as static already allows you to create separated implementation for finding - it just need to be encapsulated in those static methods of Identity class.

It will be in the same class. Thus not separate.

from yii-core.

It will be in the same class. Thus not separate.

class MyIdentity implements IdentityInterface
{
    // ....

    public static function findIdentity($id)
    {
        return Yii::$app->identityRepository->find($id);
    }
}

from yii-core.

I'm positive applying the interface-segregation principle. In the end this will result to less complex code which is more flexible and easier to extend.

from yii-core.

@klimov-paul right but that's adding intermediate class. Feels hacky. I understand your concerns about complicating yii\web\User though...

from yii-core.

I need to actually attempt it to say if it's possible to do it w/o much hassle.

from yii-core.

I'm all for it.

from yii-core.

Разделять надо, т.к. IdentityInterface не должен уметь и находить и быть идентификацией, но репозитори интерфейс не нужен со статикой, его надо вынести в yii\web\User::findIdentity (там есть identityClass)

from yii-core.

http://slides.rmcreative.ru/2016/yii2-architecture/#/26 =)

from yii-core.

@lynicidn please use English in discussions which are conducted using English. Thanks.

from yii-core.

@samdark ok, sorry, no problem, wait pr, may be i have wrong opinion

from yii-core.

This makes sense to me as well.
Note that yii\web\User::$identityClass would then become yii\web\User::$identityRepository.
It no longer needs to know the class for an identity.

The parameter would accept an object or a configuration array or closure (as normal for Yii).
A basic ActiveRecordUserRepository could be implemented and provided with core, configuration would then become:

'components' => [
    'user' => [
        'identityRepository' => [
            'modelClass' => 'app\models\User'
        ]

Note that the user component would have a default configuration for its repository which contains:

['class' => 'yii\db\ActiveRecordUserRepository']

from yii-core.

While at it, would it be possible to add another method to the new IdentityInterface: something that produces a human-readable string of the underlying identity (such as getName(), getUsername() or getLabel())? It makes sense to me at least. What else is an identity if not an abstraction of a person, and as far as I know all humans come with a name.

I guess the reason why it was originally omitted is things like oauth where you don't necessarily obtain a string name? Maybe introduce as separate identity (e.g. HumanIdentityInterface) instead? Just something to consider.

from yii-core.

Identity doesn't necessary represent a human. It could be API client or another project or a service.

from yii-core.

yiisoft/yii-web#101

from yii-core.

Closing since the implementation has it split.

from yii-core.