-
-
Notifications
You must be signed in to change notification settings - Fork 406
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
[User - WIP] add the user generator. #41
Conversation
Simperfit
commented
Nov 22, 2017
- Add a test
Thanks again for your contributions ... but I'd like to ask you to please stop sending pull requests. As we explained yesterday in https://symfony.com/blog/introducing-the-symfony-maker-bundle when introducing this bundle: We need issues to discuss about ideas first. Pull requests must be sent only for approved issues. Thanks for understanding it! |
public function serialize() | ||
{ | ||
return serialize([ | ||
$this->id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not serializing the username will cause issues (incompatible with the remember me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As i'm not using this feature I was not thinking about it, I understand, I will add it.
/** | ||
* @throws UsernameNotFoundException if the user is not found | ||
*/ | ||
public function loadUserByResourceOwner($resourceOwner, string $provider): UserInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does not make any sense in core (this is specific to HWIOAuthBundle AFAICT)
As I mentioned in #42, I'm +1 for this command (I think @Simperfit may even have gotten the idea from a list I have that I shared with him). @Simperfit could you rebase / update things for the latest |
use Symfony\Component\Security\Core\User\UserInterface; | ||
use Symfony\Component\Security\Core\User\UserProviderInterface; | ||
|
||
class UserProvider implements UserProviderInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we know the user wants an "entity" provider, then does it make sense to generate this class? They could just use the built-in entity
provider.
public function configure() | ||
{ | ||
$this | ||
->setDescription('Creates a the user doctrine class, the user provider and the user repository') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously, we want to make these commands as simple, and not interactive as possible (i.e. don't ask the user 10 questions). But, I think one question might make sense here:
Do you need to store User details in the database?
This will tell us whether or not they need a User
entity, or instead, a User
model class. This would also change whether or not we generate a UserProvider
(or just tell them to use the built-in entity
provider). And finally, it will change what recommendations we give at the end: i.e. the exact changes they'll need to make to their security.yml
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also need to ask a second question... if the first question is "yes" to storing user information:
Will you need to store an encoded password for each User in the database?
This would influence how we handle the getPassword()
method and whether or not we have a password
field. Sometimes, even when people have a local User
entity, they validate their password in a different system (e.g. via a SSO API). This is very confusing to users, so it's a great opportunity to help generate the User
class correctly based on this.
*/ | ||
public function getUsername(): string | ||
{ | ||
return $this->email; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does email come from?
Just saw this.. FYI i just released a full working https://medium.com/@ro0NL/adding-user-management-to-your-symfony-application-ceeefe2a2e9 let me know if we should talk about name conflicts. Either way i'd be very sad with |