fix: change hardcoded user entity with declared return type

[MrFrost-Nv27]

Description
Fixes #1103

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

If a dev extends the UserModel and changed $returnType, it returns incorrect CodeIgniter\Shield\Entities\User instances. So I think this is a bug.

[MrFrost-Nv27]

If a dev extends the UserModel and changed $returnType, it returns incorrect CodeIgniter\Shield\Entities\User instances. So I think this is a bug.

Yes that what i mean .. i want customize the User entity, but still extend Shield User entity base class .. before i change the code, if i use email in credential fetch its return the Hardcoded User entity base class instead my User entity class

[paulbalandan]

This will fail if you changed the return type to array, right?

[MrFrost-Nv27]

This will fail if you changed the return type to array, right?

Based on method type, yes ...because it must return User class .. its no problem to change the returntype but still extend the base user entity class

[kenjis]

@paulbalandan What do you want?
array is not allowed because User Provider must return User entities.

[paulbalandan]

Let me rephrase:

For the longest time, it has been User to be the return type. Now, this PR will change the return to be what is defined in $returnType and we are blindly instantiating whatever its value. So, in essence, anyone can put other return types, right? So, if a dev puts 'array' as return type, the fake() method will instantiate an "array class". If another dev puts their own Entity subclass which does extend User, the faking will throw a TypeError.

What I am saying is since UserProvider expects User entities, we must make sure that $returnType should follow this now that we have lax requirement.

[MrFrost-Nv27]

Let me rephrase:

For the longest time, it has been User to be the return type. Now, this PR will change the return to be what is defined in $returnType and we are blindly instantiating whatever its value. So, in essence, anyone can put other return types, right? So, if a dev puts 'array' as return type, the fake() method will instantiate an "array class". If another dev puts their own Entity subclass which does extend User, the faking will throw a TypeError.

What I am saying is since UserProvider expects User entities, we must make sure that $returnType should follow this now that we have lax requirement.

Yes bro, but in fake() method for example, there is a declarative return type that was make sure the method is returning declared type, is User class, if some dev change the model $returnType to array, there was an error of "typeError" before the array return

There was failure typeError Except the dev change model $returnType to Shield User class or extend it

[kenjis]

@paulbalandan

So, in essence, anyone can put other return types, right?

No. If a dev puts array or class-string other than User (or a sub class), UserModel does not work due to a TypeError, as @MrFrost-Nv27 says.

[paulbalandan]

Sorry to not have seen the comments earlier and dragging this along.

I am well aware that it will throw a TypeError if you put other return types there. I even put that remark in my comment. I am just saying that we must be at least prudent in checking the value of $returnType before instantiating the value. I want to see something along the lines of:

if (! is_a($this->returnType, User::class, true) {
    throw new \LogicException('Return type must be a subclass of CodeIgniter\Shield\Entities\User.');
}

As an end user who may be new to Shield, for example, I don't want to be greeted by a TypeError with message "Return value of fake must return User but returns other class."when faking data. I think it will be a better developer experience if the thrown exception is more informational of the requirement.

I hope I made myself clear.

[MrFrost-Nv27]

Sorry to not have seen the comments earlier and dragging this along.

I am well aware that it will throw a TypeError if you put other return types there. I even put that remark in my comment. I am just saying that we must be at least prudent in checking the value of $returnType before instantiating the value. I want to see something along the lines of:

if (! is_a($this->returnType, User::class, true) {
    throw new \LogicException('Return type must be a subclass of CodeIgniter\Shield\Entities\User.');
}

As an end user who may be new to Shield, for example, I don't want to be greeted by a TypeError with message "Return value of fake must return User but returns other class."when faking data. I think it will be a better developer experience if the thrown exception is more informational of the requirement.

I hope I made myself clear.

Yeah that good to implement error message .. honestly, the real purpose of my PR is for use string in user id to implement uuid wkwk (i don't know you notice that or not, i just say it so you can understand it and maybe have best solution for my real problem), because the logic working before, in authentication flow, if use an email-password combination, the return value is hardcoded User entity, and there was an integer id and i don't choose to change or deleted that because i think that not the best solution .. in final authentication of email-password is badly returning only one integer value in uuid cases (9), and all uuid in my case was 9 in first character for all user, so don't care i logged as with email-password, is always returning the first row User in database .. that my real problem bro .. so i think, one of thousand solution is to customize the User entity and change the logic to returning entity that we was define .. in faker method is just an addition, not the real problem ..

But yeah, overall is good to implement your advice too ..

But, maybe you have the other solution for my real problem, i'm very happy to hear that for long time i wait the PR was confirm ..

[kenjis]

@paulbalandan I don't know how useful that error message would be.
But it would be a little easier to understand than PHP's TypeError.

Changing the error message that way is fine.

[kenjis]

@MrFrost-Nv27 User::$id can be a string.

shield/src/Entities/User.php

Line 30 in db10ec1

* @property int|string|null $id

If you cannot use a string, it is another bug.
So please create a new issue. I hope you complete this PR.

By the way, what do you mean by "wkwk"?

[MrFrost-Nv27]

ok

[MrFrost-Nv27]

@MrFrost-Nv27 User::$id can be a string.

shield/src/Entities/User.php

Line 30 in db10ec1

* @property int|string|null $id

If you cannot use a string, it is another bug. So please create a new issue. I hope you complete this PR.

By the way, what do you mean by "wkwk"?

In base User entity that was a cast in id with "?integer" so it cant be string, i think i just customize that in my side

"wkwk" is just "a laugh in chat" in my country

[kenjis]

@MrFrost-Nv27 Oh, you are correct. Because of #336

[MrFrost-Nv27]

@MrFrost-Nv27 Oh, you are correct. Because of #336

Yeah, so i think i just want customize the User Entity in my side, not in base side .. and ensure my customize entities work in base flow ..

[kenjis]

Please sign all your commits.

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

[paulbalandan]

Thanks for the changes!

Based on your requirement to use UUIDs in $id, I think extending User would be the best solution, to say the least.

[kenjis]

@MrFrost-Nv27 Thank you!