Split IdentityInterface

[samdark]

What's the problem?

Currently IdentityInterface contains both methods to get an identity and methods which belong to the identity instance. While it is OK in context of Active Record it is an obstacle for implementing repository and entity separated.

What is the expected result?

Being able to separately implement repository which responsibility is to get an instance of identity, and an entity which actually holds the data.

What do you get instead?

A single interface forces us to implement everything in a single class.

Additional info

That's how new interfaces would look like:

interface IdentityInterface
{
    public function getId();
    public function getAuthKey();
    public function validateAuthKey($authKey);
}

interface IndentityRepositoryInterface
{
    public static function findIdentity($id);
    public static function findIdentityByAccessToken($token, $type = null);
}

[samdark]

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

[klimov-paul]

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.

[samdark]

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.

[klimov-paul]

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);
    }
}

[dynasource]

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.

[samdark]

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

[samdark]

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

[SilverFire]

I'm all for it.

[lynicidn]

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

[lynicidn]

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

[samdark]

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

[lynicidn]

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

[SamMousa]

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']

[vercotux]

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.

[samdark]

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

[samdark]

yiisoft/yii-web#101

[samdark]

Closing since the implementation has it split.