Closed MrFrost-Nv27 closed 2 months ago
If a dev extends the UserModel and changed $returnType
, it returns incorrect CodeIgniter\Shield\Entities\User
instances. So I think this is a bug.
If a dev extends the UserModel and changed
$returnType
, it returns incorrectCodeIgniter\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
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
@paulbalandan What do you want?
array
is not allowed because User Provider must return User
entities.
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.
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, thefake()
method will instantiate an "array
class". If another dev puts their own Entity subclass which does extendUser
, the faking will throw a TypeError.What I am saying is since
UserProvider
expectsUser
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
@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.
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 ..
@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.
@MrFrost-Nv27 User::$id
can be a string.
https://github.com/codeigniter4/shield/blob/db10ec1cdaf16ad99150ae4b88c6839c11f19b41/src/Entities/User.php#L30
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
User::$id
can be a string.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
@MrFrost-Nv27 Oh, you are correct. Because of #336
@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 ..
Please sign all your commits. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits
@MrFrost-Nv27 Thank you!
Description Fixes #1103
Checklist: