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

Improve NullUser with implementation of UserEntityInterface #152

Conversation

AurelienPillevesse
Copy link

Add implementation of UserEntityInterface for NullUser to be aligned with OAuth2-Server library

@chalasr
Copy link
Member

chalasr commented Aug 7, 2023

Could you please provide some link pointing to the related change on league/oauth2-server?

@AurelienPillevesse
Copy link
Author

There is no update from league/oauth2-server but this class implements everything except this little update

Copy link
Contributor

@maxhelias maxhelias left a comment

Choose a reason for hiding this comment

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

The NullUser is related to the new security implementation #34

/**
* @psalm-mutation-free
*/
public function getIdentifier(): string
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we respect the interface declaration with no return type and return a null value? https://github.com/thephpleague/oauth2-server/blob/master/src/Entities/UserEntityInterface.php#L19C7-L19C7

@X-Coder264
Copy link
Contributor

The NullUser is supposed to be used only in the context of the Symfony security layer (as that's what it was added for in the first place - in order to be able to use the client credentials grant in Symfony apps as that grant does not have a real user associated with it). I'm not sure what exactly do you want to achieve by making it implement an unrelated non Symfony interface?

@chalasr
Copy link
Member

chalasr commented Aug 12, 2023

As @X-Coder264 said, this is not desired as we have entities separated from security objects.
Thanks for proposing.

@chalasr chalasr closed this Aug 12, 2023
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.

4 participants